| Message ID | 1410891148-28849-19-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
> -- > 1.9.3 > Reviewed-by: Benoit Canet <benoit.canet@nodalink.com>
On 16.09.2014 20:12, Markus Armbruster wrote: > blockdev_init() always creates a DriveInfo, but only drive_new() fills > it in. qmp_blockdev_add() leaves it blank. This results in a drive > with type = IF_IDE, bus = 0, unit = 0. Screwed up in commit ee13ed1c. > > Board initialization code looking for IDE drive (0,0) can pick up one > of these bogus drives. Not sure whether getting the QMP command > executed early enough is likely in practice, though. > > Fix by creating DriveInfo in drive_new(). Block backends created by > blockdev-add don't get one. > > A few places assume a block backend always has a DriveInfo. Fix them > up. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > block/block-backend.c | 4 +--- > blockdev.c | 10 ++-------- > hw/block/block.c | 16 ++++++++++------ > hw/ide/qdev.c | 2 +- > hw/scsi/scsi-disk.c | 2 +- > 5 files changed, 15 insertions(+), 19 deletions(-) Reviewed-by: Max Reitz <mreitz@redhat.com>
Am 16.09.2014 um 20:12 hat Markus Armbruster geschrieben: > blockdev_init() always creates a DriveInfo, but only drive_new() fills > it in. qmp_blockdev_add() leaves it blank. This results in a drive > with type = IF_IDE, bus = 0, unit = 0. Screwed up in commit ee13ed1c. > > Board initialization code looking for IDE drive (0,0) can pick up one > of these bogus drives. Not sure whether getting the QMP command > executed early enough is likely in practice, though. > > Fix by creating DriveInfo in drive_new(). Block backends created by > blockdev-add don't get one. > > A few places assume a block backend always has a DriveInfo. Fix them > up. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > block/block-backend.c | 4 +--- > blockdev.c | 10 ++-------- > hw/block/block.c | 16 ++++++++++------ > hw/ide/qdev.c | 2 +- > hw/scsi/scsi-disk.c | 2 +- > 5 files changed, 15 insertions(+), 19 deletions(-) > > diff --git a/block/block-backend.c b/block/block-backend.c > index 141a31b..b55f0b4 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -101,9 +101,7 @@ static void drive_info_del(DriveInfo *dinfo) > if (!dinfo) { > return; > } > - if (dinfo->opts) { > - qemu_opts_del(dinfo->opts); > - } > + qemu_opts_del(dinfo->opts); > g_free(dinfo->serial); > g_free(dinfo); > } Doesn't look directly related? > diff --git a/blockdev.c b/blockdev.c > index 2def256..0d99be0 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -285,7 +285,6 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, > int on_read_error, on_write_error; > BlockBackend *blk; > BlockDriverState *bs; > - DriveInfo *dinfo; > ThrottleConfig cfg; > int snapshot = 0; > bool copy_on_read; > @@ -457,9 +456,6 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, > bdrv_set_io_limits(bs, &cfg); > } > > - dinfo = g_malloc0(sizeof(*dinfo)); > - blk_set_legacy_dinfo(blk, dinfo); > - > if (!file || !*file) { > if (has_driver_specific_opts) { > file = NULL; > @@ -903,21 +899,19 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type) > } > > /* Set legacy DriveInfo fields */ > - dinfo = blk_legacy_dinfo(blk); > + dinfo = g_malloc0(sizeof(*dinfo)); > dinfo->enable_auto_del = true; > dinfo->opts = all_opts; > - > dinfo->cyls = cyls; > dinfo->heads = heads; > dinfo->secs = secs; > dinfo->trans = translation; > - > dinfo->type = type; > dinfo->bus = bus_id; > dinfo->unit = unit_id; > dinfo->devaddr = devaddr; > - > dinfo->serial = g_strdup(serial); > + blk_set_legacy_dinfo(blk, dinfo); You don't like the grouping? > switch(type) { > case IF_IDE: Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 16.09.2014 um 20:12 hat Markus Armbruster geschrieben: >> blockdev_init() always creates a DriveInfo, but only drive_new() fills >> it in. qmp_blockdev_add() leaves it blank. This results in a drive >> with type = IF_IDE, bus = 0, unit = 0. Screwed up in commit ee13ed1c. >> >> Board initialization code looking for IDE drive (0,0) can pick up one >> of these bogus drives. Not sure whether getting the QMP command >> executed early enough is likely in practice, though. >> >> Fix by creating DriveInfo in drive_new(). Block backends created by >> blockdev-add don't get one. >> >> A few places assume a block backend always has a DriveInfo. Fix them >> up. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> block/block-backend.c | 4 +--- >> blockdev.c | 10 ++-------- >> hw/block/block.c | 16 ++++++++++------ >> hw/ide/qdev.c | 2 +- >> hw/scsi/scsi-disk.c | 2 +- >> 5 files changed, 15 insertions(+), 19 deletions(-) >> >> diff --git a/block/block-backend.c b/block/block-backend.c >> index 141a31b..b55f0b4 100644 >> --- a/block/block-backend.c >> +++ b/block/block-backend.c >> @@ -101,9 +101,7 @@ static void drive_info_del(DriveInfo *dinfo) >> if (!dinfo) { >> return; >> } >> - if (dinfo->opts) { >> - qemu_opts_del(dinfo->opts); >> - } >> + qemu_opts_del(dinfo->opts); >> g_free(dinfo->serial); >> g_free(dinfo); >> } > > Doesn't look directly related? I'll split it off. >> diff --git a/blockdev.c b/blockdev.c >> index 2def256..0d99be0 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -285,7 +285,6 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, >> int on_read_error, on_write_error; >> BlockBackend *blk; >> BlockDriverState *bs; >> - DriveInfo *dinfo; >> ThrottleConfig cfg; >> int snapshot = 0; >> bool copy_on_read; >> @@ -457,9 +456,6 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, >> bdrv_set_io_limits(bs, &cfg); >> } >> >> - dinfo = g_malloc0(sizeof(*dinfo)); >> - blk_set_legacy_dinfo(blk, dinfo); >> - >> if (!file || !*file) { >> if (has_driver_specific_opts) { >> file = NULL; >> @@ -903,21 +899,19 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type) >> } >> >> /* Set legacy DriveInfo fields */ >> - dinfo = blk_legacy_dinfo(blk); >> + dinfo = g_malloc0(sizeof(*dinfo)); >> dinfo->enable_auto_del = true; >> dinfo->opts = all_opts; >> - >> dinfo->cyls = cyls; >> dinfo->heads = heads; >> dinfo->secs = secs; >> dinfo->trans = translation; >> - >> dinfo->type = type; >> dinfo->bus = bus_id; >> dinfo->unit = unit_id; >> dinfo->devaddr = devaddr; >> - >> dinfo->serial = g_strdup(serial); >> + blk_set_legacy_dinfo(blk, dinfo); > > You don't like the grouping? No, because the comment appears as if it applied only to the first group. This is how this spot looks at the end of the series: /* Set legacy DriveInfo fields */ dinfo = g_malloc0(sizeof(*dinfo)); dinfo->opts = all_opts; dinfo->cyls = cyls; dinfo->heads = heads; dinfo->secs = secs; dinfo->trans = translation; dinfo->type = type; dinfo->bus = bus_id; dinfo->unit = unit_id; dinfo->devaddr = devaddr; dinfo->serial = g_strdup(serial); blk_set_legacy_dinfo(blk, dinfo); Could do this instead: dinfo = g_malloc0(sizeof(*dinfo)); dinfo->opts = all_opts; dinfo->cyls = cyls; dinfo->heads = heads; dinfo->secs = secs; dinfo->trans = translation; dinfo->type = type; dinfo->bus = bus_id; dinfo->unit = unit_id; dinfo->devaddr = devaddr; dinfo->serial = g_strdup(serial); blk_set_legacy_dinfo(blk, dinfo); The comment is next to useless anyway. Got a preference? >> switch(type) { >> case IF_IDE:
Am 16.09.2014 um 20:12 hat Markus Armbruster geschrieben: > blockdev_init() always creates a DriveInfo, but only drive_new() fills > it in. qmp_blockdev_add() leaves it blank. This results in a drive > with type = IF_IDE, bus = 0, unit = 0. Screwed up in commit ee13ed1c. > > Board initialization code looking for IDE drive (0,0) can pick up one > of these bogus drives. Not sure whether getting the QMP command > executed early enough is likely in practice, though. > > Fix by creating DriveInfo in drive_new(). Block backends created by > blockdev-add don't get one. > > A few places assume a block backend always has a DriveInfo. Fix them > up. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> I think I would change the subject line to describe that blockdev-add devices don't get a DriveInfo any more. The IF_IDE = 0 bug is real, but probably not the most important part of this patch. As you already mentioned, you need to squash in some parts of patch 19 for this to be correct (the changed if conditions in do_drive_del and blockdev_mark_auto_del). You may still leave patch 19 for the removal of enable_auto_del if you like, or squash in the whole patch. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 16.09.2014 um 20:12 hat Markus Armbruster geschrieben: >> blockdev_init() always creates a DriveInfo, but only drive_new() fills >> it in. qmp_blockdev_add() leaves it blank. This results in a drive >> with type = IF_IDE, bus = 0, unit = 0. Screwed up in commit ee13ed1c. >> >> Board initialization code looking for IDE drive (0,0) can pick up one >> of these bogus drives. Not sure whether getting the QMP command >> executed early enough is likely in practice, though. >> >> Fix by creating DriveInfo in drive_new(). Block backends created by >> blockdev-add don't get one. >> >> A few places assume a block backend always has a DriveInfo. Fix them >> up. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > > I think I would change the subject line to describe that blockdev-add > devices don't get a DriveInfo any more. The IF_IDE = 0 bug is real, but > probably not the most important part of this patch. > > As you already mentioned, you need to squash in some parts of patch 19 > for this to be correct (the changed if conditions in do_drive_del and > blockdev_mark_auto_del). You may still leave patch 19 for the removal > of enable_auto_del if you like, or squash in the whole patch. I'll see how the reworked pair of patches turns out, then think about the subject line.
Am 29.09.2014 um 15:05 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > Am 16.09.2014 um 20:12 hat Markus Armbruster geschrieben: > >> @@ -903,21 +899,19 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type) > >> } > >> > >> /* Set legacy DriveInfo fields */ > >> - dinfo = blk_legacy_dinfo(blk); > >> + dinfo = g_malloc0(sizeof(*dinfo)); > >> dinfo->enable_auto_del = true; > >> dinfo->opts = all_opts; > >> - > >> dinfo->cyls = cyls; > >> dinfo->heads = heads; > >> dinfo->secs = secs; > >> dinfo->trans = translation; > >> - > >> dinfo->type = type; > >> dinfo->bus = bus_id; > >> dinfo->unit = unit_id; > >> dinfo->devaddr = devaddr; > >> - > >> dinfo->serial = g_strdup(serial); > >> + blk_set_legacy_dinfo(blk, dinfo); > > > > You don't like the grouping? > > No, because the comment appears as if it applied only to the first > group. > > This is how this spot looks at the end of the series: > > /* Set legacy DriveInfo fields */ > dinfo = g_malloc0(sizeof(*dinfo)); > dinfo->opts = all_opts; > dinfo->cyls = cyls; > dinfo->heads = heads; > dinfo->secs = secs; > dinfo->trans = translation; > dinfo->type = type; > dinfo->bus = bus_id; > dinfo->unit = unit_id; > dinfo->devaddr = devaddr; > dinfo->serial = g_strdup(serial); > blk_set_legacy_dinfo(blk, dinfo); > > Could do this instead: > > dinfo = g_malloc0(sizeof(*dinfo)); > dinfo->opts = all_opts; > > dinfo->cyls = cyls; > dinfo->heads = heads; > dinfo->secs = secs; > dinfo->trans = translation; > > dinfo->type = type; > dinfo->bus = bus_id; > dinfo->unit = unit_id; > dinfo->devaddr = devaddr; > dinfo->serial = g_strdup(serial); > > blk_set_legacy_dinfo(blk, dinfo); > > The comment is next to useless anyway. Got a preference? The reason why it's there is that I like to use comments to give "headlines" to the blocks of code. In drive_new(), I can only read the comments without looking at the code and understand what functionality is implemented at a high level. So for me the "headline" is valid until the next one appears (except that this is the last one) and it's good as it is today. Your taste differs, as it seems. If I have to choose between your two alternatives, I'll reluctantly vote for removing the empty lines, because making it part of the "Actual block device init" block by removing the comment makes even less sense. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 29.09.2014 um 15:05 hat Markus Armbruster geschrieben: >> Kevin Wolf <kwolf@redhat.com> writes: >> >> > Am 16.09.2014 um 20:12 hat Markus Armbruster geschrieben: >> >> @@ -903,21 +899,19 @@ DriveInfo *drive_new(QemuOpts *all_opts, >> >> BlockInterfaceType block_default_type) >> >> } >> >> >> >> /* Set legacy DriveInfo fields */ >> >> - dinfo = blk_legacy_dinfo(blk); >> >> + dinfo = g_malloc0(sizeof(*dinfo)); >> >> dinfo->enable_auto_del = true; >> >> dinfo->opts = all_opts; >> >> - >> >> dinfo->cyls = cyls; >> >> dinfo->heads = heads; >> >> dinfo->secs = secs; >> >> dinfo->trans = translation; >> >> - >> >> dinfo->type = type; >> >> dinfo->bus = bus_id; >> >> dinfo->unit = unit_id; >> >> dinfo->devaddr = devaddr; >> >> - >> >> dinfo->serial = g_strdup(serial); >> >> + blk_set_legacy_dinfo(blk, dinfo); >> > >> > You don't like the grouping? >> >> No, because the comment appears as if it applied only to the first >> group. >> >> This is how this spot looks at the end of the series: >> >> /* Set legacy DriveInfo fields */ >> dinfo = g_malloc0(sizeof(*dinfo)); >> dinfo->opts = all_opts; >> dinfo->cyls = cyls; >> dinfo->heads = heads; >> dinfo->secs = secs; >> dinfo->trans = translation; >> dinfo->type = type; >> dinfo->bus = bus_id; >> dinfo->unit = unit_id; >> dinfo->devaddr = devaddr; >> dinfo->serial = g_strdup(serial); >> blk_set_legacy_dinfo(blk, dinfo); >> >> Could do this instead: >> >> dinfo = g_malloc0(sizeof(*dinfo)); >> dinfo->opts = all_opts; >> >> dinfo->cyls = cyls; >> dinfo->heads = heads; >> dinfo->secs = secs; >> dinfo->trans = translation; >> >> dinfo->type = type; >> dinfo->bus = bus_id; >> dinfo->unit = unit_id; >> dinfo->devaddr = devaddr; >> dinfo->serial = g_strdup(serial); >> >> blk_set_legacy_dinfo(blk, dinfo); >> >> The comment is next to useless anyway. Got a preference? > > The reason why it's there is that I like to use comments to give > "headlines" to the blocks of code. In drive_new(), I can only read the > comments without looking at the code and understand what functionality > is implemented at a high level. > > So for me the "headline" is valid until the next one appears (except > that this is the last one) and it's good as it is today. Your taste > differs, as it seems. > > If I have to choose between your two alternatives, I'll reluctantly vote > for removing the empty lines, because making it part of the "Actual > block device init" block by removing the comment makes even less sense. Okay, you care for the headline comment and the grouping more than I dislike them, so I'll keep them both.
diff --git a/block/block-backend.c b/block/block-backend.c index 141a31b..b55f0b4 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -101,9 +101,7 @@ static void drive_info_del(DriveInfo *dinfo) if (!dinfo) { return; } - if (dinfo->opts) { - qemu_opts_del(dinfo->opts); - } + qemu_opts_del(dinfo->opts); g_free(dinfo->serial); g_free(dinfo); } diff --git a/blockdev.c b/blockdev.c index 2def256..0d99be0 100644 --- a/blockdev.c +++ b/blockdev.c @@ -285,7 +285,6 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, int on_read_error, on_write_error; BlockBackend *blk; BlockDriverState *bs; - DriveInfo *dinfo; ThrottleConfig cfg; int snapshot = 0; bool copy_on_read; @@ -457,9 +456,6 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, bdrv_set_io_limits(bs, &cfg); } - dinfo = g_malloc0(sizeof(*dinfo)); - blk_set_legacy_dinfo(blk, dinfo); - if (!file || !*file) { if (has_driver_specific_opts) { file = NULL; @@ -903,21 +899,19 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type) } /* Set legacy DriveInfo fields */ - dinfo = blk_legacy_dinfo(blk); + dinfo = g_malloc0(sizeof(*dinfo)); dinfo->enable_auto_del = true; dinfo->opts = all_opts; - dinfo->cyls = cyls; dinfo->heads = heads; dinfo->secs = secs; dinfo->trans = translation; - dinfo->type = type; dinfo->bus = bus_id; dinfo->unit = unit_id; dinfo->devaddr = devaddr; - dinfo->serial = g_strdup(serial); + blk_set_legacy_dinfo(blk, dinfo); switch(type) { case IF_IDE: diff --git a/hw/block/block.c b/hw/block/block.c index 0666dd3..a625773 100644 --- a/hw/block/block.c +++ b/hw/block/block.c @@ -19,7 +19,9 @@ void blkconf_serial(BlockConf *conf, char **serial) if (!*serial) { /* try to fall back to value set with legacy -drive serial=... */ dinfo = blk_legacy_dinfo(conf->blk); - *serial = g_strdup(dinfo->serial); + if (dinfo) { + *serial = g_strdup(dinfo->serial); + } } } @@ -32,11 +34,13 @@ void blkconf_geometry(BlockConf *conf, int *ptrans, if (!conf->cyls && !conf->heads && !conf->secs) { /* try to fall back to value set with legacy -drive cyls=... */ dinfo = blk_legacy_dinfo(conf->blk); - conf->cyls = dinfo->cyls; - conf->heads = dinfo->heads; - conf->secs = dinfo->secs; - if (ptrans) { - *ptrans = dinfo->trans; + if (dinfo) { + conf->cyls = dinfo->cyls; + conf->heads = dinfo->heads; + conf->secs = dinfo->secs; + if (ptrans) { + *ptrans = dinfo->trans; + } } } if (!conf->cyls && !conf->heads && !conf->secs) { diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c index 4818334..a74c81e 100644 --- a/hw/ide/qdev.c +++ b/hw/ide/qdev.c @@ -206,7 +206,7 @@ static int ide_drive_initfn(IDEDevice *dev) { DriveInfo *dinfo = blk_legacy_dinfo(dev->conf.blk); - return ide_dev_initfn(dev, dinfo->media_cd ? IDE_CD : IDE_HD); + return ide_dev_initfn(dev, dinfo && dinfo->media_cd ? IDE_CD : IDE_HD); } #define DEFINE_IDE_DEV_PROPERTIES() \ diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index b0bca48..9deef1a 100644 --- a/hw/scsi/scsi-disk.c +++ b/hw/scsi/scsi-disk.c @@ -2336,7 +2336,7 @@ static void scsi_disk_realize(SCSIDevice *dev, Error **errp) } dinfo = blk_legacy_dinfo(dev->conf.blk); - if (dinfo->media_cd) { + if (dinfo && dinfo->media_cd) { scsi_cd_realize(dev, errp); } else { scsi_hd_realize(dev, errp);
blockdev_init() always creates a DriveInfo, but only drive_new() fills it in. qmp_blockdev_add() leaves it blank. This results in a drive with type = IF_IDE, bus = 0, unit = 0. Screwed up in commit ee13ed1c. Board initialization code looking for IDE drive (0,0) can pick up one of these bogus drives. Not sure whether getting the QMP command executed early enough is likely in practice, though. Fix by creating DriveInfo in drive_new(). Block backends created by blockdev-add don't get one. A few places assume a block backend always has a DriveInfo. Fix them up. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- block/block-backend.c | 4 +--- blockdev.c | 10 ++-------- hw/block/block.c | 16 ++++++++++------ hw/ide/qdev.c | 2 +- hw/scsi/scsi-disk.c | 2 +- 5 files changed, 15 insertions(+), 19 deletions(-)