----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21978/#review44128 -----------------------------------------------------------
Such a short piece of code, so many comments... sorry. /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp <https://reviews.apache.org/r/21978/#comment78455> I think it would be worthwhile putting all this initialisation logic in it's own static member function for clarity. /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp <https://reviews.apache.org/r/21978/#comment78454> If you follow the advice about throwing directly move this to just cover initNSS(); /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp <https://reviews.apache.org/r/21978/#comment78450> It's probably worthwhile parsing the config file so that you can set the ssl options there too. /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp <https://reviews.apache.org/r/21978/#comment78452> I think you could just throw from here without saving an error /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp <https://reviews.apache.org/r/21978/#comment78453> Similarly to above. I think you could just throw "Failed to initialise..." + e.what(). Or even not have this in a try .. catch at all, as we no longer need to ensure no exceptions are thrown. /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp <https://reviews.apache.org/r/21978/#comment78457> Is it always safe to call shutdownNSS() even when initNSS() wasn't called? (not introduced by this change, but a possible issue nonetheless) - Andrew Stitcher On May 28, 2014, 3:36 p.m., Gordon Sim wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21978/ > ----------------------------------------------------------- > > (Updated May 28, 2014, 3:36 p.m.) > > > Review request for qpid and Andrew Stitcher. > > > Bugs: QPID-5788 > https://issues.apache.org/jira/browse/QPID-5788 > > > Repository: qpid > > > Description > ------- > > Delay initialisation of NSS until the first SSL connection is created. This > allows env vars to be set programmatically if desired and allows forking > (provided connections are created *after* forking). > > > Diffs > ----- > > /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp 1597730 > > Diff: https://reviews.apache.org/r/21978/diff/ > > > Testing > ------- > > make test passes > > The two test cases described in the JIRA work as expected > > > Thanks, > > Gordon Sim > >
