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

["Anders Broman \(AL/EAB\)" <anders.broman@xxxxxxxxxxxx>]

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
>

<<winmail.dat>>