[PATCH] IDE-TAPE NULL terminate strings.

[PATCH] IDE-TAPE NULL terminate strings.

[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 <koraq@xs4all.nl>
---
 drivers/ide/ide-tape.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 1bce84b..fd87b43 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -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);
 }
-- 
1.5.6.5

Re: [PATCH] IDE-TAPE NULL terminate strings.

[Sergei Shtylyov]

Hello.

Mark de Wever wrote:

> 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 <koraq@xs4all.nl>

> diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
> index 1bce84b..fd87b43 100644
> --- a/drivers/ide/ide-tape.c
> +++ b/drivers/ide/ide-tape.c
> @@ -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

Re: [PATCH] IDE-TAPE NULL terminate strings.

[Sergei Shtylyov]

Hello, I wrote:

>> 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 <koraq@xs4all.nl>

>> diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
>> index 1bce84b..fd87b43 100644
>> --- a/drivers/ide/ide-tape.c
>> +++ b/drivers/ide/ide-tape.c
>> @@ -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

Re: [PATCH] IDE-TAPE NULL terminate strings.

[Mark de Wever]

On Mon, Sep 22, 2008 at 12:08:27AM +0400, Sergei Shtylyov wrote:
>>> diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
>>> index 1bce84b..fd87b43 100644
>>> --- a/drivers/ide/ide-tape.c
>>> +++ b/drivers/ide/ide-tape.c
>>> @@ -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

Re: [PATCH] IDE-TAPE NULL terminate strings.

[Sergei Shtylyov]

Hello.

Mark de Wever wrote:

>>>> diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
>>>> index 1bce84b..fd87b43 100644
>>>> --- a/drivers/ide/ide-tape.c
>>>> +++ b/drivers/ide/ide-tape.c
>>>> @@ -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.
>   

   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.

> Regards,
> Mark de Wever
>   

MBR, Sergei

Re: [PATCH] IDE-TAPE NULL terminate strings.

[Sergei Shtylyov]

Hello.

Mark de Wever wrote:

> 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

Re: [PATCH] IDE-TAPE NULL terminate strings.

[Boris Petkov]

On Mon, Sep 22, 2008 at 3:16 PM, Sergei Shtylyov
<sshtylyov@ru.mvista.com> wrote:
> Hello.
>
> Mark de Wever wrote:
>
>> 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

.. 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 <petkovbb@gmail.com>

diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 1bce84b..848d9df 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -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)) {


-- 
Regards/Gruss,
Boris

Re: [PATCH] IDE-TAPE NULL terminate strings.

[Mark de Wever]

On Mon, Sep 22, 2008 at 03:56:49PM +0200, Boris Petkov wrote:
> On Mon, Sep 22, 2008 at 3:16 PM, Sergei Shtylyov
> <sshtylyov@ru.mvista.com> wrote:
> > Hello.
> >
> .. 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 <koraq@xs4all.nl>

diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 1bce84b..c41f5b1 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -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)) {

-- 
Regards,
Mark de Wever

Re: [PATCH] IDE-TAPE NULL terminate strings.

[Sergei Shtylyov]

Hello.

Mark de Wever wrote:

>> .. 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.
>   

   And my NAK too. :-)

> 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 <koraq@xs4all.nl>
>
> diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
> index 1bce84b..c41f5b1 100644
> --- a/drivers/ide/ide-tape.c
> +++ b/drivers/ide/ide-tape.c
> @@ -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

Re: [PATCH] IDE-TAPE NULL terminate strings.

[Borislav Petkov]

On Tue, Sep 23, 2008 at 01:08:25AM +0400, Sergei Shtylyov wrote:
> Hello.
>
> Mark de Wever wrote:
>
>>> .. 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.
>>   
>
>   And my NAK too. :-)
>
>> 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 <koraq@xs4all.nl>
>>
>> diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
>> index 1bce84b..c41f5b1 100644
>> --- a/drivers/ide/ide-tape.c
>> +++ b/drivers/ide/ide-tape.c
>> @@ -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.

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

Is this what you had in mind?

---
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 1bce84b..3833189 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -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);
 }
 


-- 
Regards/Gruss,
    Boris.

Re: [PATCH] IDE-TAPE NULL terminate strings.

[Sergei Shtylyov]

Hello.

Borislav Petkov wrote:

>>> 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 <koraq@xs4all.nl>
>>>
>>> diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
>>> index 1bce84b..c41f5b1 100644
>>> --- a/drivers/ide/ide-tape.c
>>> +++ b/drivers/ide/ide-tape.c
>>> @@ -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.
>
>   
>> 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

Re: [PATCH] IDE-TAPE NULL terminate strings.

[Boris Petkov]

On Tue, Sep 23, 2008 at 11:29 AM, Sergei Shtylyov
<sshtylyov@ru.mvista.com> wrote:
> Hello.
>
> Borislav Petkov wrote:
>
>>>> 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 <koraq@xs4all.nl>
>>>>
>>>> diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
>>>> index 1bce84b..c41f5b1 100644
>>>> --- a/drivers/ide/ide-tape.c
>>>> +++ b/drivers/ide/ide-tape.c
>>>> @@ -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.
>>
>>
>>>
>>> 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.

-- 
Regards/Gruss,
Boris

Re: [PATCH] IDE-TAPE NULL terminate strings.

[Mark de Wever]

On Tue, Sep 23, 2008 at 01:08:25AM +0400, Sergei Shtylyov wrote:
>> diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
>> index 1bce84b..c41f5b1 100644
>> --- a/drivers/ide/ide-tape.c
>> +++ b/drivers/ide/ide-tape.c
>> @@ -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?

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.

> 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

Re: [PATCH] IDE-TAPE NULL terminate strings.

