> 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? > > Gordon Sim wrote: > 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. > > Andrew Stitcher wrote: > In theory though a client can also specify a certName, so the constructor > can't really be used like you suggest to distinguish between server and > client. This would be in the case that you use client side certificates (in > fact I don't think it's even that rare a case.)
Given that prototype is a protected member it can only be used in SSLSocket and SSLMuxSocket. looking at all the uses it is safe to create it in SSLSocket::listen. - Andrew ----------------------------------------------------------- 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 > >
