Re: [Qemu-devel] [PATCH 23/25] fdc: Move floppy geometry guessing back from block.c

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 09.07.2012 17:01, schrieb Anthony Liguori:
>> On 07/09/2012 09:16 AM, Kevin Wolf wrote:
>>> From: Markus Armbruster<address@hidden>
>>>
>>> Commit 5bbdbb46 moved it to block.c because "other geometry guessing
>>> functions already reside in block.c".  Device-specific functionality
>>> should be kept in device code, not the block layer.  Move it back.
>>>
>>> Disk geometry guessing is still in block.c.  To be moved out in a
>>> later patch series.
>>>
>>> Bonus: the floppy type used in pc_cmos_init() now obviously matches
>>> the one in the FDrive.  Before, we relied on
>>> bdrv_get_floppy_geometry_hint() picking the same type both in
>>> fd_revalidate() and in pc_cmos_init().
>>>
>>> Signed-off-by: Markus Armbruster<address@hidden>
>>> Signed-off-by: Kevin Wolf<address@hidden>
>
>>> diff --git a/hw/pc.c b/hw/pc.c
>>> index c7e9ab3..e5e7647 100644
>>> --- a/hw/pc.c
>>> +++ b/hw/pc.c
>>> @@ -335,10 +335,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t 
>>> above_4g_mem_size,
>>>                     ISADevice *floppy, BusState *idebus0, BusState *idebus1,
>>>                     ISADevice *s)
>>>   {
>>> -    int val, nb, nb_heads, max_track, last_sect, i;
>>> -    FDriveType fd_type[2] = { FDRIVE_DRV_NONE, FDRIVE_DRV_NONE };
>>> -    FDriveRate rate;
>>> -    BlockDriverState *fd[MAX_FD];
>>> +    int val, nb, i;
>>> +    FDriveType fd_type[2];
>> 
>> This results in:
>> 
>>    CC    i386-softmmu/hw/i386/../pc.o
>> /home/anthony/git/qemu/hw/i386/../pc.c: In function ‘pc_cmos_init’:
>> /home/anthony/git/qemu/hw/i386/../pc.c:339:16: error: ‘fd_type[1]’ may be 
>> used 
>> uninitialized in this function [-Werror=uninitialized]
>> /home/anthony/git/qemu/hw/i386/../pc.c:339:16: error: ‘fd_type[0]’ may be 
>> used 
>> uninitialized in this function [-Werror=uninitialized]
>> cc1: all warnings being treated as errors
>> 
>> And GCC is right as:
>> 
>>>       static pc_cmos_init_late_arg arg;
>>>
>>>       /* various important CMOS locations needed by PC/Bochs bios */
>>> @@ -381,13 +379,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t 
>>> above_4g_mem_size,
>>>
>>>       /* floppy type */
>>>       if (floppy) {
>>> -        fdc_get_bs(fd, floppy);
>>>           for (i = 0; i<  2; i++) {
>>> -            if (fd[i]) {
>>> -                bdrv_get_floppy_geometry_hint(fd[i],&nb_heads,&max_track,
>>> -&last_sect, FDRIVE_DRV_NONE,
>>> -&fd_type[i],&rate);
>>> -            }
>>> +            fd_type[i] = isa_fdc_get_drive_type(floppy, i);
>>>           }
>>>       }
>>>       val = (cmos_get_fd_drive_type(fd_type[0])<<  4) |
>> 
>> This is an unconditional use of fd_type[0].  If floppy == NULL, this is 
>> dereferencing an uninitialized value.
>> 
>> I'm not sure why the explicit initialization was removed...

Brain fart on my part, sorry.  The old loop assigns only if the drive
exists.  The new loop assigns unconditionally.  Except the whole loop is
still conditional.

Testing can't flag this, because floppy is never null.

> Looks broken indeed. I just wonder why my gcc (or the buildbots) didn't
> complain.

Me too.  Looks like I should upgrade to a more recent gcc.

> I dropped this patch from for-anthony, so you can give the pull request
> another try.

I'll respin when the merge dust settled.