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 <wdon...@linux.vnet.ibm.com>
> Signed-off-by: Chunyan Liu <cy...@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
