Qpid › Apache Qpid developers

Review Request: Add management hooks into ACL lookup engine for C++ broker

Classic

List

Threaded

9 messages Options

Chug Rolke

Review Request: Add management hooks into ACL lookup engine for C++ broker

Alan Conway

Re: Review Request: Add management hooks into ACL lookup engine for C++ broker

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4525/#review6449
-----------------------------------------------------------

trunk/qpid/cpp/src/qpid/Modules.cpp
<https://reviews.apache.org/r/4525/#comment14072>

We had this before and took it out. Causes logging before the logging subsystem is properly initialized. Don't remember the exact symptoms but the log text can wind up in inappropriate places.

trunk/qpid/cpp/src/qpid/acl/Acl.cpp
<https://reviews.apache.org/r/4525/#comment14073>

I'm not keen on individual try/catch around every step, it makes the code hard to follow. I'd put a single try-catch around the lot, or just let the exception get thrown: the management infrastructure will catch it and put the exception message in the QMF response.

- Alan

On 2012-03-27 19:10:23, Chug Rolke wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4525/
> -----------------------------------------------------------
>
> (Updated 2012-03-27 19:10:23)
>
>
> Review request for qpid, Alan Conway, Gordon Sim, and rajith attapattu.
>
>
> Summary
> -------
>
> Add management methods that let a customer drive arbitrary ACL lookup queries.
>
>
> This addresses bug QPID-3918.
> https://issues.apache.org/jira/browse/QPID-3918
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/src/qpid/Modules.cpp 1305877
> trunk/qpid/cpp/src/qpid/acl/Acl.h 1305877
> trunk/qpid/cpp/src/qpid/acl/Acl.cpp 1305877
> trunk/qpid/cpp/src/qpid/acl/AclData.cpp 1305877
> trunk/qpid/cpp/src/qpid/acl/management-schema.xml 1305877
>
> Diff: https://reviews.apache.org/r/4525/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chug
>
>

astitcher

Re: Review Request: Add management hooks into ACL lookup engine for C++ broker

In reply to this post by Chug Rolke

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4525/#review6454
-----------------------------------------------------------

trunk/qpid/cpp/src/qpid/acl/Acl.cpp
<https://reviews.apache.org/r/4525/#comment14084>

never use "catch (...)" unless you also use "throw"
- it thoroughly screws up debugging on windows.
- If you don't know at least roughly what you are expecting you shouldn't be catching it the most general you should catch is std::exception&.

