Wireshark-dev: [Wireshark-dev] New proto_add_bits function (Was: rev 21556:/trunk/epan//tr...)
Date Index · Thread Index · Other Months · All Mailing Lists
Date Prev · Date Next · Thread Prev · Thread Next
From: "Anders Broman \(AL/EAB\)" <anders.broman@xxxxxxxxxxxx>
Date: Thu, 26 Apr 2007 17:56:48 +0200
Hi,
I have used it in the h263 dissector with a h263 prefix unfortunatly there is only short sequences
Beeing dissected most being the same in all h263 frames :(
Perhaps it can be used in the PER dissector but I didn't want to use it before we had agreed on a format to
avoid to many changes.
Regrads
Anders

-----Original Message-----
From: wireshark-dev-bounces@xxxxxxxxxxxxx [mailto:wireshark-dev-bounces@xxxxxxxxxxxxx] On Behalf Of Martin Mathieson
Sent: den 26 april 2007 16:56
To: Developer support list for Wireshark
Subject: Re: [Wireshark-dev] [Wireshark-commits]rev21556:/trunk/epan//trunk/epan/: proto.c proto.h -allbuildbots rednow :-(

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
> >
>
>
>
_______________________________________________
Wireshark-dev mailing list
Wireshark-dev@xxxxxxxxxxxxx
http://www.wireshark.org/mailman/listinfo/wireshark-dev