Re: [Qemu-devel] [PATCH v20 06/26] change block layer to support both QemuOpts and QEMUOptionParameter

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4488 bytes --]

On 02/11/2014 11:33 PM, Chunyan Liu wrote:
> Change block layer to support both QemuOpts and QEMUOptionParameter.
> After this patch, it will change backend drivers one by one. At the end,
> QEMUOptionParameter will be removed and only QemuOpts is kept.
> 
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> ---
>  block.c                   |  110 ++++++++++++++++++++++++++++----------------
>  block/cow.c               |    2 +-
>  block/qcow.c              |    2 +-
>  block/qcow2.c             |    2 +-
>  block/qed.c               |    2 +-
>  block/raw_bsd.c           |    2 +-
>  block/vhdx.c              |    2 +-
>  block/vmdk.c              |    4 +-
>  block/vvfat.c             |    2 +-
>  include/block/block.h     |    4 +-
>  include/block/block_int.h |    4 +-
>  include/qemu/option.h     |    2 +
>  qemu-img.c                |   87 +++++++++++++++++++++--------------
>  util/qemu-option.c        |  111 +++++++++++++++++++++++++++++++++++++++++++++
>  14 files changed, 250 insertions(+), 86 deletions(-)
> 
> +++ b/include/block/block_int.h
> @@ -118,6 +118,8 @@ struct BlockDriver {
>      void (*bdrv_rebind)(BlockDriverState *bs);
>      int (*bdrv_create)(const char *filename, QEMUOptionParameter *options,
>                         Error **errp);
> +    int (*bdrv_create2)(const char *filename, QemuOpts *opts,
> +                       Error **errp);

Maybe a FIXME comment that shows we plan on removing the duplicate and
renaming back to a single sane name in a few more commits.

> +++ b/include/qemu/option.h
> @@ -168,4 +168,6 @@ int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void *opaque,
>  QemuOptsList *qemu_opts_append(QemuOptsList *dst, QemuOptsList *list);
>  void qemu_opts_free(QemuOptsList *list);
>  void qemu_opts_print_help(QemuOptsList *list);
> +QEMUOptionParameter *opts_to_params(QemuOpts *opts);
> +QemuOptsList *params_to_opts(QEMUOptionParameter *list);

I'd split this commit into two - one that adds these two conversion
functions, then the other that adds the create2 callback.

> +++ b/util/qemu-option.c
> @@ -1396,3 +1396,114 @@ void qemu_opts_print_help(QemuOptsList *list)
>                 list->desc[i].help : "");
>      }
>  }
> +
> +/* convert QEMUOptionParameter to QemuOpts */
> +QemuOptsList *params_to_opts(QEMUOptionParameter *list)
> +{
> +    QemuOptsList *opts = NULL;
> +    size_t num_opts, i = 0;
> +
> +    if (!list) {
> +        return NULL;
> +    }
> +
> +    num_opts = count_option_parameters(list);
> +    opts = g_malloc0(sizeof(QemuOptsList) +
> +                              (num_opts + 1) * sizeof(QemuOptDesc));

Indentation looks off.

> +    QTAILQ_INIT(&opts->head);
> +    opts->desc[i].name = NULL;
> +
> +    while (list && list->name) {
> +        opts->desc[i].name = strdup(list->name);
> +        opts->desc[i].help = strdup(list->help);
> +        switch (list->type) {
> +        case OPT_FLAG:
> +            opts->desc[i].type = QEMU_OPT_BOOL;
> +            opts->desc[i].def_value_str = list->value.n ? "on" : "off";
> +            break;
> +
> +        case OPT_NUMBER:
> +            opts->desc[i].type = QEMU_OPT_NUMBER;
> +            if (list->value.n) {
> +                char tmp[100];
> +                sprintf(tmp, "%" PRIu64, list->value.n);
> +                opts->desc[i].def_value_str = strdup(tmp);

Eww.  Just use g_strdup_printf and avoid tmp[] altogether.

> +            }
> +            break;
> +
> +        case OPT_SIZE:
> +            opts->desc[i].type = QEMU_OPT_SIZE;
> +            if (list->value.n) {
> +                char tmp[100];
> +                sprintf(tmp, "%" PRIu64, list->value.n);
> +                opts->desc[i].def_value_str = strdup(tmp);

and again

> +            }
> +            break;
> +
> +        case OPT_STRING:
> +            opts->desc[i].type = QEMU_OPT_STRING;
> +            if (list->value.s) {
> +                opts->desc[i].def_value_str = strdup(list->value.s);

This is a lot of use of strdup() without checking for malloc failure;
wouldn't it be better to use g_strdup (which can't fail, and which
gracefully handles NULL so you can also drop the 'if') and fixing the
counterpart free to use g_free?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]