[I also agree with Alan's assessment about the try blocks being way too granular]

- Andrew

On 2012-03-27 19:10:23, Chug Rolke wrote:

Chug Rolke

Re: Review Request: Add management hooks into ACL lookup engine for C++ broker

In reply to this post by Chug Rolke

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4525/
-----------------------------------------------------------

(Updated 2012-03-30 15:54:07.491139)

Review request for qpid, Alan Conway, Gordon Sim, and rajith attapattu.

Changes
-------

1. Don't log module load.
2. Check in unrelated bug fixes separately.
3. Change function try/catch scheme: Just have one try/catch. Change 'throw str' exception generation to 'throw qpid::Exception(str)' and then catch only std::exception.

There was some push back to exclude this patch: it does not add to the function of a broker and becomes one more piece of unrelated code that fails to be maintained. An alternative is to create a separate library or executable that loads acl.so and implements the query functions there. This has the possibility of loading any acl.so and to give query results from any version of qpidd.

I favor keeping this patch not just because it's complete and ready to go. Adding the proposed library or executable is a much larger task and is likely 'not worth doing'. This patch is very focused and exists in an optional, loadable module; as such it doesn't burden the broker at run-time or change any of its performance characteristics. Also, by being in the broker itself it can never give wrong results because it loaded the wrong acl.so file.

Summary
-------

Add management methods that let a customer drive arbitrary ACL lookup queries.

This addresses bug QPID-3918.
https://issues.apache.org/jira/browse/QPID-3918

Diffs (updated)
-----

trunk/qpid/cpp/src/qpid/acl/Acl.h 1307073
trunk/qpid/cpp/src/qpid/acl/Acl.cpp 1307073
trunk/qpid/cpp/src/qpid/acl/management-schema.xml 1307073
trunk/qpid/cpp/src/qpid/broker/AclModule.h 1307073

Diff: https://reviews.apache.org/r/4525/diff

Testing
-------

Thanks,

Chug

Chug Rolke

Re: Review Request: Add management hooks into ACL lookup engine for C++ broker

Alan Conway

Re: Review Request: Add management hooks into ACL lookup engine for C++ broker

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4525/
> -----------------------------------------------------------
>
> (Updated 2012-04-04 15:42:02)
>
>
> Review request for qpid, Alan Conway, Gordon Sim, and rajith attapattu.
>
>
> Summary
> -------
>
> Add management methods that let a customer drive arbitrary ACL lookup queries.
>
>
> This addresses bug QPID-3918.
> https://issues.apache.org/jira/browse/QPID-3918
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/src/qpid/acl/Acl.h 1307073
> trunk/qpid/cpp/src/qpid/acl/Acl.cpp 1307073
> trunk/qpid/cpp/src/qpid/acl/management-schema.xml 1307073
> trunk/qpid/cpp/src/qpid/broker/AclModule.h 1307073
> trunk/qpid/cpp/src/tests/acl.py 1307073
> trunk/qpid/tools/src/py/qpidtoollibs/broker.py 1307073
>
> Diff: https://reviews.apache.org/r/4525/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chug
>
>

Alan Conway

Re: Review Request: Add management hooks into ACL lookup engine for C++ broker

In reply to this post by Chug Rolke

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4525/
> -----------------------------------------------------------
>
> (Updated 2012-04-04 15:42:02)
>
>
> Review request for qpid, Alan Conway, Gordon Sim, and rajith attapattu.
>
>
> Summary
> -------
>
> Add management methods that let a customer drive arbitrary ACL lookup queries.
>
>
> This addresses bug QPID-3918.
> https://issues.apache.org/jira/browse/QPID-3918
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/src/qpid/acl/Acl.h 1307073
> trunk/qpid/cpp/src/qpid/acl/Acl.cpp 1307073
> trunk/qpid/cpp/src/qpid/acl/management-schema.xml 1307073
> trunk/qpid/cpp/src/qpid/broker/AclModule.h 1307073
> trunk/qpid/cpp/src/tests/acl.py 1307073
> trunk/qpid/tools/src/py/qpidtoollibs/broker.py 1307073
>
> Diff: https://reviews.apache.org/r/4525/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chug
>
>

astitcher

Re: Review Request: Add management hooks into ACL lookup engine for C++ broker

In reply to this post by Chug Rolke

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4525/#review6690
-----------------------------------------------------------

Ship it!

Except for the whitespace, this looks good.

trunk/qpid/cpp/src/tests/acl.py
<https://reviews.apache.org/r/4525/#comment14493>

Looks like a bunch of whitespace errors here.

- Andrew

On 2012-04-04 15:42:02, Chug Rolke wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4525/
> -----------------------------------------------------------
>
> (Updated 2012-04-04 15:42:02)
>
>
> Review request for qpid, Alan Conway, Gordon Sim, and rajith attapattu.
>
>
> Summary
> -------
>
> Add management methods that let a customer drive arbitrary ACL lookup queries.
>
>
> This addresses bug QPID-3918.
> https://issues.apache.org/jira/browse/QPID-3918
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/src/qpid/acl/Acl.h 1307073
> trunk/qpid/cpp/src/qpid/acl/Acl.cpp 1307073
> trunk/qpid/cpp/src/qpid/acl/management-schema.xml 1307073
> trunk/qpid/cpp/src/qpid/broker/AclModule.h 1307073
> trunk/qpid/cpp/src/tests/acl.py 1307073
> trunk/qpid/tools/src/py/qpidtoollibs/broker.py 1307073
>
> Diff: https://reviews.apache.org/r/4525/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chug
>
>

tross

Re: Review Request: Add management hooks into ACL lookup engine for C++ broker

In reply to this post by Chug Rolke

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4525/#review6693
-----------------------------------------------------------

Ship it!

This looks great. Clean up the whitespace in the Python code.

- Ted

On 2012-04-04 15:42:02, Chug Rolke wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4525/
> -----------------------------------------------------------
>
> (Updated 2012-04-04 15:42:02)
>
>
> Review request for qpid, Alan Conway, Gordon Sim, and rajith attapattu.
>
>
> Summary
> -------
>
> Add management methods that let a customer drive arbitrary ACL lookup queries.
>
>
> This addresses bug QPID-3918.
> https://issues.apache.org/jira/browse/QPID-3918
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/src/qpid/acl/Acl.h 1307073
> trunk/qpid/cpp/src/qpid/acl/Acl.cpp 1307073
> trunk/qpid/cpp/src/qpid/acl/management-schema.xml 1307073
> trunk/qpid/cpp/src/qpid/broker/AclModule.h 1307073
> trunk/qpid/cpp/src/tests/acl.py 1307073
> trunk/qpid/tools/src/py/qpidtoollibs/broker.py 1307073
>
> Diff: https://reviews.apache.org/r/4525/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chug
>
>