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.
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
> 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
Thank you,
Alex.
Received on Tue Jul 02 2013 - 18:11:50 MDT
This archive was generated by hypermail 2.2.0 : Wed Jul 03 2013 - 12:00:32 MDT