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
