libdvbv5: more fixes in the T2 delivery descriptor handler

PATCH In review Nikolaus Schulz 3 participants 5 messages Dec 5, 2013 – Dec 19, 2013 Media (V4L/DVB)

Share & tools

Nikolaus Schulz

author

2013-12-05 15:15 UTC #0 lore raw

PATCH 1/1 libdvbv5: more fixes in the T2 delivery descriptor handler

* Fix a couple of memcpy calls, and remove a bogus one
* Properly use lengths of centre_frequency loop and subcell_info loop (they count bytes, not entries)

Signed-off-by Nikolaus Schulz <[email protected]>

---
 lib/libdvbv5/descriptors/desc_t2_delivery.c |   35 ++++++++++++++-------------
 1 files changed, 18 insertions(+), 17 deletions(-)

lib/libdvbv5/descriptors/desc_t2_delivery.c +18 ~~-17~~

index ab4361d..07a0956 100644
@@ -32,6 +32,7 @@ void dvb_desc_t2_delivery_init(struct dvb_v5_fe_parms *parms,
 	struct dvb_desc_t2_delivery *d = desc;
 	unsigned char *p = (unsigned char *) buf;
 	size_t desc_len = ext->length - 1, len, len2;
+	uint8_t nmemb;
 	int i;
 
 	len = offsetof(struct dvb_desc_t2_delivery, bitfield);

@@ -42,7 +43,7 @@ void dvb_desc_t2_delivery_init(struct dvb_v5_fe_parms *parms,
 		return;
 	}
 	if (desc_len < len2) {
-		memcpy(p, buf, len);
+		memcpy(d, p, len);
 		bswap16(d->system_id);
 
 		if (desc_len != len)

@@ -50,44 +51,41 @@ void dvb_desc_t2_delivery_init(struct dvb_v5_fe_parms *parms,
 
 		return;
 	}
-	memcpy(p, buf, len2);
+	memcpy(d, p, len2);
 	p += len2;
 
-	len = desc_len - (p - buf);
-	memcpy(&d->centre_frequency, p, len);
-	p += len;
-
 	if (d->tfs_flag)
-		d->frequency_loop_length = 1;
+		d->frequency_loop_length = sizeof(*d->centre_frequency);
 	else {
 		d->frequency_loop_length = *p;
 		p++;
 	}
+	nmemb = d->frequency_loop_length / sizeof(*d->centre_frequency);
 
-	d->centre_frequency = calloc(d->frequency_loop_length,
-				     sizeof(*d->centre_frequency));
+	d->centre_frequency = calloc(nmemb, sizeof(*d->centre_frequency));
 	if (!d->centre_frequency) {
 		dvb_perror("Out of memory");
 		return;
 	}
 
-	memcpy(d->centre_frequency, p, sizeof(*d->centre_frequency) * d->frequency_loop_length);
-	p += sizeof(*d->centre_frequency) * d->frequency_loop_length;
+	memcpy(d->centre_frequency, p, d->frequency_loop_length);
+	p += d->frequency_loop_length;
 
-	for (i = 0; i < d->frequency_loop_length; i++)
+	for (i = 0; i < nmemb; i++)
 		bswap32(d->centre_frequency[i]);
 
 	d->subcel_info_loop_length = *p;
 	p++;
+	nmemb = d->subcel_info_loop_length / sizeof(*d->subcell);
 
-	d->subcell = calloc(d->subcel_info_loop_length, sizeof(*d->subcell));
+	d->subcell = calloc(nmemb, sizeof(*d->subcell));
 	if (!d->subcell) {
 		dvb_perror("Out of memory");
 		return;
 	}
-	memcpy(d->subcell, p, sizeof(*d->subcell) * d->subcel_info_loop_length);
+	memcpy(d->subcell, p, d->subcel_info_loop_length);
 
-	for (i = 0; i < d->subcel_info_loop_length; i++)
+	for (i = 0; i < nmemb; i++)
 		bswap16(d->subcell[i].transposer_frequency);
 }

@@ -97,6 +95,7 @@ void dvb_desc_t2_delivery_print(struct dvb_v5_fe_parms *parms,
 {
 	const struct dvb_desc_t2_delivery *d = desc;
 	int i;
+	uint8_t nmemb;
 
 	dvb_log("|       DVB-T2 delivery");
 	dvb_log("|           plp_id                    %d", d->plp_id);

@@ -113,10 +112,12 @@ void dvb_desc_t2_delivery_print(struct dvb_v5_fe_parms *parms,
 	dvb_log("|           bandwidth                 %d", d->bandwidth);
 	dvb_log("|           SISO MISO                 %d", d->SISO_MISO);
 
-	for (i = 0; i < d->frequency_loop_length; i++)
+	nmemb = d->frequency_loop_length / sizeof(*d->centre_frequency);
+	for (i = 0; i < nmemb; i++)
 		dvb_log("|           centre frequency[%d]   %d", i, d->centre_frequency[i]);
 
-	for (i = 0; i < d->subcel_info_loop_length; i++) {
+	nmemb = d->subcel_info_loop_length / sizeof(*d->subcell);
+	for (i = 0; i < nmemb; i++) {
 		dvb_log("|           cell_id_extension[%d]  %d", i, d->subcell[i].cell_id_extension);
 		dvb_log("|           transposer frequency   %d", d->subcell[i].transposer_frequency);
 	}

Signature · 1 line

1.7.2.5

Mauro Carvalho Chehab

2013-12-18 12:49 UTC #1 lore raw

Thanks for the patch!

Em Thu, 5 Dec 2013 16:10:03 +0100
Nikolaus Schulz <[email protected]> escreveu:

Quoted 1 line

* Fix a couple of memcpy calls, and remove a bogus one

That changes look OK to me.

Quoted 2 lines

* Properly use lengths of centre_frequency loop and subcell_info loop (they count bytes, not entries)

I'm not sure about this one. On all other similar descriptors, we're storing the number of elements at the descriptor's structure, instead of its size, as that makes easier to handle them.

Also, having all delivery_system descriptors using the number of elements makes easier to avoid confusion when writing the handling code.

Btw, I think that this change will likely break the code that handles this descriptor at dvb-scan.

So, could you please redo your patch in order for it to keep storing the number of elements at the dvb_desc_t2_delivery::frequency_loop_length field? If you prefer, you can rename it to frequency_loop_nmemb (or a similar naming) to make it clearer.

Regards,
Mauro

Quoted 106 lines

Signed-off-by Nikolaus Schulz <[email protected]>

---
 lib/libdvbv5/descriptors/desc_t2_delivery.c |   35 ++++++++++++++-------------
 1 files changed, 18 insertions(+), 17 deletions(-)

lib/libdvbv5/descriptors/desc_t2_delivery.c +18 ~~-17~~

index ab4361d..07a0956 100644
@@ -32,6 +32,7 @@ void dvb_desc_t2_delivery_init(struct dvb_v5_fe_parms *parms,
 	struct dvb_desc_t2_delivery *d = desc;
 	unsigned char *p = (unsigned char *) buf;
 	size_t desc_len = ext->length - 1, len, len2;
+	uint8_t nmemb;
 	int i;
 
 	len = offsetof(struct dvb_desc_t2_delivery, bitfield);

@@ -42,7 +43,7 @@ void dvb_desc_t2_delivery_init(struct dvb_v5_fe_parms *parms,
 		return;
 	}
 	if (desc_len < len2) {
-		memcpy(p, buf, len);
+		memcpy(d, p, len);
 		bswap16(d->system_id);
 
 		if (desc_len != len)

@@ -50,44 +51,41 @@ void dvb_desc_t2_delivery_init(struct dvb_v5_fe_parms *parms,
 
 		return;
 	}
-	memcpy(p, buf, len2);
+	memcpy(d, p, len2);
 	p += len2;
 
-	len = desc_len - (p - buf);
-	memcpy(&d->centre_frequency, p, len);
-	p += len;
-
 	if (d->tfs_flag)
-		d->frequency_loop_length = 1;
+		d->frequency_loop_length = sizeof(*d->centre_frequency);
 	else {
 		d->frequency_loop_length = *p;
 		p++;
 	}
+	nmemb = d->frequency_loop_length / sizeof(*d->centre_frequency);
 
-	d->centre_frequency = calloc(d->frequency_loop_length,
-				     sizeof(*d->centre_frequency));
+	d->centre_frequency = calloc(nmemb, sizeof(*d->centre_frequency));
 	if (!d->centre_frequency) {
 		dvb_perror("Out of memory");
 		return;
 	}
 
-	memcpy(d->centre_frequency, p, sizeof(*d->centre_frequency) * d->frequency_loop_length);
-	p += sizeof(*d->centre_frequency) * d->frequency_loop_length;
+	memcpy(d->centre_frequency, p, d->frequency_loop_length);
+	p += d->frequency_loop_length;
 
-	for (i = 0; i < d->frequency_loop_length; i++)
+	for (i = 0; i < nmemb; i++)
 		bswap32(d->centre_frequency[i]);
 
 	d->subcel_info_loop_length = *p;
 	p++;
+	nmemb = d->subcel_info_loop_length / sizeof(*d->subcell);
 
-	d->subcell = calloc(d->subcel_info_loop_length, sizeof(*d->subcell));
+	d->subcell = calloc(nmemb, sizeof(*d->subcell));
 	if (!d->subcell) {
 		dvb_perror("Out of memory");
 		return;
 	}
-	memcpy(d->subcell, p, sizeof(*d->subcell) * d->subcel_info_loop_length);
+	memcpy(d->subcell, p, d->subcel_info_loop_length);
 
-	for (i = 0; i < d->subcel_info_loop_length; i++)
+	for (i = 0; i < nmemb; i++)
 		bswap16(d->subcell[i].transposer_frequency);
 }

@@ -97,6 +95,7 @@ void dvb_desc_t2_delivery_print(struct dvb_v5_fe_parms *parms,
 {
 	const struct dvb_desc_t2_delivery *d = desc;
 	int i;
+	uint8_t nmemb;
 
 	dvb_log("|       DVB-T2 delivery");
 	dvb_log("|           plp_id                    %d", d->plp_id);

@@ -113,10 +112,12 @@ void dvb_desc_t2_delivery_print(struct dvb_v5_fe_parms *parms,
 	dvb_log("|           bandwidth                 %d", d->bandwidth);
 	dvb_log("|           SISO MISO                 %d", d->SISO_MISO);
 
-	for (i = 0; i < d->frequency_loop_length; i++)
+	nmemb = d->frequency_loop_length / sizeof(*d->centre_frequency);
+	for (i = 0; i < nmemb; i++)
 		dvb_log("|           centre frequency[%d]   %d", i, d->centre_frequency[i]);
 
-	for (i = 0; i < d->subcel_info_loop_length; i++) {
+	nmemb = d->subcel_info_loop_length / sizeof(*d->subcell);
+	for (i = 0; i < nmemb; i++) {
 		dvb_log("|           cell_id_extension[%d]  %d", i, d->subcell[i].cell_id_extension);
 		dvb_log("|           transposer frequency   %d", d->subcell[i].transposer_frequency);
 	}

Signature · 2 lines

Cheers,
Mauro

Nikolaus Schulz

author

2013-12-19 02:38 UTC #2 lore raw

Hi Mauro,

I didn't have the time yet to look at your comments and that patch again, but:

Please note that my email address <[email protected]> is shut down as of today[1].

Please use my alternate address, <[email protected]>, for any further replies or followups.

Maybe Patchwork[2] should also be updated accordingly?

Thanks,
Nikolaus

[1] I don't know if the local admin will disable the old email address <[email protected]> in time, but I definitely will no longer read messages sent to it, from now on.
[2] https://patchwork.linuxtv.org/patch/20943/

Mauro Carvalho Chehab

2013-12-19 11:30 UTC #3 lore raw

Em Thu, 19 Dec 2013 03:37:48 +0100
Nikolaus Schulz <[email protected]> escreveu:

Quoted 10 lines

Hi Mauro,
I didn't have the time yet to look at your comments and that patch again, but:
Please note that my email address <[email protected]> is shut down as of today[1].
Please use my alternate address, <[email protected]>, for any further replies or followups.

Ok. Well, if anyone replies to the thread, it will to to both addresses, so it should reach you anyway.

Quoted 1 line

Maybe Patchwork[2] should also be updated accordingly?

I think that you'll need to create an account at patchwork, in order to add your new address. Not sure if it allows you to delete the old address through.

Quoted 8 lines

Thanks,
Nikolaus
[1] I don't know if the local admin will disable the old email address <[email protected]> in time, but I definitely will no longer read messages sent to it, from now on.
[2] https://patchwork.linuxtv.org/patch/20943/

Signature · 2 lines

Cheers,
Mauro

Nikolaus Schulz

2013-12-19 21:46 UTC #4 lore raw

In reply to Mauro Carvalho Chehab

Quoted 6 lines · Mauro Carvalho Chehab

Quoted 2 lines
* Properly use lengths of centre_frequency loop and subcell_info loop (they count bytes, not entries)
I'm not sure about this one. On all other similar descriptors, we're storing the number of elements at the descriptor's structure, instead of its size, as that makes easier to handle them.

OK, I had been looking into other descriptor handlers and didn't spot anything like that, but isdbt_desc_terrestrial_delivery_system.num_freqs at least indeed does count array members.

Quoted 2 lines

Also, having all delivery_system descriptors using the number of elements makes easier to avoid confusion when writing the handling code.

I agree, it also is way more natural. Actually I felt that from the start, but considering that the structs in the code very closely correlate to the binary DVBSI format, I adopted the byte count accordingly.

Quoted 2 lines

Btw, I think that this change will likely break the code that handles this descriptor at dvb-scan.

Indeed, add_update_nit_dvbt2() would be affected.

Quoted 4 lines

So, could you please redo your patch in order for it to keep storing the number of elements at the dvb_desc_t2_delivery::frequency_loop_length field? If you prefer, you can rename it to frequency_loop_nmemb (or a similar naming) to make it clearer.

I'll re-send it using frequency_loop_nmemb and subcel_info_loop_nmemb, in order to avoid confusion with the {frequency,subcell_info}_loop_length fields in the binary DVBSI format.

Thanks,
Nikolaus

Quoted 105 lines

Signed-off-by Nikolaus Schulz <[email protected]>

---
 lib/libdvbv5/descriptors/desc_t2_delivery.c |   35 ++++++++++++++-------------
 1 files changed, 18 insertions(+), 17 deletions(-)

lib/libdvbv5/descriptors/desc_t2_delivery.c +18 ~~-17~~

index ab4361d..07a0956 100644
@@ -32,6 +32,7 @@ void dvb_desc_t2_delivery_init(struct dvb_v5_fe_parms *parms,
 	struct dvb_desc_t2_delivery *d = desc;
 	unsigned char *p = (unsigned char *) buf;
 	size_t desc_len = ext->length - 1, len, len2;
+	uint8_t nmemb;
 	int i;
 
 	len = offsetof(struct dvb_desc_t2_delivery, bitfield);

@@ -42,7 +43,7 @@ void dvb_desc_t2_delivery_init(struct dvb_v5_fe_parms *parms,
 		return;
 	}
 	if (desc_len < len2) {
-		memcpy(p, buf, len);
+		memcpy(d, p, len);
 		bswap16(d->system_id);
 
 		if (desc_len != len)

@@ -50,44 +51,41 @@ void dvb_desc_t2_delivery_init(struct dvb_v5_fe_parms *parms,
 
 		return;
 	}
-	memcpy(p, buf, len2);
+	memcpy(d, p, len2);
 	p += len2;
 
-	len = desc_len - (p - buf);
-	memcpy(&d->centre_frequency, p, len);
-	p += len;
-
 	if (d->tfs_flag)
-		d->frequency_loop_length = 1;
+		d->frequency_loop_length = sizeof(*d->centre_frequency);
 	else {
 		d->frequency_loop_length = *p;
 		p++;
 	}
+	nmemb = d->frequency_loop_length / sizeof(*d->centre_frequency);
 
-	d->centre_frequency = calloc(d->frequency_loop_length,
-				     sizeof(*d->centre_frequency));
+	d->centre_frequency = calloc(nmemb, sizeof(*d->centre_frequency));
 	if (!d->centre_frequency) {
 		dvb_perror("Out of memory");
 		return;
 	}
 
-	memcpy(d->centre_frequency, p, sizeof(*d->centre_frequency) * d->frequency_loop_length);
-	p += sizeof(*d->centre_frequency) * d->frequency_loop_length;
+	memcpy(d->centre_frequency, p, d->frequency_loop_length);
+	p += d->frequency_loop_length;
 
-	for (i = 0; i < d->frequency_loop_length; i++)
+	for (i = 0; i < nmemb; i++)
 		bswap32(d->centre_frequency[i]);
 
 	d->subcel_info_loop_length = *p;
 	p++;
+	nmemb = d->subcel_info_loop_length / sizeof(*d->subcell);
 
-	d->subcell = calloc(d->subcel_info_loop_length, sizeof(*d->subcell));
+	d->subcell = calloc(nmemb, sizeof(*d->subcell));
 	if (!d->subcell) {
 		dvb_perror("Out of memory");
 		return;
 	}
-	memcpy(d->subcell, p, sizeof(*d->subcell) * d->subcel_info_loop_length);
+	memcpy(d->subcell, p, d->subcel_info_loop_length);
 
-	for (i = 0; i < d->subcel_info_loop_length; i++)
+	for (i = 0; i < nmemb; i++)
 		bswap16(d->subcell[i].transposer_frequency);
 }

@@ -97,6 +95,7 @@ void dvb_desc_t2_delivery_print(struct dvb_v5_fe_parms *parms,
 {
 	const struct dvb_desc_t2_delivery *d = desc;
 	int i;
+	uint8_t nmemb;
 
 	dvb_log("|       DVB-T2 delivery");
 	dvb_log("|           plp_id                    %d", d->plp_id);

@@ -113,10 +112,12 @@ void dvb_desc_t2_delivery_print(struct dvb_v5_fe_parms *parms,
 	dvb_log("|           bandwidth                 %d", d->bandwidth);
 	dvb_log("|           SISO MISO                 %d", d->SISO_MISO);
 
-	for (i = 0; i < d->frequency_loop_length; i++)
+	nmemb = d->frequency_loop_length / sizeof(*d->centre_frequency);
+	for (i = 0; i < nmemb; i++)
 		dvb_log("|           centre frequency[%d]   %d", i, d->centre_frequency[i]);
 
-	for (i = 0; i < d->subcel_info_loop_length; i++) {
+	nmemb = d->subcel_info_loop_length / sizeof(*d->subcell);
+	for (i = 0; i < nmemb; i++) {
 		dvb_log("|           cell_id_extension[%d]  %d", i, d->subcell[i].cell_id_extension);
 		dvb_log("|           transposer frequency   %d", d->subcell[i].transposer_frequency);
 	}