> 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 > >
