Re: [PREVIEW] Encapsulate SSL configuration settings

On 07/02/2013 04:32 PM, Amos Jeffries wrote:
> On 3/07/2013 6:11 a.m., Alex Rousskov wrote:
>> On 07/02/2013 06:49 AM, Amos Jeffries wrote:
>>> +/// parse and store the configuration options used
>>> +/// for generating an SSL context
>>> +class ConfigOptions

>> Thank you for writing a class description!
>>
>> I suggest renaming this class to Ssl::ContexConfig or
>> Ssl::ContextOptions to distinguish it from the existing Ssl::Config
>> class.
>>
>> However, if this class is actually not specific to SSL context (I have
>> not checked) but is specific to a "port" (as in http_port or
>> https_port), then something like Ssl::PortOptions would be better. We do
>> not have an outgoing "port" configuration option, but we kind of have
>> that concept so it may work for both outgoing and incoming connections.

> It is not specific to a port either. It is shared by the outbound SSL
> configuration.

Yes, that is why I said that we can use an implicit outbound port
concept here (Squid has that concept even though there is no single
squid.conf option like http_port for the server side). If all those
options you want to group are common to all SSL connections going
through an outbound port (e.g., all SSL connections to a peer), then we
can use something like Ssl::PortOptions.

> Called it Ssl::LinkContextOptions for now. Leaning towards
> Ssl::LinkContextConfig.
>
> Preference or better alternatives welcome.

What is a "link"? I know about SSL ports, SSL connections, and SSL
contexts, but am not familiar with the term "link" in this context.
Please note that I am not saying that it is the wrong term. I am just
curious what it means.

Either *Options or *Config suffix should work OK IMO.

>> I would also remove "parse and store the" from the description to avoid
>> limiting it too much and to focus on the class meaning rather than a few
>> of its actions. For example:
>>
>> /// manages SSL context configuration options
>> class ContextConfig
>
> It is intentionally limited and designed not to manage anything. Just to
> encapsulate the particular set of config options and provide a parse
> routine for them. Context management is done differently by the port and
> peer code, and also by the cert generator code which does not even have
> one of these.

Storing, parsing, and copying options is already "managing" them. Note
that I did not describe the class as managing SSL _context_. Only as
managing SSL context configuration _options_. I am sure that, with time,
this class will gain more methods. In fact, I suspect there is already
SSL gadgets/support code that should be moved inside this class, but I
am not asking you to implement that move :-)!

>> * implement assignment operator for the new class
>
> Hmm. That should not be needed. Added as a un-implemented and made the
> object ref-countable to assist the port clone().

OK. Declaring but not implementing an assignment operator is much better
than hoping that the auto-generated operator will not be called
(although it is odd to have a copy constructor but not the assignment
operator).

Thank you,

Alex.
Received on Tue Jul 09 2013 - 04:37:38 MDT