IDE-TAPE NULL terminate strings.

PATCH Applied Mark de Wever 4 participants 26 messages Sep 21, 2008 – Oct 8, 2008 libata

Share & tools

Mark de Wever

author

2008-09-21 19:06 UTC #0 lore raw

PATCH 1/1 IDE-TAPE NULL terminate strings.

After updating my kernel to 2.6.26 the output for the ide-tape drive during booting is garbled eg
ide-tape: hdd <-> ht0: Seagate <98>ß8A51|1À<81>Üº<98>ß STT20000A rev 8A51|1À<81>Üº<98>ß

This patch fixes the problem by NULL terminating the strings.

Regards,
Mark de Wever

PS: please CC me since I'm not subscribed.
PPS: there are more problems with my tapestreamer in 2.6.26 but I'll post a separate message for that.

Signed-off-by Mark de Wever <[email protected]>

---
 drivers/ide/ide-tape.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

drivers/ide/ide-tape.c +4 -0

index 1bce84b..fd87b43 100644
@@ -2354,6 +2354,10 @@ static void idetape_get_inquiry_results(ide_drive_t *drive)
 	ide_fixstring(product_id, 18, 0);
 	ide_fixstring(fw_rev, 6, 0);
 
+	fw_rev[4] = '\0';
+	vendor_id[8] = '\0';
+	product_id[16] = '\0';
+
 	printk(KERN_INFO "ide-tape: %s <-> %s: %s %s rev %s\n",
 			drive->name, tape->name, vendor_id, product_id, fw_rev);
 }

Signature · 1 line

1.5.6.5

Sergei Shtylyov

2008-09-21 19:23 UTC #1 lore raw

Hello.

Quoted 20 lines · Mark de Wever

After updating my kernel to 2.6.26 the output for the ide-tape drive during booting is garbled eg
ide-tape: hdd <-> ht0: Seagate <98>ß8A51|1À<81>Üº<98>ß STT20000A rev 8A51|1À<81>Üº<98>ß
This patch fixes the problem by NULL terminating the strings.
Regards,
Mark de Wever
PS: please CC me since I'm not subscribed.
PPS: there are more problems with my tapestreamer in 2.6.26 but I'll post a separate message for that.
Signed-off-by Mark de Wever <[email protected]>
drivers/ide/ide-tape.c +0 -0
index 1bce84b..fd87b43 100644

drivers/ide/ide-tape.c static void idetape_get_inquiry_results(ide_drive_t *drive) · line 2362

Quoted 9 lines

drivers/ide/ide-tape.c +4 -0

@@ -2354,6 +2354,10 @@ static void idetape_get_inquiry_results(ide_drive_t *drive)
 	ide_fixstring(product_id, 18, 0);
 	ide_fixstring(fw_rev, 6, 0);
 
+	fw_rev[4] = '\0';
+	vendor_id[8] = '\0';
+	product_id[16] = '\0';
+
 	printk(KERN_INFO "ide-tape: %s <-> %s: %s %s rev %s\n",
 			drive->name, tape->name, vendor_id, product_id, fw_rev);

This is not quite correct way to tackle this. More correct would be to
fix the format specifiers in this printk() to only print no more than N string characters like this:

printk(KERN_INFO "ide-tape: %s <-> %s: %.8s %.16s rev %.4s\n",
drive->name, tape->name, vendor_id, product_id, fw_rev);

MBR, Sergei

Sergei Shtylyov

2008-09-21 20:07 UTC #2 lore raw

Quoted 15 lines · Hello, I wrote:

Quoted 14 lines
After updating my kernel to 2.6.26 the output for the ide-tape drive during booting is garbled eg
ide-tape: hdd <-> ht0: Seagate <98>ß8A51|1À<81>Üº<98>ß STT20000A rev 8A51|1À<81>Üº<98>ß
This patch fixes the problem by NULL terminating the strings.
Regards,
Mark de Wever
PS: please CC me since I'm not subscribed.
PPS: there are more problems with my tapestreamer in 2.6.26 but I'll post a separate message for that.
Signed-off-by Mark de Wever <[email protected]>

Quoted 8 lines

drivers/ide/ide-tape.c static void · line 2356

Quoted 8 lines

drivers/ide/ide-tape.c +0 -0

index 1bce84b..fd87b43 100644
@@ -2354,6 +2354,10 @@ static void 
 idetape_get_inquiry_results(ide_drive_t *drive)
     ide_fixstring(product_id, 18, 0);
     ide_fixstring(fw_rev, 6, 0);

Hm, I see that every string variable declared there has 2 extra
characters, and yet the author have managed to make a mistake... these extra chars don't seem needed.

MBR, Sergei

Mark de Wever

author

2008-09-21 20:29 UTC #3 lore raw

Quoted 4 lines · Sergei Shtylyov

Quoted 4 lines
Quoted 4 lines
drivers/ide/ide-tape.c +0 -0
index 1bce84b..fd87b43 100644

Quoted 8 lines

Quoted 4 lines
drivers/ide/ide-tape.c static void · line 2356
Quoted 4 lines
drivers/ide/ide-tape.c +0 -0
@@ -2354,6 +2354,10 @@ static void  
 idetape_get_inquiry_results(ide_drive_t *drive)
     ide_fixstring(product_id, 18, 0);
     ide_fixstring(fw_rev, 6, 0);
Hm, I see that every string variable declared there has 2 extra
characters, and yet the author have managed to make a mistake... these extra chars don't seem needed.

Those extra characters made me believe the intention was setting the NULL character, therefore I used that solution in my patch.

Regards,
Mark de Wever

Sergei Shtylyov

2008-09-21 22:12 UTC #4 lore raw

Hello.

Quoted 13 lines · Mark de Wever

Quoted 12 lines
Quoted 9 lines
drivers/ide/ide-tape.c static void · line 2357
Quoted 9 lines
drivers/ide/ide-tape.c +0 -0
index 1bce84b..fd87b43 100644
@@ -2354,6 +2354,10 @@ static void  
 idetape_get_inquiry_results(ide_drive_t *drive)
     ide_fixstring(product_id, 18, 0);
     ide_fixstring(fw_rev, 6, 0);
 
Hm, I see that every string variable declared there has 2 extra
characters, and yet the author have managed to make a mistake... these extra chars don't seem needed.

Quoted 3 lines

Those extra characters made me believe the intention was setting the NULL character, therefore I used that solution in my patch.

It was wrong to call ide_fixstring() on those "long" stings without
first setting their last byte to 0 -- this way that function just copied the garbage at the end of the passed unterminated strings. I suggest that you get rid of those extra bytes as well, and use the "%.<n>s" format specifiers.

Quoted 3 lines

Regards,
Mark de Wever

MBR, Sergei

Sergei Shtylyov

2008-09-22 13:15 UTC #5 lore raw

In reply to Mark de Wever

Hello.

Quoted 5 lines · Mark de Wever

After updating my kernel to 2.6.26 the output for the ide-tape drive during booting is garbled eg
ide-tape: hdd <-> ht0: Seagate <98>ß8A51|1À<81>Üº<98>ß STT20000A rev 8A51|1À<81>Üº<98>ß
This patch fixes the problem by NULL terminating the strings.

Looks like this bugs was introduced by this commit:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=41f81d545b6b1f585a02d1d8545978714f710e91

MBR, Sergei

Boris Petkov

2008-09-22 13:56 UTC #6 lore raw

Quoted 9 lines · Sergei Shtylyov

Hello.
Quoted 4 lines · Mark de Wever
After updating my kernel to 2.6.26 the output for the ide-tape drive during booting is garbled eg
ide-tape: hdd <-> ht0: Seagate <98>ß8A51|1À<81>Üº<98>ß STT20000A rev 8A51|1À<81>Üº<98>ß

Quoted 5 lines

Quoted 1 line
This patch fixes the problem by NULL terminating the strings.
Looks like this bugs was introduced by this commit:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=41f81d545b6b1f585a02d1d8545978714f710e91

.. and I know why :). Those ide_tape_obj members (char fw_rev[6], vendor_id[10], product_id[18]) were used only once in idetape_get_inquiry_results() so I moved them there as local stack variables. Originally, they were kzalloc'ed as part of struct ide_tape_obj and now they contain stack garbage therefore the funny values. The simple solution would be to zero them out or:

Does the following patch help?

Signed-off-by Borislav Petkov <[email protected]>

drivers/ide/ide-tape.c +0 -0

index 1bce84b..848d9df 100644
@@ -2338,7 +2338,7 @@ static void

idetape_get_inquiry_results(ide_drive_t *drive)
{
idetape_tape_t *tape = drive->driver_data; struct ide_atapi_pc pc;
- char fw_rev[6], vendor_id[10], product_id[18];
+ static char fw_rev[6], vendor_id[10], product_id[18];

idetape_create_inquiry_cmd(&pc);
if (idetape_queue_pc_tail(drive, &pc)) {

Signature · 2 lines

Regards/Gruss,
Boris

Mark de Wever

author

2008-09-22 20:43 UTC #7 lore raw

Quoted 4 lines · Boris Petkov

Quoted 2 lines · Sergei Shtylyov
Hello.

Quoted 8 lines

.. and I know why :). Those ide_tape_obj members (char fw_rev[6], vendor_id[10], product_id[18]) were used only once in idetape_get_inquiry_results() so I moved them there as local stack variables. Originally, they were kzalloc'ed as part of struct ide_tape_obj and now they contain stack garbage therefore the funny values. The simple solution would be to zero them out or:
Does the following patch help?

Yes feel free to add my tested-by.

Only not sure whether the static is the best solution, the following patch also works, by zeroing the memory as you suggested.

Signed-off-by Mark de Wever <[email protected]>

drivers/ide/ide-tape.c +1 -1

index 1bce84b..c41f5b1 100644
@@ -2338,7 +2338,7 @@ static void idetape_get_inquiry_results(ide_drive_t *drive)
 {
 	idetape_tape_t *tape = drive->driver_data;
 	struct ide_atapi_pc pc;
-	char fw_rev[6], vendor_id[10], product_id[18];
+	char fw_rev[6] = {'\0'}, vendor_id[10] = {'\0'}, product_id[18] = {'\0'};
 
 	idetape_create_inquiry_cmd(&pc);
 	if (idetape_queue_pc_tail(drive, &pc)) {

Signature · 2 lines

Regards,
Mark de Wever

Sergei Shtylyov

2008-09-22 21:08 UTC #8 lore raw

Hello.

Quoted 7 lines · Mark de Wever

Quoted 7 lines
.. and I know why :). Those ide_tape_obj members (char fw_rev[6], vendor_id[10], product_id[18]) were used only once in idetape_get_inquiry_results() so I moved them there as local stack variables. Originally, they were kzalloc'ed as part of struct ide_tape_obj and now they contain stack garbage therefore the funny values. The simple solution would be to zero them out or:

Quoted 5 lines

Quoted 2 lines
Does the following patch help?
Yes feel free to add my tested-by.

And my NAK too. :-)

Quoted 10 lines

Only not sure whether the static is the best solution, the following patch also works, by zeroing the memory as you suggested.
Signed-off-by Mark de Wever <[email protected]>
drivers/ide/ide-tape.c +0 -0
index 1bce84b..c41f5b1 100644

drivers/ide/ide-tape.c static void idetape_get_inquiry_results(ide_drive_t *drive) · line 2342

Quoted 6 lines

drivers/ide/ide-tape.c +1 -1

@@ -2338,7 +2338,7 @@ static void idetape_get_inquiry_results(ide_drive_t *drive)
 {
 	idetape_tape_t *tape = drive->driver_data;
 	struct ide_atapi_pc pc;
-	char fw_rev[6], vendor_id[10], product_id[18];
+	char fw_rev[6] = {'\0'}, vendor_id[10] = {'\0'}, product_id[18] = {'\0'};

Do you realize how much *absolutely unnecessary* code will this bring
in? This is certainly worse than your initial patch (if it was correct). Ugh, looks like I'll have t submit the patch myself to stop this ugliness...

MBR, Sergei

Borislav Petkov

2008-09-23 07:48 UTC #9 lore raw

Quoted 24 lines · Sergei Shtylyov

Hello.
Quoted 7 lines · Mark de Wever
Quoted 7 lines
.. and I know why :). Those ide_tape_obj members (char fw_rev[6], vendor_id[10], product_id[18]) were used only once in idetape_get_inquiry_results() so I moved them there as local stack variables. Originally, they were kzalloc'ed as part of struct ide_tape_obj and now they contain stack garbage therefore the funny values. The simple solution would be to zero them out or:
Quoted 5 lines
Quoted 2 lines
Does the following patch help?
Yes feel free to add my tested-by.
And my NAK too. :-)
Quoted 5 lines
Only not sure whether the static is the best solution, the following patch also works, by zeroing the memory as you suggested.
Signed-off-by Mark de Wever <[email protected]>

