2014-02-13 8:12 GMT+08:00 Eric Blake <ebl...@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 <wdon...@linux.vnet.ibm.com>
> > Signed-off-by: Chunyan Liu <cy...@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
>
>
