["Martin Mathieson" <martin.r.mathieson@xxxxxxxxxxxxxx>]

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
Index: epan/proto.c
===================================================================
--- epan/proto.c	(revision 21595)
+++ epan/proto.c	(working copy)
@@ -5636,22 +5636,25 @@
 		/* One or 2 bytes */
 		if(length == 1){
 			/* Spans 1 byte */
-			mask8 = mask8>>(bit_offset&0x7);
-			value = tvb_get_guint8(tvb,offset)&mask8;
+			mask8 = mask8>>(8-no_of_bits);
+			value = (tvb_get_guint8(tvb,offset) >>
+			        ((8-((bit_offset&0x7)+no_of_bits))) & mask8);
 			mask = 0x80;
 		}else{
 			/* Spans 2 bytes */	
-			mask16 = mask16>>(bit_offset&0x7);
+			mask16 = mask16>>(16-no_of_bits);
 			if(little_endian){
 				value=tvb_get_letohs(tvb, offset);
 			} else {
-				value=tvb_get_ntohs(tvb, offset);
+				value=(tvb_get_ntohs(tvb, offset) >>
+			          ((16-((bit_offset&0x7)+no_of_bits))) & mask16);
 			}
 			mask = 0x8000;
 		}
+
+		/* Make mask start at first bit of shifted value */
 		shift = 8-((bit_offset + no_of_bits)&0x7);
 		if (shift<8){
-			value = value >> shift;
 			mask = mask >> shift;
 		}
 		
@@ -5823,4 +5826,4 @@
 		;
 	}
 
-}
\ No newline at end of file
+}