qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v22 07/25] change block layer to support both Qe


From: Chunyan Liu
Subject: Re: [Qemu-devel] [PATCH v22 07/25] change block layer to support both QemuOpts and QEMUOptionParamter
Date: Wed, 12 Mar 2014 14:26:25 +0800




2014-03-12 0:54 GMT+08:00 Eric Blake <address@hidden>:
On 03/10/2014 01:31 AM, 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 <address@hidden>
> Signed-off-by: Chunyan Liu <address@hidden>
> ---

>  int bdrv_create(BlockDriver *drv, const char* filename,
> -    QEMUOptionParameter *options, Error **errp)
> +    QEMUOptionParameter *options, QemuOpts *opts, Error **errp)
>  {
>      int ret;

In addition to my remarks last night:

I wonder if you should add:

assert(!(options && opts))

to prove that all callers are passing at most one of the two supported
option lists, while doing the conversion.

> @@ -5248,28 +5266,31 @@ void bdrv_img_create(const char *filename, const char *fmt,
>          return;
>      }
>
> -    create_options = append_option_parameters(create_options,
> -                                              drv->create_options);
> -    create_options = append_option_parameters(create_options,
> -                                              proto_drv->create_options);
> +    if (drv->bdrv_create2) {
> +        create_opts = qemu_opts_append(create_opts, drv->create_opts);
> +        create_opts = qemu_opts_append(create_opts, proto_drv->create_opts);

In addition to the memory leak I pointed out earlier,

Should use g_realloc in qemu_opts_append, realloc create_opts
to the desired space and append 2nd list to it. Saving the effort to free
memory while append many times.

I see another
potential problem: What if drv has been converted to QemuOpts, but
proto_drv is still on QEMUOptionParameter? 

> +    } else {
> +        create_options = append_option_parameters(create_options,
> +                                                  drv->create_options);
> +        create_options = append_option_parameters(create_options,
> +                                                  proto_drv->create_options);

Or the converse, if drv is still on QEMUOptionParameter, but proto_drv
has been converted to QEMUOpts?

Either way, you will be appending NULL for the proto_drv case, when what
you really wanted to be doing was merging both the QemuOpts and
QEMUOptionParameters into a single list.

> +        create_opts = params_to_opts(create_options);
> +    }

I think a better path to conversion would be doing everything possible
in QemuOpts, and only switching to QEMUOptionParameters at the last
possible moment, rather than having two separate append code paths.

My suggestion: until the conversion is complete, add a new function,
something like:

void qemu_opts_append_pair(QemuOpts *dst, QemuOpts *opts,
                           QEMUOptionParameter *options) {
    assert(!(opts && options));
    ... modify dst in-place to have it cover the entries from opts or
options
}

then in this code, you do:
    create_options = /* generate empty QemuOpts */;
    qemu_opts_append_pair(options, drv->create_opts,
                          drv->create_options);
    qemu_opts_append_pair(options, proto_drv->create_options,
                          proto_drv->create_options);


Good. Together with always using QemuOpts and only converting to
QEMUOptionParameter until it calls .bdrv_create in specific driver,
the drv/proto_drv inconsistent problem could be solved.
Will update.

Thanks very much for all your suggestions!

Chunyan

>
>      /* Create parameter list with default values */
> -    param = parse_option_parameters("", create_options, param);
> -
> -    set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size);
> +    opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
> +    qemu_opt_set_number(opts, BLOCK_OPT_SIZE, img_size);

This part worked nicely - once you convert both incoming styles to
QemuOpts, it really is easier to have this part of the code just use
QemuOpts functions for setting the size....

> ...
> @@ -5343,16 +5364,23 @@ void bdrv_img_create(const char *filename, const char *fmt,
>
>      if (!quiet) {
>          printf("Formatting '%s', fmt=%s ", filename, fmt);
> -        print_option_parameters(param);
> +        qemu_opts_print(opts);
>          puts("");
>      }
> -    ret = bdrv_create(drv, filename, param, &local_err);
> +
> +    if (drv->bdrv_create2) {
> +        ret = bdrv_create(drv, filename, NULL, opts, &local_err);
> +    } else {
> +        param = opts_to_params(opts);
> +        ret = bdrv_create(drv, filename, param, NULL, &local_err);
> +    }

...and finally convert back to QEMUOptionParameters at the last moment.
 But wait a minute - why are you checking for drv->bdrv_create2 here?
Isn't the last possible moment really inside bdrv_create() (that is, the
actual function that decides whether to call drv->bdrv_create vs.
drv->bdrv_create2)?  For the code to be as clean as possible, you want
to use QemuOpts everywhere until the actual point where you are calling
the unconverted callback.


