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 <[EMAIL PROTECTED]> 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: [EMAIL PROTECTED] > [mailto:[EMAIL PROTECTED] 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 <[EMAIL PROTECTED]> wrote: > > > > > > -----Ursprungligt meddelande----- > > Från: [EMAIL PROTECTED] > > [mailto:[EMAIL PROTECTED] 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 > > [email protected] > > http://www.wireshark.org/mailman/listinfo/wireshark-dev > > > > _______________________________________________ > > Wireshark-dev mailing list > > [email protected] > > http://www.wireshark.org/mailman/listinfo/wireshark-dev > > > _______________________________________________ > Wireshark-dev mailing list > [email protected] > http://www.wireshark.org/mailman/listinfo/wireshark-dev > > _______________________________________________ > Wireshark-dev mailing list > [email protected] > http://www.wireshark.org/mailman/listinfo/wireshark-dev > _______________________________________________ Wireshark-dev mailing list [email protected] http://www.wireshark.org/mailman/listinfo/wireshark-dev
