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

[Chunyan Liu <cyliu@suse.com>]

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

2014-02-13 8:12 GMT+08:00 Eric Blake <eblake@redhat.com>:

> 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?
>
> Will correct. Thanks.


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

[-- Attachment #2: Type: text/html, Size: 6723 bytes --]