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

[Anthony Liguori]

On 07/09/2012 10:24 AM, Kevin Wolf wrote:
> 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...
>
> Looks broken indeed. I just wonder why my gcc (or the buildbots) didn't
> complain.

:-)

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

Okay, am building now and testing.  Looks good so far.

Regards,

Anthony Liguori

> Kevin