[Mark de Wever]

On Tue, Sep 23, 2008 at 09:48:45AM +0200, Borislav Petkov wrote:
> ---
> diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
> index 1bce84b..3833189 100644
> --- a/drivers/ide/ide-tape.c
> +++ b/drivers/ide/ide-tape.c
> @@ -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);
>  }
>  
>

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 ;-)

Re: [PATCH] IDE-TAPE NULL terminate strings.

[Sergei Shtylyov]

Mark de Wever wrote:

>>>diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
>>>index 1bce84b..c41f5b1 100644
>>>--- a/drivers/ide/ide-tape.c
>>>+++ b/drivers/ide/ide-tape.c
>>>@@ -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?

> 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.

>>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.

> 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. :-)

> Regards,
> Mark de Wever

WBR, Sergei

Re: [PATCH] IDE-TAPE NULL terminate strings.

[Boris Petkov]

On Tue, Sep 23, 2008 at 6:59 PM, Mark de Wever <koraq@xs4all.nl> wrote:
> On Tue, Sep 23, 2008 at 09:48:45AM +0200, Borislav Petkov wrote:
>> ---
>> diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
>> index 1bce84b..3833189 100644
>> --- a/drivers/ide/ide-tape.c
>> +++ b/drivers/ide/ide-tape.c
>> @@ -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);
>>  }
>>
>>
>
> 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 ;-)

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.

-- 
Regards/Gruss,
Boris

Re: [PATCH] IDE-TAPE NULL terminate strings.

[Mark de Wever]

On Tue, Sep 23, 2008 at 09:10:43PM +0400, Sergei Shtylyov wrote:
> Mark de Wever wrote:
>
>>>> diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
>>>> index 1bce84b..c41f5b1 100644
>>>> --- a/drivers/ide/ide-tape.c
>>>> +++ b/drivers/ide/ide-tape.c
>>>> @@ -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?
>
>> 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.

>>> 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 ;-)

>> 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

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

[Mark de Wever]

On Tue, Sep 23, 2008 at 07:11:04PM +0200, Boris Petkov wrote:
> 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 <koraq@xs4all.nl>

diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 3833189..7258eca 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -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

Re: [PATCH] IDE-TAPE NULL terminate strings.

[Borislav Petkov]

On Tue, Sep 23, 2008 at 06:59:14PM +0200, Mark de Wever wrote:
> On Tue, Sep 23, 2008 at 09:48:45AM +0200, Borislav Petkov wrote:
> > ---
> > diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
> > index 1bce84b..3833189 100644
> > --- a/drivers/ide/ide-tape.c
> > +++ b/drivers/ide/ide-tape.c
> > @@ -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);
> >  }
> >  
> >
> 
> 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 ;-)

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 <petkovbb@gmail.com>
Tested-by: Mark de Wever <koraq@xs4all.nl>

--

diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 1bce84b..3833189 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -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);
 }
 

-- 
Regards/Gruss,
    Boris.

Re: [PATCH] IDE-TAPE NULL terminate strings.

[Sergei Shtylyov]

Hello.

Borislav Petkov wrote:

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

   Just my ACK.

> --
> 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 <petkovbb@gmail.com>
> Tested-by: Mark de Wever <koraq@xs4all.nl>
>   

Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

MBR, Sergei

Re: [PATCH] IDE-TAPE NULL terminate strings.

[Bartlomiej Zolnierkiewicz]

On Wednesday 24 September 2008, Sergei Shtylyov wrote:

[...]

> > 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 <petkovbb@gmail.com>
> > Tested-by: Mark de Wever <koraq@xs4all.nl>
> >   
> 
> Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

applied

[patch][repost] ide-tape build fix

[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.

Signed-off-by: Mark de Wever <koraq@xs4all.nl>

diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 3833189..7258eca 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -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

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

[Borislav Petkov]

Hi,

On Tue, Oct 07, 2008 at 08:26:15PM +0200, Mark de Wever wrote:
> 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...

> Signed-off-by: Mark de Wever <koraq@xs4all.nl>
> 
> diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
> index 3833189..7258eca 100644
> --- a/drivers/ide/ide-tape.c
> +++ b/drivers/ide/ide-tape.c
> @@ -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.

>  			" 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.

-- 
Regards/Gruss,
    Boris.

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

[Mark de Wever]

On Wed, Oct 08, 2008 at 08:33:52AM +0200, Borislav Petkov wrote:
Hi,
> 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.

> 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 <koraq@xs4all.nl>
---
 drivers/ide/ide-tape.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 3833189..3d5f782 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -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

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

[Boris Petkov]

On Wed, Oct 8, 2008 at 5:45 PM, Mark de Wever <koraq@xs4all.nl> wrote:
> On Wed, Oct 08, 2008 at 08:33:52AM +0200, Borislav Petkov wrote:
> Hi,
>> 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.
>
>> 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 <petkovbb@gmail.com>

> @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 <koraq@xs4all.nl>
> ---
>  drivers/ide/ide-tape.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
> index 3833189..3d5f782 100644
> --- a/drivers/ide/ide-tape.c
> +++ b/drivers/ide/ide-tape.c
> @@ -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
>

-- 
Regards/Gruss,
Boris

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

[Bartlomiej Zolnierkiewicz]

On Wednesday 08 October 2008, Boris Petkov wrote:
> On Wed, Oct 8, 2008 at 5:45 PM, Mark de Wever <koraq@xs4all.nl> wrote:
> > On Wed, Oct 08, 2008 at 08:33:52AM +0200, Borislav Petkov wrote:
> > Hi,
> >> 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.
> >
> >> 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 <petkovbb@gmail.com>
> 
> > @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 <koraq@xs4all.nl>

applied