Wireshark-dev: Re: [Wireshark-dev] (34169) Pre-commit check failing incorrectly?

From: Dario Lombardo <lomato@xxxxxxxxx>
Date: Mon, 5 Aug 2019 22:02:27 +0200
The message is telling you that ENC_BIG_ENDIAN has been used on a FT_UINT8 fiield, that is 1 byte long, then no point is setting the endianess. From a quick look of the dissector I can tell that hf_cdp_data has been used with variable lengths. What's its len? If it's a variable len field ("data" would suggest that), FT_UINT8 is not the right choice. FT_BYTES should be the right one.

On Mon, Aug 5, 2019 at 9:45 PM 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. 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.

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);



- Oliver
___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev@xxxxxxxxxxxxx>
Archives:    https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request@xxxxxxxxxxxxx?subject=unsubscribe


--
Naima is online.