You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Gordon Sim <gs...@redhat.com> on 2014/05/28 17:36:39 UTC
Review Request 21978: QPID-5788: lazy initialisation of NSS
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21978/
-----------------------------------------------------------
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
Re: Review Request 21978: QPID-5788: lazy initialisation of NSS
Posted by Andrew Stitcher <as...@apache.org>.
> On May 28, 2014, 5:18 p.m., Andrew Stitcher wrote:
> > /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp, line 127
> > <https://reviews.apache.org/r/21978/diff/1/?file=597359#file597359line127>
> >
> > It's probably worthwhile parsing the config file so that you can set the ssl options there too.
>
> Gordon Sim wrote:
> That's already done in parse() I believe, not in the constructor to the SslOptions. (Or am I missing something?)
It is done in parse, but unless the constructor tells it the file to use (ie QPIDC_CONF_FILE) I don't see how it could figure out the file to parse.
> On May 28, 2014, 5:18 p.m., Andrew Stitcher wrote:
> > /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp, line 152
> > <https://reviews.apache.org/r/21978/diff/1/?file=597359#file597359line152>
> >
> > Is it always safe to call shutdownNSS() even when initNSS() wasn't called?
> >
> > (not introduced by this change, but a possible issue nonetheless)
>
> Gordon Sim wrote:
> It seems to be. It will occur whenever you exit a client that did not open any ssl connections and I haven't seen any issues with that.
Well you can now easily make the initialised boolean a class boolean and test that to determine whether you shutdwnNSS() or not.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21978/#review44128
-----------------------------------------------------------
On May 28, 2014, 7:51 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, 7:51 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
>
>
Re: Review Request 21978: QPID-5788: lazy initialisation of NSS
Posted by Gordon Sim <gs...@redhat.com>.
> On May 28, 2014, 5:18 p.m., Andrew Stitcher wrote:
> > /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp, line 127
> > <https://reviews.apache.org/r/21978/diff/1/?file=597359#file597359line127>
> >
> > It's probably worthwhile parsing the config file so that you can set the ssl options there too.
That's already done in parse() I believe, not in the constructor to the SslOptions. (Or am I missing something?)
> On May 28, 2014, 5:18 p.m., Andrew Stitcher wrote:
> > /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp, line 137
> > <https://reviews.apache.org/r/21978/diff/1/?file=597359#file597359line137>
> >
> > 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.
I want to ensure that only exceptions of the right type are thrown. However I can rearrange this a little.
> On May 28, 2014, 5:18 p.m., Andrew Stitcher wrote:
> > /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp, line 152
> > <https://reviews.apache.org/r/21978/diff/1/?file=597359#file597359line152>
> >
> > Is it always safe to call shutdownNSS() even when initNSS() wasn't called?
> >
> > (not introduced by this change, but a possible issue nonetheless)
It seems to be. It will occur whenever you exit a client that did not open any ssl connections and I haven't seen any issues with that.
- Gordon
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21978/#review44128
-----------------------------------------------------------
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
>
>
Re: Review Request 21978: QPID-5788: lazy initialisation of NSS
Posted by Andrew Stitcher <as...@apache.org>.
> On May 28, 2014, 5:18 p.m., Andrew Stitcher wrote:
> > /trunk/qpid/cpp/src/qpid/client/SslConnector.cpp, line 127
> > <https://reviews.apache.org/r/21978/diff/1/?file=597359#file597359line127>
> >
> > It's probably worthwhile parsing the config file so that you can set the ssl options there too.
>
> Gordon Sim wrote:
> That's already done in parse() I believe, not in the constructor to the SslOptions. (Or am I missing something?)
>
> Andrew Stitcher wrote:
> It is done in parse, but unless the constructor tells it the file to use (ie QPIDC_CONF_FILE) I don't see how it could figure out the file to parse.
Sorry - ignore that last comment - your original point is correct - parse *is* using the QPIDC_CONF)_FILE, but via the common options configFile member! Actually the only reason we use CommonOptions at all here is so we can override the config file from the environment variables as we don't (can't) parse the command line from here.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21978/#review44128
-----------------------------------------------------------
On May 28, 2014, 7:51 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, 7:51 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
>
>
Re: Review Request 21978: QPID-5788: lazy initialisation of NSS
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 21978: QPID-5788: lazy initialisation of NSS
Posted by Gordon Sim <gs...@redhat.com>.
> On May 28, 2014, 8:14 p.m., Andrew Stitcher wrote:
> > I don't understand your comment about not changing the static variables to class members, as function statics will be duplicated in any forking process in exactly the same way.
> >
> > Function statics are really the same executable layout-wise as globals but they are only in scope in the function itself. The only difference is that for function statics that are a class type there will be some extra state and code to ensure that it is constructed thread safely only once (at least this is true for gcc and since C++11).
Ah, I assumed that the static locals would be initialised when they first came into scope. If that is not the case then I agree there is no advantage. I actually have a patch with them as member variables as that is what I started with. I'll upload that.
- Gordon
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21978/#review44159
-----------------------------------------------------------
On May 28, 2014, 7:51 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, 7:51 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
>
>
Re: Review Request 21978: QPID-5788: lazy initialisation of NSS
Posted by Gordon Sim <gs...@redhat.com>.
> On May 28, 2014, 8:14 p.m., Andrew Stitcher wrote:
> > I don't understand your comment about not changing the static variables to class members, as function statics will be duplicated in any forking process in exactly the same way.
> >
> > Function statics are really the same executable layout-wise as globals but they are only in scope in the function itself. The only difference is that for function statics that are a class type there will be some extra state and code to ensure that it is constructed thread safely only once (at least this is true for gcc and since C++11).
>
> Gordon Sim wrote:
> Ah, I assumed that the static locals would be initialised when they first came into scope. If that is not the case then I agree there is no advantage. I actually have a patch with them as member variables as that is what I started with. I'll upload that.
>
> Andrew Stitcher wrote:
> TBH I would leave the lock as a function static, and move the boolean (and use it in the destructor)
What's the thinking behind that?
- Gordon
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21978/#review44159
-----------------------------------------------------------
On May 28, 2014, 7:51 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, 7:51 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
>
>
Re: Review Request 21978: QPID-5788: lazy initialisation of NSS
Posted by Andrew Stitcher <as...@apache.org>.
> On May 28, 2014, 8:14 p.m., Andrew Stitcher wrote:
> > I don't understand your comment about not changing the static variables to class members, as function statics will be duplicated in any forking process in exactly the same way.
> >
> > Function statics are really the same executable layout-wise as globals but they are only in scope in the function itself. The only difference is that for function statics that are a class type there will be some extra state and code to ensure that it is constructed thread safely only once (at least this is true for gcc and since C++11).
>
> Gordon Sim wrote:
> Ah, I assumed that the static locals would be initialised when they first came into scope. If that is not the case then I agree there is no advantage. I actually have a patch with them as member variables as that is what I started with. I'll upload that.
TBH I would leave the lock as a function static, and move the boolean (and use it in the destructor)
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21978/#review44159
-----------------------------------------------------------
On May 28, 2014, 7:51 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, 7:51 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
>
>
Re: Review Request 21978: QPID-5788: lazy initialisation of NSS
Posted by Andrew Stitcher <as...@apache.org>.
> On May 28, 2014, 8:14 p.m., Andrew Stitcher wrote:
> > I don't understand your comment about not changing the static variables to class members, as function statics will be duplicated in any forking process in exactly the same way.
> >
> > Function statics are really the same executable layout-wise as globals but they are only in scope in the function itself. The only difference is that for function statics that are a class type there will be some extra state and code to ensure that it is constructed thread safely only once (at least this is true for gcc and since C++11).
>
> Gordon Sim wrote:
> Ah, I assumed that the static locals would be initialised when they first came into scope. If that is not the case then I agree there is no advantage. I actually have a patch with them as member variables as that is what I started with. I'll upload that.
>
> Andrew Stitcher wrote:
> TBH I would leave the lock as a function static, and move the boolean (and use it in the destructor)
>
> Gordon Sim wrote:
> What's the thinking behind that?
Keeping the member as encapsulated as possible: the lock is only used in the function so keep it there - I'm suggesting that the boolean should also be used to not call shutdownNSS() if NSS did not initialise so it would need to be a class member, if not for sharing the boolean with the destructor I'd keep it static in the initialise function.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21978/#review44159
-----------------------------------------------------------
On May 28, 2014, 7:51 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, 7:51 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
>
>
Re: Review Request 21978: QPID-5788: lazy initialisation of NSS
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21978/#review44159
-----------------------------------------------------------
I don't understand your comment about not changing the static variables to class members, as function statics will be duplicated in any forking process in exactly the same way.
Function statics are really the same executable layout-wise as globals but they are only in scope in the function itself. The only difference is that for function statics that are a class type there will be some extra state and code to ensure that it is constructed thread safely only once (at least this is true for gcc and since C++11).
- Andrew Stitcher
On May 28, 2014, 7:51 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, 7:51 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
>
>
Re: Review Request 21978: QPID-5788: lazy initialisation of NSS
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21978/
-----------------------------------------------------------
(Updated May 28, 2014, 7:51 p.m.)
Review request for qpid and Andrew Stitcher.
Changes
-------
Moved lazy initialisation logic into static member function and rearranged the exception handling/throwing as requested. (The lock and initialised flag are not static member variables as I don't want them to be shared when forking).
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 (updated)
-----
/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