> -int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter *options)
> +int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter *options,
> +                       QemuOpts *opts)
>  {
> -    if (bs->drv->bdrv_amend_options == NULL) {
> +    if (!bs->drv->bdrv_amend_options && !bs->drv->bdrv_amend_options2) {
>          return -ENOTSUP;
>      }
> -    return bs->drv->bdrv_amend_options(bs, options);
> +    if (bs->drv->bdrv_amend_options2) {
> +        return bs->drv->bdrv_amend_options2(bs, opts);
> +    } else {
> +        return bs->drv->bdrv_amend_options(bs, options);

Similar to the create/create2 situation, _this_ function is the last
possible place to make the conversions.  You have a problem in that if
the user passes you options but the callback expects opts, you are
passing NULL to the callback.  I think this should look more like:

int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter *options,
                       QemuOpts *opts)
{
    int ret;
    assert(!(opts && options));
    if (!bs->drv->bdrv_amend_options && !bs->drv->bdrv_amend_options2) {
        return -ENOTSUP;
    }
    if (bs->drv->bdrv_amend_options2) {
        if (options) {
            opts = params_to_opts(options);
        }
        ret = bs->drv->bdrv_amend_options2(bs, opts);
        if (options) {
            g_free(opts); // or whatever correct spelling to avoid leak
        }
    } else {
        if (opts) {
            options = opts_to_params(opts);
        }
        ret = bs->drv->bdrv_amend_options(bs, options);
        if (opts) {
            g_free(options); // again, with correct spelling
        }
    }
    return ret;

> +++ 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);
> +    /* FIXME: will remove the duplicate and rename back to bdrv_create later */
> +    int (*bdrv_create2)(const char *filename, QemuOpts *opts, Error **errp);

I like the FIXME here; maybe also mention that bdrv_create and
bdrv_create2 are mutually exclusive (at most one can be non-NULL).

>      int (*bdrv_set_key)(BlockDriverState *bs, const char *key);
>      int (*bdrv_make_empty)(BlockDriverState *bs);
>      /* aio */
> @@ -217,7 +219,7 @@ struct BlockDriver {
>
>      /* List of options for creating images, terminated by name == NULL */
>      QEMUOptionParameter *create_options;
> -
> +    QemuOptsList *create_opts;

A similar FIXME comment here might be nice, likewise mentioning that at
most one of these two fields should be non-NULL.

> @@ -244,21 +245,34 @@ static int print_block_option_help(const char *filename, const char *fmt)
>          return 1;
>      }
>
> -    create_options = append_option_parameters(create_options,
> -                                              drv->create_options);
> -
> +    if (drv->create_opts) {
> +        create_opts = qemu_opts_append(create_opts, drv->create_opts);
> +    } else {
> +        create_options = append_option_parameters(create_options,
> +                                                  drv->create_options);
> +    }
>      if (filename) {
>          proto_drv = bdrv_find_protocol(filename, true);
>          if (!proto_drv) {
>              error_report("Unknown protocol '%s'", filename);
>              return 1;
>          }
> -        create_options = append_option_parameters(create_options,
> -                                                  proto_drv->create_options);
> +        if (drv->create_opts) {
> +            create_opts = qemu_opts_append(create_opts, proto_drv->create_opts);

Memory leak.

> +        } else {
> +            create_options =
> +                append_option_parameters(create_options,
> +                                         proto_drv->create_options);
> +        }
>      }
>
> -    print_option_help(create_options);
> +    if (drv->create_opts) {
> +        qemu_opts_print_help(create_opts);
> +    } else {
> +        print_option_help(create_options);
> +    }

Another case where if you add a new helper function that absorbs either
style of options into a QemuOpts, then all you have to do here is print
the QemuOpts, instead of trying to switch between print methods here.


> @@ -1340,40 +1356,42 @@ static int img_convert(int argc, char **argv)
>          goto out;
>      }

> -        }
> +    if (drv->bdrv_create2) {
> +        create_opts = qemu_opts_append(create_opts, drv->create_opts);
> +        create_opts = qemu_opts_append(create_opts, proto_drv->create_opts);

More memory leaks.


> @@ -1400,7 +1418,12 @@ static int img_convert(int argc, char **argv)
>
>      if (!skip_create) {
>          /* Create the new image */
> -        ret = bdrv_create(drv, out_filename, param, &local_err);
> +        if (drv->bdrv_create2) {
> +            ret = bdrv_create(drv, out_filename, NULL, opts, &local_err);
> +        } else {
> +            param = opts_to_params(opts);
> +            ret = bdrv_create(drv, out_filename, param, NULL, &local_err);
> +        }

Another case where you should just always pass opts to bdrv_create(),
and let bdrv_create() do the last minute conversion to
QEMUOptionParameters based on what callbacks drv supports, rather than
trying to make decisions here based on contents of drv.


> @@ -2721,17 +2748,26 @@ static int img_amend(int argc, char **argv)
>          goto out;
>      }
>
> -    create_options = append_option_parameters(create_options,
> -            bs->drv->create_options);
> -    options_param = parse_option_parameters(options, create_options,
> -            options_param);
> -    if (options_param == NULL) {
> +    if (bs->drv->bdrv_amend_options2) {

And again, you should not be making decisions on the contents of
bs->drv, but just blindly setting up QemuOpts here...

> +        create_opts = qemu_opts_append(create_opts, bs->drv->create_opts);
> +    } else {
> +        create_options = append_option_parameters(create_options,
> +                                                  bs->drv->create_options);
> +        create_opts = params_to_opts(create_options);
> +    }
> +    opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
> +    if (options && qemu_opts_do_parse(opts, options, NULL)) {
>          error_report("Invalid options for file format '%s'", fmt);
>          ret = -1;
>          goto out;
>      }
>
> -    ret = bdrv_amend_options(bs, options_param);
> +    if (bs->drv->bdrv_amend_options2) {
> +        ret = bdrv_amend_options(bs, NULL, opts);
> +    } else {
> +        options_param = opts_to_params(opts);
> +        ret = bdrv_amend_options(bs, options_param, NULL);

...and blindly letting bdrv_amend_options() be the place that converts
to QEMUOptionParameters as needed.

Here's hoping v23 is nicer; I'm looking forward to ditching
QEMUOptionParameters. 

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]