Comment # 5
on bug 9612
from Pavel Moravec
(In reply to comment #4)
> Hi Pavel, thanks for the patch. It looks really good already, just a few
> minor comments:
>
> - The explicit THROW_ON in dissect_amqp_1_0_list seems to duplicate what the
> AMQP_INCREMENT macro does in the next line?
>
> - Some of the other explicit THROW_ON calls are unnecessary if the
> dissection would naturally just run off the end of the packet (which
> Wireshark already catches and handles automatically).
>
> - You set a lot of items as type FT_NONE when they appear to have normal
> types (FT_BOOL, FT_UINT, etc). It is usually preferred to add these to the
> tree with their correct type and value so that they can be filtered on by
> more than simply their presence.
>
> Cheers,
> Evan
Thanks for the feedback, I am applying your suggestions. As the FT_NONE ->
FT_.. replacement is quite tricky (many AMQP types can be decoded as uint8 or
uint16 or so, depending on type descriptor; hence I have to dynamically
change/set proper ft_amqp_1_0_* variable after decoding the descriptor), it
will take very few weeks to propose new version of patch.
You are receiving this mail because:
- You are watching all bug changes.