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 <kw...@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
>
>
