[prev in list] [next in list] [prev in thread] [next in thread]
List: wireshark-dev
Subject: [Wireshark-dev] New proto_add_bits function (Was: rev
From: "Anders Broman \(AL/EAB\)" <anders.broman () ericsson ! com>
Date: 2007-04-26 15:56:48
Message-ID: E48F3A0F80C4B642BF6A5FF3257DFBB901020BAF () esealmw107 ! eemea ! ericsson ! se
[Download RAW message or body]
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@wireshark.org \
[mailto:wireshark-dev-bounces@wireshark.org] 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@ericsson.com> 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@wireshark.org 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@ericsson.com> 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@wireshark.org
> > [mailto:wireshark-dev-bounces@wireshark.org] 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@telia.com> 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@wireshark.org
> > > [mailto:wireshark-dev-bounces@wireshark.org] 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@telia.com> wrote:
> > > >
> > > >
> > > > -----Ursprungligt meddelande-----
> > > > Från: wireshark-dev-bounces@wireshark.org
> > > > [mailto:wireshark-dev-bounces@wireshark.org] 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@wireshark.org
> > > > http://www.wireshark.org/mailman/listinfo/wireshark-dev
> > > >
> > > > _______________________________________________
> > > > Wireshark-dev mailing list
> > > > Wireshark-dev@wireshark.org
> > > > http://www.wireshark.org/mailman/listinfo/wireshark-dev
> > > >
> > > _______________________________________________
> > > Wireshark-dev mailing list
> > > Wireshark-dev@wireshark.org
> > > http://www.wireshark.org/mailman/listinfo/wireshark-dev
> > >
> > > _______________________________________________
> > > Wireshark-dev mailing list
> > > Wireshark-dev@wireshark.org
> > > http://www.wireshark.org/mailman/listinfo/wireshark-dev
> > >
> > _______________________________________________
> > Wireshark-dev mailing list
> > Wireshark-dev@wireshark.org
> > http://www.wireshark.org/mailman/listinfo/wireshark-dev
> > _______________________________________________
> > Wireshark-dev mailing list
> > Wireshark-dev@wireshark.org
> > http://www.wireshark.org/mailman/listinfo/wireshark-dev
> >
>
>
>
_______________________________________________
Wireshark-dev mailing list
Wireshark-dev@wireshark.org
http://www.wireshark.org/mailman/listinfo/wireshark-dev
_______________________________________________
Wireshark-dev mailing list
Wireshark-dev@wireshark.org
http://www.wireshark.org/mailman/listinfo/wireshark-dev
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic