| Message ID | 1379678070-14346-7-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
Le Friday 20 Sep 2013 à 13:54:19 (+0200), Kevin Wolf a écrit : > Working on a QDict instead of a QemuOpts that accepts anything is more > in line with bdrv_open(). A QDict is what qmp_blockdev_add() already has > anyway, so this saves additional conversions. And last, but not least, > it allows later patches to easily extract legacy options into a > separate, typed QemuOpts for drive_init() (the untyped QemuOpts that > drive_init already has doesn't allow access to numbers, only strings, > and is therefore useless without conversion). > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > blockdev.c | 33 +++++++++++++++------------------ > 1 file changed, 15 insertions(+), 18 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index e3cff31..3a1444c 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -302,7 +302,8 @@ static bool check_throttle_config(ThrottleConfig *cfg, Error **errp) > return true; > } > > -static DriveInfo *blockdev_init(QemuOpts *all_opts, > +/* Takes the ownership of bs_opts */ > +static DriveInfo *blockdev_init(QDict *bs_opts, > BlockInterfaceType block_default_type) > { > const char *buf; > @@ -326,7 +327,6 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts, > int ret; > Error *error = NULL; > QemuOpts *opts; > - QDict *bs_opts; > const char *id; > bool has_driver_specific_opts; > BlockDriver *drv = NULL; > @@ -334,9 +334,9 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts, > translation = BIOS_ATA_TRANSLATION_AUTO; > media = MEDIA_DISK; > > - /* Check common options by copying from all_opts to opts, all other options > - * are stored in bs_opts. */ > - id = qemu_opts_id(all_opts); > + /* Check common options by copying from bs_opts to opts, all other options > + * stay in bs_opts for processing by bdrv_open(). */ > + id = qdict_get_try_str(bs_opts, "id"); > opts = qemu_opts_create(&qemu_common_drive_opts, id, 1, &error); > if (error_is_set(&error)) { > qerror_report_err(error); > @@ -344,8 +344,6 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts, > return NULL; > } > > - bs_opts = qdict_new(); > - qemu_opts_to_qdict(all_opts, bs_opts); > qemu_opts_absorb_qdict(opts, bs_opts, &error); > if (error_is_set(&error)) { > qerror_report_err(error); > @@ -634,7 +632,6 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts, > dinfo->heads = heads; > dinfo->secs = secs; > dinfo->trans = translation; > - dinfo->opts = all_opts; > dinfo->refcount = 1; > if (serial != NULL) { > dinfo->serial = g_strdup(serial); > @@ -759,6 +756,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type) > { > const char *value; > DriveInfo *dinfo; > + QDict *bs_opts; > > /* Change legacy command line options into QMP ones */ > qemu_opt_rename(all_opts, "iops", "throttling.iops-total"); > @@ -807,14 +805,19 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type) > qemu_opt_unset(all_opts, "cache"); > } > > + /* Get a QDict for processing the options */ > + bs_opts = qdict_new(); > + qemu_opts_to_qdict(all_opts, bs_opts); > + > /* Actual block device init: Functionality shared with blockdev-add */ > - dinfo = blockdev_init(all_opts, block_default_type); > + dinfo = blockdev_init(bs_opts, block_default_type); > if (dinfo == NULL) { > goto fail; > } > > /* Set legacy DriveInfo fields */ > dinfo->enable_auto_del = true; > + dinfo->opts = all_opts; > > fail: > return dinfo; > @@ -2109,16 +2112,10 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp) > > qdict_flatten(qdict); > > - QemuOpts *opts = qemu_opts_from_qdict(&qemu_drive_opts, qdict, &local_err); > - if (error_is_set(&local_err)) { > - error_propagate(errp, local_err); > + dinfo = blockdev_init(qdict, IF_NONE); > + if (!dinfo) { > + error_setg(errp, "Could not open image"); > goto fail; > - } else { > - dinfo = blockdev_init(opts, IF_NONE); > - if (!dinfo) { > - error_setg(errp, "Could not open image"); > - goto fail; > - } > } > > fail: > -- > 1.8.1.4 > > Reviewed-by: Benoit Canet <benoit@irqsave.net>
On Fri, 09/20 13:54, Kevin Wolf wrote: > Working on a QDict instead of a QemuOpts that accepts anything is more > in line with bdrv_open(). A QDict is what qmp_blockdev_add() already has > anyway, so this saves additional conversions. And last, but not least, > it allows later patches to easily extract legacy options into a > separate, typed QemuOpts for drive_init() (the untyped QemuOpts that > drive_init already has doesn't allow access to numbers, only strings, > and is therefore useless without conversion). > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > blockdev.c | 33 +++++++++++++++------------------ > 1 file changed, 15 insertions(+), 18 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index e3cff31..3a1444c 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -302,7 +302,8 @@ static bool check_throttle_config(ThrottleConfig *cfg, Error **errp) > return true; > } > > -static DriveInfo *blockdev_init(QemuOpts *all_opts, > +/* Takes the ownership of bs_opts */ > +static DriveInfo *blockdev_init(QDict *bs_opts, > BlockInterfaceType block_default_type) > { > const char *buf; > @@ -326,7 +327,6 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts, > int ret; > Error *error = NULL; > QemuOpts *opts; > - QDict *bs_opts; > const char *id; > bool has_driver_specific_opts; > BlockDriver *drv = NULL; > @@ -334,9 +334,9 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts, > translation = BIOS_ATA_TRANSLATION_AUTO; > media = MEDIA_DISK; > > - /* Check common options by copying from all_opts to opts, all other options > - * are stored in bs_opts. */ > - id = qemu_opts_id(all_opts); > + /* Check common options by copying from bs_opts to opts, all other options > + * stay in bs_opts for processing by bdrv_open(). */ > + id = qdict_get_try_str(bs_opts, "id"); > opts = qemu_opts_create(&qemu_common_drive_opts, id, 1, &error); > if (error_is_set(&error)) { > qerror_report_err(error); > @@ -344,8 +344,6 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts, > return NULL; > } > > - bs_opts = qdict_new(); > - qemu_opts_to_qdict(all_opts, bs_opts); > qemu_opts_absorb_qdict(opts, bs_opts, &error); > if (error_is_set(&error)) { > qerror_report_err(error); > @@ -634,7 +632,6 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts, > dinfo->heads = heads; > dinfo->secs = secs; > dinfo->trans = translation; > - dinfo->opts = all_opts; Not setting dinfo->opts in blockdev-add will SIGSEGV drive_del: static void drive_uninit(DriveInfo *dinfo) { qemu_opts_del(dinfo->opts); ... and: void qemu_opts_del(QemuOpts *opts) { QemuOpt *opt; for (;;) { opt = QTAILQ_FIRST(&opts->head); ... Fam > dinfo->refcount = 1; > if (serial != NULL) { > dinfo->serial = g_strdup(serial); > @@ -759,6 +756,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type) > { > const char *value; > DriveInfo *dinfo; > + QDict *bs_opts; > > /* Change legacy command line options into QMP ones */ > qemu_opt_rename(all_opts, "iops", "throttling.iops-total"); > @@ -807,14 +805,19 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type) > qemu_opt_unset(all_opts, "cache"); > } > > + /* Get a QDict for processing the options */ > + bs_opts = qdict_new(); > + qemu_opts_to_qdict(all_opts, bs_opts); > + > /* Actual block device init: Functionality shared with blockdev-add */ > - dinfo = blockdev_init(all_opts, block_default_type); > + dinfo = blockdev_init(bs_opts, block_default_type); > if (dinfo == NULL) { > goto fail; > } > > /* Set legacy DriveInfo fields */ > dinfo->enable_auto_del = true; > + dinfo->opts = all_opts; > > fail: > return dinfo; > @@ -2109,16 +2112,10 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp) > > qdict_flatten(qdict); > > - QemuOpts *opts = qemu_opts_from_qdict(&qemu_drive_opts, qdict, &local_err); > - if (error_is_set(&local_err)) { > - error_propagate(errp, local_err); > + dinfo = blockdev_init(qdict, IF_NONE); > + if (!dinfo) { > + error_setg(errp, "Could not open image"); > goto fail; > - } else { > - dinfo = blockdev_init(opts, IF_NONE); > - if (!dinfo) { > - error_setg(errp, "Could not open image"); > - goto fail; > - } > } > > fail: > -- > 1.8.1.4 > >
diff --git a/blockdev.c b/blockdev.c index e3cff31..3a1444c 100644 --- a/blockdev.c +++ b/blockdev.c @@ -302,7 +302,8 @@ static bool check_throttle_config(ThrottleConfig *cfg, Error **errp) return true; } -static DriveInfo *blockdev_init(QemuOpts *all_opts, +/* Takes the ownership of bs_opts */ +static DriveInfo *blockdev_init(QDict *bs_opts, BlockInterfaceType block_default_type) { const char *buf; @@ -326,7 +327,6 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts, int ret; Error *error = NULL; QemuOpts *opts; - QDict *bs_opts; const char *id; bool has_driver_specific_opts; BlockDriver *drv = NULL; @@ -334,9 +334,9 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts, translation = BIOS_ATA_TRANSLATION_AUTO; media = MEDIA_DISK; - /* Check common options by copying from all_opts to opts, all other options - * are stored in bs_opts. */ - id = qemu_opts_id(all_opts); + /* Check common options by copying from bs_opts to opts, all other options + * stay in bs_opts for processing by bdrv_open(). */ + id = qdict_get_try_str(bs_opts, "id"); opts = qemu_opts_create(&qemu_common_drive_opts, id, 1, &error); if (error_is_set(&error)) { qerror_report_err(error); @@ -344,8 +344,6 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts, return NULL; } - bs_opts = qdict_new(); - qemu_opts_to_qdict(all_opts, bs_opts); qemu_opts_absorb_qdict(opts, bs_opts, &error); if (error_is_set(&error)) { qerror_report_err(error); @@ -634,7 +632,6 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts, dinfo->heads = heads; dinfo->secs = secs; dinfo->trans = translation; - dinfo->opts = all_opts; dinfo->refcount = 1; if (serial != NULL) { dinfo->serial = g_strdup(serial); @@ -759,6 +756,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type) { const char *value; DriveInfo *dinfo; + QDict *bs_opts; /* Change legacy command line options into QMP ones */ qemu_opt_rename(all_opts, "iops", "throttling.iops-total"); @@ -807,14 +805,19 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type) qemu_opt_unset(all_opts, "cache"); } + /* Get a QDict for processing the options */ + bs_opts = qdict_new(); + qemu_opts_to_qdict(all_opts, bs_opts); + /* Actual block device init: Functionality shared with blockdev-add */ - dinfo = blockdev_init(all_opts, block_default_type); + dinfo = blockdev_init(bs_opts, block_default_type); if (dinfo == NULL) { goto fail; } /* Set legacy DriveInfo fields */ dinfo->enable_auto_del = true; + dinfo->opts = all_opts; fail: return dinfo; @@ -2109,16 +2112,10 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp) qdict_flatten(qdict); - QemuOpts *opts = qemu_opts_from_qdict(&qemu_drive_opts, qdict, &local_err); - if (error_is_set(&local_err)) { - error_propagate(errp, local_err); + dinfo = blockdev_init(qdict, IF_NONE); + if (!dinfo) { + error_setg(errp, "Could not open image"); goto fail; - } else { - dinfo = blockdev_init(opts, IF_NONE); - if (!dinfo) { - error_setg(errp, "Could not open image"); - goto fail; - } } fail:
Working on a QDict instead of a QemuOpts that accepts anything is more in line with bdrv_open(). A QDict is what qmp_blockdev_add() already has anyway, so this saves additional conversions. And last, but not least, it allows later patches to easily extract legacy options into a separate, typed QemuOpts for drive_init() (the untyped QemuOpts that drive_init already has doesn't allow access to numbers, only strings, and is therefore useless without conversion). Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- blockdev.c | 33 +++++++++++++++------------------ 1 file changed, 15 insertions(+), 18 deletions(-)