Wireshark · Wireshark-dev: Re: [Wireshark-dev] [Wireshark-commits] rev21556:/trunk/epan//trunk/epan/: proto.c proto.h - allbuil

["Martin Mathieson" <martin.r.mathieson@xxxxxxxxxxxxxx>]

OK,

I'll wait until you check in the tvb_get_bits() change, then look at
it again.  Any test files you could share with longer bit sequences
and/or different bit offsets would be welcome (so I don't trash it
just to make my little examples work :) ).

Martin

On 4/26/07, Anders Broman (AL/EAB) <anders.broman@xxxxxxxxxxxx> wrote:
> Hi,
> The intention is to use tvb_get bits from inside proto_tree_add_bits so
> there will be no overlap.
> /Anders
>
> ________________________________
>
> Från: wireshark-dev-bounces@xxxxxxxxxxxxx genom Martin Mathieson
> Skickat: to 2007-04-26 13:49
> Till: Developer support list for Wireshark
> Ämne: Re: [Wireshark-dev] [Wireshark-commits]
> rev21556:/trunk/epan//trunk/epan/: proto.c proto.h - allbuildbots rednow :-(
>
>
>
> Hi Anders,
>
> Your tvb_get_bits() has no much in common with the add_bits()
> functions that it would be a shame not to share all of the fiddly
> bits.
>
> Anyway, here is the patch I forgot to send earlier.  It may be a few
> days before I can look at this again :(
>
> Martin
>
>
> On 4/26/07, Anders Broman (AL/EAB) <anders.broman@xxxxxxxxxxxx> wrote:
> > Hi,
> > I think you forgot the patch :)
> > I have been looking at the funktion in packet-ansi_801.c
> > ansi_801_tvb_get_bits() which may be better
> > To use with some changes to handle endianess and not to use pointers to
> > offsets. Feel free to check
> > In any changes I'm a bit short on time pressently.
> > Best regards
> > Anders
> > P.S
> > Untested unused first draft of tvb_get_bits() just extracting the code fom
> > the other funktions.
> > guint32
> > tvb_get_bits(tvbuff_t *tvb, gint bit_offset, gint no_of_bits, gboolean
> > little_endian)
> > {
> >       gboolean is_bytealigned = FALSE;
> >       gint offset;
> >       guint length;
> >       guint bit_length;
> >       guint32 value = 0;
> >       guint32 mask = 0;
> >       guint8 mask8    = 0xff;
> >       guint16 mask16  = 0xffff;
> >       guint32 mask24  = 0xffffff;
> >       guint32 mask32  = 0xffffffff;
> >       guint8 shift;
> >
> >       if((bit_offset&0x7)==0)
> >               is_bytealigned = TRUE;
> >       offset = bit_offset>>3;
> >       bit_length = ((bit_offset&0x7)+no_of_bits);
> >       length = bit_length >>3;
> >       if((bit_length&0x7)!=0)
> >               length = length +1;
> >
> >       if (no_of_bits < 2){
> >               /* Single bit */
> >               mask8 = mask8 >>(bit_offset&0x7);
> >               value = tvb_get_guint8(tvb,offset) & mask8;
> >               mask = 0x80;
> >               shift = 8-((bit_offset + no_of_bits)&0x7);
> >               if (shift<8){
> >                       value = value >> shift;
> >                       mask = mask >> shift;
> >               }
> >       }else if(no_of_bits < 9){
> >               /* One or 2 bytes */
> >               if(length == 1){
> >                       /* Spans 1 byte */
> >                       mask8 = mask8>>(bit_offset&0x7);
> >                       value = tvb_get_guint8(tvb,offset)&mask8;
> >                       mask = 0x80;
> >               }else{
> >                       /* Spans 2 bytes */
> >                       mask16 = mask16>>(bit_offset&0x7);
> >                       if(little_endian){
> >                               value=tvb_get_letohs(tvb, offset);
> >                       } else {
> >                               value=tvb_get_ntohs(tvb, offset);
> >                       }
> >                       mask = 0x8000;
> >               }
> >               shift = 8-((bit_offset + no_of_bits)&0x7);
> >               if (shift<8){
> >                       value = value >> shift;
> >                       mask = mask >> shift;
> >               }
> >
> >       }else if (no_of_bits < 17){
> >               /* 2 or 3 bytes */
> >               if(length == 2){
> >                       /* Spans 2 bytes */
> >                       mask16 = mask16>>(bit_offset&0x7);
> >                       if(little_endian){
> >                               value=tvb_get_letohs(tvb, offset);
> >                       } else {
> >                               value=tvb_get_ntohs(tvb, offset);
> >                       }
> >                       mask = 0x8000;
> >               }else{
> >                       /* Spans 3 bytes */
> >                       mask24 = mask24>>(bit_offset&0x7);
> >                       if(little_endian){
> >                               value=tvb_get_letoh24(tvb, offset);
> >                       } else {
> >                               value=tvb_get_ntoh24(tvb, offset);
> >                       }
> >                       mask = 0x800000;
> >               }
> >               shift = 8-((bit_offset + no_of_bits)&0x7);
> >               if (shift<8){
> >                       value = value >> shift;
> >                       mask = mask >> shift;
> >               }
> >
> >       }else if (no_of_bits < 25){
> >               /* 3 or 4 bytes */
> >               if(length == 3){
> >                       /* Spans 3 bytes */
> >                       mask24 = mask24>>(bit_offset&0x7);
> >                       if(little_endian){
> >                               value=tvb_get_letoh24(tvb, offset);
> >                       } else {
> >                               value=tvb_get_ntoh24(tvb, offset);
> >                       }
> >                       mask = 0x800000;
> >               }else{
> >                       /* Spans 4 bytes */
> >                       mask32 = mask32>>(bit_offset&0x7);
> >                       if(little_endian){
> >                               value=tvb_get_letohl(tvb, offset);
> >                       } else {
> >                               value=tvb_get_ntohl(tvb, offset);
> >                       }
> >                       mask = 0x80000000;
> >               }
> >               shift = 8-((bit_offset + no_of_bits)&0x7);
> >               if (shift<8){
> >                       value = value >> shift;
> >                       mask = mask >> shift;
> >               }
> >
> >       }else if (no_of_bits < 33){
> >               /* 4 or 5 bytes */
> >               if(length == 4){
> >                       /* Spans 4 bytes */
> >                       mask32 = mask32>>(bit_offset&0x7);
> >                       if(little_endian){
> >                               value=tvb_get_letohl(tvb, offset);
> >                       } else {
> >                               value=tvb_get_ntohl(tvb, offset);
> >                       }
> >                       mask = 0x80000000;
> >               }else{
> >                       /* Spans 5 bytes
> >                        * Does not handle unaligned bits over 24
> >                        */
> >                       DISSECTOR_ASSERT_NOT_REACHED();
> >               }
> >               shift = 8-((bit_offset + no_of_bits)&0x7);
> >               if (shift<8){
> >                       value = value >> shift;
> >                       mask = mask >> shift;
> >               }
> >
> >       }else{
> >               DISSECTOR_ASSERT_NOT_REACHED();
> >       }
> >
> >       return value;
> >
> > }
> > -----Original Message-----
> > From: wireshark-dev-bounces@xxxxxxxxxxxxx
> > [mailto:wireshark-dev-bounces@xxxxxxxxxxxxx] On Behalf Of Martin Mathieson
> > Sent: den 26 april 2007 13:20
> > To: Developer support list for Wireshark
> > Subject: Re: [Wireshark-dev] [Wireshark-commits] rev
> > 21556:/trunk/epan//trunk/epan/: proto.c proto.h - all buildbots rednow :-(
> >
> > Anders,
> >
> > I've tried the functions (with new prototypes) in the FP dissector, and
> made
> > the changes indicated in the attached patch.
> >
> > I didn't want to commit it as you may not want to change it in this way,
> > also I've only looked at the 1 and 2 byte length cases.
> >
> > In my patch, I shift the mask8/mask16 and the raw value immediately to
> > occupy the lsb and and them to the final value there.  This seems simpler
> to
> > me than working out a more complicated mask (which is where the bug was)
> > then shifting the resulting value to occupy the lsb to get the final
> result.
> >
> > Does this seem sane to you?
> > Best regards,
> > Martin
> >
> > On 4/25/07, Anders Broman <a.broman@xxxxxxxxx> wrote:
> > > Hi,
> > > I'm looking inot spliting it into:
> > >
> > > /*This function will call proto_tree_add_bits_ret_val() without the
> > > return value. */ extern proto_item * proto_tree_add_bits(proto_tree
> > > *tree, int hf_index, tvbuff_t *tvb, gint bit_offset, gint no_of_bits,
> > > gboolean little_endian);
> > >
> > > /* Calls tvb_get bits */
> > > extern proto_item *
> > > proto_tree_add_bits_ret_val(proto_tree *tree, int hf_index, tvbuff_t
> > > *tvb, gint bit_offset, gint no_of_bits, guint32 *return_value,
> > > gboolean little_endian);
> > >
> > >
> > > guint32
> > > tvb_get_bits(tvbuff_t *tvb, gint bit_offset, gint no_of_bits, gboolean
> > > little_endian)
> > >
> > > (Should it handle 64bits?)
> > >
> > > If that's a more acceptable format. It would be good if we could agree
> > > on a format and get down to the nitty gritti of fixing up the functions.
> > >
> > > Best regards
> > > Anders
> > > BTW
> > > dissectors.lib(packet-ansi_801.obj) : warning LNK4006: _tvb_get_bits
> > > already def ined in tvbuff.obj; second definition ignored
> > >
> > > -----Ursprungligt meddelande-----
> > > Från: wireshark-dev-bounces@xxxxxxxxxxxxx
> > > [mailto:wireshark-dev-bounces@xxxxxxxxxxxxx] För Martin Mathieson
> > > Skickat: den 25 april 2007 19:23
> > > Till: Developer support list for Wireshark
> > > Ämne: Re: [Wireshark-dev] [Wireshark-commits] rev 21556:
> > > /trunk/epan//trunk/epan/: proto.c proto.h - all buildbots red now :-(
> > >
> > > I've had a play with this function, I like it - it can improve and
> > > simplify parts of packet-umts_fp.c which are bit-oriented, like E-DCH
> > > data.  Having the function spit out the return value is helpful here,
> > > as this will avoid the dissector doing the equivalent shiting and
> > > masking work for a second time.
> > >
> > > Having said that, although it shows the correct bytes for me, it is
> > > computing the wrong value at the moment.  For example, I selected 6
> > > bits, offset 4 bytes into the bytes 0x4917.  It shows the correct
> > > bits, i.e.    .... 100101.. .... but computes the value to be 101,
> > > i.e. the bits 1100101.  So it looks like maybe the masking is out by 1
> > > bit - I'll look at it tomorrow if no-one else does first.
> > >
> > > Best regards,
> > > Martin
> > >
> > > On 4/24/07, Anders Broman <a.broman@xxxxxxxxx> wrote:
> > > >
> > > >
> > > > -----Ursprungligt meddelande-----
> > > > Från: wireshark-dev-bounces@xxxxxxxxxxxxx
> > > > [mailto:wireshark-dev-bounces@xxxxxxxxxxxxx] För Ulf Lamping
> > > > Skickat: den 24 april 2007 23:05
> > > > Till: Developer support list for Wireshark
> > > > Ämne: Re: [Wireshark-dev] [Wireshark-commits] rev 21556:
> > > > /trunk/epan/
> > > > /trunk/epan/: proto.c proto.h - all buildbots red now :-(
> > > >
> > > > Anders Broman wrote:
> > > > >> Hi,
> > > > >> I have no problem with backing out my changes but a Proto_...
> > > > >> function should not be local to iuup in my opinion.
> > > > >> Perhaps it should be renamed iuup_proto_.. instead until We have
> > > > >> "the real thing" in proto.c
> > > > >>
> > > > >> What do others think?
> > > > >>
> > > > >Well, first of all, we shouldn't have a buildbot going into deep
> > > > >red for a long time, so there's something to be done.
> > > >
> > > > >I'll fully agree that a function starting with proto_ shouldn't be
> > > > >in any dissector code - a name clash will be the result sooner or
> > > > >later - and today is sooner ;-)
> > > >
> > > > >Could you please:
> > > >
> > > > >1.) fix the issue in iuup by prepending iuup_ so the buildbot get's
> > > > >green again - I'm currently waiting for it :-(
> > > >
> > > > Joerg beat me to it.
> > > >
> > > > >2.) if your new function doesn't follow the common function
> > > > >pattern, fix this issue in your implementation (I didn't had a look
> > > > >at the code in proto myself)
> > > >
> > > >
> > > > Well what pattern it should have is being discussed.
> > > > As I see it:
> > > > Alt
> > > > 1) Leave it as it is and change other proto_add.. functions to
> > > > behave similarly.
> > > > 1b) Leave the signature but rename it to something like
> > > > proto_tree_add_bits_ret_val() and add similar functions for other
> stuff.
> > > >
> > > > 2) use(proto_tree* tree, int hf, tvbuff_t* tvb, int offset, int
> > > bit_offset,
> > > > guint bits) as in iuup
> > > >
> > > > 3) use (proto_tree *tree, int hf_index, tvbuff_t *tvb, gint
> > > > bit_offset,
> > > gint
> > > > no_of_bits, gboolean little_endian)
> > > >
> > > > Other?
> > > > Regards
> > > > Anders
> > > >
> > > >
> > > >
> > > > _______________________________________________
> > > > Wireshark-dev mailing list
> > > > Wireshark-dev@xxxxxxxxxxxxx
> > > > http://www.wireshark.org/mailman/listinfo/wireshark-dev
> > > >
> > > > _______________________________________________
> > > > Wireshark-dev mailing list
> > > > Wireshark-dev@xxxxxxxxxxxxx
> > > > http://www.wireshark.org/mailman/listinfo/wireshark-dev
> > > >
> > > _______________________________________________
> > > Wireshark-dev mailing list
> > > Wireshark-dev@xxxxxxxxxxxxx
> > > http://www.wireshark.org/mailman/listinfo/wireshark-dev
> > >
> > > _______________________________________________
> > > Wireshark-dev mailing list
> > > Wireshark-dev@xxxxxxxxxxxxx
> > > http://www.wireshark.org/mailman/listinfo/wireshark-dev
> > >
> > _______________________________________________
> > Wireshark-dev mailing list
> > Wireshark-dev@xxxxxxxxxxxxx
> > http://www.wireshark.org/mailman/listinfo/wireshark-dev
> > _______________________________________________
> > Wireshark-dev mailing list
> > Wireshark-dev@xxxxxxxxxxxxx
> > http://www.wireshark.org/mailman/listinfo/wireshark-dev
> >