Re: (34169) Pre-commit check failing incorrectly?

[Dario Lombardo <lomato () gmail com>]

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 () 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. 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 () wireshark org>
> Archives:    https://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
>              mailto:wireshark-dev-request () wireshark org
> ?subject=unsubscribe



-- 

Naima is online.

___________________________________________________________________________
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-request () wireshark org?subject=unsubscribe