[bugzilla-daemon@xxxxxxxxxxxxx]

https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=4478

--- Comment #6 from Vincent Pelletier <plr.vincent@xxxxxxxxx> 2011-01-04 04:08:22 PST ---
(In reply to comment #5)
> And looking back at my examples above for LE/be, I guess I really didn't get it
> right either as I skipped bits before reconstructing the LE value and then
> shifting as needed.  I think the following for LE/be is correct though (but
> feel free to check it to be sure):
> 
> - file data
>   0x12 0x34 0x56 0x78 0x9A
>  - bit_offset, no_of_bits = result // comments
>    0, 32 = 0x78563412 // regular conversion of 4 first bytes
>    4, 24 = 0x00856341 // Take 24 bits, skipping the 1st 4 bits of the 1st byte
>    4, 32 = 0xA7856341 // Take 32 bits, skipping the 1st 4 bits of the 1st byte

There is one remaining difference between what I wrote initially and those 2
last cases.
Achieving "4, 24" in the example you gave can be decomposed into:
  0, 32 = 0x78563412 // initial state
  0, 28 = 0x08563412 // mask out LSB's high nibble
  4, 24 = 0x00856341 // y >> 4 ("y" being the result of "0, 28")
To reach "4, 24", it's a simple 4-bits right shift on the result of "0, 28".
But if the offset is expressed in file order, it requires more manipulations
based on "0, 28" result and duplicates bitmask's length used in tvb_get_bits*:
  0, 24 = 0x00856342 // (y & 0xF) | ((y & 0xFFFFF) >> 4)

About the leading 0x7 in the "4, 24" case I wrote originally, it's just a
mistake on my side (32 - 4 = 24 + 4, so I had to remove that nibble too to
honor the 24 bits length).

> So ... is this what your patch implements?

It is what I would like it to implement. It seems (from comment 2) that I found
a problem, and I don't think I fixed it nor remember what the problem was. I
think I was too lazy to write a stripped-down test code with my own sample data
set, and I should fix that at least. I'll try to write a python mockup this
evening (I'm nowadays much more fluent in python than in C...) to check my
implementation.

> Another idea might be to use a bitmask, which would also allow for
> non-consecutive bits to be obtained as a single value.  For example (here I
> assume BE/be):
> - file data
>   0x12 0x34 0x56 0x78 0x9A
> - byte_width, mask // comments
>   4, 0xffffffff = 0x78563412 // take all 32 bits of the 1st 4 bytes
>   4, 0x0ffffff0 = 0x00234567 // Take 24 bits, skipping the 1st 4 bits
>   5, 0x0ffffffff0 = 0x23456789 // Take 32 bits, skipping the 1st 4 bits

I feel there is something wrong with using 0 LSb in masks, as it duplicates in
my view the offset parameter (instead, shift mask right once and increase
offset by one until mask has its LSb to 1). I feel this should be dealt with
when writing a dissector rather than at run-time. Having a value which must (or
"should, to be efficient") follow some rules triggers a warning in my brain.
I'm not used to the kind of beasts which can be found in protocol world, so I
might just be wrong.

> So far that didn't buy us much because I used masks to show how you could get
> the same thing as with bit widths, but what if you wanted to get
> non-consecutive bits, like every other bit (just as an example):
> - byte_width, mask // comments
>   4, 0xaaaaaaaa = 0x1416 // take 16 bits (every other one) of the 1st 4 bytes

This triggers another warning in my brain, as this "prevents" dissector from
accessing the 0x55555555 bits. I feel such feature can, if needed, be
implemented separately from bit field function. But the same disclaimer as
above applies here, I really don't know so much about protocol dissecting
needs.

-- 
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.