Quoted 14 lines

drivers/ide/ide-tape.c static void idetape_get_inquiry_results(ide_drive_t *drive) · line 2342
Quoted 11 lines
drivers/ide/ide-tape.c +1 -1
index 1bce84b..c41f5b1 100644
@@ -2338,7 +2338,7 @@ static void idetape_get_inquiry_results(ide_drive_t *drive)
 {
 	idetape_tape_t *tape = drive->driver_data;
 	struct ide_atapi_pc pc;
-	char fw_rev[6], vendor_id[10], product_id[18];
+	char fw_rev[6] = {'\0'}, vendor_id[10] = {'\0'}, product_id[18] = {'\0'};
 
Do you realize how much *absolutely unnecessary* code will this bring
in? This is certainly worse than your initial patch (if it was correct).

Yep, Sergei's right. Both of our patches are dumb.

Quoted 1 line

Ugh, looks like I'll have t submit the patch myself to stop this ugliness...

Is this what you had in mind?

---

drivers/ide/ide-tape.c +5 -5

index 1bce84b..3833189 100644
@@ -2338,7 +2338,7 @@ static void idetape_get_inquiry_results(ide_drive_t *drive)
 {
 	idetape_tape_t *tape = drive->driver_data;
 	struct ide_atapi_pc pc;
-	char fw_rev[6], vendor_id[10], product_id[18];
+	char fw_rev[4], vendor_id[8], product_id[16];
 
 	idetape_create_inquiry_cmd(&pc);
 	if (idetape_queue_pc_tail(drive, &pc)) {

@@ -2350,11 +2350,11 @@ static void idetape_get_inquiry_results(ide_drive_t *drive)
 	memcpy(product_id, &pc.buf[16], 16);
 	memcpy(fw_rev, &pc.buf[32], 4);
 
-	ide_fixstring(vendor_id, 10, 0);
-	ide_fixstring(product_id, 18, 0);
-	ide_fixstring(fw_rev, 6, 0);
+	ide_fixstring(vendor_id, 8, 0);
+	ide_fixstring(product_id, 16, 0);
+	ide_fixstring(fw_rev, 4, 0);
 
-	printk(KERN_INFO "ide-tape: %s <-> %s: %s %s rev %s\n",
+	printk(KERN_INFO "ide-tape: %s <-> %s: %.8s %.16s rev %.4s\n",
 			drive->name, tape->name, vendor_id, product_id, fw_rev);
 }

Signature · 2 lines

Regards/Gruss,
    Boris.

Sergei Shtylyov

2008-09-23 09:29 UTC #10 lore raw

Hello.

Quoted 21 lines · Borislav Petkov

Quoted 5 lines
Quoted 5 lines
Only not sure whether the static is the best solution, the following patch also works, by zeroing the memory as you suggested.
Signed-off-by Mark de Wever <[email protected]>
Quoted 15 lines
drivers/ide/ide-tape.c static void idetape_get_inquiry_results(ide_drive_t *drive) · line 2343
Quoted 12 lines
drivers/ide/ide-tape.c +1 -1
index 1bce84b..c41f5b1 100644
@@ -2338,7 +2338,7 @@ static void idetape_get_inquiry_results(ide_drive_t *drive)
 {
 	idetape_tape_t *tape = drive->driver_data;
 	struct ide_atapi_pc pc;
-	char fw_rev[6], vendor_id[10], product_id[18];
+	char fw_rev[6] = {'\0'}, vendor_id[10] = {'\0'}, product_id[18] = {'\0'};
 
 
Do you realize how much *absolutely unnecessary* code will this bring
in? This is certainly worse than your initial patch (if it was correct).

Quoted 8 lines

Yep, Sergei's right. Both of our patches are dumb.
Quoted 2 lines
Ugh, looks like I'll have t submit the patch myself to stop this ugliness...
Is this what you had in mind?

Sure.

WBR, Sergei

Boris Petkov

2008-09-23 13:40 UTC #11 lore raw

Quoted 31 lines · Sergei Shtylyov

Hello.
Quoted 27 lines · Borislav Petkov
Quoted 5 lines
Quoted 5 lines
Only not sure whether the static is the best solution, the following patch also works, by zeroing the memory as you suggested.
Signed-off-by Mark de Wever <[email protected]>
Quoted 17 lines
drivers/ide/ide-tape.c static void · line 2344
Quoted 13 lines
drivers/ide/ide-tape.c +1 -1
index 1bce84b..c41f5b1 100644
@@ -2338,7 +2338,7 @@ static void
 idetape_get_inquiry_results(ide_drive_t *drive)
 {
       idetape_tape_t *tape = drive->driver_data;
       struct ide_atapi_pc pc;
-       char fw_rev[6], vendor_id[10], product_id[18];
+       char fw_rev[6] = {'\0'}, vendor_id[10] = {'\0'}, product_id[18]
 = {'\0'};
 
Do you realize how much *absolutely unnecessary* code will this bring in? This is certainly worse than your initial patch (if it was correct).
Yep, Sergei's right. Both of our patches are dumb.
Quoted 1 line

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Ugh, looks like I'll have t submit the patch myself to stop this ugliness...
Is this what you had in mind?
Sure.

Mark, can you please test?

Thanks.

Signature · 2 lines

Regards/Gruss,
Boris

Mark de Wever

author

2008-09-23 17:03 UTC #12 lore raw

In reply to Borislav Petkov

Quoted 21 lines · Borislav Petkov

---

drivers/ide/ide-tape.c +1 -3

index 1bce84b..3833189 100644
@@ -2338,7 +2338,7 @@ static void idetape_get_inquiry_results(ide_drive_t *drive)
 {
 	idetape_tape_t *tape = drive->driver_data;
 	struct ide_atapi_pc pc;
-	char fw_rev[6], vendor_id[10], product_id[18];
+	char fw_rev[4], vendor_id[8], product_id[16];
 
 	idetape_create_inquiry_cmd(&pc);
 	if (idetape_queue_pc_tail(drive, &pc)) {

@@ -2350,11 +2350,11 @@ static void idetape_get_inquiry_results(ide_drive_t *drive)
 	memcpy(product_id, &pc.buf[16], 16);
 	memcpy(fw_rev, &pc.buf[32], 4);
 
-	ide_fixstring(vendor_id, 10, 0);
-	ide_fixstring(product_id, 18, 0);

drivers/ide/ide-tape.c static void idetape_get_inquiry_results(ide_drive_t *drive) · line 2360

Quoted 10 lines

drivers/ide/ide-tape.c +4 -2

-	ide_fixstring(fw_rev, 6, 0);
+	ide_fixstring(vendor_id, 8, 0);
+	ide_fixstring(product_id, 16, 0);
+	ide_fixstring(fw_rev, 4, 0);
 
-	printk(KERN_INFO "ide-tape: %s <-> %s: %s %s rev %s\n",
+	printk(KERN_INFO "ide-tape: %s <-> %s: %.8s %.16s rev %.4s\n",
 			drive->name, tape->name, vendor_id, product_id, fw_rev);
 }

I think this patch looks good, better as all previous versions (including my initial version). I just tested it and it solves the problem. Feel free to add my tested-by.

Regards,
Mark de Wever

PS I was not able to test earlier ;-)

Boris Petkov

2008-09-23 17:11 UTC #13 lore raw

Quoted 37 lines · Mark de Wever

Quoted 21 lines · Borislav Petkov

---

drivers/ide/ide-tape.c +1 -3

index 1bce84b..3833189 100644
@@ -2338,7 +2338,7 @@ static void idetape_get_inquiry_results(ide_drive_t *drive)
 {
      idetape_tape_t *tape = drive->driver_data;
      struct ide_atapi_pc pc;
-     char fw_rev[6], vendor_id[10], product_id[18];
+     char fw_rev[4], vendor_id[8], product_id[16];
 
      idetape_create_inquiry_cmd(&pc);
      if (idetape_queue_pc_tail(drive, &pc)) {

@@ -2350,11 +2350,11 @@ static void idetape_get_inquiry_results(ide_drive_t *drive)
      memcpy(product_id, &pc.buf[16], 16);
      memcpy(fw_rev, &pc.buf[32], 4);
 
-     ide_fixstring(vendor_id, 10, 0);
-     ide_fixstring(product_id, 18, 0);

drivers/ide/ide-tape.c static void idetape_get_inquiry_results(ide_drive_t *drive) · line 2360

Quoted 10 lines

drivers/ide/ide-tape.c +4 -2

-     ide_fixstring(fw_rev, 6, 0);
+     ide_fixstring(vendor_id, 8, 0);
+     ide_fixstring(product_id, 16, 0);
+     ide_fixstring(fw_rev, 4, 0);
 
-     printk(KERN_INFO "ide-tape: %s <-> %s: %s %s rev %s\n",
+     printk(KERN_INFO "ide-tape: %s <-> %s: %.8s %.16s rev %.4s\n",
                      drive->name, tape->name, vendor_id, product_id, fw_rev);
 }

I think this patch looks good, better as all previous versions (including my initial version). I just tested it and it solves the problem. Feel free to add my tested-by.

Quoted 4 lines

Regards,
Mark de Wever
PS I was not able to test earlier ;-)

No worries and thanks. By the way, you mentioned something about other problems with ide-tape. FWIW, it seems you're its only user and we were about to kill it but decided not to yet and did the whole cleanup kinda only by compile-testing since almost no one (well, except you :)) has the hardware. So, feel free to send me any dmesg/debug/error messages you get and I'll look into them. Also, set IDETAPE_DEBUG_LOG to 1 before compiling for full debug info.

Thanks.

Signature · 2 lines

Regards/Gruss,
Boris

Mark de Wever

author

2008-09-23 21:20 UTC #14 lore raw

IDE-TAPE regressions [was: [PATCH] IDE-TAPE NULL terminate strings.]

Quoted 6 lines · Boris Petkov

No worries and thanks. By the way, you mentioned something about other problems with ide-tape. FWIW, it seems you're its only user and we were about to kill it but decided not to yet and did the whole cleanup kinda only by compile-testing since almost no one (well, except you :)) has the hardware. So, feel free to send me any dmesg/debug/error messages you get and I'll look into them. Also, set IDETAPE_DEBUG_LOG to 1 before compiling for full debug info.

Thanks for not killing the driver :-) Feel free to add me to your list of testers in case you need more testing in the future.

I already posted the problem [1] but didn't CC you since your name wasn't listed in MAINTAINERS. I recompiled the kernel and tested again, but the output doesn't seem to contain extra output.

In order to compile the kernel with IDETAPE_DEBUG_LOG enabled I had to apply the following build fix.

Signed-off-by Mark de Wever <[email protected]>

drivers/ide/ide-tape.c +3 -2

index 3833189..7258eca 100644
@@ -978,9 +978,10 @@ static ide_startstop_t idetape_do_request(ide_drive_t *drive,
 	struct request *postponed_rq = tape->postponed_rq;
 	u8 stat;
 
-	debug_log(DBG_SENSE, "sector: %ld, nr_sectors: %ld,"
+	debug_log(DBG_SENSE, "sector: %llu, nr_sectors: %ld,"
 			" current_nr_sectors: %d\n",
-			rq->sector, rq->nr_sectors, rq->current_nr_sectors);
+			(unsigned long long)rq->sector, rq->nr_sectors, 
+			rq->current_nr_sectors);
 
 	if (!blk_special_request(rq)) {
 		/* We do not support buffer cache originated requests. */

[1] http://marc.info/?l=linux-kernel&m=122203193728465&w=2

Regards,
Mark de Wever

Mark de Wever

author

2008-10-07 18:26 UTC #15 lore raw

PATCH 1/1 ide-tape build fix

I never got a reaction on this patch, but please apply it.

In order to compile the kernel with IDETAPE_DEBUG_LOG enabled I had to apply the following build fix.

Signed-off-by Mark de Wever <[email protected]>

drivers/ide/ide-tape.c +3 -2

index 3833189..7258eca 100644
@@ -978,9 +978,10 @@ static ide_startstop_t idetape_do_request(ide_drive_t *drive,
 	struct request *postponed_rq = tape->postponed_rq;
 	u8 stat;
 
-	debug_log(DBG_SENSE, "sector: %ld, nr_sectors: %ld,"
+	debug_log(DBG_SENSE, "sector: %llu, nr_sectors: %ld,"
 			" current_nr_sectors: %d\n",
-			rq->sector, rq->nr_sectors, rq->current_nr_sectors);
+			(unsigned long long)rq->sector, rq->nr_sectors, 
+			rq->current_nr_sectors);
 
 	if (!blk_special_request(rq)) {
 		/* We do not support buffer cache originated requests. */

Regards,
Mark de Wever

Borislav Petkov

2008-10-08 06:34 UTC #16 lore raw

Re: [patch][repost] ide-tape build fix

Hi,

Quoted 4 lines · Mark de Wever

I never got a reaction on this patch, but please apply it.
In order to compile the kernel with IDETAPE_DEBUG_LOG enabled I had to apply the following build fix.

Bugger, I should be getting at least warnings when compiling this but I don't. This is because I don't have CONFIG_LBD enabled. However, the %ld and %d format specifiers are also not entirely correct but gcc doesn't warn about them
- I guess it checks only size but not signedness...

drivers/ide/ide-tape.c static ide_startstop_t idetape_do_request(ide_drive_t *drive, · line 981

Quoted 12 lines

Signed-off-by Mark de Wever <[email protected]>

drivers/ide/ide-tape.c +1 -1

index 3833189..7258eca 100644
@@ -978,9 +978,10 @@ static ide_startstop_t idetape_do_request(ide_drive_t *drive,
 	struct request *postponed_rq = tape->postponed_rq;
 	u8 stat;
 
-	debug_log(DBG_SENSE, "sector: %ld, nr_sectors: %ld,"
+	debug_log(DBG_SENSE, "sector: %llu, nr_sectors: %ld,"

the other format specifiers should be rq->nr_sectors: %lu, rq->current_nr_sectors: %u.

drivers/ide/ide-tape.c static ide_startstop_t idetape_do_request(ide_drive_t *drive, · line 987

Quoted 8 lines

drivers/ide/ide-tape.c +2 -1

 			" current_nr_sectors: %d\n",
-			rq->sector, rq->nr_sectors, rq->current_nr_sectors);
+			(unsigned long long)rq->sector, rq->nr_sectors, 
+			rq->current_nr_sectors);
 
 	if (!blk_special_request(rq)) {
 		/* We do not support buffer cache originated requests. */

Would you like to redo your patch and add a proper comment ontop?

Thanks.

Signature · 2 lines

Regards/Gruss,
    Boris.

Mark de Wever

author

2008-10-08 15:47 UTC #17 lore raw

Re: [patch][repost] ide-tape build fix

On Wed, Oct 08, 2008 at 08:33:52AM +0200, Borislav Petkov wrote:
Hi,

Quoted 4 lines

Bugger, I should be getting at least warnings when compiling this but I don't. This is because I don't have CONFIG_LBD enabled. However, the %ld and %d format specifiers are also not entirely correct but gcc doesn't warn about them
- I guess it checks only size but not signedness...

I guess so too.

Quoted 1 line

Would you like to redo your patch and add a proper comment ontop?

Here you go.

@Boris do you want to add your Signed-off-by too?

@Bart please apply this patch.

--
Subject: ide-tape: Buildfix when IDETAPE_DEBUG_LOG is set to 1.

The format specifier for rq->sector didn't specify the proper size and signedness. Borislav Petkov discovered that the signedness for rq->nr_sectors and rq->current_nr_sectors also were incorrect.

Signed-off-by Mark de Wever <[email protected]>

---
 drivers/ide/ide-tape.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

drivers/ide/ide-tape.c +4 -3

index 3833189..3d5f782 100644
@@ -978,9 +978,10 @@ static ide_startstop_t idetape_do_request(ide_drive_t *drive,
 	struct request *postponed_rq = tape->postponed_rq;
 	u8 stat;
 
-	debug_log(DBG_SENSE, "sector: %ld, nr_sectors: %ld,"
-			" current_nr_sectors: %d\n",
-			rq->sector, rq->nr_sectors, rq->current_nr_sectors);
+	debug_log(DBG_SENSE, "sector: %llu, nr_sectors: %lu,"
+			" current_nr_sectors: %u\n",
+			(unsigned long long)rq->sector, rq->nr_sectors, 
+			rq->current_nr_sectors);
 
 	if (!blk_special_request(rq)) {
 		/* We do not support buffer cache originated requests. */

Signature · 4 lines

1.5.6.5

Regards,
Mark de Wever

Boris Petkov

Acked-by Borislav Petkov

2008-10-08 16:22 UTC #18 lore raw

Re: [patch][repost] ide-tape build fix

Quoted 7 lines · Mark de Wever

On Wed, Oct 08, 2008 at 08:33:52AM +0200, Borislav Petkov wrote:
Hi,
Quoted 4 lines
Bugger, I should be getting at least warnings when compiling this but I don't. This is because I don't have CONFIG_LBD enabled. However, the %ld and %d format specifiers are also not entirely correct but gcc doesn't warn about them
- I guess it checks only size but not signedness...

Quoted 7 lines

I guess so too.
Quoted 1 line
Would you like to redo your patch and add a proper comment ontop?
Here you go.
@Boris do you want to add your Signed-off-by too?

no, just

Acked-by Borislav Petkov <[email protected]>

Quoted 38 lines

@Bart please apply this patch.

--
Subject: ide-tape: Buildfix when IDETAPE_DEBUG_LOG is set to 1.

Signed-off-by Mark de Wever <[email protected]>

---
 drivers/ide/ide-tape.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

drivers/ide/ide-tape.c +4 -4

index 3833189..3d5f782 100644
@@ -978,9 +978,10 @@ static ide_startstop_t idetape_do_request(ide_drive_t *drive,
       struct request *postponed_rq = tape->postponed_rq;
       u8 stat;
 
-       debug_log(DBG_SENSE, "sector: %ld, nr_sectors: %ld,"
-                       " current_nr_sectors: %d\n",
-                       rq->sector, rq->nr_sectors, rq->current_nr_sectors);
+       debug_log(DBG_SENSE, "sector: %llu, nr_sectors: %lu,"
+                       " current_nr_sectors: %u\n",
+                       (unsigned long long)rq->sector, rq->nr_sectors,
+                       rq->current_nr_sectors);
 
       if (!blk_special_request(rq)) {
               /* We do not support buffer cache originated requests. */
--

1.5.6.5

Regards,
Mark de Wever

Signature · 2 lines

Regards/Gruss,
Boris

Bartlomiej Zolnierkiewicz

Applied

2008-10-08 19:28 UTC #19 lore raw

Re: [patch][repost] ide-tape build fix

Quoted 12 lines · Boris Petkov

Quoted 11 lines · Mark de Wever
On Wed, Oct 08, 2008 at 08:33:52AM +0200, Borislav Petkov wrote:
Hi,
Quoted 4 lines
Bugger, I should be getting at least warnings when compiling this but I don't. This is because I don't have CONFIG_LBD enabled. However, the %ld and %d format specifiers are also not entirely correct but gcc doesn't warn about them
- I guess it checks only size but not signedness...
I guess so too.
Quoted 1 line
Would you like to redo your patch and add a proper comment ontop?

Quoted 18 lines

Quoted 3 lines
Here you go.
@Boris do you want to add your Signed-off-by too?
no, just
Acked-by Borislav Petkov <[email protected]>
Quoted 10 lines
@Bart please apply this patch.
--
Subject: ide-tape: Buildfix when IDETAPE_DEBUG_LOG is set to 1.
The format specifier for rq->sector didn't specify the proper size and signedness. Borislav Petkov discovered that the signedness for rq->nr_sectors and rq->current_nr_sectors also were incorrect.
Signed-off-by Mark de Wever <[email protected]>

applied

Borislav Petkov

Tested-by Mark de Wever

2008-09-24 07:10 UTC #20 lore raw

In reply to Mark de Wever

Quoted 37 lines · Mark de Wever

Quoted 21 lines · Borislav Petkov

---

drivers/ide/ide-tape.c +1 -3

index 1bce84b..3833189 100644
@@ -2338,7 +2338,7 @@ static void idetape_get_inquiry_results(ide_drive_t *drive)
 {
 	idetape_tape_t *tape = drive->driver_data;
 	struct ide_atapi_pc pc;
-	char fw_rev[6], vendor_id[10], product_id[18];
+	char fw_rev[4], vendor_id[8], product_id[16];
 
 	idetape_create_inquiry_cmd(&pc);
 	if (idetape_queue_pc_tail(drive, &pc)) {

@@ -2350,11 +2350,11 @@ static void idetape_get_inquiry_results(ide_drive_t *drive)
 	memcpy(product_id, &pc.buf[16], 16);
 	memcpy(fw_rev, &pc.buf[32], 4);
 
-	ide_fixstring(vendor_id, 10, 0);
-	ide_fixstring(product_id, 18, 0);

drivers/ide/ide-tape.c static void idetape_get_inquiry_results(ide_drive_t *drive) · line 2360

Quoted 10 lines

drivers/ide/ide-tape.c +4 -2

-	ide_fixstring(fw_rev, 6, 0);
+	ide_fixstring(vendor_id, 8, 0);
+	ide_fixstring(product_id, 16, 0);
+	ide_fixstring(fw_rev, 4, 0);
 
-	printk(KERN_INFO "ide-tape: %s <-> %s: %s %s rev %s\n",
+	printk(KERN_INFO "ide-tape: %s <-> %s: %.8s %.16s rev %.4s\n",
 			drive->name, tape->name, vendor_id, product_id, fw_rev);
 }

I think this patch looks good, better as all previous versions (including my initial version). I just tested it and it solves the problem. Feel free to add my tested-by.

Quoted 4 lines

Regards,
Mark de Wever
PS I was not able to test earlier ;-)

Bart,

please apply the following patch.

@Sergei: would you like to add your Signed-off-by too?

--
Subject: ide-tape: fix vendor strings

Remove superfluous two bytes from each string buffer and add proper length format specifiers.

There should be no functional change resulting from this patch.

Signed-off-by Borislav Petkov <[email protected]>

Tested-by Mark de Wever <[email protected]>

drivers/ide/ide-tape.c +5 -5

index 1bce84b..3833189 100644
@@ -2338,7 +2338,7 @@ static void idetape_get_inquiry_results(ide_drive_t *drive)
 {
 	idetape_tape_t *tape = drive->driver_data;
 	struct ide_atapi_pc pc;
-	char fw_rev[6], vendor_id[10], product_id[18];
+	char fw_rev[4], vendor_id[8], product_id[16];
 
 	idetape_create_inquiry_cmd(&pc);
 	if (idetape_queue_pc_tail(drive, &pc)) {

@@ -2350,11 +2350,11 @@ static void idetape_get_inquiry_results(ide_drive_t *drive)
 	memcpy(product_id, &pc.buf[16], 16);
 	memcpy(fw_rev, &pc.buf[32], 4);
 
-	ide_fixstring(vendor_id, 10, 0);
-	ide_fixstring(product_id, 18, 0);
-	ide_fixstring(fw_rev, 6, 0);
+	ide_fixstring(vendor_id, 8, 0);
+	ide_fixstring(product_id, 16, 0);
+	ide_fixstring(fw_rev, 4, 0);
 
-	printk(KERN_INFO "ide-tape: %s <-> %s: %s %s rev %s\n",
+	printk(KERN_INFO "ide-tape: %s <-> %s: %.8s %.16s rev %.4s\n",
 			drive->name, tape->name, vendor_id, product_id, fw_rev);
 }

Signature · 2 lines

Regards/Gruss,
    Boris.

Sergei Shtylyov

Acked-by

2008-09-24 09:37 UTC #21 lore raw

Hello.

Quoted 6 lines · Borislav Petkov

Bart,
please apply the following patch.
@Sergei: would you like to add your Signed-off-by too?

Just my ACK.

Quoted 11 lines

--
Subject: ide-tape: fix vendor strings
Remove superfluous two bytes from each string buffer and add proper length format specifiers.
There should be no functional change resulting from this patch.
Signed-off-by Borislav Petkov <[email protected]>
Tested-by Mark de Wever <[email protected]>

Acked-by Sergei Shtylyov <[email protected]>

MBR, Sergei

Bartlomiej Zolnierkiewicz

Applied

2008-09-27 17:29 UTC #22 lore raw

On Wednesday 24 September 2008, Sergei Shtylyov wrote:

[...]

Quoted 5 lines

Quoted 5 lines
Subject: ide-tape: fix vendor strings
Remove superfluous two bytes from each string buffer and add proper length format specifiers.

Quoted 7 lines

Quoted 5 lines
There should be no functional change resulting from this patch.
Signed-off-by Borislav Petkov <[email protected]>
Tested-by Mark de Wever <[email protected]>
Acked-by Sergei Shtylyov <[email protected]>

applied

Mark de Wever

author

2008-09-23 16:16 UTC #23 lore raw

In reply to Sergei Shtylyov

Quoted 6 lines · Sergei Shtylyov

Quoted 6 lines

drivers/ide/ide-tape.c +0 -0

index 1bce84b..c41f5b1 100644
@@ -2338,7 +2338,7 @@ static void idetape_get_inquiry_results(ide_drive_t *drive)
 {

Quoted 8 lines

drivers/ide/ide-tape.c static void idetape_get_inquiry_results(ide_drive_t *drive) · line 2342
Quoted 5 lines
drivers/ide/ide-tape.c +1 -1
 	idetape_tape_t *tape = drive->driver_data;
 	struct ide_atapi_pc pc;
-	char fw_rev[6], vendor_id[10], product_id[18];
+	char fw_rev[6] = {'\0'}, vendor_id[10] = {'\0'}, product_id[18] = {'\0'};
 
Do you realize how much *absolutely unnecessary* code will this bring
in?

I did not, I just had a look at the code GCC produced. I did expect much smaller code, but maybe that's only generated with -Os.

Quoted 1 line

This is certainly worse than your initial patch (if it was correct).

My initial patch did work, but that doesn't matter much, since Boris posted another patch based on your suggestions. I like that patch better as my initial patch. I'm testing it now and I expect it to work.

Regards,
Mark de Wever

Sergei Shtylyov

2008-09-23 17:09 UTC #24 lore raw

Quoted 14 lines · Mark de Wever

Quoted 5 lines
Quoted 5 lines
drivers/ide/ide-tape.c +0 -0
index 1bce84b..c41f5b1 100644
@@ -2338,7 +2338,7 @@ static void idetape_get_inquiry_results(ide_drive_t *drive)
Quoted 8 lines
drivers/ide/ide-tape.c static void idetape_get_inquiry_results(ide_drive_t *drive) · line 2341
Quoted 5 lines
drivers/ide/ide-tape.c +1 -1
 {
 	idetape_tape_t *tape = drive->driver_data;
 	struct ide_atapi_pc pc;
-	char fw_rev[6], vendor_id[10], product_id[18];
+	char fw_rev[6] = {'\0'}, vendor_id[10] = {'\0'}, product_id[18] = {'\0'};
Do you realize how much *absolutely unnecessary* code will this bring
in?

Quoted 2 lines

I did not, I just had a look at the code GCC produced. I did expect much smaller code, but maybe that's only generated with -Os.

My imagination sufficed to foresee how much code a compiler would have to
produce to completely initialize the arrays of the *auto* memory class -- even regardless of optimization. And all that mostly to no purpose.

Quoted 3 lines

Quoted 1 line
This is certainly worse than your initial patch (if it was correct).
My initial patch did work,

If ide_fixstring() wouldn't have to do any space compression, it would
work. If it would have to compress spaces, 2 garbage characters would be copied by it and then printed.

Quoted 3 lines

but that doesn't matter much, since Boris
posted another patch based on your suggestions. I like that patch better as my initial patch. I'm testing it now and I expect it to work.

Me too. :-)

Quoted 2 lines

Regards,
Mark de Wever

WBR, Sergei

Mark de Wever

author

2008-09-23 17:29 UTC #25 lore raw

Quoted 16 lines · Sergei Shtylyov

Quoted 13 lines · Mark de Wever
Quoted 13 lines
drivers/ide/ide-tape.c static void idetape_get_inquiry_results(ide_drive_t *drive) · line 2341
Quoted 10 lines
drivers/ide/ide-tape.c +1 -1
index 1bce84b..c41f5b1 100644
@@ -2338,7 +2338,7 @@ static void idetape_get_inquiry_results(ide_drive_t *drive)
 {
 	idetape_tape_t *tape = drive->driver_data;
 	struct ide_atapi_pc pc;
-	char fw_rev[6], vendor_id[10], product_id[18];
+	char fw_rev[6] = {'\0'}, vendor_id[10] = {'\0'}, product_id[18] = {'\0'};
Do you realize how much *absolutely unnecessary* code will this bring
in?

Quoted 6 lines

Quoted 2 lines
I did not, I just had a look at the code GCC produced. I did expect much smaller code, but maybe that's only generated with -Os.
My imagination sufficed to foresee how much code a compiler would have
to produce to completely initialize the arrays of the *auto* memory class -- even regardless of optimization. And all that mostly to no purpose.

I expected a rep stos based code, which isn't much code. But the code was created without a rep, instead it used several stos opcodes. I agree with the mostly no purpose part, since the memcpy later overwrites most of the array.

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
This is certainly worse than your initial patch (if it was correct).
My initial patch did work,
If ide_fixstring() wouldn't have to do any space compression, it would
work. If it would have to compress spaces, 2 garbage characters would be copied by it and then printed.

I didn't know that, I guess I got lucky ;-)

Quoted 5 lines

Quoted 3 lines
but that doesn't matter much, since Boris
posted another patch based on your suggestions. I like that patch better as my initial patch. I'm testing it now and I expect it to work.
   Me too. :-)

It worked, thanks for your suggestion :-)

Regards,
Mark de Wever