Re: [Qemu-devel] [v19 05/25] change block layer to support both QemuOpts and QEMUOptionParameter

[Kevin Wolf <kwolf@redhat.com>]

Am 20.01.2014 um 15:19 hat Chunyan Liu geschrieben:
> 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                   |  339 +++++++++++++++++++++++++++++++--------------
>  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 +-
>  qemu-img.c                |  172 ++++++++++++++++-------
>  12 files changed, 372 insertions(+), 165 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 64e7d22..0dc0b09 100644
> --- a/block.c
> +++ b/block.c
> @@ -395,6 +395,7 @@ typedef struct CreateCo {
>      BlockDriver *drv;
>      char *filename;
>      QEMUOptionParameter *options;
> +    QemuOpts *opts;
>      int ret;
>      Error *err;
>  } CreateCo;
> @@ -407,7 +408,10 @@ static void coroutine_fn bdrv_create_co_entry(void *opaque)
>      CreateCo *cco = opaque;
>      assert(cco->drv);
>  
> -    ret = cco->drv->bdrv_create(cco->filename, cco->options, &local_err);
> +    if (cco->drv->bdrv_create2)
> +        ret = cco->drv->bdrv_create2(cco->filename, cco->opts, &local_err);
> +    else
> +        ret = cco->drv->bdrv_create(cco->filename, cco->options, &local_err);

Braces

