Re: [PREVIEW] Encapsulate SSL configuration settings

On 3/07/2013 6:11 a.m., Alex Rousskov wrote:
> On 07/02/2013 06:49 AM, Amos Jeffries wrote:
>
>> This creates an Ssl::Config class to manage SSL configuration settings
>> and creates a ConfigOptions sub-class to hold the settings which are
>> needed passing around in the SSL code.
> The above description does not match the patch: We already have an
> Ssl::Config class and the ConfigOptions you are adding is not a subclass
> of that.
>
>
>> +/// 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.
Called it Ssl::LinkContextOptions for now. Leaning towards
Ssl::LinkContextConfig.

Preference or better alternatives welcome.

> 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.

>
>> TODO:
>> * implement dump/free functions properly
>> * implement parse for backward-compatible loading of the old DIRECT SSL
>> settings options.
> * 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().

Amos
Received on Tue Jul 02 2013 - 22:32:44 MDT