You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Ernie Allen <ea...@redhat.com> on 2014/06/25 21:18:58 UTC
Review Request 22975: bz693721 - Session names longer than 256 bytes cause
errors when encoding session management objects
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22975/
-----------------------------------------------------------
Review request for qpid, Alan Conway and Gordon Sim.
Repository: qpid
Description
-------
AMQP 0-10 allows session names to be up to 2^16 bytes long; the QMF management schema for the broker however defines the name of a session object as being up to 2^8 bytes long. If a session is created with a name greater than 256 bytes, the broker cannot send out management objects for it i.e. periodic processing fails.
This patch adds a new field in the management schema that handles the full length name. The existing name field is truncated if needed.
Some of the management tools could be updated to use the new field. qpid-tool works without modifications.
Diffs
-----
/trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1605518
/trunk/qpid/cpp/src/qpid/broker/amqp/ManagedSession.cpp 1605518
/trunk/qpid/cpp/src/qpid/broker/management-schema.xml 1605518
Diff: https://reviews.apache.org/r/22975/diff/
Testing
-------
- Added session name that was 1024 characters long.
- Checked the message log to verify the absence of the following exception [System] error Exception thrown by timer task ManagementAgent::periodicProcessing: Could not encode string of 1061 bytes as uint8_t string. (/home/eallen/current/qpid/cpp/src/qpid/framing/Buffer.cpp:246)
- Ran qpid-tool and saw the presence of the truncated session name in the existing field, and the full name in the new field.
The new field is named "fullName".
Thanks,
Ernie Allen
Re: Review Request 22975: bz693721 - Session names longer than 256 bytes
cause errors when encoding session management objects
Posted by Ernie Allen <ea...@redhat.com>.
> On June 26, 2014, 8:18 a.m., Gordon Sim wrote:
> > No objection to this change. However, what would be the impact of change the field type? I'm assuming the python tools wouldn't fail, would they? If we can get a solid understanding of the impact of just changing the type, and decide that the impact is acceptable, then it would be cleaner/simpler. Of course if the impact is unacceptable, this patch is fine. I'd just like to be explicit about what the impact is.
Cumin's database relies on the management-schema. If we change a field name or type, Cumin would get errors during object updates.
- Ernie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22975/#review46724
-----------------------------------------------------------
On June 25, 2014, 7:18 p.m., Ernie Allen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22975/
> -----------------------------------------------------------
>
> (Updated June 25, 2014, 7:18 p.m.)
>
>
> Review request for qpid, Alan Conway and Gordon Sim.
>
>
> Repository: qpid
>
>
> Description
> -------
>
> AMQP 0-10 allows session names to be up to 2^16 bytes long; the QMF management schema for the broker however defines the name of a session object as being up to 2^8 bytes long. If a session is created with a name greater than 256 bytes, the broker cannot send out management objects for it i.e. periodic processing fails.
>
> This patch adds a new field in the management schema that handles the full length name. The existing name field is truncated if needed.
> Some of the management tools could be updated to use the new field. qpid-tool works without modifications.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1605518
> /trunk/qpid/cpp/src/qpid/broker/amqp/ManagedSession.cpp 1605518
> /trunk/qpid/cpp/src/qpid/broker/management-schema.xml 1605518
>
> Diff: https://reviews.apache.org/r/22975/diff/
>
>
> Testing
> -------
>
> - Added session name that was 1024 characters long.
> - Checked the message log to verify the absence of the following exception [System] error Exception thrown by timer task ManagementAgent::periodicProcessing: Could not encode string of 1061 bytes as uint8_t string. (/home/eallen/current/qpid/cpp/src/qpid/framing/Buffer.cpp:246)
> - Ran qpid-tool and saw the presence of the truncated session name in the existing field, and the full name in the new field.
>
> The new field is named "fullName".
>
>
> Thanks,
>
> Ernie Allen
>
>
Re: Review Request 22975: bz693721 - Session names longer than 256 bytes
cause errors when encoding session management objects
Posted by Ernie Allen <ea...@redhat.com>.
> On June 26, 2014, 8:18 a.m., Gordon Sim wrote:
> > No objection to this change. However, what would be the impact of change the field type? I'm assuming the python tools wouldn't fail, would they? If we can get a solid understanding of the impact of just changing the type, and decide that the impact is acceptable, then it would be cleaner/simpler. Of course if the impact is unacceptable, this patch is fine. I'd just like to be explicit about what the impact is.
>
> Ernie Allen wrote:
> Cumin's database relies on the management-schema. If we change a field name or type, Cumin would get errors during object updates.
Also the qpid-snmp package would need to be updated. The mib would need to be regenerated and the code updated.
- Ernie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22975/#review46724
-----------------------------------------------------------
On June 25, 2014, 7:18 p.m., Ernie Allen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22975/
> -----------------------------------------------------------
>
> (Updated June 25, 2014, 7:18 p.m.)
>
>
> Review request for qpid, Alan Conway and Gordon Sim.
>
>
> Repository: qpid
>
>
> Description
> -------
>
> AMQP 0-10 allows session names to be up to 2^16 bytes long; the QMF management schema for the broker however defines the name of a session object as being up to 2^8 bytes long. If a session is created with a name greater than 256 bytes, the broker cannot send out management objects for it i.e. periodic processing fails.
>
> This patch adds a new field in the management schema that handles the full length name. The existing name field is truncated if needed.
> Some of the management tools could be updated to use the new field. qpid-tool works without modifications.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1605518
> /trunk/qpid/cpp/src/qpid/broker/amqp/ManagedSession.cpp 1605518
> /trunk/qpid/cpp/src/qpid/broker/management-schema.xml 1605518
>
> Diff: https://reviews.apache.org/r/22975/diff/
>
>
> Testing
> -------
>
> - Added session name that was 1024 characters long.
> - Checked the message log to verify the absence of the following exception [System] error Exception thrown by timer task ManagementAgent::periodicProcessing: Could not encode string of 1061 bytes as uint8_t string. (/home/eallen/current/qpid/cpp/src/qpid/framing/Buffer.cpp:246)
> - Ran qpid-tool and saw the presence of the truncated session name in the existing field, and the full name in the new field.
>
> The new field is named "fullName".
>
>
> Thanks,
>
> Ernie Allen
>
>
Re: Review Request 22975: bz693721 - Session names longer than 256 bytes
cause errors when encoding session management objects
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22975/#review46724
-----------------------------------------------------------
No objection to this change. However, what would be the impact of change the field type? I'm assuming the python tools wouldn't fail, would they? If we can get a solid understanding of the impact of just changing the type, and decide that the impact is acceptable, then it would be cleaner/simpler. Of course if the impact is unacceptable, this patch is fine. I'd just like to be explicit about what the impact is.
- Gordon Sim
On June 25, 2014, 7:18 p.m., Ernie Allen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22975/
> -----------------------------------------------------------
>
> (Updated June 25, 2014, 7:18 p.m.)
>
>
> Review request for qpid, Alan Conway and Gordon Sim.
>
>
> Repository: qpid
>
>
> Description
> -------
>
> AMQP 0-10 allows session names to be up to 2^16 bytes long; the QMF management schema for the broker however defines the name of a session object as being up to 2^8 bytes long. If a session is created with a name greater than 256 bytes, the broker cannot send out management objects for it i.e. periodic processing fails.
>
> This patch adds a new field in the management schema that handles the full length name. The existing name field is truncated if needed.
> Some of the management tools could be updated to use the new field. qpid-tool works without modifications.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1605518
> /trunk/qpid/cpp/src/qpid/broker/amqp/ManagedSession.cpp 1605518
> /trunk/qpid/cpp/src/qpid/broker/management-schema.xml 1605518
>
> Diff: https://reviews.apache.org/r/22975/diff/
>
>
> Testing
> -------
>
> - Added session name that was 1024 characters long.
> - Checked the message log to verify the absence of the following exception [System] error Exception thrown by timer task ManagementAgent::periodicProcessing: Could not encode string of 1061 bytes as uint8_t string. (/home/eallen/current/qpid/cpp/src/qpid/framing/Buffer.cpp:246)
> - Ran qpid-tool and saw the presence of the truncated session name in the existing field, and the full name in the new field.
>
> The new field is named "fullName".
>
>
> Thanks,
>
> Ernie Allen
>
>
Re: Review Request 22975: bz693721 - Session names longer than 256 bytes
cause errors when encoding session management objects
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22975/#review46912
-----------------------------------------------------------
Actually it turns out when applying this patch that there is an issue. With the patch applied qpid-stat -u seems not to function. Also session listing e.g. through qpid-tool, now have the name duplicated as both short and long name are indexed. When these are the same this has no value and merely reduces readability.
(Btw the diff to the management spec is to a file that no longer exists on trunk, it moved to cpp/src/qpid/broker/management-spec.xml. However I fixed that and found the issue above.)
- Gordon Sim
On June 25, 2014, 7:18 p.m., Ernie Allen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22975/
> -----------------------------------------------------------
>
> (Updated June 25, 2014, 7:18 p.m.)
>
>
> Review request for qpid, Alan Conway and Gordon Sim.
>
>
> Repository: qpid
>
>
> Description
> -------
>
> AMQP 0-10 allows session names to be up to 2^16 bytes long; the QMF management schema for the broker however defines the name of a session object as being up to 2^8 bytes long. If a session is created with a name greater than 256 bytes, the broker cannot send out management objects for it i.e. periodic processing fails.
>
> This patch adds a new field in the management schema that handles the full length name. The existing name field is truncated if needed.
> Some of the management tools could be updated to use the new field. qpid-tool works without modifications.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1605518
> /trunk/qpid/cpp/src/qpid/broker/amqp/ManagedSession.cpp 1605518
> /trunk/qpid/cpp/src/qpid/broker/management-schema.xml 1605518
>
> Diff: https://reviews.apache.org/r/22975/diff/
>
>
> Testing
> -------
>
> - Added session name that was 1024 characters long.
> - Checked the message log to verify the absence of the following exception [System] error Exception thrown by timer task ManagementAgent::periodicProcessing: Could not encode string of 1061 bytes as uint8_t string. (/home/eallen/current/qpid/cpp/src/qpid/framing/Buffer.cpp:246)
> - Ran qpid-tool and saw the presence of the truncated session name in the existing field, and the full name in the new field.
>
> The new field is named "fullName".
>
>
> Thanks,
>
> Ernie Allen
>
>
Re: Review Request 22975: bz693721 - Session names longer than 256 bytes
cause errors when encoding session management objects
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22975/#review46707
-----------------------------------------------------------
Ship it!
There's a typo in your "testing" comment - you say 1024 byte string, where I think you mean 65536 byte string - but the code is right :)
- Alan Conway
On June 25, 2014, 7:18 p.m., Ernie Allen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22975/
> -----------------------------------------------------------
>
> (Updated June 25, 2014, 7:18 p.m.)
>
>
> Review request for qpid, Alan Conway and Gordon Sim.
>
>
> Repository: qpid
>
>
> Description
> -------
>
> AMQP 0-10 allows session names to be up to 2^16 bytes long; the QMF management schema for the broker however defines the name of a session object as being up to 2^8 bytes long. If a session is created with a name greater than 256 bytes, the broker cannot send out management objects for it i.e. periodic processing fails.
>
> This patch adds a new field in the management schema that handles the full length name. The existing name field is truncated if needed.
> Some of the management tools could be updated to use the new field. qpid-tool works without modifications.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1605518
> /trunk/qpid/cpp/src/qpid/broker/amqp/ManagedSession.cpp 1605518
> /trunk/qpid/cpp/src/qpid/broker/management-schema.xml 1605518
>
> Diff: https://reviews.apache.org/r/22975/diff/
>
>
> Testing
> -------
>
> - Added session name that was 1024 characters long.
> - Checked the message log to verify the absence of the following exception [System] error Exception thrown by timer task ManagementAgent::periodicProcessing: Could not encode string of 1061 bytes as uint8_t string. (/home/eallen/current/qpid/cpp/src/qpid/framing/Buffer.cpp:246)
> - Ran qpid-tool and saw the presence of the truncated session name in the existing field, and the full name in the new field.
>
> The new field is named "fullName".
>
>
> Thanks,
>
> Ernie Allen
>
>
Re: Review Request 22975: bz693721 - Session names longer than 256 bytes
cause errors when encoding session management objects
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22975/#review46738
-----------------------------------------------------------
Ship it!
Fair enough, thanks for the explanation.
- Gordon Sim
On June 25, 2014, 7:18 p.m., Ernie Allen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22975/
> -----------------------------------------------------------
>
> (Updated June 25, 2014, 7:18 p.m.)
>
>
> Review request for qpid, Alan Conway and Gordon Sim.
>
>
> Repository: qpid
>
>
> Description
> -------
>
> AMQP 0-10 allows session names to be up to 2^16 bytes long; the QMF management schema for the broker however defines the name of a session object as being up to 2^8 bytes long. If a session is created with a name greater than 256 bytes, the broker cannot send out management objects for it i.e. periodic processing fails.
>
> This patch adds a new field in the management schema that handles the full length name. The existing name field is truncated if needed.
> Some of the management tools could be updated to use the new field. qpid-tool works without modifications.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1605518
> /trunk/qpid/cpp/src/qpid/broker/amqp/ManagedSession.cpp 1605518
> /trunk/qpid/cpp/src/qpid/broker/management-schema.xml 1605518
>
> Diff: https://reviews.apache.org/r/22975/diff/
>
>
> Testing
> -------
>
> - Added session name that was 1024 characters long.
> - Checked the message log to verify the absence of the following exception [System] error Exception thrown by timer task ManagementAgent::periodicProcessing: Could not encode string of 1061 bytes as uint8_t string. (/home/eallen/current/qpid/cpp/src/qpid/framing/Buffer.cpp:246)
> - Ran qpid-tool and saw the presence of the truncated session name in the existing field, and the full name in the new field.
>
> The new field is named "fullName".
>
>
> Thanks,
>
> Ernie Allen
>
>