Wireshark · Wireshark-dev: Re: [Wireshark-dev] [Wireshark-commits] rev 39143: /trunk/epan/dissectors/ /trunk/epan/dissectors/:

["Maynard, Chris" <Christopher.Maynard@xxxxxxxxx>]

________________________________________
From: wireshark-dev-bounces@xxxxxxxxxxxxx [wireshark-dev-bounces@xxxxxxxxxxxxx] On Behalf Of Martin Kaiser [lists@xxxxxxxxx]
Sent: Monday, September 26, 2011 5:27 PM
To: wireshark-dev@xxxxxxxxxxxxx
Subject: Re: [Wireshark-dev] [Wireshark-commits] rev    39143:  /trunk/epan/dissectors/ /trunk/epan/dissectors/:        packet-dvbci.c

> Hi^2,
> 
> Thus wrote Maynard, Chris (Christopher.Maynard@xxxxxxxxx):
> 
> > So that would be option (c) then?
> > c) Define ENC_NA differently from both ENC_LITTLE_ENDIAN and ENC_BIG_ENDIAN.
> 
> ah, ENC_NA == ENC_BIG_ENDIAN == 0x0 at the moment. That's the problem
> you mentioned in your 1st mail?

Yes, that's it.  And because ENC_NA == ENC_BIG_ENDIAN, it's technically not a problem for big-endian protocols, only for little-endian protocols.

> > The impact of this would imply these other changes:
> 
> > For every proto_tree_add_*() function:
> > -> Change the "const gboolean little_endian" argument to something like "gint endian"
> > -> Verify "endian" is valid for the given FT_ and choke on invalid ones.
> 
> You would then check explicitly that for FT_BYTES, ENC_NA is set and for
> FT_(U)INT16/32/.. encoding != ENC_NA?

Basically, yes although there are other types than FT_BYTES that ENC_NA is intended to be used for.  I think we need to define exactly which ones will use ENC_NA though.

> That sounds good. People will realize more quickly when they got it
> wrong.

I suppose, although I still question the real need for ENC_NA at all.  What's the point of checking that an FT_ field, whose endian-ness doesn't matter, is set to a particular value?  I guess ENC_NA helps to self-document the code, but as far as I can tell, that's its only value.  Now that being said, changing the "const gboolean little_endian" argument to an "int endian" or better yet, "int encoding" argument and then performing verification could open up other possibilities.  To quote from README.developer:

                                                   In the future, other
    elements of the encoding, such as the character encoding for
    character strings, might be supported.

If other elements of the encoding are to be added, then we will also likely need to change the definition of ENC_BIG_ENDIAN (and possibly ENC_LITTLE_ENDIAN too) so that they are both single bit assignments, thus leaving room for other bits to be set, which will give the encoding a different meaning.

- Chris

CONFIDENTIALITY NOTICE: The contents of this email are confidential
and for the exclusive use of the intended recipient. If you receive this
email in error, please delete it from your system immediately and 
notify us either by email, telephone or fax. You should not copy,
forward, or otherwise disclose the content of the email.