On Aug 3, 2019, at 7:47 AM, Oliver Brown <cellot...@gmail.com> 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/ ___________________________________________________________________________ Sent via: Wireshark-dev mailing list <wireshark-dev@wireshark.org> Archives: https://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
