[ 
https://issues.apache.org/jira/browse/QPID-3665?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13164562#comment-13164562
 ] 

jirapos...@reviews.apache.org commented on QPID-3665:
-----------------------------------------------------



bq.  On 2011-12-07 14:31:11, Gordon Sim wrote:
bq.  > 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.


bq.  On 2011-12-07 14:31:11, Gordon Sim wrote:
bq.  > 
http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/acl/Acl.cpp, line 
140
bq.  > <https://reviews.apache.org/r/3041/diff/1/?file=62654#file62654line140>
bq.  >
bq.  >     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:
bq.  
bq.  -----------------------------------------------------------
bq.  This is an automatically generated e-mail. To reply, visit:
bq.  https://reviews.apache.org/r/3041/
bq.  -----------------------------------------------------------
bq.  
bq.  (Updated 2011-12-07 01:52:53)
bq.  
bq.  
bq.  Review request for qpid, Alan Conway, Gordon Sim, and Kim van der Riet.
bq.  
bq.  
bq.  Summary
bq.  -------
bq.  
bq.  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
bq.  
bq.  Alan was kind enough to do initial review and testing.
bq.  
bq.  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. 
bq.  
bq.  
bq.  This addresses bug QPID-3665.
bq.      https://issues.apache.org/jira/browse/QPID-3665
bq.  
bq.  
bq.  Diffs
bq.  -----
bq.  
bq.    http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/acl/Acl.h 
1209722 
bq.    http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/acl/Acl.cpp 
1209722 
bq.    
http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/acl/AclData.h 
1209722 
bq.    
http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/acl/AclData.cpp 
1209722 
bq.    
http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/broker/AclModule.h 
1209722 
bq.  
bq.  Diff: https://reviews.apache.org/r/3041/diff
bq.  
bq.  
bq.  Testing
bq.  -------
bq.  
bq.  I believe Alan already has a test case as part of QPID-3665.
bq.  
bq.  
bq.  Thanks,
bq.  
bq.  rajith
bq.  
bq.


                
> Add at least basic functionality to add ACL rules dynamically
> -------------------------------------------------------------
>
>                 Key: QPID-3665
>                 URL: https://issues.apache.org/jira/browse/QPID-3665
>             Project: Qpid
>          Issue Type: Bug
>          Components: C++ Broker
>    Affects Versions: 0.14
>            Reporter: Rajith Attapattu
>            Assignee: Rajith Attapattu
>             Fix For: 0.15
>
>
> There have been several requests in the past to provide some sort of 
> functionality to add ACL rules dynamically.
> Recently QPID-3652 needed a way to add an ACL rule programatically.
> For starters it would be nice to add a method in the ACL interface to add a 
> rule for a given user.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: 
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project:      http://qpid.apache.org
Use/Interact: mailto:dev-subscr...@qpid.apache.org

Reply via email to