On Aug 3, 2019, at 7:47 AM, Oliver Brown <cellotape@xxxxxxxxx> wrote:
> Re: https://code.wireshark.org/review/c/34169/
>
> I've simply expanding definitions for existing flags in CDP packets, specifically for four additional CDP capabilities, yet the Ubuntu pre-commit check is failing due to code that shouldn't have anything to do with changes I made.
> epan/dissectors/packet-cdp.c: FT_UINT8: proto_tree_add_item(tlv_tree, hf_cdp_data, tvb, offset + 4, 2, [[ENC_BIG_ENDIAN]-->[ENC_NA]]);
> It is not a descriptive message at all and it isn't entirely clear what the issue is.
The pre-commit check appears to be broken, to the extent that it's showing a bogus field type value.
The issue is that "cdp.data" is of type FT_BYTES; a byte string has no byte order, so it should be added with ENC_NA, not ENC_BIG_ENDIAN.
> I saw this same issue when trying to commit, myself, but I simply skipped the check since I don't believe its an issue.
>
> To complicate matters, the "FT_UINT8" seemed to change each time I attempted to commit, myself; it would be UINT8, UINT16, UINT32, BOOLEAN, BYTES, STRINGZ, IPV4.. and without knowing what exactly the error is supposed to be, it seems tedious to track down what the issue is.
Perhaps it's picking some random value to show.
> I've had a bit of a search and it seems, years ago, there was a mass change to using ENC_NA everywhere.. which is perhaps what the error is trying to suggest. Indeed, changing line 620 to use ENC_NA fixes this I believe - but this shouldn't need to be changed, since it has remained this way for 2 years..
>
> Additionally, it seems there was a legitimate reason why this is ENC_BIG_ENDIAN, although I'm kind of curious to see examples (@Guy Harris, do you have any example captures where you found this to be the case?). I'd be happy to pick that up, but the point remains that I don't believe that this should be causing an issue.
>
> if (length == 6) {
> /*
> * This is some unknown value; it's typically 0x20 0x00,
> * which, as a big-endian value, is not a VLAN ID, as
> * VLAN IDs are 12 bits long.
> */
> proto_tree_add_item(tlv_tree, hf_cdp_data, tvb, offset + 4, 2, ENC_BIG_ENDIAN);
That's not saying the value is big-endian. It says that *if* you interpret it as a big-endian 2-byte value, it's not a VLAN ID, in contrast to the case where the length isn't 6 and where there *is* a 2-byte VLAN ID.
It should be fixed to use ENC_NA; I've done that in
https://code.wireshark.org/review/c/34176/