Re: [PATCH 06/22] ide-tape: struct idetape_tape_t: remove unused members

["Boris Petkov" <petkovbb@googlemail.com>]

On Mon, Sep 22, 2008 at 3:25 PM, Sergei Shtylyov
<sshtylyov@ru.mvista.com> wrote:
> Hello.
>
> Borislav Petkov wrote:
>
>> - last_frame_position: only being written to once
>> - firmware_revision, product_id, vendor_id: used once, remove from struct
>>  idetape_tape_t and deal with them locally
>> - firmware_revision_num: only written to once
>> - tape_still_time_begin: completely unused
>> - tape_still_time: never written to; remove corresponding code chunk
>> - uncontrolled_last_pipeline_head: only once written to
>> - blocks_in_buffer: only written to
>
>> Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
>
>   Late complaint but anyway... :-)
>
>> diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
>> index e0e8184..126e8a9 100644
>> --- a/drivers/ide/ide-tape.c
>> +++ b/drivers/ide/ide-tape.c
>> @@ -399,11 +397,6 @@ typedef struct ide_tape_obj {
>>        int avg_size;
>>        int avg_speed;
>>  -       char vendor_id[10];
>> -       char product_id[18];
>> -       char firmware_revision[6];
>> -       int firmware_revision_num;
>> -
>>        /* the door is currently locked */
>>        int door_locked;
>>        /* the tape hardware is write protected */
>
> [...]
>
>> @@ -3438,9 +3419,9 @@ static int idetape_identify_device (ide_drive_t
>> *drive)
>>   static void idetape_get_inquiry_results(ide_drive_t *drive)
>>  {
>> -       char *r;
>>        idetape_tape_t *tape = drive->driver_data;
>>        idetape_pc_t pc;
>> +       char fw_rev[6], vendor_id[10], product_id[18];
>>          idetape_create_inquiry_cmd(&pc);
>>        if (idetape_queue_pc_tail(drive, &pc)) {
>> @@ -3448,20 +3429,16 @@ static void
>> idetape_get_inquiry_results(ide_drive_t *drive)
>>                                tape->name);
>>                return;
>>        }
>> -       memcpy(tape->vendor_id, &pc.buffer[8], 8);
>> -       memcpy(tape->product_id, &pc.buffer[16], 16);
>> -       memcpy(tape->firmware_revision, &pc.buffer[32], 4);
>> -
>> -       ide_fixstring(tape->vendor_id, 10, 0);
>> -       ide_fixstring(tape->product_id, 18, 0);
>> -       ide_fixstring(tape->firmware_revision, 6, 0);
>> -       r = tape->firmware_revision;
>> -       if (*(r + 1) == '.')
>> -               tape->firmware_revision_num = (*r - '0') * 100 +
>> -                       (*(r + 2) - '0') * 10 + *(r + 3) - '0';
>> +       memcpy(vendor_id, &pc.buffer[8], 8);
>> +       memcpy(product_id, &pc.buffer[16], 16);
>> +       memcpy(fw_rev, &pc.buffer[32], 4);
>> +
>> +       ide_fixstring(vendor_id, 10, 0);
>> +       ide_fixstring(product_id, 18, 0);
>> +       ide_fixstring(fw_rev, 6, 0);
>
>   It was wrong to call ide_fixstring() on unterminated strings and expecting
> them to become terminated strings after that; plus it was useless to add 2
> characters padding at the end. When these variables were the fields of
> 'struct ide_tape_obj', those bytes were 0 because of the variable of this
> type being a static array. When they became local variables, they got
> garbage bytes at the end which ide_fixdriveid() either honestly copied when
> compressing spaces or just left where they were...

LOL, i just sent out a similar fix :).

>
>> +
>>        printk(KERN_INFO "ide-tape: %s <-> %s: %s %s rev %s\n",
>
>   Should've rather changed the string format to print only N characters
> max...
>
> MBR, Sergei
>



-- 
Regards/Gruss,
Boris