Re: [Wireshark-commits] rev 39143: /trunk/epan/dissectors/ /trunk/epan/dissectors/: packet-dvbci.c

["Maynard, Chris" <Christopher.Maynard () GTECH COM>]

________________________________________
From: wireshark-dev-bounces () wireshark org [wireshark-dev-bounces () wireshark org] On Behalf Of Martin Kaiser [lists 
() kaiser cx]
Sent: Monday, September 26, 2011 5:27 PM
To: wireshark-dev () wireshark org
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 () GTECH COM):
>
> > 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.

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