>      if (error_is_set(&local_err)) {
>          error_propagate(&cco->err, local_err);
>      }
> @@ -415,7 +419,7 @@ static void coroutine_fn bdrv_create_co_entry(void *opaque)
>  }
>  
>  int bdrv_create(BlockDriver *drv, const char* filename,
> -    QEMUOptionParameter *options, Error **errp)
> +    QEMUOptionParameter *options, QemuOpts *opts, Error **errp)
>  {
>      int ret;
>  
> @@ -424,11 +428,12 @@ int bdrv_create(BlockDriver *drv, const char* filename,
>          .drv = drv,
>          .filename = g_strdup(filename),
>          .options = options,
> +        .opts = opts,
>          .ret = NOT_DONE,
>          .err = NULL,
>      };
>  
> -    if (!drv->bdrv_create) {
> +    if (!drv->bdrv_create && !drv->bdrv_create2) {
>          error_setg(errp, "Driver '%s' does not support image creation", drv->format_name);
>          ret = -ENOTSUP;
>          goto out;
> @@ -460,7 +465,7 @@ out:
>  }
>  
>  int bdrv_create_file(const char* filename, QEMUOptionParameter *options,
> -                     Error **errp)
> +                     QemuOpts *opts, Error **errp)
>  {
>      BlockDriver *drv;
>      Error *local_err = NULL;
> @@ -472,7 +477,7 @@ int bdrv_create_file(const char* filename, QEMUOptionParameter *options,
>          return -ENOENT;
>      }
>  
> -    ret = bdrv_create(drv, filename, options, &local_err);
> +    ret = bdrv_create(drv, filename, options, opts, &local_err);
>      if (error_is_set(&local_err)) {
>          error_propagate(errp, local_err);
>      }
> @@ -1053,7 +1058,8 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>          BlockDriverState *bs1;
>          int64_t total_size;
>          BlockDriver *bdrv_qcow2;
> -        QEMUOptionParameter *create_options;
> +        QEMUOptionParameter *create_options = NULL;
> +        QemuOpts *opts = NULL; 

Trailing whitespace

>          QDict *snapshot_options;
>  
>          /* if snapshot, we create a temporary backing file and open it
> @@ -1080,13 +1086,18 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>          }
>  
>          bdrv_qcow2 = bdrv_find_format("qcow2");
> -        create_options = parse_option_parameters("", bdrv_qcow2->create_options,
> -                                                 NULL);
> -
> -        set_option_parameter_int(create_options, BLOCK_OPT_SIZE, total_size);
> +        if (bdrv_qcow2->bdrv_create2) {
> +            opts = qemu_opts_create(bdrv_qcow2->create_opts, NULL, 0, &error_abort);
> +            qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_size);
> +        } else {
> +            create_options = 

Here as well

> +                parse_option_parameters("", bdrv_qcow2->create_options, NULL);
> +            set_option_parameter_int(create_options, BLOCK_OPT_SIZE, total_size);
> +        }
>  
> -        ret = bdrv_create(bdrv_qcow2, tmp_filename, create_options, &local_err);
> +        ret = bdrv_create(bdrv_qcow2, tmp_filename, create_options, opts, &local_err);
>          free_option_parameters(create_options);
> +        qemu_opts_del(opts);
>          if (ret < 0) {
>              error_setg_errno(errp, -ret, "Could not create temporary overlay "
>                               "'%s': %s", tmp_filename,
> @@ -4712,7 +4723,8 @@ void bdrv_img_create(const char *filename, const char *fmt,
>                       Error **errp, bool quiet)
>  {
>      QEMUOptionParameter *param = NULL, *create_options = NULL;
> -    QEMUOptionParameter *backing_fmt, *backing_file, *size;
> +    QemuOptsList *create_opts = NULL;
> +    QemuOpts *opts = NULL;
>      BlockDriver *drv, *proto_drv;
>      BlockDriver *backing_drv = NULL;
>      Error *local_err = NULL;
> @@ -4731,125 +4743,246 @@ 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) {
> +        const char *backing_fmt, *backing_file;
> +        int64_t size;
>  
> -    /* Create parameter list with default values */
> -    param = parse_option_parameters("", create_options, param);
> +        create_opts = qemu_opts_append(create_opts, drv->create_opts);
> +        create_opts = qemu_opts_append(create_opts, proto_drv->create_opts);
> +        /* Create parameter list with default values */
> +        opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
>  
> -    set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size);
> +        qemu_opt_set_number(opts, BLOCK_OPT_SIZE, img_size);
>  
> -    /* Parse -o options */
> -    if (options) {
> -        param = parse_option_parameters(options, create_options, param);
> -        if (param == NULL) {
> -            error_setg(errp, "Invalid options for file format '%s'.", fmt);
> -            goto out;
> +        /* Parse -o options */
> +        if (options) {
> +            if (qemu_opts_do_parse(opts, options, NULL) != 0) {
> +                error_setg(errp, "Invalid options for file format '%s'.", fmt);
> +                goto out;
> +            }
>          }
> -    }
>  
> -    if (base_filename) {
> -        if (set_option_parameter(param, BLOCK_OPT_BACKING_FILE,
> -                                 base_filename)) {
> -            error_setg(errp, "Backing file not supported for file format '%s'",
> -                       fmt);
> -            goto out;
> +        if (base_filename) {
> +            if (qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename)) {
> +                error_setg(errp, "Backing file not supported for file format '%s'",
> +                           fmt);
> +                goto out;
> +            }
>          }
> -    }
>  
> -    if (base_fmt) {
> -        if (set_option_parameter(param, BLOCK_OPT_BACKING_FMT, base_fmt)) {
> -            error_setg(errp, "Backing file format not supported for file "
> -                             "format '%s'", fmt);
> -            goto out;
> +        if (base_fmt) {
> +            if (qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt)) {
> +                error_setg(errp, "Backing file format not supported for file "
> +                                 "format '%s'", fmt);
> +                goto out;
> +            }
>          }
> -    }
>  
> -    backing_file = get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
> -    if (backing_file && backing_file->value.s) {
> -        if (!strcmp(filename, backing_file->value.s)) {
> -            error_setg(errp, "Error: Trying to create an image with the "
> -                             "same filename as the backing file");
> -            goto out;
> +        backing_file = qemu_opt_get(opts, BLOCK_OPT_BACKING_FILE);
> +        if (backing_file) {
> +            if (!strcmp(filename, backing_file)) {
> +                error_setg(errp, "Error: Trying to create an image with the "
> +                                 "same filename as the backing file");
> +                goto out;
> +            }
>          }
> -    }
>  
> -    backing_fmt = get_option_parameter(param, BLOCK_OPT_BACKING_FMT);
> -    if (backing_fmt && backing_fmt->value.s) {
> -        backing_drv = bdrv_find_format(backing_fmt->value.s);
> -        if (!backing_drv) {
> -            error_setg(errp, "Unknown backing file format '%s'",
> -                       backing_fmt->value.s);
> -            goto out;
> +        backing_fmt = qemu_opt_get(opts, BLOCK_OPT_BACKING_FMT);
> +        if (backing_fmt) {
> +            backing_drv = bdrv_find_format(backing_fmt);
> +            if (!backing_drv) {
> +                error_setg(errp, "Unknown backing file format '%s'",
> +                           backing_fmt);
> +                goto out;
> +            }
> +        }
> +
> +        // The size for the image must always be specified, with one exception:
> +        // If we are using a backing file, we can obtain the size from there
> +        size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
> +        if (size == -1) {
> +            if (backing_file) {
> +                BlockDriverState *bs;
> +                uint64_t size;
> +                char buf[32];
> +                int back_flags;
> +
> +                /* backing files always opened read-only */
> +                back_flags =
> +                     flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
> +
> +                bs = bdrv_new("");
> +
> +                ret = bdrv_open(bs, backing_file, NULL, back_flags,
> +                                backing_drv, &local_err);
> +                if (ret < 0) {
> +                    error_setg_errno(errp, -ret, "Could not open '%s': %s",
> +                                     backing_file,
> +                                     error_get_pretty(local_err));
> +                    error_free(local_err);
> +                    local_err = NULL;
> +                    bdrv_unref(bs);
> +                    goto out;
> +                }
> +                bdrv_get_geometry(bs, &size);
> +                size *= 512;
> +
> +                snprintf(buf, sizeof(buf), "%" PRId64, size);
> +                qemu_opt_set_number(opts, BLOCK_OPT_SIZE, size);
> +
> +                bdrv_unref(bs);
> +            } else {
> +                error_setg(errp, "Image creation needs a size parameter");
> +                goto out;
> +            }
> +        }
> +
> +        if (!quiet) {
> +            printf("Formatting '%s', fmt=%s ", filename, fmt);
> +            qemu_opts_print(opts);
> +            puts("");
> +        }
> +
> +        ret = bdrv_create(drv, filename, NULL, opts, &local_err);
> +        if (ret == -EFBIG) {
> +            /* This is generally a better message than whatever the driver would
> +             * deliver (especially because of the cluster_size_hint), since that
> +             * is most probably not much different from "image too large". */
> +            const char *cluster_size_hint = "";
> +            if (qemu_opt_get_size(opts, BLOCK_OPT_CLUSTER_SIZE, 0)) {
> +                cluster_size_hint = " (try using a larger cluster size)";
> +            }
> +            error_setg(errp, "The image size is too large for file format '%s'"
> +                       "%s", fmt, cluster_size_hint);
> +            error_free(local_err);
> +            local_err = NULL;
>          }
> -    }
>  
> -    // The size for the image must always be specified, with one exception:
> -    // If we are using a backing file, we can obtain the size from there
> -    size = get_option_parameter(param, BLOCK_OPT_SIZE);
> -    if (size && size->value.n == -1) {
> +    } else {
> +        QEMUOptionParameter *backing_fmt, *backing_file, *size;
> +
> +        create_options = append_option_parameters(create_options,
> +                                                  drv->create_options);
> +        create_options = append_option_parameters(create_options,
> +                                                  proto_drv->create_options);
> +
> +        /* Create parameter list with default values */
> +        param = parse_option_parameters("", create_options, param);
> +
> +        set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size);
> +
> +        /* Parse -o options */
> +        if (options) {
> +            param = parse_option_parameters(options, create_options, param);
> +            if (param == NULL) {
> +                error_setg(errp, "Invalid options for file format '%s'.", fmt);
> +                goto out;
> +            }
> +        }
> +
> +        if (base_filename) {
> +            if (set_option_parameter(param, BLOCK_OPT_BACKING_FILE,
> +                                     base_filename)) {
> +                error_setg(errp, "Backing file not supported for file format '%s'",
> +                           fmt);
> +                goto out;
> +            }
> +        }
> +
> +        if (base_fmt) {
> +            if (set_option_parameter(param, BLOCK_OPT_BACKING_FMT, base_fmt)) {
> +                error_setg(errp, "Backing file format not supported for file "
> +                                 "format '%s'", fmt);
> +                goto out;
> +            }
> +        }
> +
> +        backing_file = get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
>          if (backing_file && backing_file->value.s) {
> -            BlockDriverState *bs;
> -            uint64_t size;
> -            char buf[32];
> -            int back_flags;
> +            if (!strcmp(filename, backing_file->value.s)) {
> +                error_setg(errp, "Error: Trying to create an image with the "
> +                                 "same filename as the backing file");
> +                goto out;
> +            }
> +        }
>  
> -            /* backing files always opened read-only */
> -            back_flags =
> -                flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
> +        backing_fmt = get_option_parameter(param, BLOCK_OPT_BACKING_FMT);
> +        if (backing_fmt && backing_fmt->value.s) {
> +            backing_drv = bdrv_find_format(backing_fmt->value.s);
> +            if (!backing_drv) {
> +                error_setg(errp, "Unknown backing file format '%s'",
> +                           backing_fmt->value.s);
> +                goto out;
> +            }
> +        }
>  
> -            bs = bdrv_new("");
> +        // The size for the image must always be specified, with one exception:
> +        // If we are using a backing file, we can obtain the size from there
> +        size = get_option_parameter(param, BLOCK_OPT_SIZE);
> +        if (size && size->value.n == -1) {
> +            if (backing_file && backing_file->value.s) {
> +                BlockDriverState *bs;
> +                uint64_t size;
> +                char buf[32];
> +                int back_flags;
> +
> +                /* backing files always opened read-only */
> +                back_flags =
> +                    flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
> +
> +                bs = bdrv_new("");
> +
> +                ret = bdrv_open(bs, backing_file->value.s, NULL, back_flags,
> +                                backing_drv, &local_err);
> +                if (ret < 0) {
> +                    error_setg_errno(errp, -ret, "Could not open '%s': %s",
> +                                     backing_file->value.s,
> +                                     error_get_pretty(local_err));
> +                    error_free(local_err);
> +                    local_err = NULL;
> +                    bdrv_unref(bs);
> +                    goto out;
> +                }
> +                bdrv_get_geometry(bs, &size);
> +                size *= 512;
> +
> +                snprintf(buf, sizeof(buf), "%" PRId64, size);
> +                set_option_parameter(param, BLOCK_OPT_SIZE, buf);
>  
> -            ret = bdrv_open(bs, backing_file->value.s, NULL, back_flags,
> -                            backing_drv, &local_err);
> -            if (ret < 0) {
> -                error_setg_errno(errp, -ret, "Could not open '%s': %s",
> -                                 backing_file->value.s,
> -                                 error_get_pretty(local_err));
> -                error_free(local_err);
> -                local_err = NULL;
>                  bdrv_unref(bs);
> +            } else {
> +                error_setg(errp, "Image creation needs a size parameter");
>                  goto out;
>              }
> -            bdrv_get_geometry(bs, &size);
> -            size *= 512;
> -
> -            snprintf(buf, sizeof(buf), "%" PRId64, size);
> -            set_option_parameter(param, BLOCK_OPT_SIZE, buf);
> +        }
>  
> -            bdrv_unref(bs);
> -        } else {
> -            error_setg(errp, "Image creation needs a size parameter");
> -            goto out;
> +        if (!quiet) {
> +            printf("Formatting '%s', fmt=%s ", filename, fmt);
> +            print_option_parameters(param);
> +            puts("");
>          }
> -    }
>  
> -    if (!quiet) {
> -        printf("Formatting '%s', fmt=%s ", filename, fmt);
> -        print_option_parameters(param);
> -        puts("");
> -    }
> -    ret = bdrv_create(drv, filename, param, &local_err);
> -    if (ret == -EFBIG) {
> -        /* This is generally a better message than whatever the driver would
> -         * deliver (especially because of the cluster_size_hint), since that
> -         * is most probably not much different from "image too large". */
> -        const char *cluster_size_hint = "";
> -        if (get_option_parameter(create_options, BLOCK_OPT_CLUSTER_SIZE)) {
> -            cluster_size_hint = " (try using a larger cluster size)";
> +        ret = bdrv_create(drv, filename, param, NULL, &local_err);
> +        if (ret == -EFBIG) {
> +            /* This is generally a better message than whatever the driver would
> +             * deliver (especially because of the cluster_size_hint), since that
> +             * is most probably not much different from "image too large". */
> +            const char *cluster_size_hint = "";
> +            if (get_option_parameter(create_options, BLOCK_OPT_CLUSTER_SIZE)) {
> +                cluster_size_hint = " (try using a larger cluster size)";
> +            }
> +            error_setg(errp, "The image size is too large for file format '%s'"
> +                       "%s", fmt, cluster_size_hint);
> +            error_free(local_err);
> +            local_err = NULL;
>          }
> -        error_setg(errp, "The image size is too large for file format '%s'"
> -                   "%s", fmt, cluster_size_hint);
> -        error_free(local_err);
> -        local_err = NULL;
> -    }
> +   }
>  
>  out:
>      free_option_parameters(create_options);
>      free_option_parameters(param);
> -
> +    qemu_opts_del(opts);
> +    qemu_opts_free(create_opts);
>      if (error_is_set(&local_err)) {
>          error_propagate(errp, local_err);
>      }

Don't you think it would be easier to support only one method in this
function, and add some conversion to bdrv_create? Just iterating over
QemuOpts and pushing all fields into QEMUOptionParameters (or the other
way round) should be easy enough.

Kevin