-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18423/
-----------------------------------------------------------
(Updated Feb. 26, 2014, 12:22 p.m.)
Review request for qpid, Chug Rolke and Gordon Sim.
Changes
-------
Posting modified Chug's patch that applies the comments.
Patch is extended to cover "'qpid.max_count':0" set by client while ACL having
queuemaxcountlowerlimit=10. Currently, broker passes 0 to ACL module that marks
the rule as not matching (0<10).
As max_count and max_size value 0 means "no limit" (*), I replaced zero by
std::numeric_limits<uint64_t>::max().
Anyway, on my RHEL6 system, that is evaluated to 18446744073709551615 (and all
works fine), while both cpp/src/tests/Variant.cpp and cpp/src/tests/acl.py
assumes it is 9223372036854775807..
(*) Note it makes (bigger) sense to have zero as "unlimited" only for those two
parameters, as:
- max_pages: what is the user story behind having unlimited number of pages in
memory, for paged queue?
- page_factor, file_size and file_count: zero is invalid value
Bugs: QPID-5561 and QPID-5565
https://issues.apache.org/jira/browse/QPID-5561
https://issues.apache.org/jira/browse/QPID-5565
Repository: qpid
Description
-------
This patch fixes both:
QPID-5561: C++ Broker queue creation ACL checks against wrong limit values
QPID-5565: [C++ broker] ACL queue create rules to take zero as unlimited value
The patch:
- passes to ACL persistency-related / paging queue options only if the queue is
marked as durable / with paging
- fills proper default values
Potential issues:
- I had to copy constant value "static const u_int32_t defJrnlFileSizePgs = 24"
and also "defNumJrnlFiles = 8" as there is no simple way (known to me) to test
if legacy store module is loaded
- anyway as legacy store is not supposed to be changed (due to linearstore),
I see this a acceptable
- ACL line like:
acl allow all create queue filemaxcountupperlimit=32
is _not_ matched for a transient queue. That differs from current behaviour,
anyway I see it as an improvement rather (as is, the above ACL is meaningless
for transient queues that dont have filecount limit applicable)
Diffs (updated)
-----
/trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1571980
/trunk/qpid/cpp/src/qpid/broker/QueueFactory.h 1571980
/trunk/qpid/cpp/src/qpid/broker/QueueFactory.cpp 1571980
/trunk/qpid/cpp/src/tests/acl.py 1571980
Diff: https://reviews.apache.org/r/18423/diff/
Testing
-------
- different variants of use cases (esp. from the JIRAs) passed
- automated tests passed
Thanks,
Pavel Moravec
