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.
