You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by rajith attapattu <ra...@gmail.com> on 2011/12/07 02:52:53 UTC
Review Request: Add at least basic functionality to add ACL rules dynamically
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3041/
-----------------------------------------------------------
Review request for qpid, Alan Conway, Gordon Sim, and Kim van der Riet.
Summary
-------
The following patch allows an ACL rule to be added to the AclData object held in memory. Updates are not propagated in a clustered setup. Therefore qmf folks are discouraged from using this method until a proper solution to the clustered setup is worked out. This was done to facilitate Alan Conway's work on QPID-3665
Alan was kind enough to do initial review and testing.
P.S I promise to remove all trailing whitespaces before commit. I already updated the patch after removing whitespaces in Acl.cpp, but doing so for other files like AclData.cpp produces a much larger diff and distracts from the changes that I want to be reviewed.
This addresses bug QPID-3665.
https://issues.apache.org/jira/browse/QPID-3665
Diffs
-----
http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/acl/Acl.h 1209722
http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/acl/Acl.cpp 1209722
http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/acl/AclData.h 1209722
http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/acl/AclData.cpp 1209722
http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/broker/AclModule.h 1209722
Diff: https://reviews.apache.org/r/3041/diff
Testing
-------
I believe Alan already has a test case as part of QPID-3665.
Thanks,
rajith
Re: Review Request: Add at least basic functionality to add ACL rules
dynamically
Posted by rajith attapattu <ra...@gmail.com>.
> On 2011-12-07 14:31:11, Gordon Sim wrote:
> > Looks broken to me. Other than for QPID-3652, why might you want to add rules dynamically? I'm not convinced that is a good thing (the ACL system is already convoluted enough that I find I need to read the source code to be able to use it).
I agree that the ACL design needs to be re-evaluated at some point and perhaps adding these new methods may open up more problems as people might end up using them in a way it's not designed to.
After speaking with Alan, I decided to abandon this approach.
> On 2011-12-07 14:31:11, Gordon Sim wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/acl/Acl.cpp, line 140
> > <https://reviews.apache.org/r/3041/diff/1/?file=62654#file62654line140>
> >
> > AclData::addAclRule() appears to have no locking in it. If this method can be called concurrently (as the use of locks above and below suggests), this would seem to suggest this is not threadsafe.
It is indeed. Not sure what I was thinking there.
I believe the correct approach is to just copy, update & validate the model inside one block after acquiring the lock.
- rajith
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3041/#review3701
-----------------------------------------------------------
On 2011-12-07 01:52:53, rajith attapattu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3041/
> -----------------------------------------------------------
>
> (Updated 2011-12-07 01:52:53)
>
>
> Review request for qpid, Alan Conway, Gordon Sim, and Kim van der Riet.
>
>
> Summary
> -------
>
> The following patch allows an ACL rule to be added to the AclData object held in memory. Updates are not propagated in a clustered setup. Therefore qmf folks are discouraged from using this method until a proper solution to the clustered setup is worked out. This was done to facilitate Alan Conway's work on QPID-3665
>
> Alan was kind enough to do initial review and testing.
>
> P.S I promise to remove all trailing whitespaces before commit. I already updated the patch after removing whitespaces in Acl.cpp, but doing so for other files like AclData.cpp produces a much larger diff and distracts from the changes that I want to be reviewed.
>
>
> This addresses bug QPID-3665.
> https://issues.apache.org/jira/browse/QPID-3665
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/acl/Acl.h 1209722
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/acl/Acl.cpp 1209722
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/acl/AclData.h 1209722
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/acl/AclData.cpp 1209722
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/broker/AclModule.h 1209722
>
> Diff: https://reviews.apache.org/r/3041/diff
>
>
> Testing
> -------
>
> I believe Alan already has a test case as part of QPID-3665.
>
>
> Thanks,
>
> rajith
>
>
Re: Review Request: Add at least basic functionality to add ACL rules
dynamically
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3041/#review3701
-----------------------------------------------------------
Looks broken to me. Other than for QPID-3652, why might you want to add rules dynamically? I'm not convinced that is a good thing (the ACL system is already convoluted enough that I find I need to read the source code to be able to use it).
http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/acl/Acl.cpp
<https://reviews.apache.org/r/3041/#comment8268>
AclData::addAclRule() appears to have no locking in it. If this method can be called concurrently (as the use of locks above and below suggests), this would seem to suggest this is not threadsafe.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/acl/Acl.cpp
<https://reviews.apache.org/r/3041/#comment8269>
What is the point of this? The 'd' pointer already points to the same object that 'data' does.
- Gordon
On 2011-12-07 01:52:53, rajith attapattu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3041/
> -----------------------------------------------------------
>
> (Updated 2011-12-07 01:52:53)
>
>
> Review request for qpid, Alan Conway, Gordon Sim, and Kim van der Riet.
>
>
> Summary
> -------
>
> The following patch allows an ACL rule to be added to the AclData object held in memory. Updates are not propagated in a clustered setup. Therefore qmf folks are discouraged from using this method until a proper solution to the clustered setup is worked out. This was done to facilitate Alan Conway's work on QPID-3665
>
> Alan was kind enough to do initial review and testing.
>
> P.S I promise to remove all trailing whitespaces before commit. I already updated the patch after removing whitespaces in Acl.cpp, but doing so for other files like AclData.cpp produces a much larger diff and distracts from the changes that I want to be reviewed.
>
>
> This addresses bug QPID-3665.
> https://issues.apache.org/jira/browse/QPID-3665
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/acl/Acl.h 1209722
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/acl/Acl.cpp 1209722
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/acl/AclData.h 1209722
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/acl/AclData.cpp 1209722
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/broker/AclModule.h 1209722
>
> Diff: https://reviews.apache.org/r/3041/diff
>
>
> Testing
> -------
>
> I believe Alan already has a test case as part of QPID-3665.
>
>
> Thanks,
>
> rajith
>
>