| Message ID | 1341843388-5663-1-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 07/09/2012 09:16 AM, Kevin Wolf wrote: > From: Markus Armbruster<armbru@redhat.com> > > 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<armbru@redhat.com> > Signed-off-by: Kevin Wolf<kwolf@redhat.com> > --- > block.c | 101 --------------------------------------------------- > block.h | 18 --------- > hw/fdc.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++----- > hw/fdc.h | 10 +++++- > hw/pc.c | 13 ++----- > 5 files changed, 124 insertions(+), 140 deletions(-) > > diff --git a/block.c b/block.c > index f2540b9..fa1789c 100644 > --- a/block.c > +++ b/block.c > @@ -2272,107 +2272,6 @@ void bdrv_set_io_limits(BlockDriverState *bs, > bs->io_limits_enabled = bdrv_io_limits_enabled(bs); > } > > -/* Recognize floppy formats */ > -typedef struct FDFormat { > - FDriveType drive; > - uint8_t last_sect; > - uint8_t max_track; > - uint8_t max_head; > - FDriveRate rate; > -} FDFormat; > - > -static const FDFormat fd_formats[] = { > - /* First entry is default format */ > - /* 1.44 MB 3"1/2 floppy disks */ > - { FDRIVE_DRV_144, 18, 80, 1, FDRIVE_RATE_500K, }, > - { FDRIVE_DRV_144, 20, 80, 1, FDRIVE_RATE_500K, }, > - { FDRIVE_DRV_144, 21, 80, 1, FDRIVE_RATE_500K, }, > - { FDRIVE_DRV_144, 21, 82, 1, FDRIVE_RATE_500K, }, > - { FDRIVE_DRV_144, 21, 83, 1, FDRIVE_RATE_500K, }, > - { FDRIVE_DRV_144, 22, 80, 1, FDRIVE_RATE_500K, }, > - { FDRIVE_DRV_144, 23, 80, 1, FDRIVE_RATE_500K, }, > - { FDRIVE_DRV_144, 24, 80, 1, FDRIVE_RATE_500K, }, > - /* 2.88 MB 3"1/2 floppy disks */ > - { FDRIVE_DRV_288, 36, 80, 1, FDRIVE_RATE_1M, }, > - { FDRIVE_DRV_288, 39, 80, 1, FDRIVE_RATE_1M, }, > - { FDRIVE_DRV_288, 40, 80, 1, FDRIVE_RATE_1M, }, > - { FDRIVE_DRV_288, 44, 80, 1, FDRIVE_RATE_1M, }, > - { FDRIVE_DRV_288, 48, 80, 1, FDRIVE_RATE_1M, }, > - /* 720 kB 3"1/2 floppy disks */ > - { FDRIVE_DRV_144, 9, 80, 1, FDRIVE_RATE_250K, }, > - { FDRIVE_DRV_144, 10, 80, 1, FDRIVE_RATE_250K, }, > - { FDRIVE_DRV_144, 10, 82, 1, FDRIVE_RATE_250K, }, > - { FDRIVE_DRV_144, 10, 83, 1, FDRIVE_RATE_250K, }, > - { FDRIVE_DRV_144, 13, 80, 1, FDRIVE_RATE_250K, }, > - { FDRIVE_DRV_144, 14, 80, 1, FDRIVE_RATE_250K, }, > - /* 1.2 MB 5"1/4 floppy disks */ > - { FDRIVE_DRV_120, 15, 80, 1, FDRIVE_RATE_500K, }, > - { FDRIVE_DRV_120, 18, 80, 1, FDRIVE_RATE_500K, }, > - { FDRIVE_DRV_120, 18, 82, 1, FDRIVE_RATE_500K, }, > - { FDRIVE_DRV_120, 18, 83, 1, FDRIVE_RATE_500K, }, > - { FDRIVE_DRV_120, 20, 80, 1, FDRIVE_RATE_500K, }, > - /* 720 kB 5"1/4 floppy disks */ > - { FDRIVE_DRV_120, 9, 80, 1, FDRIVE_RATE_250K, }, > - { FDRIVE_DRV_120, 11, 80, 1, FDRIVE_RATE_250K, }, > - /* 360 kB 5"1/4 floppy disks */ > - { FDRIVE_DRV_120, 9, 40, 1, FDRIVE_RATE_300K, }, > - { FDRIVE_DRV_120, 9, 40, 0, FDRIVE_RATE_300K, }, > - { FDRIVE_DRV_120, 10, 41, 1, FDRIVE_RATE_300K, }, > - { FDRIVE_DRV_120, 10, 42, 1, FDRIVE_RATE_300K, }, > - /* 320 kB 5"1/4 floppy disks */ > - { FDRIVE_DRV_120, 8, 40, 1, FDRIVE_RATE_250K, }, > - { FDRIVE_DRV_120, 8, 40, 0, FDRIVE_RATE_250K, }, > - /* 360 kB must match 5"1/4 better than 3"1/2... */ > - { FDRIVE_DRV_144, 9, 80, 0, FDRIVE_RATE_250K, }, > - /* end */ > - { FDRIVE_DRV_NONE, -1, -1, 0, 0, }, > -}; > - > -void bdrv_get_floppy_geometry_hint(BlockDriverState *bs, int *nb_heads, > - int *max_track, int *last_sect, > - FDriveType drive_in, FDriveType *drive, > - FDriveRate *rate) > -{ > - const FDFormat *parse; > - uint64_t nb_sectors, size; > - int i, first_match, match; > - > - bdrv_get_geometry(bs,&nb_sectors); > - match = -1; > - first_match = -1; > - for (i = 0; ; i++) { > - parse =&fd_formats[i]; > - if (parse->drive == FDRIVE_DRV_NONE) { > - break; > - } > - if (drive_in == parse->drive || > - drive_in == FDRIVE_DRV_NONE) { > - size = (parse->max_head + 1) * parse->max_track * > - parse->last_sect; > - if (nb_sectors == size) { > - match = i; > - break; > - } > - if (first_match == -1) { > - first_match = i; > - } > - } > - } > - if (match == -1) { > - if (first_match == -1) { > - match = 1; > - } else { > - match = first_match; > - } > - parse =&fd_formats[match]; > - } > - *nb_heads = parse->max_head + 1; > - *max_track = parse->max_track; > - *last_sect = parse->last_sect; > - *drive = parse->drive; > - *rate = parse->rate; > -} > - > int bdrv_get_translation_hint(BlockDriverState *bs) > { > return bs->translation; > diff --git a/block.h b/block.h > index 3af93c6..c1c7500 100644 > --- a/block.h > +++ b/block.h > @@ -267,24 +267,6 @@ void bdrv_set_geometry_hint(BlockDriverState *bs, > void bdrv_set_translation_hint(BlockDriverState *bs, int translation); > void bdrv_get_geometry_hint(BlockDriverState *bs, > int *pcyls, int *pheads, int *psecs); > -typedef enum FDriveType { > - FDRIVE_DRV_144 = 0x00, /* 1.44 MB 3"5 drive */ > - FDRIVE_DRV_288 = 0x01, /* 2.88 MB 3"5 drive */ > - FDRIVE_DRV_120 = 0x02, /* 1.2 MB 5"25 drive */ > - FDRIVE_DRV_NONE = 0x03, /* No drive connected */ > -} FDriveType; > - > -typedef enum FDriveRate { > - FDRIVE_RATE_500K = 0x00, /* 500 Kbps */ > - FDRIVE_RATE_300K = 0x01, /* 300 Kbps */ > - FDRIVE_RATE_250K = 0x02, /* 250 Kbps */ > - FDRIVE_RATE_1M = 0x03, /* 1 Mbps */ > -} FDriveRate; > - > -void bdrv_get_floppy_geometry_hint(BlockDriverState *bs, int *nb_heads, > - int *max_track, int *last_sect, > - FDriveType drive_in, FDriveType *drive, > - FDriveRate *rate); > int bdrv_get_translation_hint(BlockDriverState *bs); > void bdrv_set_on_error(BlockDriverState *bs, BlockErrorAction on_read_error, > BlockErrorAction on_write_error); > diff --git a/hw/fdc.c b/hw/fdc.c > index edf0706..41191c7 100644 > --- a/hw/fdc.c > +++ b/hw/fdc.c > @@ -52,6 +52,113 @@ > /********************************************************/ > /* Floppy drive emulation */ > > +typedef enum FDriveRate { > + FDRIVE_RATE_500K = 0x00, /* 500 Kbps */ > + FDRIVE_RATE_300K = 0x01, /* 300 Kbps */ > + FDRIVE_RATE_250K = 0x02, /* 250 Kbps */ > + FDRIVE_RATE_1M = 0x03, /* 1 Mbps */ > +} FDriveRate; > + > +typedef struct FDFormat { > + FDriveType drive; > + uint8_t last_sect; > + uint8_t max_track; > + uint8_t max_head; > + FDriveRate rate; > +} FDFormat; > + > +static const FDFormat fd_formats[] = { > + /* First entry is default format */ > + /* 1.44 MB 3"1/2 floppy disks */ > + { FDRIVE_DRV_144, 18, 80, 1, FDRIVE_RATE_500K, }, > + { FDRIVE_DRV_144, 20, 80, 1, FDRIVE_RATE_500K, }, > + { FDRIVE_DRV_144, 21, 80, 1, FDRIVE_RATE_500K, }, > + { FDRIVE_DRV_144, 21, 82, 1, FDRIVE_RATE_500K, }, > + { FDRIVE_DRV_144, 21, 83, 1, FDRIVE_RATE_500K, }, > + { FDRIVE_DRV_144, 22, 80, 1, FDRIVE_RATE_500K, }, > + { FDRIVE_DRV_144, 23, 80, 1, FDRIVE_RATE_500K, }, > + { FDRIVE_DRV_144, 24, 80, 1, FDRIVE_RATE_500K, }, > + /* 2.88 MB 3"1/2 floppy disks */ > + { FDRIVE_DRV_288, 36, 80, 1, FDRIVE_RATE_1M, }, > + { FDRIVE_DRV_288, 39, 80, 1, FDRIVE_RATE_1M, }, > + { FDRIVE_DRV_288, 40, 80, 1, FDRIVE_RATE_1M, }, > + { FDRIVE_DRV_288, 44, 80, 1, FDRIVE_RATE_1M, }, > + { FDRIVE_DRV_288, 48, 80, 1, FDRIVE_RATE_1M, }, > + /* 720 kB 3"1/2 floppy disks */ > + { FDRIVE_DRV_144, 9, 80, 1, FDRIVE_RATE_250K, }, > + { FDRIVE_DRV_144, 10, 80, 1, FDRIVE_RATE_250K, }, > + { FDRIVE_DRV_144, 10, 82, 1, FDRIVE_RATE_250K, }, > + { FDRIVE_DRV_144, 10, 83, 1, FDRIVE_RATE_250K, }, > + { FDRIVE_DRV_144, 13, 80, 1, FDRIVE_RATE_250K, }, > + { FDRIVE_DRV_144, 14, 80, 1, FDRIVE_RATE_250K, }, > + /* 1.2 MB 5"1/4 floppy disks */ > + { FDRIVE_DRV_120, 15, 80, 1, FDRIVE_RATE_500K, }, > + { FDRIVE_DRV_120, 18, 80, 1, FDRIVE_RATE_500K, }, > + { FDRIVE_DRV_120, 18, 82, 1, FDRIVE_RATE_500K, }, > + { FDRIVE_DRV_120, 18, 83, 1, FDRIVE_RATE_500K, }, > + { FDRIVE_DRV_120, 20, 80, 1, FDRIVE_RATE_500K, }, > + /* 720 kB 5"1/4 floppy disks */ > + { FDRIVE_DRV_120, 9, 80, 1, FDRIVE_RATE_250K, }, > + { FDRIVE_DRV_120, 11, 80, 1, FDRIVE_RATE_250K, }, > + /* 360 kB 5"1/4 floppy disks */ > + { FDRIVE_DRV_120, 9, 40, 1, FDRIVE_RATE_300K, }, > + { FDRIVE_DRV_120, 9, 40, 0, FDRIVE_RATE_300K, }, > + { FDRIVE_DRV_120, 10, 41, 1, FDRIVE_RATE_300K, }, > + { FDRIVE_DRV_120, 10, 42, 1, FDRIVE_RATE_300K, }, > + /* 320 kB 5"1/4 floppy disks */ > + { FDRIVE_DRV_120, 8, 40, 1, FDRIVE_RATE_250K, }, > + { FDRIVE_DRV_120, 8, 40, 0, FDRIVE_RATE_250K, }, > + /* 360 kB must match 5"1/4 better than 3"1/2... */ > + { FDRIVE_DRV_144, 9, 80, 0, FDRIVE_RATE_250K, }, > + /* end */ > + { FDRIVE_DRV_NONE, -1, -1, 0, 0, }, > +}; > + > +static void pick_geometry(BlockDriverState *bs, int *nb_heads, > + int *max_track, int *last_sect, > + FDriveType drive_in, FDriveType *drive, > + FDriveRate *rate) > +{ > + const FDFormat *parse; > + uint64_t nb_sectors, size; > + int i, first_match, match; > + > + bdrv_get_geometry(bs,&nb_sectors); > + match = -1; > + first_match = -1; > + for (i = 0; ; i++) { > + parse =&fd_formats[i]; > + if (parse->drive == FDRIVE_DRV_NONE) { > + break; > + } > + if (drive_in == parse->drive || > + drive_in == FDRIVE_DRV_NONE) { > + size = (parse->max_head + 1) * parse->max_track * > + parse->last_sect; > + if (nb_sectors == size) { > + match = i; > + break; > + } > + if (first_match == -1) { > + first_match = i; > + } > + } > + } > + if (match == -1) { > + if (first_match == -1) { > + match = 1; > + } else { > + match = first_match; > + } > + parse =&fd_formats[match]; > + } > + *nb_heads = parse->max_head + 1; > + *max_track = parse->max_track; > + *last_sect = parse->last_sect; > + *drive = parse->drive; > + *rate = parse->rate; > +} > + > #define GET_CUR_DRV(fdctrl) ((fdctrl)->cur_drv) > #define SET_CUR_DRV(fdctrl, drive) ((fdctrl)->cur_drv = (drive)) > > @@ -187,8 +294,8 @@ static void fd_revalidate(FDrive *drv) > FLOPPY_DPRINTF("revalidate\n"); > if (drv->bs != NULL) { > ro = bdrv_is_read_only(drv->bs); > - bdrv_get_floppy_geometry_hint(drv->bs,&nb_heads,&max_track, > -&last_sect, drv->drive,&drive,&rate); > + pick_geometry(drv->bs,&nb_heads,&max_track, > +&last_sect, drv->drive,&drive,&rate); > if (!bdrv_is_inserted(drv->bs)) { > FLOPPY_DPRINTF("No disk in drive\n"); > } else { > @@ -2054,18 +2161,13 @@ static int sun4m_fdc_init1(SysBusDevice *dev) > return fdctrl_init_common(fdctrl); > } > > -void fdc_get_bs(BlockDriverState *bs[], ISADevice *dev) > +FDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i) > { > - FDCtrlISABus *isa = DO_UPCAST(FDCtrlISABus, busdev, dev); > - FDCtrl *fdctrl =&isa->state; > - int i; > + FDCtrlISABus *isa = DO_UPCAST(FDCtrlISABus, busdev, fdc); > > - for (i = 0; i< MAX_FD; i++) { > - bs[i] = fdctrl->drives[i].bs; > - } > + return isa->state.drives[i].drive; > } > > - > static const VMStateDescription vmstate_isa_fdc ={ > .name = "fdc", > .version_id = 2, > diff --git a/hw/fdc.h b/hw/fdc.h > index 1b32b17..b5c9f31 100644 > --- a/hw/fdc.h > +++ b/hw/fdc.h > @@ -6,11 +6,19 @@ > /* fdc.c */ > #define MAX_FD 2 > > +typedef enum FDriveType { > + FDRIVE_DRV_144 = 0x00, /* 1.44 MB 3"5 drive */ > + FDRIVE_DRV_288 = 0x01, /* 2.88 MB 3"5 drive */ > + FDRIVE_DRV_120 = 0x02, /* 1.2 MB 5"25 drive */ > + FDRIVE_DRV_NONE = 0x03, /* No drive connected */ > +} FDriveType; > + > ISADevice *fdctrl_init_isa(ISABus *bus, DriveInfo **fds); > void fdctrl_init_sysbus(qemu_irq irq, int dma_chann, > target_phys_addr_t mmio_base, DriveInfo **fds); > void sun4m_fdctrl_init(qemu_irq irq, target_phys_addr_t io_base, > DriveInfo **fds, qemu_irq *fdc_tc); > -void fdc_get_bs(BlockDriverState *bs[], ISADevice *dev); > + > +FDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i); > > #endif > 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... Regards, Anthony Liguori
Am 09.07.2012 17:01, schrieb Anthony Liguori: > On 07/09/2012 09:16 AM, Kevin Wolf wrote: >> From: Markus Armbruster<armbru@redhat.com> >> >> 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<armbru@redhat.com> >> Signed-off-by: Kevin Wolf<kwolf@redhat.com> >> 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. Kevin
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<armbru@redhat.com> >>> >>> 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<armbru@redhat.com> >>> Signed-off-by: Kevin Wolf<kwolf@redhat.com> > >>> 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
Kevin Wolf <kwolf@redhat.com> writes: > Am 09.07.2012 17:01, schrieb Anthony Liguori: >> On 07/09/2012 09:16 AM, Kevin Wolf wrote: >>> From: Markus Armbruster<armbru@redhat.com> >>> >>> 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<armbru@redhat.com> >>> Signed-off-by: Kevin Wolf<kwolf@redhat.com> > >>> 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.
On 07/09/2012 10:07 AM, Markus Armbruster wrote: >>> 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. It's probably not the version of the gcc you used, but whether or not your CFLAGS include -O2. Gcc has the (IMO very annoying) limitation that uninitialized-use analysis can only be performed if you are also doing optimization. You have to use a tool like clang or Coverity if you want more reliable uninitialized-use analysis even while building -O0 debug images.
On 07/09/2012 09:16 AM, Kevin Wolf wrote: > The following changes since commit 84988cf910a6881f2180fdcec516b60f8f0dc8c4: > > bitops.h: Add functions to extract and deposit bitfields (2012-07-07 09:07:01 +0000) > > are available in the git repository at: > > git://repo.or.cz/qemu/kevin.git for-anthony Pulled updated versiong. Thanks. Regards, Anthony Liguori > > MORITA Kazutaka (6): > sheepdog: fix dprintf format strings > sheepdog: restart I/O when socket becomes ready in do_co_req() > sheepdog: use coroutine based socket functions in coroutine context > sheepdog: make sure we don't free aiocb before sending all requests > sheepdog: split outstanding list into inflight and pending > sheepdog: traverse pending_list from the first for each time > > Markus Armbruster (4): > fdc: Drop broken code for user-defined floppy geometry > fdc: Move floppy geometry guessing back from block.c > qtest: Tidy up temporary files properly > block: Factor bdrv_read_unthrottled() out of guess_disk_lchs() > > Paolo Bonzini (8): > blkdebug: remove sync i/o events > blkdebug: tiny cleanup > blkdebug: pass getlength to underlying file > blkdebug: store list of active rules > blkdebug: optionally tie errors to a specific sector > raw: hook into blkdebug > block: copy over job and dirty bitmap fields in bdrv_append > block: introduce bdrv_swap, implement bdrv_append on top of it > > Pavel Hrdina (4): > fdc: rewrite seek and DSKCHG bit handling > fdc: fix interrupt handling > fdc_test: update media_change test > fdc_test: introduce test_sense_interrupt > > Stefan Hajnoczi (3): > qcow2: fix #ifdef'd qcow2_check_refcounts() callers > qcow2: preserve free_byte_offset when qcow2_alloc_bytes() fails > blockdev: warn when copy_on_read=on and readonly=on > > block.c | 306 +++++++++++++++++++----------------------------- > block.h | 23 +--- > block/blkdebug.c | 107 ++++++++++------- > block/qcow2-refcount.c | 7 +- > block/qcow2-snapshot.c | 6 +- > block/qcow2.c | 2 +- > block/qed.c | 2 +- > block/raw.c | 2 + > block/sheepdog.c | 130 +++++++++++++-------- > blockdev.c | 4 + > hw/fdc.c | 238 +++++++++++++++++++++++++++---------- > hw/fdc.h | 10 ++- > hw/pc.c | 13 +-- > tests/fdc-test.c | 50 +++++++-- > tests/libqtest.c | 29 +++-- > 15 files changed, 522 insertions(+), 407 deletions(-) > >
On 07/09/2012 11:46 AM, Eric Blake wrote: > On 07/09/2012 10:07 AM, Markus Armbruster wrote: > >>>> 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. > > It's probably not the version of the gcc you used, but whether or not > your CFLAGS include -O2. Gcc has the (IMO very annoying) limitation > that uninitialized-use analysis can only be performed if you are also > doing optimization. You have to use a tool like clang or Coverity if > you want more reliable uninitialized-use analysis even while building > -O0 debug images. > Specifically, without -O, GCC doesn't do data flow analysis so any warning that requires DFA won't get triggered. So in general, if you are normally building with -O0, make sure to also build with -O in order to get full warnings. Regards, Anthony Liguori
Am 09.07.2012 19:01, schrieb Anthony Liguori: > On 07/09/2012 11:46 AM, Eric Blake wrote: >> On 07/09/2012 10:07 AM, Markus Armbruster wrote: >> >>>>> 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. >> >> It's probably not the version of the gcc you used, but whether or not >> your CFLAGS include -O2. Gcc has the (IMO very annoying) limitation >> that uninitialized-use analysis can only be performed if you are also >> doing optimization. You have to use a tool like clang or Coverity if >> you want more reliable uninitialized-use analysis even while building >> -O0 debug images. >> > > Specifically, without -O, GCC doesn't do data flow analysis so any warning that > requires DFA won't get triggered. > > So in general, if you are normally building with -O0, make sure to also build > with -O in order to get full warnings. Just checked it to be sure, this doesn't seem to be the reason: CFLAGS=-O2 -D_FORTIFY_SOURCE=2 -g Kevin
The following changes since commit 84988cf910a6881f2180fdcec516b60f8f0dc8c4: bitops.h: Add functions to extract and deposit bitfields (2012-07-07 09:07:01 +0000) are available in the git repository at: git://repo.or.cz/qemu/kevin.git for-anthony MORITA Kazutaka (6): sheepdog: fix dprintf format strings sheepdog: restart I/O when socket becomes ready in do_co_req() sheepdog: use coroutine based socket functions in coroutine context sheepdog: make sure we don't free aiocb before sending all requests sheepdog: split outstanding list into inflight and pending sheepdog: traverse pending_list from the first for each time Markus Armbruster (4): fdc: Drop broken code for user-defined floppy geometry fdc: Move floppy geometry guessing back from block.c qtest: Tidy up temporary files properly block: Factor bdrv_read_unthrottled() out of guess_disk_lchs() Paolo Bonzini (8): blkdebug: remove sync i/o events blkdebug: tiny cleanup blkdebug: pass getlength to underlying file blkdebug: store list of active rules blkdebug: optionally tie errors to a specific sector raw: hook into blkdebug block: copy over job and dirty bitmap fields in bdrv_append block: introduce bdrv_swap, implement bdrv_append on top of it Pavel Hrdina (4): fdc: rewrite seek and DSKCHG bit handling fdc: fix interrupt handling fdc_test: update media_change test fdc_test: introduce test_sense_interrupt Stefan Hajnoczi (3): qcow2: fix #ifdef'd qcow2_check_refcounts() callers qcow2: preserve free_byte_offset when qcow2_alloc_bytes() fails blockdev: warn when copy_on_read=on and readonly=on block.c | 306 +++++++++++++++++++----------------------------- block.h | 23 +--- block/blkdebug.c | 107 ++++++++++------- block/qcow2-refcount.c | 7 +- block/qcow2-snapshot.c | 6 +- block/qcow2.c | 2 +- block/qed.c | 2 +- block/raw.c | 2 + block/sheepdog.c | 130 +++++++++++++-------- blockdev.c | 4 + hw/fdc.c | 238 +++++++++++++++++++++++++++---------- hw/fdc.h | 10 ++- hw/pc.c | 13 +-- tests/fdc-test.c | 50 +++++++-- tests/libqtest.c | 29 +++-- 15 files changed, 522 insertions(+), 407 deletions(-)