> On March 7, 2017, 8:22 p.m., Andrew Stitcher wrote: > > src/qpid/sys/ssl/SslSocket.cpp > > Line 125 (original), 130 (patched) > > <https://reviews.apache.org/r/57392/diff/1/?file=1658526#file1658526line130> > > > > You could have simply changed this to: > > > > ```c++ > > if (certName.empty()) return; > > > > prototype = ... > > > > ... > > } > > ``` > > > > Or it is possible and meaningful to have an empty certName but check > > the client auth?
The certName can be set through the setCertName() method, so in theory you could pass in an empty string to that constructor and true for client auth, then set the certname after that. Nothing does that at present however nor is it really an obvious pattern. However I felt that overloading the certname to imply a server connection was potentially misleading (client sockets can also set the certname, they just don't do it on construction). My thought was that since we currently have a pattern where client sockets are always created with no args, and server sockets specify args, it would be neater to have separate constructors for each case which is still a very small change. Something more explicit, or as you say moving the prototype creation to listen(), would be better, but I didn't want to spend ages analysing the implications, so kept it simple to fix the client leak quickly. - Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57392/#review168176 ----------------------------------------------------------- On March 7, 2017, 7:42 p.m., Gordon Sim wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57392/ > ----------------------------------------------------------- > > (Updated March 7, 2017, 7:42 p.m.) > > > Review request for qpid, Andrew Stitcher and Cliff Jansen. > > > Bugs: QPID-7693 > https://issues.apache.org/jira/browse/QPID-7693 > > > Repository: qpid-cpp > > > Description > ------- > > This avoids leaking a protoype socket for every client SslSocket created. (I > assume the prototype is still leaked for the server case, but since the > broker only closes the socket it listens on when shutting down that has much > less impact). This change just distinguishes sockets used for listening on > from those used for connecting. > > > Diffs > ----- > > src/qpid/sys/ssl/SslSocket.h 733a47a > src/qpid/sys/ssl/SslSocket.cpp 731151c > > > Diff: https://reviews.apache.org/r/57392/diff/1/ > > > Testing > ------- > > > Thanks, > > Gordon Sim > >
