[PATCH 1/2] Fix bug in uplink header type 1 parsing

Holger Freyther holger at freyther.de
Thu Apr 7 22:19:03 UTC 2016


> On 07 Apr 2016, at 13:57, Bhargava Abhyankar <Bhargava.Abhyankar at radisys.com> wrote:
> 
> Fix bug in extraction of E and TI header fields in parsing of uplink
> header type 1 RLC data block. Test suite for the same is also updated.
> 

> -	e_ti_header = (data[offs-1] + (data[offs] << 8)) >> 7;
> +	e_ti_header = (data[offs - 1] & (0xc0)) >> 6;

Could you please elaborate about this hunk? What is the issue, how was it found, how are we certain that this is the fix?


> -	rlc->block_info[0].e   = !!(e_ti_header & 0x01);
> -	rlc->block_info[0].ti  = !!(e_ti_header & 0x02);
> +	rlc->block_info[0].e   = (e_ti_header & 0x01);
> +	rlc->block_info[0].ti  = (e_ti_header & 0x02);

what is the issue with this? The intention of !! is to normalize the value to 0 and 1. Now .ti can have 0x02 and 0x00 as value. Why is that needed?


> 	cur_bit += 2;
> 
> 	rlc->block_info[1].cv  = egprs1->cv;
> @@ -524,20 +524,21 @@ int Decoding::rlc_parse_ul_data_header_egprs_type_1(
> 		((egprs1->bsn2_a << 0) | (egprs1->bsn2_b << 2));
> 	rlc->block_info[1].bsn = rlc->block_info[1].bsn &  (RLC_EGPRS_SNS - 1);
> 
> -	cur_bit += rlc->data_offs_bits[1] - 2;
> +	cur_bit = rlc->data_offs_bits[1] - 2;

Why don't we need to advance this cursor anymore?

	offs = rlc->data_offs_bits[1] / 8;
> 	OSMO_ASSERT(rlc->data_offs_bits[1] % 8 == 2);
> 
> -	e_ti_header = (data[offs-1] + (data[offs] << 8)) >> 7;
> +	e_ti_header = (data[offs] & (0x03));

What is the misunderstanding here. How did we think we needed to take bits from two different octets?


> -	rlc->block_info[1].e   = !!(e_ti_header & 0x01);
> -	rlc->block_info[1].ti  = !!(e_ti_header & 0x02);
> +	rlc->block_info[1].e   = (e_ti_header & 0x01);
> +	rlc->block_info[1].ti  = (e_ti_header & 0x02);

Again, why the change from 0 or 1 to .. 0 or 1 .. and 0 or 2?


> 

> diff --git a/tests/edge/EdgeTest.cpp b/tests/edge/EdgeTest.cpp
> index ff080f9..9b4a1a5 100644
> --- a/tests/edge/EdgeTest.cpp
> +++ b/tests/edge/EdgeTest.cpp
> @@ -1376,18 +1376,16 @@ static gprs_rlcmac_ul_tbf *uplink_header_parsing_test(BTS *the_bts,
> 	egprs1->r = 1;
> 	egprs1->cv = 7;
> 	egprs1->tfi_a = tfi & (~((~0) << 2));
> -	egprs1->tfi_b = tfi & (~((~0) << 3)) << 2;
> -	egprs1->bsn1_a = 10;
> -	egprs1->bsn1_b = 17;
> -	egprs1->bsn2_a = 0;
> -	egprs1->bsn2_b = 25;
> +	egprs1->tfi_b = (tfi & (~((~0) << 3)) << 2) >> 2;
> +	egprs1->bsn1_a = 0;
> +	egprs1->bsn1_b = 0;
> +	egprs1->bsn2_a = 1;
> +	egprs1->bsn2_b = 0;
> 	egprs1->cps = 15;
> 	egprs1->rsb = 0;
> 	egprs1->pi = 0;
> -	data[6] = 1;
> -	data[6 + 68] = 1;
> -	data[75] = 1;
> -	data[75 + 68] = 1;
> +	data[5] = 0x40;
> +	data[5 + 69] = 1;


Please explain the intention here. What is the issue with tfi_b? Why do you change it. Why do you pick the new bsn1_a and bsn1_b values?

holger



More information about the osmocom-net-gprs mailing list