Re: [Qemu-devel] [PATCH v25 11/31] change block layer to support both QemuOpts and QEMUOptionParamter

[Eric Blake]

On 04/10/2014 11:54 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>
> ---

> @@ -419,8 +420,27 @@ static void coroutine_fn bdrv_create_co_entry(void 
> *opaque)
>  
>      CreateCo *cco = opaque;

> +    if (cco->drv->bdrv_create2) {
> +        QemuOptsList *opts_list = NULL;
> +        QemuOpts *opts = NULL;
> +        if (!cco->opts) {

Isn't your conversion pair-wise per driver, in that you always pair
bdrv_create2 with options, and bdrv_create with opts?  That is, won't
cco->opts always be false if cco->drv->bdrv_create2 is non-NULL, since
we already guaranteed that there is at most one of the two creation
function callbacks defined?  Maybe you have a missing assertion at the
point where the callbacks are registered? [1]

> +            opts_list = params_to_opts(cco->options);
> +            cco->opts = opts =
> +                qemu_opts_create(opts_list, NULL, 0, &error_abort);

Ouch.  This assigns to cco->opts,...

> +        }
> +        ret = cco->drv->bdrv_create2(cco->filename, cco->opts, &local_err);
> +        qemu_opts_del(opts);

...but this frees the memory.  You have modified the caller's opaque
pointer to point to what is now stale memory.  Is this intentional? [2]

> +        qemu_opts_free(opts_list);
> +    } else {
> +        QEMUOptionParameter *options = NULL;
> +        if (!cco->options) {
> +            cco->options = options = opts_to_params(cco->opts);
> +        }
> +        ret = cco->drv->bdrv_create(cco->filename, cco->options, &local_err);
> +        free_option_parameters(options);

Same question [2] in the opposite direction for cco->options.



> @@ -5296,28 +5328,25 @@ void bdrv_img_create(const char *filename, const char 
> *fmt,

>      /* Parse -o options */
>      if (options) {
> -        param = parse_option_parameters(options, create_options, param);
> -        if (param == NULL) {
> +        if (qemu_opts_do_parse(opts, options, NULL) != 0) {
>              error_setg(errp, "Invalid options for file format '%s'.", fmt);

Pre-existing (and probably worth an independent patch to qemu-trivial),
but error messages typically should not end in '.'

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

Hmm, should we also guarantee that at most one of these callback
functions exist?  [1]

>          return -ENOTSUP;
>      }
> -    return bs->drv->bdrv_amend_options(bs, options);
> +    if (bs->drv->bdrv_amend_options2) {
> +        QemuOptsList *tmp_opts_list = NULL;
> +        QemuOpts *tmp_opts = NULL;
> +        if (options) {
> +            tmp_opts_list = params_to_opts(options);
> +            opts = tmp_opts =
> +                qemu_opts_create(tmp_opts_list, NULL, 0, &error_abort);
> +        }
> +        ret = bs->drv->bdrv_amend_options2(bs, opts);
> +        qemu_opts_del(tmp_opts);

Why do you need tmp_opts?  Isn't manipulation of 'opts' sufficient here?

> +++ 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);
>      int (*bdrv_set_key)(BlockDriverState *bs, const char *key);
>      int (*bdrv_make_empty)(BlockDriverState *bs);
>      /* aio */
> @@ -217,7 +219,10 @@ struct BlockDriver {
>  
>      /* List of options for creating images, terminated by name == NULL */
>      QEMUOptionParameter *create_options;
> -
> +    /* FIXME: will replace create_options.
> +     * These two fields are mutually exclusive. At most one is non-NULL.
> +     */
> +    QemuOptsList *create_opts;

Do you also want to mention that create_options may only be set with
bdrv_create, and that create_opts may only be set with bdrv_create2? [1]


> @@ -1340,40 +1340,36 @@ static int img_convert(int argc, char **argv)

>  
> -    if (options) {
> -        param = parse_option_parameters(options, create_options, param);
> -        if (param == NULL) {
> -            error_report("Invalid options for file format '%s'.", out_fmt);
> -            ret = -1;
> -            goto out;
> -        }
> -    } else {
> -        param = parse_option_parameters("", create_options, param);
> +    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'.", out_fmt);

Pre-existing, but another instance of trailing '.' in the error message.

Back to [1], rather than asserting in individual functions at use time,
why not do all the sanity checking up front in bdrv_register()?  That
is, I think it is easier to add:

if (bdrv->bdrv_create) {
    assert(!bdrv->bdrv_create2);
    assert(!bdrv->opts && !bdrv->bdrv_amend_options2);
} else if (bdrv_{
    assert(!bdrv->options && !bdrv->bdrv_amend_options);
}

at the point where drivers are registered, rather than having to
duplicate checks across all points that might use a driver.

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

signature.asc
Description: OpenPGP digital signature