["Anders Broman \(AL/EAB\)" <anders.broman@xxxxxxxxxxxx>]

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