Re: [Qemu-devel] [PATCH 06/17] blockdev: Pass QDict to blockdev_init()

[Fam Zheng]

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 <address@hidden>
> ---
>  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
> 
>