* Re: [Qemu-devel] [PATCH V12 0/4] replace QEMUOptionParameter with QemuOpts parser
[not found] <1362043236-4635-1-git-send-email-wdongxu@linux.vnet.ibm.com>
@ 2013-03-05 9:06 ` Stefan Hajnoczi
2013-03-07 7:36 ` Markus Armbruster
[not found] ` <1362043236-4635-4-git-send-email-wdongxu@linux.vnet.ibm.com>
` (3 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Stefan Hajnoczi @ 2013-03-05 9:06 UTC (permalink / raw)
To: Dong Xu Wang; +Cc: kwolf, qemu-devel, armbru
On Thu, Feb 28, 2013 at 05:20:32PM +0800, Dong Xu Wang wrote:
> Patch 1 add def_value_str and use it in qemu_opts_print.
>
> Patch 2 Create functions to pair with QemuOpts parser.
>
> Patch 3 Use QemuOpts parser in Block.
>
> Patch 4 Remove QEMUOptionParameter parser related code.
>
>
>
> Dong Xu Wang (4):
> add def_value_str and use it in qemu_opts_print
> Create four QemuOptsList related functions
> Use QemuOpts support in block layer
> remove QEMUOptionParameter related code
>
> block.c | 91 ++++-----
> block/cow.c | 46 +++--
> block/gluster.c | 37 ++--
> block/iscsi.c | 8 +-
> block/qcow.c | 61 +++---
> block/qcow2.c | 173 ++++++++--------
> block/qed.c | 86 ++++----
> block/qed.h | 2 +-
> block/raw-posix.c | 59 +++---
> block/raw-win32.c | 31 +--
> block/raw.c | 30 +--
> block/rbd.c | 62 +++---
> block/sheepdog.c | 75 ++++---
> block/vdi.c | 70 ++++---
> block/vmdk.c | 90 +++++----
> block/vpc.c | 57 +++---
> block/vvfat.c | 11 +-
> include/block/block.h | 4 +-
> include/block/block_int.h | 6 +-
> include/qemu/option.h | 46 ++---
> qemu-img.c | 61 +++---
> util/qemu-option.c | 490 ++++++++++++++++++----------------------------
> 22 files changed, 723 insertions(+), 873 deletions(-)
>
> --
> 1.7.11.7
>
Looks fine at the high level. Markus had specific comments in the last
version, please wait for his review before applying.
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH V12 3/4] Use QemuOpts support in block layer
[not found] ` <1362043236-4635-4-git-send-email-wdongxu@linux.vnet.ibm.com>
@ 2013-03-06 1:51 ` Dong Xu Wang
2013-03-21 15:31 ` Markus Armbruster
1 sibling, 0 replies; 10+ messages in thread
From: Dong Xu Wang @ 2013-03-06 1:51 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Dong Xu Wang, armbru, Stefan Hajnoczi
On Thu, Feb 28, 2013 at 5:20 PM, Dong Xu Wang
<wdongxu@linux.vnet.ibm.com> wrote:
> This patch will use QemuOpts related functions in block layer, add
> a member bdrv_create_opts to BlockDriver struct, it will return
> a QemuOptsList pointer, which includes the image format's create
> options.
>
> And create options's primary consumer is block creating related
> functions, so modify them together.
>
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> ---
>
> v11->v12:
> 1) create functions, such as qemu_opt_get_del and qemu_opt_replace_set.
> These functions works like origin code.
> 2) use QEMU_OPT_SIZE, not QEMU_OPT_NUMBER.
> 3) in bdrv_create, if opts is NULL, will create an empty one, so can
> discard if(opts) code safely.
>
> v10->v11:
> 1) qed.h move QED_DEFAULT_CLUSTER_SIZE from enum to macro, or
> qemu_opts_print produce un-expanded cluster_size.
> 2) In qcow2.c and qcow.c, bdrv_create_file(filename, NULL), NULL ->
> opts,
> or while using protocol, there will be an error.
>
> v8->v9:
> 1) add qemu_ prefix to gluster_create_opts.
> 2) fix bug: bdrv_gluster_unix and bdrv_gluster_rdma should also be
> converted.
>
> v7->v8:
> 1) rebase to upstream source tree.
> 2) add gluster.c, raw-win32.c, and rbd.c.
>
> v6->v7:
> 1) use osdep.h:stringify(), not redefining new macro.
> 2) preserve TODO comment.
> 3) fix typo. BLOCK_OPT_ENCRYPT->BLOCK_OPT_STATIC.
> 4) initialize disk_type even when opts is NULL.
>
> v5->v6:
> 1) judge if opts == NULL in block layer create functions.
> 2) use bdrv_create_file(filename, NULL) in qcow_create and cow_create
> funtion.
> 3) made more readable while using qemu_opt_get_number.
>
>
> block.c | 91 ++++++++++++------------
> block/cow.c | 46 ++++++------
> block/gluster.c | 37 +++++-----
> block/iscsi.c | 8 +--
> block/qcow.c | 61 ++++++++--------
> block/qcow2.c | 173 +++++++++++++++++++++++-----------------------
> block/qed.c | 86 +++++++++++------------
> block/qed.h | 2 +-
> block/raw-posix.c | 59 +++++++---------
> block/raw-win32.c | 31 +++++----
> block/raw.c | 30 ++++----
> block/rbd.c | 62 ++++++++---------
> block/sheepdog.c | 75 ++++++++++----------
> block/vdi.c | 70 +++++++++----------
> block/vmdk.c | 90 ++++++++++++------------
> block/vpc.c | 57 +++++++--------
> block/vvfat.c | 11 +--
> include/block/block.h | 4 +-
> include/block/block_int.h | 6 +-
> include/qemu/option.h | 13 +++-
> qemu-img.c | 61 ++++++++--------
> util/qemu-option.c | 93 +++++++++++++++++++++++--
> 22 files changed, 613 insertions(+), 553 deletions(-)
>
> diff --git a/block.c b/block.c
> index 4582961..975c3d8 100644
> --- a/block.c
> +++ b/block.c
> @@ -357,7 +357,7 @@ BlockDriver *bdrv_find_whitelisted_format(const char *format_name)
> typedef struct CreateCo {
> BlockDriver *drv;
> char *filename;
> - QEMUOptionParameter *options;
> + QemuOpts *opts;
> int ret;
> } CreateCo;
>
> @@ -366,11 +366,11 @@ static void coroutine_fn bdrv_create_co_entry(void *opaque)
> CreateCo *cco = opaque;
> assert(cco->drv);
>
> - cco->ret = cco->drv->bdrv_create(cco->filename, cco->options);
> + cco->ret = cco->drv->bdrv_create(cco->filename, cco->opts);
> }
>
> int bdrv_create(BlockDriver *drv, const char* filename,
> - QEMUOptionParameter *options)
> + QemuOpts *opts)
> {
> int ret;
>
> @@ -378,7 +378,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
> CreateCo cco = {
> .drv = drv,
> .filename = g_strdup(filename),
> - .options = options,
> + .opts = opts ?: qemu_opts_create_nofail(drv->bdrv_create_opts),
> .ret = NOT_DONE,
> };
>
> @@ -405,7 +405,7 @@ out:
> return ret;
> }
>
> -int bdrv_create_file(const char* filename, QEMUOptionParameter *options)
> +int bdrv_create_file(const char *filename, QemuOpts *opts)
> {
> BlockDriver *drv;
>
> @@ -414,7 +414,7 @@ int bdrv_create_file(const char* filename, QEMUOptionParameter *options)
> return -ENOENT;
> }
>
> - return bdrv_create(drv, filename, options);
> + return bdrv_create(drv, filename, opts);
> }
>
> /*
> @@ -814,7 +814,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
> int64_t total_size;
> int is_protocol = 0;
> BlockDriver *bdrv_qcow2;
> - QEMUOptionParameter *options;
> + QemuOpts *opts;
> char backing_filename[PATH_MAX];
>
> /* if snapshot, we create a temporary backing file and open it
> @@ -847,17 +847,16 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
> return -errno;
>
> bdrv_qcow2 = bdrv_find_format("qcow2");
> - options = parse_option_parameters("", bdrv_qcow2->create_options, NULL);
> + opts = qemu_opts_create_nofail(bdrv_qcow2->bdrv_create_opts);
>
> - set_option_parameter_int(options, BLOCK_OPT_SIZE, total_size);
> - set_option_parameter(options, BLOCK_OPT_BACKING_FILE, backing_filename);
> + qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_size);
> + qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, backing_filename);
> if (drv) {
> - set_option_parameter(options, BLOCK_OPT_BACKING_FMT,
> - drv->format_name);
> + qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, drv->format_name);
> }
>
> - ret = bdrv_create(bdrv_qcow2, tmp_filename, options);
> - free_option_parameters(options);
> + ret = bdrv_create(bdrv_qcow2, tmp_filename, opts);
> + qemu_opts_del(opts);
> if (ret < 0) {
> return ret;
> }
> @@ -4502,8 +4501,10 @@ void bdrv_img_create(const char *filename, const char *fmt,
> char *options, uint64_t img_size, int flags,
> Error **errp, bool quiet)
> {
> - QEMUOptionParameter *param = NULL, *create_options = NULL;
> - QEMUOptionParameter *backing_fmt, *backing_file, *size;
> + QemuOpts *opts = NULL;
> + QemuOptsList *create_opts = NULL;
> + const char *backing_fmt, *backing_file;
> + int64_t size;
> BlockDriverState *bs = NULL;
> BlockDriver *drv, *proto_drv;
> BlockDriver *backing_drv = NULL;
> @@ -4521,28 +4522,23 @@ void bdrv_img_create(const char *filename, const char *fmt,
> error_setg(errp, "Unknown protocol '%s'", filename);
> return;
> }
> -
> - create_options = append_option_parameters(create_options,
> - drv->create_options);
> - create_options = append_option_parameters(create_options,
> - proto_drv->create_options);
> -
> + create_opts = qemu_opts_append(drv->bdrv_create_opts,
> + proto_drv->bdrv_create_opts);
> /* Create parameter list with default values */
> - param = parse_option_parameters("", create_options, param);
> + opts = qemu_opts_create_nofail(create_opts);
>
> - 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) {
> + if (qemu_opts_do_parse_replace(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,
> + if (qemu_opt_replace_set(opts, BLOCK_OPT_BACKING_FILE,
> base_filename)) {
> error_setg(errp, "Backing file not supported for file format '%s'",
> fmt);
> @@ -4551,39 +4547,37 @@ void bdrv_img_create(const char *filename, const char *fmt,
> }
>
> if (base_fmt) {
> - if (set_option_parameter(param, BLOCK_OPT_BACKING_FMT, base_fmt)) {
> + if (qemu_opt_replace_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)) {
> + 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);
> + 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->value.s);
> + 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 = get_option_parameter(param, BLOCK_OPT_SIZE);
> - if (size && size->value.n == -1) {
> - if (backing_file && backing_file->value.s) {
> + size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
> + if (size == -1) {
> + if (backing_file) {
> uint64_t size;
> - char buf[32];
> int back_flags;
>
> /* backing files always opened read-only */
> @@ -4592,17 +4586,16 @@ void bdrv_img_create(const char *filename, const char *fmt,
>
> bs = bdrv_new("");
>
> - ret = bdrv_open(bs, backing_file->value.s, back_flags, backing_drv);
> + ret = bdrv_open(bs, backing_file, back_flags, backing_drv);
> if (ret < 0) {
> error_setg_errno(errp, -ret, "Could not open '%s'",
> - backing_file->value.s);
> + backing_file);
> goto out;
> }
> bdrv_get_geometry(bs, &size);
> size *= 512;
>
> - snprintf(buf, sizeof(buf), "%" PRId64, size);
> - set_option_parameter(param, BLOCK_OPT_SIZE, buf);
> + qemu_opt_set_number(opts, BLOCK_OPT_SIZE, size);
> } else {
> error_setg(errp, "Image creation needs a size parameter");
> goto out;
> @@ -4611,10 +4604,10 @@ 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, NULL);
> puts("");
> }
> - ret = bdrv_create(drv, filename, param);
> + ret = bdrv_create(drv, filename, opts);
> if (ret < 0) {
> if (ret == -ENOTSUP) {
> error_setg(errp,"Formatting or formatting option not supported for "
> @@ -4629,8 +4622,10 @@ void bdrv_img_create(const char *filename, const char *fmt,
> }
>
> out:
> - free_option_parameters(create_options);
> - free_option_parameters(param);
> + qemu_opts_free(create_opts);
> + if (opts) {
> + qemu_opts_del(opts);
> + }
>
> if (bs) {
> bdrv_delete(bs);
> diff --git a/block/cow.c b/block/cow.c
> index 4baf904..135fa33 100644
> --- a/block/cow.c
> +++ b/block/cow.c
> @@ -255,7 +255,7 @@ static void cow_close(BlockDriverState *bs)
> {
> }
>
> -static int cow_create(const char *filename, QEMUOptionParameter *options)
> +static int cow_create(const char *filename, QemuOpts *opts)
> {
> struct cow_header_v2 cow_header;
> struct stat st;
> @@ -264,17 +264,11 @@ static int cow_create(const char *filename, QEMUOptionParameter *options)
> int ret;
> BlockDriverState *cow_bs;
>
> - /* Read out options */
> - while (options && options->name) {
> - if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> - image_sectors = options->value.n / 512;
> - } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FILE)) {
> - image_filename = options->value.s;
> - }
> - options++;
> - }
> + /* Read out opts */
> + image_sectors = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0) / 512;
> + image_filename = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
>
> - ret = bdrv_create_file(filename, options);
> + ret = bdrv_create_file(filename, opts);
> if (ret < 0) {
> return ret;
> }
> @@ -318,18 +312,22 @@ exit:
> return ret;
> }
>
> -static QEMUOptionParameter cow_create_options[] = {
> - {
> - .name = BLOCK_OPT_SIZE,
> - .type = OPT_SIZE,
> - .help = "Virtual disk size"
> - },
> - {
> - .name = BLOCK_OPT_BACKING_FILE,
> - .type = OPT_STRING,
> - .help = "File name of a base image"
> - },
> - { NULL }
> +static QemuOptsList cow_create_opts = {
> + .name = "cow-create-opts",
> + .head = QTAILQ_HEAD_INITIALIZER(cow_create_opts.head),
> + .desc = {
> + {
> + .name = BLOCK_OPT_SIZE,
> + .type = QEMU_OPT_SIZE,
> + .help = "Virtual disk size"
> + },
> + {
> + .name = BLOCK_OPT_BACKING_FILE,
> + .type = QEMU_OPT_STRING,
> + .help = "File name of a base image"
> + },
> + { /* end of list */ }
> + }
> };
>
> static BlockDriver bdrv_cow = {
> @@ -345,7 +343,7 @@ static BlockDriver bdrv_cow = {
> .bdrv_write = cow_co_write,
> .bdrv_co_is_allocated = cow_co_is_allocated,
>
> - .create_options = cow_create_options,
> + .bdrv_create_opts = &cow_create_opts,
> };
>
> static void bdrv_cow_init(void)
> diff --git a/block/gluster.c b/block/gluster.c
> index ccd684d..a219201 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -335,8 +335,7 @@ out:
> return ret;
> }
>
> -static int qemu_gluster_create(const char *filename,
> - QEMUOptionParameter *options)
> +static int qemu_gluster_create(const char *filename, QemuOpts *opts)
> {
> struct glfs *glfs;
> struct glfs_fd *fd;
> @@ -350,12 +349,8 @@ static int qemu_gluster_create(const char *filename,
> goto out;
> }
>
> - while (options && options->name) {
> - if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> - total_size = options->value.n / BDRV_SECTOR_SIZE;
> - }
> - options++;
> - }
> + total_size =
> + qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0) / BDRV_SECTOR_SIZE;
>
> fd = glfs_creat(glfs, gconf->image,
> O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, S_IRUSR | S_IWUSR);
> @@ -544,13 +539,17 @@ static void qemu_gluster_close(BlockDriverState *bs)
> glfs_fini(s->glfs);
> }
>
> -static QEMUOptionParameter qemu_gluster_create_options[] = {
> - {
> - .name = BLOCK_OPT_SIZE,
> - .type = OPT_SIZE,
> - .help = "Virtual disk size"
> - },
> - { NULL }
> +static QemuOptsList qemu_gluster_create_opts = {
> + .name = "qemu-gluster-create-opts",
> + .head = QTAILQ_HEAD_INITIALIZER(qemu_gluster_create_opts.head),
> + .desc = {
> + {
> + .name = BLOCK_OPT_SIZE,
> + .type = QEMU_OPT_SIZE,
> + .help = "Virtual disk size"
> + },
> + { /* end of list */ }
> + }
> };
>
> static BlockDriver bdrv_gluster = {
> @@ -565,7 +564,7 @@ static BlockDriver bdrv_gluster = {
> .bdrv_aio_readv = qemu_gluster_aio_readv,
> .bdrv_aio_writev = qemu_gluster_aio_writev,
> .bdrv_aio_flush = qemu_gluster_aio_flush,
> - .create_options = qemu_gluster_create_options,
> + .bdrv_create_opts = &qemu_gluster_create_opts,
> };
>
> static BlockDriver bdrv_gluster_tcp = {
> @@ -580,7 +579,7 @@ static BlockDriver bdrv_gluster_tcp = {
> .bdrv_aio_readv = qemu_gluster_aio_readv,
> .bdrv_aio_writev = qemu_gluster_aio_writev,
> .bdrv_aio_flush = qemu_gluster_aio_flush,
> - .create_options = qemu_gluster_create_options,
> + .bdrv_create_opts = &qemu_gluster_create_opts,
> };
>
> static BlockDriver bdrv_gluster_unix = {
> @@ -595,7 +594,7 @@ static BlockDriver bdrv_gluster_unix = {
> .bdrv_aio_readv = qemu_gluster_aio_readv,
> .bdrv_aio_writev = qemu_gluster_aio_writev,
> .bdrv_aio_flush = qemu_gluster_aio_flush,
> - .create_options = qemu_gluster_create_options,
> + .bdrv_create_opts = &qemu_gluster_create_opts,
> };
>
> static BlockDriver bdrv_gluster_rdma = {
> @@ -610,7 +609,7 @@ static BlockDriver bdrv_gluster_rdma = {
> .bdrv_aio_readv = qemu_gluster_aio_readv,
> .bdrv_aio_writev = qemu_gluster_aio_writev,
> .bdrv_aio_flush = qemu_gluster_aio_flush,
> - .create_options = qemu_gluster_create_options,
> + .bdrv_create_opts = &qemu_gluster_create_opts,
> };
>
> static void bdrv_gluster_init(void)
> diff --git a/block/iscsi.c b/block/iscsi.c
> index deb3b68..def377a 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -789,9 +789,7 @@ static char *parse_initiator_name(const char *target)
> if (!opts) {
> opts = QTAILQ_FIRST(&list->head);
> }
> - if (opts) {
> - name = qemu_opt_get(opts, "initiator-name");
> - }
> + name = qemu_opt_get(opts, "initiator-name");
> }
>
> if (name) {
> @@ -1073,7 +1071,7 @@ out:
> return ret;
> }
>
> -static QEMUOptionParameter iscsi_create_options[] = {
> +static QEMUOptionParameter iscsi_create_opts[] = {
> {
> .name = BLOCK_OPT_SIZE,
> .type = OPT_SIZE,
> @@ -1090,7 +1088,7 @@ static BlockDriver bdrv_iscsi = {
> .bdrv_file_open = iscsi_open,
> .bdrv_close = iscsi_close,
> .bdrv_create = iscsi_create,
> - .create_options = iscsi_create_options,
> + .create_opts = iscsi_create_opts,
>
> .bdrv_getlength = iscsi_getlength,
>
> diff --git a/block/qcow.c b/block/qcow.c
> index a7135ee..f0c4c38 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -651,7 +651,7 @@ static void qcow_close(BlockDriverState *bs)
> error_free(s->migration_blocker);
> }
>
> -static int qcow_create(const char *filename, QEMUOptionParameter *options)
> +static int qcow_create(const char *filename, QemuOpts *opts)
> {
> int header_size, backing_filename_len, l1_size, shift, i;
> QCowHeader header;
> @@ -662,19 +662,14 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options)
> int ret;
> BlockDriverState *qcow_bs;
>
> - /* Read out options */
> - while (options && options->name) {
> - if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> - total_size = options->value.n / 512;
> - } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FILE)) {
> - backing_file = options->value.s;
> - } else if (!strcmp(options->name, BLOCK_OPT_ENCRYPT)) {
> - flags |= options->value.n ? BLOCK_FLAG_ENCRYPT : 0;
> - }
> - options++;
> + /* Read out opts */
> + total_size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0) / 512;
> + backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
> + if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, 0)) {
> + flags |= BLOCK_FLAG_ENCRYPT;
> }
>
> - ret = bdrv_create_file(filename, options);
> + ret = bdrv_create_file(filename, opts);
> if (ret < 0) {
> return ret;
> }
> @@ -851,24 +846,28 @@ static int qcow_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
> return 0;
> }
>
> -
> -static QEMUOptionParameter qcow_create_options[] = {
> - {
> - .name = BLOCK_OPT_SIZE,
> - .type = OPT_SIZE,
> - .help = "Virtual disk size"
> - },
> - {
> - .name = BLOCK_OPT_BACKING_FILE,
> - .type = OPT_STRING,
> - .help = "File name of a base image"
> - },
> - {
> - .name = BLOCK_OPT_ENCRYPT,
> - .type = OPT_FLAG,
> - .help = "Encrypt the image"
> - },
> - { NULL }
> +static QemuOptsList qcow_create_opts = {
> + .name = "qcow-create-opts",
> + .head = QTAILQ_HEAD_INITIALIZER(qcow_create_opts.head),
> + .desc = {
> + {
> + .name = BLOCK_OPT_SIZE,
> + .type = QEMU_OPT_SIZE,
> + .help = "Virtual disk size"
> + },
> + {
> + .name = BLOCK_OPT_BACKING_FILE,
> + .type = QEMU_OPT_STRING,
> + .help = "File name of a base image"
> + },
> + {
> + .name = BLOCK_OPT_ENCRYPT,
> + .type = QEMU_OPT_BOOL,
> + .help = "Encrypt the image",
> + .def_value_str = "off"
> + },
> + { /* end of list */ }
> + }
> };
>
> static BlockDriver bdrv_qcow = {
> @@ -889,7 +888,7 @@ static BlockDriver bdrv_qcow = {
> .bdrv_write_compressed = qcow_write_compressed,
> .bdrv_get_info = qcow_get_info,
>
> - .create_options = qcow_create_options,
> + .bdrv_create_opts = &qcow_create_opts,
> };
>
> static void bdrv_qcow_init(void)
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 7610e56..83288a8 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1177,7 +1177,7 @@ static int preallocate(BlockDriverState *bs)
> static int qcow2_create2(const char *filename, int64_t total_size,
> const char *backing_file, const char *backing_format,
> int flags, size_t cluster_size, int prealloc,
> - QEMUOptionParameter *options, int version)
> + QemuOpts *opts, int version)
> {
> /* Calculate cluster_bits */
> int cluster_bits;
> @@ -1208,7 +1208,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
> uint8_t* refcount_table;
> int ret;
>
> - ret = bdrv_create_file(filename, options);
> + ret = bdrv_create_file(filename, opts);
> if (ret < 0) {
> return ret;
> }
> @@ -1311,7 +1311,7 @@ out:
> return ret;
> }
>
> -static int qcow2_create(const char *filename, QEMUOptionParameter *options)
> +static int qcow2_create(const char *filename, QemuOpts *opts)
> {
> const char *backing_file = NULL;
> const char *backing_fmt = NULL;
> @@ -1320,45 +1320,41 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options)
> size_t cluster_size = DEFAULT_CLUSTER_SIZE;
> int prealloc = 0;
> int version = 2;
> + const char *buf;
>
> /* Read out options */
> - while (options && options->name) {
> - if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> - sectors = options->value.n / 512;
> - } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FILE)) {
> - backing_file = options->value.s;
> - } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FMT)) {
> - backing_fmt = options->value.s;
> - } else if (!strcmp(options->name, BLOCK_OPT_ENCRYPT)) {
> - flags |= options->value.n ? BLOCK_FLAG_ENCRYPT : 0;
> - } else if (!strcmp(options->name, BLOCK_OPT_CLUSTER_SIZE)) {
> - if (options->value.n) {
> - cluster_size = options->value.n;
> - }
> - } else if (!strcmp(options->name, BLOCK_OPT_PREALLOC)) {
> - if (!options->value.s || !strcmp(options->value.s, "off")) {
> - prealloc = 0;
> - } else if (!strcmp(options->value.s, "metadata")) {
> - prealloc = 1;
> - } else {
> - fprintf(stderr, "Invalid preallocation mode: '%s'\n",
> - options->value.s);
> - return -EINVAL;
> - }
> - } else if (!strcmp(options->name, BLOCK_OPT_COMPAT_LEVEL)) {
> - if (!options->value.s || !strcmp(options->value.s, "0.10")) {
> - version = 2;
> - } else if (!strcmp(options->value.s, "1.1")) {
> - version = 3;
> - } else {
> - fprintf(stderr, "Invalid compatibility level: '%s'\n",
> - options->value.s);
> - return -EINVAL;
> - }
> - } else if (!strcmp(options->name, BLOCK_OPT_LAZY_REFCOUNTS)) {
> - flags |= options->value.n ? BLOCK_FLAG_LAZY_REFCOUNTS : 0;
> - }
> - options++;
> + sectors = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0) / 512;
> + backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
> + backing_fmt = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FMT);
> + if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, 0)) {
> + flags |= BLOCK_FLAG_ENCRYPT;
> + }
> + cluster_size = qemu_opt_get_size_del(opts, BLOCK_OPT_CLUSTER_SIZE,
> + DEFAULT_CLUSTER_SIZE);
> + buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC);
> + if (!buf || !strcmp(buf, "off")) {
> + prealloc = 0;
> + } else if (!strcmp(buf, "metadata")) {
> + prealloc = 1;
> + } else {
> + fprintf(stderr, "Invalid preallocation mode: '%s'\n",
> + buf);
> + return -EINVAL;
> + }
> +
> + buf = qemu_opt_get_del(opts, BLOCK_OPT_COMPAT_LEVEL);
> + if (!buf || !strcmp(buf, "0.10")) {
> + version = 2;
> + } else if (!strcmp(buf, "1.1")) {
> + version = 3;
> + } else {
> + fprintf(stderr, "Invalid compatibility level: '%s'\n",
> + buf);
> + return -EINVAL;
> + }
> +
> + if (qemu_opt_get_bool_del(opts, BLOCK_OPT_LAZY_REFCOUNTS, 0)) {
> + flags |= BLOCK_FLAG_LAZY_REFCOUNTS;
> }
>
> if (backing_file && prealloc) {
> @@ -1374,7 +1370,7 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options)
> }
>
> return qcow2_create2(filename, sectors, backing_file, backing_fmt, flags,
> - cluster_size, prealloc, options, version);
> + cluster_size, prealloc, opts, version);
> }
>
> static int qcow2_make_empty(BlockDriverState *bs)
> @@ -1635,49 +1631,55 @@ static int qcow2_load_vmstate(BlockDriverState *bs, uint8_t *buf,
> return ret;
> }
>
> -static QEMUOptionParameter qcow2_create_options[] = {
> - {
> - .name = BLOCK_OPT_SIZE,
> - .type = OPT_SIZE,
> - .help = "Virtual disk size"
> - },
> - {
> - .name = BLOCK_OPT_COMPAT_LEVEL,
> - .type = OPT_STRING,
> - .help = "Compatibility level (0.10 or 1.1)"
> - },
> - {
> - .name = BLOCK_OPT_BACKING_FILE,
> - .type = OPT_STRING,
> - .help = "File name of a base image"
> - },
> - {
> - .name = BLOCK_OPT_BACKING_FMT,
> - .type = OPT_STRING,
> - .help = "Image format of the base image"
> - },
> - {
> - .name = BLOCK_OPT_ENCRYPT,
> - .type = OPT_FLAG,
> - .help = "Encrypt the image"
> - },
> - {
> - .name = BLOCK_OPT_CLUSTER_SIZE,
> - .type = OPT_SIZE,
> - .help = "qcow2 cluster size",
> - .value = { .n = DEFAULT_CLUSTER_SIZE },
> - },
> - {
> - .name = BLOCK_OPT_PREALLOC,
> - .type = OPT_STRING,
> - .help = "Preallocation mode (allowed values: off, metadata)"
> - },
> - {
> - .name = BLOCK_OPT_LAZY_REFCOUNTS,
> - .type = OPT_FLAG,
> - .help = "Postpone refcount updates",
> - },
> - { NULL }
> +static QemuOptsList qcow2_create_opts = {
> + .name = "qcow2-create-opts",
> + .head = QTAILQ_HEAD_INITIALIZER(qcow2_create_opts.head),
> + .desc = {
> + {
> + .name = BLOCK_OPT_SIZE,
> + .type = QEMU_OPT_SIZE,
> + .help = "Virtual disk size"
> + },
> + {
> + .name = BLOCK_OPT_COMPAT_LEVEL,
> + .type = QEMU_OPT_STRING,
> + .help = "Compatibility level (0.10 or 1.1)"
> + },
> + {
> + .name = BLOCK_OPT_BACKING_FILE,
> + .type = QEMU_OPT_STRING,
> + .help = "File name of a base image"
> + },
> + {
> + .name = BLOCK_OPT_BACKING_FMT,
> + .type = QEMU_OPT_STRING,
> + .help = "Image format of the base image"
> + },
> + {
> + .name = BLOCK_OPT_ENCRYPT,
> + .type = QEMU_OPT_BOOL,
> + .help = "Encrypt the image",
> + .def_value_str = "off"
> + },
> + {
> + .name = BLOCK_OPT_CLUSTER_SIZE,
> + .type = QEMU_OPT_SIZE,
> + .help = "qcow2 cluster size",
> + .def_value_str = stringify(DEFAULT_CLUSTER_SIZE)
> + },
> + {
> + .name = BLOCK_OPT_PREALLOC,
> + .type = QEMU_OPT_STRING,
> + .help = "Preallocation mode (allowed values: off, metadata)"
> + },
> + {
> + .name = BLOCK_OPT_LAZY_REFCOUNTS,
> + .type = QEMU_OPT_BOOL,
> + .help = "Postpone refcount updates",
> + .def_value_str = "off"
> + },
> + { /* end of list */ }
> + }
> };
>
> static BlockDriver bdrv_qcow2 = {
> @@ -1715,8 +1717,9 @@ static BlockDriver bdrv_qcow2 = {
>
> .bdrv_invalidate_cache = qcow2_invalidate_cache,
>
> - .create_options = qcow2_create_options,
> .bdrv_check = qcow2_check,
> +
> + .bdrv_create_opts = &qcow2_create_opts,
> };
>
> static void bdrv_qcow2_init(void)
> diff --git a/block/qed.c b/block/qed.c
> index b8515e5..6032ea6 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -603,7 +603,7 @@ out:
> return ret;
> }
>
> -static int bdrv_qed_create(const char *filename, QEMUOptionParameter *options)
> +static int bdrv_qed_create(const char *filename, QemuOpts *opts)
> {
> uint64_t image_size = 0;
> uint32_t cluster_size = QED_DEFAULT_CLUSTER_SIZE;
> @@ -611,24 +611,14 @@ static int bdrv_qed_create(const char *filename, QEMUOptionParameter *options)
> const char *backing_file = NULL;
> const char *backing_fmt = NULL;
>
> - while (options && options->name) {
> - if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> - image_size = options->value.n;
> - } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FILE)) {
> - backing_file = options->value.s;
> - } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FMT)) {
> - backing_fmt = options->value.s;
> - } else if (!strcmp(options->name, BLOCK_OPT_CLUSTER_SIZE)) {
> - if (options->value.n) {
> - cluster_size = options->value.n;
> - }
> - } else if (!strcmp(options->name, BLOCK_OPT_TABLE_SIZE)) {
> - if (options->value.n) {
> - table_size = options->value.n;
> - }
> - }
> - options++;
> - }
> + image_size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0);
> + backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
> + backing_fmt = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FMT);
> + cluster_size = qemu_opt_get_size_del(opts,
> + BLOCK_OPT_CLUSTER_SIZE,
> + QED_DEFAULT_CLUSTER_SIZE);
> + table_size = qemu_opt_get_size_del(opts, BLOCK_OPT_TABLE_SIZE,
> + QED_DEFAULT_TABLE_SIZE);
>
> if (!qed_is_cluster_size_valid(cluster_size)) {
> fprintf(stderr, "QED cluster size must be within range [%u, %u] and power of 2\n",
> @@ -1537,36 +1527,44 @@ static int bdrv_qed_check(BlockDriverState *bs, BdrvCheckResult *result,
> return qed_check(s, result, !!fix);
> }
>
> -static QEMUOptionParameter qed_create_options[] = {
> - {
> - .name = BLOCK_OPT_SIZE,
> - .type = OPT_SIZE,
> - .help = "Virtual disk size (in bytes)"
> - }, {
> - .name = BLOCK_OPT_BACKING_FILE,
> - .type = OPT_STRING,
> - .help = "File name of a base image"
> - }, {
> - .name = BLOCK_OPT_BACKING_FMT,
> - .type = OPT_STRING,
> - .help = "Image format of the base image"
> - }, {
> - .name = BLOCK_OPT_CLUSTER_SIZE,
> - .type = OPT_SIZE,
> - .help = "Cluster size (in bytes)",
> - .value = { .n = QED_DEFAULT_CLUSTER_SIZE },
> - }, {
> - .name = BLOCK_OPT_TABLE_SIZE,
> - .type = OPT_SIZE,
> - .help = "L1/L2 table size (in clusters)"
> - },
> - { /* end of list */ }
> +static QemuOptsList qed_create_opts = {
> + .name = "qed-create-opts",
> + .head = QTAILQ_HEAD_INITIALIZER(qed_create_opts.head),
> + .desc = {
> + {
> + .name = BLOCK_OPT_SIZE,
> + .type = QEMU_OPT_SIZE,
> + .help = "Virtual disk size"
> + },
> + {
> + .name = BLOCK_OPT_BACKING_FILE,
> + .type = QEMU_OPT_STRING,
> + .help = "File name of a base image"
> + },
> + {
> + .name = BLOCK_OPT_BACKING_FMT,
> + .type = QEMU_OPT_STRING,
> + .help = "Image format of the base image"
> + },
> + {
> + .name = BLOCK_OPT_CLUSTER_SIZE,
> + .type = QEMU_OPT_SIZE,
> + .help = "Cluster size (in bytes)",
> + .def_value_str = stringify(QED_DEFAULT_CLUSTER_SIZE),
> + },
> + {
> + .name = BLOCK_OPT_TABLE_SIZE,
> + .type = QEMU_OPT_SIZE,
> + .help = "L1/L2 table size (in clusters)"
> + },
> + { /* end of list */ }
> + }
> };
>
> static BlockDriver bdrv_qed = {
> .format_name = "qed",
> .instance_size = sizeof(BDRVQEDState),
> - .create_options = qed_create_options,
> + .bdrv_create_opts = &qed_create_opts,
>
> .bdrv_probe = bdrv_qed_probe,
> .bdrv_rebind = bdrv_qed_rebind,
> diff --git a/block/qed.h b/block/qed.h
> index 2b4dded..99a7726 100644
> --- a/block/qed.h
> +++ b/block/qed.h
> @@ -43,6 +43,7 @@
> *
> * All fields are little-endian on disk.
> */
> +#define QED_DEFAULT_CLUSTER_SIZE 65536
>
> enum {
> QED_MAGIC = 'Q' | 'E' << 8 | 'D' << 16 | '\0' << 24,
> @@ -69,7 +70,6 @@ enum {
> */
> QED_MIN_CLUSTER_SIZE = 4 * 1024, /* in bytes */
> QED_MAX_CLUSTER_SIZE = 64 * 1024 * 1024,
> - QED_DEFAULT_CLUSTER_SIZE = 64 * 1024,
>
> /* Allocated clusters are tracked using a 2-level pagetable. Table size is
> * a multiple of clusters so large maximum image sizes can be supported
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 4dfdf98..18e965f 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -123,6 +123,19 @@
>
> #define MAX_BLOCKSIZE 4096
>
> +static QemuOptsList file_proto_create_opts = {
> + .name = "file-proto-create-opts",
> + .head = QTAILQ_HEAD_INITIALIZER(file_proto_create_opts.head),
> + .desc = {
> + {
> + .name = BLOCK_OPT_SIZE,
> + .type = QEMU_OPT_SIZE,
> + .help = "Virtual disk size"
> + },
> + { /* end of list */ }
> + }
> +};
> +
> typedef struct BDRVRawState {
> int fd;
> int type;
> @@ -1006,19 +1019,14 @@ static int64_t raw_get_allocated_file_size(BlockDriverState *bs)
> return (int64_t)st.st_blocks * 512;
> }
>
> -static int raw_create(const char *filename, QEMUOptionParameter *options)
> +static int raw_create(const char *filename, QemuOpts *opts)
> {
> int fd;
> int result = 0;
> int64_t total_size = 0;
>
> - /* Read out options */
> - while (options && options->name) {
> - if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> - total_size = options->value.n / BDRV_SECTOR_SIZE;
> - }
> - options++;
> - }
> + total_size =
> + qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0) / BDRV_SECTOR_SIZE;
>
> fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
> 0644);
> @@ -1145,15 +1153,6 @@ static coroutine_fn BlockDriverAIOCB *raw_aio_discard(BlockDriverState *bs,
> cb, opaque, QEMU_AIO_DISCARD);
> }
>
> -static QEMUOptionParameter raw_create_options[] = {
> - {
> - .name = BLOCK_OPT_SIZE,
> - .type = OPT_SIZE,
> - .help = "Virtual disk size"
> - },
> - { NULL }
> -};
> -
> static BlockDriver bdrv_file = {
> .format_name = "file",
> .protocol_name = "file",
> @@ -1176,8 +1175,7 @@ static BlockDriver bdrv_file = {
> .bdrv_getlength = raw_getlength,
> .bdrv_get_allocated_file_size
> = raw_get_allocated_file_size,
> -
> - .create_options = raw_create_options,
> + .bdrv_create_opts = &file_proto_create_opts,
> };
>
> /***********************************************/
> @@ -1459,20 +1457,15 @@ static coroutine_fn BlockDriverAIOCB *hdev_aio_discard(BlockDriverState *bs,
> cb, opaque, QEMU_AIO_DISCARD|QEMU_AIO_BLKDEV);
> }
>
> -static int hdev_create(const char *filename, QEMUOptionParameter *options)
> +static int hdev_create(const char *filename, QemuOpts *opts)
> {
> int fd;
> int ret = 0;
> struct stat stat_buf;
> int64_t total_size = 0;
>
> - /* Read out options */
> - while (options && options->name) {
> - if (!strcmp(options->name, "size")) {
> - total_size = options->value.n / BDRV_SECTOR_SIZE;
> - }
> - options++;
> - }
> + total_size =
> + qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0) / BDRV_SECTOR_SIZE;
>
> fd = qemu_open(filename, O_WRONLY | O_BINARY);
> if (fd < 0)
> @@ -1496,7 +1489,7 @@ static int hdev_has_zero_init(BlockDriverState *bs)
>
> static BlockDriver bdrv_host_device = {
> .format_name = "host_device",
> - .protocol_name = "host_device",
> + .protocol_name = "host_device",
> .instance_size = sizeof(BDRVRawState),
> .bdrv_probe_device = hdev_probe_device,
> .bdrv_file_open = hdev_open,
> @@ -1505,7 +1498,6 @@ static BlockDriver bdrv_host_device = {
> .bdrv_reopen_commit = raw_reopen_commit,
> .bdrv_reopen_abort = raw_reopen_abort,
> .bdrv_create = hdev_create,
> - .create_options = raw_create_options,
> .bdrv_has_zero_init = hdev_has_zero_init,
>
> .bdrv_aio_readv = raw_aio_readv,
> @@ -1514,9 +1506,10 @@ static BlockDriver bdrv_host_device = {
> .bdrv_aio_discard = hdev_aio_discard,
>
> .bdrv_truncate = raw_truncate,
> - .bdrv_getlength = raw_getlength,
> + .bdrv_getlength = raw_getlength,
> .bdrv_get_allocated_file_size
> = raw_get_allocated_file_size,
> + .bdrv_create_opts = &file_proto_create_opts,
>
> /* generic scsi device */
> #ifdef __linux__
> @@ -1630,7 +1623,6 @@ static BlockDriver bdrv_host_floppy = {
> .bdrv_reopen_commit = raw_reopen_commit,
> .bdrv_reopen_abort = raw_reopen_abort,
> .bdrv_create = hdev_create,
> - .create_options = raw_create_options,
> .bdrv_has_zero_init = hdev_has_zero_init,
>
> .bdrv_aio_readv = raw_aio_readv,
> @@ -1646,6 +1638,7 @@ static BlockDriver bdrv_host_floppy = {
> .bdrv_is_inserted = floppy_is_inserted,
> .bdrv_media_changed = floppy_media_changed,
> .bdrv_eject = floppy_eject,
> + .bdrv_create_opts = &file_proto_create_opts,
> };
>
> static int cdrom_open(BlockDriverState *bs, const char *filename, int flags)
> @@ -1732,7 +1725,6 @@ static BlockDriver bdrv_host_cdrom = {
> .bdrv_reopen_commit = raw_reopen_commit,
> .bdrv_reopen_abort = raw_reopen_abort,
> .bdrv_create = hdev_create,
> - .create_options = raw_create_options,
> .bdrv_has_zero_init = hdev_has_zero_init,
>
> .bdrv_aio_readv = raw_aio_readv,
> @@ -1752,6 +1744,8 @@ static BlockDriver bdrv_host_cdrom = {
> /* generic scsi device */
> .bdrv_ioctl = hdev_ioctl,
> .bdrv_aio_ioctl = hdev_aio_ioctl,
> +
> + .bdrv_create_opts = &file_proto_create_opts,
> };
> #endif /* __linux__ */
>
> @@ -1854,7 +1848,6 @@ static BlockDriver bdrv_host_cdrom = {
> .bdrv_reopen_commit = raw_reopen_commit,
> .bdrv_reopen_abort = raw_reopen_abort,
> .bdrv_create = hdev_create,
> - .create_options = raw_create_options,
> .bdrv_has_zero_init = hdev_has_zero_init,
>
> .bdrv_aio_readv = raw_aio_readv,
> diff --git a/block/raw-win32.c b/block/raw-win32.c
> index b89ac19..d000f88 100644
> --- a/block/raw-win32.c
> +++ b/block/raw-win32.c
> @@ -382,18 +382,15 @@ static int64_t raw_get_allocated_file_size(BlockDriverState *bs)
> return st.st_size;
> }
>
> -static int raw_create(const char *filename, QEMUOptionParameter *options)
> +static int raw_create(const char *filename, QemuOpts *opts)
> {
> int fd;
> int64_t total_size = 0;
>
> /* Read out options */
> - while (options && options->name) {
> - if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> - total_size = options->value.n / 512;
> - }
> - options++;
> - }
> +
> + total_size =
> + qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0) / 512;
>
> fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
> 0644);
> @@ -405,13 +402,17 @@ static int raw_create(const char *filename, QEMUOptionParameter *options)
> return 0;
> }
>
> -static QEMUOptionParameter raw_create_options[] = {
> - {
> - .name = BLOCK_OPT_SIZE,
> - .type = OPT_SIZE,
> - .help = "Virtual disk size"
> - },
> - { NULL }
> +static QemuOptsList raw_create_opts = {
> + .name = "raw-create-opts",
> + .head = QTAILQ_HEAD_INITIALIZER(raw_create_opts.head),
> + .desc = {
> + {
> + .name = BLOCK_OPT_SIZE,
> + .type = QEMU_OPT_SIZE,
> + .help = "Virtual disk size"
> + },
> + { /* end of list */ }
> + }
> };
>
> static BlockDriver bdrv_file = {
> @@ -431,7 +432,7 @@ static BlockDriver bdrv_file = {
> .bdrv_get_allocated_file_size
> = raw_get_allocated_file_size,
>
> - .create_options = raw_create_options,
> + .bdrv_create_opts = &raw_create_opts,
> };
>
> /***********************************************/
> diff --git a/block/raw.c b/block/raw.c
> index 75812db..8cb26c2 100644
> --- a/block/raw.c
> +++ b/block/raw.c
> @@ -95,18 +95,22 @@ static BlockDriverAIOCB *raw_aio_ioctl(BlockDriverState *bs,
> return bdrv_aio_ioctl(bs->file, req, buf, cb, opaque);
> }
>
> -static int raw_create(const char *filename, QEMUOptionParameter *options)
> -{
> - return bdrv_create_file(filename, options);
> -}
> -
> -static QEMUOptionParameter raw_create_options[] = {
> - {
> - .name = BLOCK_OPT_SIZE,
> - .type = OPT_SIZE,
> - .help = "Virtual disk size"
> - },
> - { NULL }
> +static int raw_create(const char *filename, QemuOpts *opts)
> +{
> + return bdrv_create_file(filename, opts);
> +}
> +
> +static QemuOptsList raw_create_opts = {
> + .name = "raw-create-opts",
> + .head = QTAILQ_HEAD_INITIALIZER(raw_create_opts.head),
> + .desc = {
> + {
> + .name = BLOCK_OPT_SIZE,
> + .type = QEMU_OPT_SIZE,
> + .help = "Virtual disk size"
> + },
> + { /* end of list */ }
> + }
> };
>
> static int raw_has_zero_init(BlockDriverState *bs)
> @@ -143,8 +147,8 @@ static BlockDriver bdrv_raw = {
> .bdrv_aio_ioctl = raw_aio_ioctl,
>
> .bdrv_create = raw_create,
> - .create_options = raw_create_options,
> .bdrv_has_zero_init = raw_has_zero_init,
> + .bdrv_create_opts = &raw_create_opts,
> };
>
> static void bdrv_raw_init(void)
> diff --git a/block/rbd.c b/block/rbd.c
> index 8cd10a7..2b56ed7 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -287,7 +287,7 @@ static int qemu_rbd_set_conf(rados_t cluster, const char *conf)
> return ret;
> }
>
> -static int qemu_rbd_create(const char *filename, QEMUOptionParameter *options)
> +static int qemu_rbd_create(const char *filename, QemuOpts *opts)
> {
> int64_t bytes = 0;
> int64_t objsize;
> @@ -310,24 +310,18 @@ static int qemu_rbd_create(const char *filename, QEMUOptionParameter *options)
> }
>
> /* Read out options */
> - while (options && options->name) {
> - if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> - bytes = options->value.n;
> - } else if (!strcmp(options->name, BLOCK_OPT_CLUSTER_SIZE)) {
> - if (options->value.n) {
> - objsize = options->value.n;
> - if ((objsize - 1) & objsize) { /* not a power of 2? */
> - error_report("obj size needs to be power of 2");
> - return -EINVAL;
> - }
> - if (objsize < 4096) {
> - error_report("obj size too small");
> - return -EINVAL;
> - }
> - obj_order = ffs(objsize) - 1;
> - }
> + bytes = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0);
> + objsize = qemu_opt_get_size_del(opts, BLOCK_OPT_CLUSTER_SIZE, 0);
> + if (objsize) {
> + if ((objsize - 1) & objsize) { /* not a power of 2? */
> + error_report("obj size needs to be power of 2");
> + return -EINVAL;
> + }
> + if (objsize < 4096) {
> + error_report("obj size too small");
> + return -EINVAL;
> }
> - options++;
> + obj_order = ffs(objsize) - 1;
> }
>
> clientname = qemu_rbd_parse_clientname(conf, clientname_buf);
> @@ -920,20 +914,24 @@ static BlockDriverAIOCB* qemu_rbd_aio_discard(BlockDriverState *bs,
> }
> #endif
>
> -static QEMUOptionParameter qemu_rbd_create_options[] = {
> - {
> - .name = BLOCK_OPT_SIZE,
> - .type = OPT_SIZE,
> - .help = "Virtual disk size"
> - },
> - {
> - .name = BLOCK_OPT_CLUSTER_SIZE,
> - .type = OPT_SIZE,
> - .help = "RBD object size"
> - },
> - {NULL}
> +static QemuOptsList rbd_create_opts = {
> + .name = "rbd-create-opts",
> + .head = QTAILQ_HEAD_INITIALIZER(rbd_create_opts.head),
> + .desc = {
> + {
> + .name = BLOCK_OPT_SIZE,
> + .type = QEMU_OPT_SIZE,
> + .help = "Virtual disk size"
> + },
> + {
> + .name = BLOCK_OPT_CLUSTER_SIZE,
> + .type = QEMU_OPT_SIZE,
> + .help = "RBD object size",
> + .def_value_str = stringify(0),
> + },
> + { /* end of list */ }
> + }
> };
> -
> static BlockDriver bdrv_rbd = {
> .format_name = "rbd",
> .instance_size = sizeof(BDRVRBDState),
> @@ -941,7 +939,7 @@ static BlockDriver bdrv_rbd = {
> .bdrv_close = qemu_rbd_close,
> .bdrv_create = qemu_rbd_create,
> .bdrv_get_info = qemu_rbd_getinfo,
> - .create_options = qemu_rbd_create_options,
> + .bdrv_create_opts = &rbd_create_opts,
> .bdrv_getlength = qemu_rbd_getlength,
> .bdrv_truncate = qemu_rbd_truncate,
> .protocol_name = "rbd",
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index d466b23..56397cc 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -1274,12 +1274,12 @@ out:
> return ret;
> }
>
> -static int sd_create(const char *filename, QEMUOptionParameter *options)
> +static int sd_create(const char *filename, QemuOpts *opts)
> {
> int ret = 0;
> uint32_t vid = 0, base_vid = 0;
> int64_t vdi_size = 0;
> - char *backing_file = NULL;
> + const char *backing_file = NULL, *buf = NULL;
> BDRVSheepdogState *s;
> char vdi[SD_MAX_VDI_LEN], tag[SD_MAX_VDI_TAG_LEN];
> uint32_t snapid;
> @@ -1298,26 +1298,18 @@ static int sd_create(const char *filename, QEMUOptionParameter *options)
> goto out;
> }
>
> - while (options && options->name) {
> - if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> - vdi_size = options->value.n;
> - } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FILE)) {
> - backing_file = options->value.s;
> - } else if (!strcmp(options->name, BLOCK_OPT_PREALLOC)) {
> - if (!options->value.s || !strcmp(options->value.s, "off")) {
> - prealloc = false;
> - } else if (!strcmp(options->value.s, "full")) {
> - prealloc = true;
> - } else {
> - error_report("Invalid preallocation mode: '%s'",
> - options->value.s);
> - ret = -EINVAL;
> - goto out;
> - }
> - }
> - options++;
> + vdi_size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0);
> + backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
> + buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC);
> + if (!buf || !strcmp(buf, "off")) {
> + prealloc = false;
> + } else if (!strcmp(buf, "full")) {
> + prealloc = true;
> + } else {
> + error_report("Invalid preallocation mode: '%s'", buf);
> + ret = -EINVAL;
> + goto out;
> }
> -
> if (vdi_size > SD_MAX_VDI_SIZE) {
> error_report("too big image size");
> ret = -EINVAL;
> @@ -2043,24 +2035,27 @@ static int sd_load_vmstate(BlockDriverState *bs, uint8_t *data,
> return do_load_save_vmstate(s, data, pos, size, 1);
> }
>
> -
> -static QEMUOptionParameter sd_create_options[] = {
> - {
> - .name = BLOCK_OPT_SIZE,
> - .type = OPT_SIZE,
> - .help = "Virtual disk size"
> - },
> - {
> - .name = BLOCK_OPT_BACKING_FILE,
> - .type = OPT_STRING,
> - .help = "File name of a base image"
> - },
> - {
> - .name = BLOCK_OPT_PREALLOC,
> - .type = OPT_STRING,
> - .help = "Preallocation mode (allowed values: off, full)"
> - },
> - { NULL }
> +static QemuOptsList sd_create_opts = {
> + .name = "sheepdog-create-opts",
> + .head = QTAILQ_HEAD_INITIALIZER(sd_create_opts.head),
> + .desc = {
> + {
> + .name = BLOCK_OPT_SIZE,
> + .type = QEMU_OPT_SIZE,
> + .help = "Virtual disk size"
> + },
> + {
> + .name = BLOCK_OPT_BACKING_FILE,
> + .type = QEMU_OPT_STRING,
> + .help = "File name of a base image"
> + },
> + {
> + .name = BLOCK_OPT_PREALLOC,
> + .type = QEMU_OPT_STRING,
> + .help = "Preallocation mode (allowed values: off, full)"
> + },
> + { /* end of list */ }
> + }
> };
>
> BlockDriver bdrv_sheepdog = {
> @@ -2085,7 +2080,7 @@ BlockDriver bdrv_sheepdog = {
> .bdrv_save_vmstate = sd_save_vmstate,
> .bdrv_load_vmstate = sd_load_vmstate,
>
> - .create_options = sd_create_options,
> + .bdrv_create_opts = &sd_create_opts,
> };
>
> static void bdrv_sheepdog_init(void)
> diff --git a/block/vdi.c b/block/vdi.c
> index 87c691b..48b1bec 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -633,7 +633,7 @@ static int vdi_co_write(BlockDriverState *bs,
> return ret;
> }
>
> -static int vdi_create(const char *filename, QEMUOptionParameter *options)
> +static int vdi_create(const char *filename, QemuOpts *opts)
> {
> int fd;
> int result = 0;
> @@ -648,25 +648,18 @@ static int vdi_create(const char *filename, QEMUOptionParameter *options)
> logout("\n");
>
> /* Read out options. */
> - while (options && options->name) {
> - if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> - bytes = options->value.n;
> + bytes = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0);
> #if defined(CONFIG_VDI_BLOCK_SIZE)
> - } else if (!strcmp(options->name, BLOCK_OPT_CLUSTER_SIZE)) {
> - if (options->value.n) {
> - /* TODO: Additional checks (SECTOR_SIZE * 2^n, ...). */
> - block_size = options->value.n;
> - }
> + /* TODO: Additional checks (SECTOR_SIZE * 2^n, ...). */
> + block_size = qemu_opt_get_size_del(opts,
> + BLOCK_OPT_CLUSTER_SIZE,
> + DEFAULT_CLUSTER_SIZE);
> #endif
> #if defined(CONFIG_VDI_STATIC_IMAGE)
> - } else if (!strcmp(options->name, BLOCK_OPT_STATIC)) {
> - if (options->value.n) {
> - image_type = VDI_TYPE_STATIC;
> - }
> -#endif
> - }
> - options++;
> + if (qemu_opt_get_bool_del(opts, BLOCK_OPT_STATIC, 0)) {
> + image_type = VDI_TYPE_STATIC;
> }
> +#endif
>
> fd = qemu_open(filename,
> O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
> @@ -746,29 +739,34 @@ static void vdi_close(BlockDriverState *bs)
> error_free(s->migration_blocker);
> }
>
> -static QEMUOptionParameter vdi_create_options[] = {
> - {
> - .name = BLOCK_OPT_SIZE,
> - .type = OPT_SIZE,
> - .help = "Virtual disk size"
> - },
> +static QemuOptsList vdi_create_opts = {
> + .name = "vdi-create-opts",
> + .head = QTAILQ_HEAD_INITIALIZER(vdi_create_opts.head),
> + .desc = {
> + {
> + .name = BLOCK_OPT_SIZE,
> + .type = QEMU_OPT_SIZE,
> + .help = "Virtual disk size"
> + },
> #if defined(CONFIG_VDI_BLOCK_SIZE)
> - {
> - .name = BLOCK_OPT_CLUSTER_SIZE,
> - .type = OPT_SIZE,
> - .help = "VDI cluster (block) size",
> - .value = { .n = DEFAULT_CLUSTER_SIZE },
> - },
> + {
> + .name = BLOCK_OPT_CLUSTER_SIZE,
> + .type = QEMU_OPT_SIZE,
> + .help = "VDI cluster (block) size",
> + .def_value_str = stringify(DEFAULT_CLUSTER_SIZE)
> + },
> #endif
> #if defined(CONFIG_VDI_STATIC_IMAGE)
> - {
> - .name = BLOCK_OPT_STATIC,
> - .type = OPT_FLAG,
> - .help = "VDI static (pre-allocated) image"
> - },
> + {
> + .name = BLOCK_OPT_STATIC,
> + .type = QEMU_OPT_BOOL,
> + .help = "VDI static (pre-allocated) image",
> + .def_value_str = "off"
> + },
> #endif
> - /* TODO: An additional option to set UUID values might be useful. */
> - { NULL }
> + /* TODO: An additional option to set UUID values might be useful. */
> + { /* end of list */ }
> + }
> };
>
> static BlockDriver bdrv_vdi = {
> @@ -789,7 +787,7 @@ static BlockDriver bdrv_vdi = {
>
> .bdrv_get_info = vdi_get_info,
>
> - .create_options = vdi_create_options,
> + .bdrv_create_opts = &vdi_create_opts,
> .bdrv_check = vdi_check,
> };
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index aef1abc..5428b1a 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1429,7 +1429,7 @@ static int relative_path(char *dest, int dest_size,
> return 0;
> }
>
> -static int vmdk_create(const char *filename, QEMUOptionParameter *options)
> +static int vmdk_create(const char *filename, QemuOpts *opts)
> {
> int fd, idx = 0;
> char desc[BUF_SIZE];
> @@ -1470,21 +1470,14 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options)
> if (filename_decompose(filename, path, prefix, postfix, PATH_MAX)) {
> return -EINVAL;
> }
> - /* Read out options */
> - while (options && options->name) {
> - if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> - total_size = options->value.n;
> - } else if (!strcmp(options->name, BLOCK_OPT_ADAPTER_TYPE)) {
> - adapter_type = options->value.s;
> - } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FILE)) {
> - backing_file = options->value.s;
> - } else if (!strcmp(options->name, BLOCK_OPT_COMPAT6)) {
> - flags |= options->value.n ? BLOCK_FLAG_COMPAT6 : 0;
> - } else if (!strcmp(options->name, BLOCK_OPT_SUBFMT)) {
> - fmt = options->value.s;
> - }
> - options++;
> + /* Read out opts */
> + total_size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0);
> + adapter_type = qemu_opt_get_del(opts, BLOCK_OPT_ADAPTER_TYPE);
> + backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
> + if (qemu_opt_get_bool_del(opts, BLOCK_OPT_COMPAT6, 0)) {
> + flags |= BLOCK_FLAG_COMPAT6;
> }
> + fmt = qemu_opt_get_del(opts, BLOCK_OPT_SUBFMT);
> if (!adapter_type) {
> adapter_type = "ide";
> } else if (strcmp(adapter_type, "ide") &&
> @@ -1665,36 +1658,41 @@ static int64_t vmdk_get_allocated_file_size(BlockDriverState *bs)
> return ret;
> }
>
> -static QEMUOptionParameter vmdk_create_options[] = {
> - {
> - .name = BLOCK_OPT_SIZE,
> - .type = OPT_SIZE,
> - .help = "Virtual disk size"
> - },
> - {
> - .name = BLOCK_OPT_ADAPTER_TYPE,
> - .type = OPT_STRING,
> - .help = "Virtual adapter type, can be one of "
> - "ide (default), lsilogic, buslogic or legacyESX"
> - },
> - {
> - .name = BLOCK_OPT_BACKING_FILE,
> - .type = OPT_STRING,
> - .help = "File name of a base image"
> - },
> - {
> - .name = BLOCK_OPT_COMPAT6,
> - .type = OPT_FLAG,
> - .help = "VMDK version 6 image"
> - },
> - {
> - .name = BLOCK_OPT_SUBFMT,
> - .type = OPT_STRING,
> - .help =
> - "VMDK flat extent format, can be one of "
> - "{monolithicSparse (default) | monolithicFlat | twoGbMaxExtentSparse | twoGbMaxExtentFlat | streamOptimized} "
> - },
> - { NULL }
> +static QemuOptsList vmdk_create_opts = {
> + .name = "vmdk-create-opts",
> + .head = QTAILQ_HEAD_INITIALIZER(vmdk_create_opts.head),
> + .desc = {
> + {
> + .name = BLOCK_OPT_SIZE,
> + .type = QEMU_OPT_SIZE,
> + .help = "Virtual disk size"
> + },
> + {
> + .name = BLOCK_OPT_ADAPTER_TYPE,
> + .type = OPT_STRING,
> + .help = "Virtual adapter type, can be one of "
> + "ide (default), lsilogic, buslogic or legacyESX"
> + },
> + {
> + .name = BLOCK_OPT_BACKING_FILE,
> + .type = QEMU_OPT_STRING,
> + .help = "File name of a base image"
> + },
> + {
> + .name = BLOCK_OPT_COMPAT6,
> + .type = QEMU_OPT_BOOL,
> + .help = "VMDK version 6 image",
> + .def_value_str = "off"
> + },
> + {
> + .name = BLOCK_OPT_SUBFMT,
> + .type = QEMU_OPT_STRING,
> + .help =
> + "VMDK flat extent format, can be one of "
> + "{monolithicSparse (default) | monolithicFlat | twoGbMaxExtentSparse | twoGbMaxExtentFlat | streamOptimized} "
> + },
> + { /* end of list */ }
> + }
> };
>
> static BlockDriver bdrv_vmdk = {
> @@ -1711,7 +1709,7 @@ static BlockDriver bdrv_vmdk = {
> .bdrv_co_is_allocated = vmdk_co_is_allocated,
> .bdrv_get_allocated_file_size = vmdk_get_allocated_file_size,
>
> - .create_options = vmdk_create_options,
> + .bdrv_create_opts = &vmdk_create_opts,
> };
>
> static void bdrv_vmdk_init(void)
> diff --git a/block/vpc.c b/block/vpc.c
> index 82229ef..e8c439f 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -683,34 +683,31 @@ static int create_fixed_disk(int fd, uint8_t *buf, int64_t total_size)
> return ret;
> }
>
> -static int vpc_create(const char *filename, QEMUOptionParameter *options)
> +static int vpc_create(const char *filename, QemuOpts *opts)
> {
> uint8_t buf[1024];
> struct vhd_footer *footer = (struct vhd_footer *) buf;
> - QEMUOptionParameter *disk_type_param;
> + const char *disk_type_param = NULL;
> int fd, i;
> uint16_t cyls = 0;
> uint8_t heads = 0;
> uint8_t secs_per_cyl = 0;
> int64_t total_sectors;
> - int64_t total_size;
> - int disk_type;
> + int64_t total_size = 0;
> + int disk_type = VHD_DYNAMIC;
> int ret = -EIO;
>
> - /* Read out options */
> - total_size = get_option_parameter(options, BLOCK_OPT_SIZE)->value.n;
> -
> - disk_type_param = get_option_parameter(options, BLOCK_OPT_SUBFMT);
> - if (disk_type_param && disk_type_param->value.s) {
> - if (!strcmp(disk_type_param->value.s, "dynamic")) {
> + /* Read out opts */
> + total_size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0);
> + disk_type_param = qemu_opt_get_del(opts, BLOCK_OPT_SUBFMT);
> + if (disk_type_param) {
> + if (!strcmp(disk_type_param, "dynamic")) {
> disk_type = VHD_DYNAMIC;
> - } else if (!strcmp(disk_type_param->value.s, "fixed")) {
> + } else if (!strcmp(disk_type_param, "fixed")) {
> disk_type = VHD_FIXED;
> } else {
> return -EINVAL;
> }
> - } else {
> - disk_type = VHD_DYNAMIC;
> }
>
> /* Create the file */
> @@ -798,20 +795,24 @@ static void vpc_close(BlockDriverState *bs)
> error_free(s->migration_blocker);
> }
>
> -static QEMUOptionParameter vpc_create_options[] = {
> - {
> - .name = BLOCK_OPT_SIZE,
> - .type = OPT_SIZE,
> - .help = "Virtual disk size"
> - },
> - {
> - .name = BLOCK_OPT_SUBFMT,
> - .type = OPT_STRING,
> - .help =
> - "Type of virtual hard disk format. Supported formats are "
> - "{dynamic (default) | fixed} "
> - },
> - { NULL }
> +static QemuOptsList vpc_create_opts = {
> + .name = "vpc-create-opts",
> + .head = QTAILQ_HEAD_INITIALIZER(vpc_create_opts.head),
> + .desc = {
> + {
> + .name = BLOCK_OPT_SIZE,
> + .type = QEMU_OPT_SIZE,
> + .help = "Virtual disk size"
> + },
> + {
> + .name = BLOCK_OPT_SUBFMT,
> + .type = OPT_STRING,
> + .help =
> + "Type of virtual hard disk format. Supported formats are "
> + "{dynamic (default) | fixed} "
> + },
> + { /* end of list */ }
> + }
> };
>
> static BlockDriver bdrv_vpc = {
> @@ -827,7 +828,7 @@ static BlockDriver bdrv_vpc = {
> .bdrv_read = vpc_co_read,
> .bdrv_write = vpc_co_write,
>
> - .create_options = vpc_create_options,
> + .bdrv_create_opts = &vpc_create_opts,
> };
>
> static void bdrv_vpc_init(void)
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 06e6654..26ebe7c 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -2802,7 +2802,7 @@ static BlockDriver vvfat_write_target = {
> static int enable_write_target(BDRVVVFATState *s)
> {
> BlockDriver *bdrv_qcow;
> - QEMUOptionParameter *options;
> + QemuOpts *opts;
> int ret;
> int size = sector2cluster(s, s->sector_count);
> s->used_clusters = calloc(size, 1);
> @@ -2818,12 +2818,13 @@ static int enable_write_target(BDRVVVFATState *s)
> }
>
> bdrv_qcow = bdrv_find_format("qcow");
> - options = parse_option_parameters("", bdrv_qcow->create_options, NULL);
> - set_option_parameter_int(options, BLOCK_OPT_SIZE, s->sector_count * 512);
> - set_option_parameter(options, BLOCK_OPT_BACKING_FILE, "fat:");
> + opts = qemu_opts_create_nofail(bdrv_qcow->bdrv_create_opts);
> + qemu_opt_set_number(opts, BLOCK_OPT_SIZE, s->sector_count * 512);
> + qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, "fat:");
>
> - if (bdrv_create(bdrv_qcow, s->qcow_filename, options) < 0)
> + if (bdrv_create(bdrv_qcow, s->qcow_filename, opts) < 0) {
> return -1;
> + }
>
> s->qcow = bdrv_new("");
> if (s->qcow == NULL) {
> diff --git a/include/block/block.h b/include/block/block.h
> index 0f750d7..a477b7a 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -126,8 +126,8 @@ BlockDriver *bdrv_find_protocol(const char *filename);
> BlockDriver *bdrv_find_format(const char *format_name);
> BlockDriver *bdrv_find_whitelisted_format(const char *format_name);
> int bdrv_create(BlockDriver *drv, const char* filename,
> - QEMUOptionParameter *options);
> -int bdrv_create_file(const char* filename, QEMUOptionParameter *options);
> + QemuOpts *options);
> +int bdrv_create_file(const char *filename, QemuOpts *options);
> BlockDriverState *bdrv_new(const char *device_name);
> void bdrv_make_anon(BlockDriverState *bs);
> void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old);
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index eaad53e..4f942ef 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -90,7 +90,7 @@ struct BlockDriver {
> const uint8_t *buf, int nb_sectors);
> void (*bdrv_close)(BlockDriverState *bs);
> void (*bdrv_rebind)(BlockDriverState *bs);
> - int (*bdrv_create)(const char *filename, QEMUOptionParameter *options);
> + int (*bdrv_create)(const char *filename, QemuOpts *options);
> int (*bdrv_set_key)(BlockDriverState *bs, const char *key);
> int (*bdrv_make_empty)(BlockDriverState *bs);
> /* aio */
> @@ -179,9 +179,7 @@ struct BlockDriver {
> unsigned long int req, void *buf,
> BlockDriverCompletionFunc *cb, void *opaque);
>
> - /* List of options for creating images, terminated by name == NULL */
> - QEMUOptionParameter *create_options;
> -
> + QemuOptsList *bdrv_create_opts;
>
> /*
> * Returns 0 for completed check, -errno for internal errors.
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index c58db43..0d5fb66 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -108,6 +108,7 @@ struct QemuOptsList {
> };
>
> const char *qemu_opt_get(QemuOpts *opts, const char *name);
> +const char *qemu_opt_get_del(QemuOpts *opts, const char *name);
> /**
> * qemu_opt_has_help_opt:
> * @opts: options to search for a help request
> @@ -121,9 +122,13 @@ const char *qemu_opt_get(QemuOpts *opts, const char *name);
> */
> bool qemu_opt_has_help_opt(QemuOpts *opts);
> bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval);
> +bool qemu_opt_get_bool_del(QemuOpts *opts, const char *name, bool defval);
> uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval);
> uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval);
> +uint64_t qemu_opt_get_size_del(QemuOpts *opts, const char *name,
> + uint64_t defval);
> int qemu_opt_set(QemuOpts *opts, const char *name, const char *value);
> +int qemu_opt_replace_set(QemuOpts *opts, const char *name, const char *value);
> void qemu_opt_set_err(QemuOpts *opts, const char *name, const char *value,
> Error **errp);
> int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val);
> @@ -144,7 +149,10 @@ const char *qemu_opts_id(QemuOpts *opts);
> void qemu_opts_del(QemuOpts *opts);
> void qemu_opts_validate(QemuOpts *opts, const QemuOptDesc *desc, Error **errp);
> int qemu_opts_do_parse(QemuOpts *opts, const char *params, const char *firstname);
> -QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params, int permit_abbrev);
> +int qemu_opts_do_parse_replace(QemuOpts *opts, const char *params,
> + const char *firstname);
> +QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params,
> + int permit_abbrev);
> void qemu_opts_set_defaults(QemuOptsList *list, const char *params,
> int permit_abbrev);
> QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict,
> @@ -156,8 +164,7 @@ int qemu_opts_print(QemuOpts *opts, void *dummy);
> int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void *opaque,
> int abort_on_failure);
>
> -QemuOptsList *qemu_opts_append(QemuOptsList *first,
> - QemuOptsList *second);
> +QemuOptsList *qemu_opts_append(QemuOptsList *first, QemuOptsList *second);
> void qemu_opts_free(QemuOptsList *list);
> void qemu_opts_print_help(QemuOptsList *list);
> #endif
> diff --git a/qemu-img.c b/qemu-img.c
> index 471de7d..9339957 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -229,7 +229,7 @@ static int read_password(char *buf, int buf_size)
> static int print_block_option_help(const char *filename, const char *fmt)
> {
> BlockDriver *drv, *proto_drv;
> - QEMUOptionParameter *create_options = NULL;
> + QemuOptsList *create_opts = NULL;
>
> /* Find driver and parse its options */
> drv = bdrv_find_format(fmt);
> @@ -244,12 +244,10 @@ static int print_block_option_help(const char *filename, const char *fmt)
> return 1;
> }
>
> - create_options = append_option_parameters(create_options,
> - drv->create_options);
> - create_options = append_option_parameters(create_options,
> - proto_drv->create_options);
> - print_option_help(create_options);
> - free_option_parameters(create_options);
> + create_opts = qemu_opts_append(drv->bdrv_create_opts,
> + proto_drv->bdrv_create_opts);
> + qemu_opts_print_help(create_opts);
> + qemu_opts_free(create_opts);
> return 0;
> }
>
> @@ -301,19 +299,19 @@ fail:
> return NULL;
> }
>
> -static int add_old_style_options(const char *fmt, QEMUOptionParameter *list,
> +static int add_old_style_options(const char *fmt, QemuOpts *list,
> const char *base_filename,
> const char *base_fmt)
> {
> if (base_filename) {
> - if (set_option_parameter(list, BLOCK_OPT_BACKING_FILE, base_filename)) {
> + if (qemu_opt_set(list, BLOCK_OPT_BACKING_FILE, base_filename)) {
> error_report("Backing file not supported for file format '%s'",
> fmt);
> return -1;
> }
> }
> if (base_fmt) {
> - if (set_option_parameter(list, BLOCK_OPT_BACKING_FMT, base_fmt)) {
> + if (qemu_opt_set(list, BLOCK_OPT_BACKING_FMT, base_fmt)) {
> error_report("Backing file format not supported for file "
> "format '%s'", fmt);
> return -1;
> @@ -1120,8 +1118,9 @@ static int img_convert(int argc, char **argv)
> uint8_t * buf = NULL;
> const uint8_t *buf1;
> BlockDriverInfo bdi;
> - QEMUOptionParameter *param = NULL, *create_options = NULL;
> - QEMUOptionParameter *out_baseimg_param;
> + QemuOpts *param = NULL;
> + QemuOptsList *create_opts = NULL;
> + const char *out_baseimg_param;
> char *options = NULL;
> const char *snapshot_name = NULL;
> float local_progress = 0;
> @@ -1265,40 +1264,36 @@ static int img_convert(int argc, char **argv)
> goto out;
> }
>
> - create_options = append_option_parameters(create_options,
> - drv->create_options);
> - create_options = append_option_parameters(create_options,
> - proto_drv->create_options);
> + create_opts = qemu_opts_append(drv->bdrv_create_opts,
> + proto_drv->bdrv_create_opts);
>
> if (options) {
> - param = parse_option_parameters(options, create_options, param);
> - if (param == NULL) {
> + if (qemu_opts_do_parse_replace(param, options, NULL) != 0) {
> error_report("Invalid options for file format '%s'.", out_fmt);
> ret = -1;
> goto out;
> }
> } else {
> - param = parse_option_parameters("", create_options, param);
> + param = qemu_opts_create_nofail(create_opts);
> }
> -
> - set_option_parameter_int(param, BLOCK_OPT_SIZE, total_sectors * 512);
> + qemu_opt_set_number(param, BLOCK_OPT_SIZE, total_sectors * 512);
> ret = add_old_style_options(out_fmt, param, out_baseimg, NULL);
> if (ret < 0) {
> goto out;
> }
>
> /* Get backing file name if -o backing_file was used */
> - out_baseimg_param = get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
> + out_baseimg_param = qemu_opt_get(param, BLOCK_OPT_BACKING_FILE);
> if (out_baseimg_param) {
> - out_baseimg = out_baseimg_param->value.s;
> + out_baseimg = out_baseimg_param;
> }
>
> /* Check if compression is supported */
> if (compress) {
> - QEMUOptionParameter *encryption =
> - get_option_parameter(param, BLOCK_OPT_ENCRYPT);
> - QEMUOptionParameter *preallocation =
> - get_option_parameter(param, BLOCK_OPT_PREALLOC);
> + bool encryption =
> + qemu_opt_get_bool(param, BLOCK_OPT_ENCRYPT, false);
> + const char *preallocation =
> + qemu_opt_get(param, BLOCK_OPT_PREALLOC);
>
> if (!drv->bdrv_write_compressed) {
> error_report("Compression not supported for this file format");
> @@ -1306,15 +1301,15 @@ static int img_convert(int argc, char **argv)
> goto out;
> }
>
> - if (encryption && encryption->value.n) {
> + if (encryption) {
> error_report("Compression and encryption not supported at "
> "the same time");
> ret = -1;
> goto out;
> }
>
> - if (preallocation && preallocation->value.s
> - && strcmp(preallocation->value.s, "off"))
> + if (preallocation
> + && strcmp(preallocation, "off"))
> {
> error_report("Compression and preallocation not supported at "
> "the same time");
> @@ -1532,8 +1527,10 @@ static int img_convert(int argc, char **argv)
> }
> out:
> qemu_progress_end();
> - free_option_parameters(create_options);
> - free_option_parameters(param);
> + qemu_opts_free(create_opts);
> + if (param) {
> + qemu_opts_del(param);
> + }
> qemu_vfree(buf);
> if (out_bs) {
> bdrv_delete(out_bs);
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 0e42452..dba82b4 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -33,6 +33,8 @@
> #include "qapi/qmp/qerror.h"
> #include "qemu/option_int.h"
>
> +static void qemu_opt_del(QemuOpt *opt);
> +
> /*
> * Extracts the name of an option from the parameter string (p points at the
> * first byte of the option name)
> @@ -531,6 +533,16 @@ const char *qemu_opt_get(QemuOpts *opts, const char *name)
> return opt ? opt->str : NULL;
> }
>
> +const char *qemu_opt_get_del(QemuOpts *opts, const char *name)
> +{
> + QemuOpt *opt = qemu_opt_find(opts, name);
> + const char *str = opt ? opt->str : NULL;
> + if (opt) {
> + qemu_opt_del(opt);
> + }
> + return str;
> +}
Sorry, should use g_strdup,please ignore the mistake and give other
comments.Thanks.
> +
> bool qemu_opt_has_help_opt(QemuOpts *opts)
> {
> QemuOpt *opt;
> @@ -553,6 +565,22 @@ bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval)
> return opt->value.boolean;
> }
>
> +bool qemu_opt_get_bool_del(QemuOpts *opts, const char *name, bool defval)
> +{
> + QemuOpt *opt = qemu_opt_find(opts, name);
> + bool ret;
> +
> + if (opt == NULL) {
> + return defval;
> + }
> + ret = opt->value.boolean;
> + assert(opt->desc && opt->desc->type == QEMU_OPT_BOOL);
> + if (opt) {
> + qemu_opt_del(opt);
> + }
> + return ret;
> +}
> +
> uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval)
> {
> QemuOpt *opt = qemu_opt_find(opts, name);
> @@ -573,6 +601,23 @@ uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval)
> return opt->value.uint;
> }
>
> +uint64_t qemu_opt_get_size_del(QemuOpts *opts, const char *name,
> + uint64_t defval)
> +{
> + QemuOpt *opt = qemu_opt_find(opts, name);
> + uint64_t ret;
> +
> + if (opt == NULL) {
> + return defval;
> + }
> + ret = opt->value.uint;
> + assert(opt->desc && opt->desc->type == QEMU_OPT_SIZE);
> + if (opt) {
> + qemu_opt_del(opt);
> + }
> + return ret;
> +}
> +
> static void qemu_opt_parse(QemuOpt *opt, Error **errp)
> {
> if (opt->desc == NULL)
> @@ -624,7 +669,7 @@ static const QemuOptDesc *find_desc_by_name(const QemuOptDesc *desc,
> }
>
> static void opt_set(QemuOpts *opts, const char *name, const char *value,
> - bool prepend, Error **errp)
> + bool prepend, bool replace, Error **errp)
> {
> QemuOpt *opt;
> const QemuOptDesc *desc;
> @@ -636,6 +681,11 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value,
> return;
> }
>
> + opt = qemu_opt_find(opts, name);
> + if (replace && opt) {
> + qemu_opt_del(opt);
> + }
> +
> opt = g_malloc0(sizeof(*opt));
> opt->name = g_strdup(name);
> opt->opts = opts;
> @@ -658,7 +708,29 @@ int qemu_opt_set(QemuOpts *opts, const char *name, const char *value)
> {
> Error *local_err = NULL;
>
> - opt_set(opts, name, value, false, &local_err);
> + opt_set(opts, name, value, false, false, &local_err);
> + if (error_is_set(&local_err)) {
> + qerror_report_err(local_err);
> + error_free(local_err);
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * If name exists, the function will delete the opt first and then add a new
> + * one.
> + */
> +int qemu_opt_replace_set(QemuOpts *opts, const char *name, const char *value)
> +{
> + Error *local_err = NULL;
> + QemuOpt *opt = qemu_opt_find(opts, name);
> +
> + if (opt) {
> + qemu_opt_del(opt);
> + }
> + opt_set(opts, name, value, false, true, &local_err);
> if (error_is_set(&local_err)) {
> qerror_report_err(local_err);
> error_free(local_err);
> @@ -671,7 +743,7 @@ int qemu_opt_set(QemuOpts *opts, const char *name, const char *value)
> void qemu_opt_set_err(QemuOpts *opts, const char *name, const char *value,
> Error **errp)
> {
> - opt_set(opts, name, value, false, errp);
> + opt_set(opts, name, value, false, false, errp);
> }
>
> int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val)
> @@ -895,7 +967,7 @@ int qemu_opts_print(QemuOpts *opts, void *dummy)
> }
>
> static int opts_do_parse(QemuOpts *opts, const char *params,
> - const char *firstname, bool prepend)
> + const char *firstname, bool prepend, bool replace)
> {
> char option[128], value[1024];
> const char *p,*pe,*pc;
> @@ -931,7 +1003,7 @@ static int opts_do_parse(QemuOpts *opts, const char *params,
> }
> if (strcmp(option, "id") != 0) {
> /* store and parse */
> - opt_set(opts, option, value, prepend, &local_err);
> + opt_set(opts, option, value, prepend, replace, &local_err);
> if (error_is_set(&local_err)) {
> qerror_report_err(local_err);
> error_free(local_err);
> @@ -947,9 +1019,16 @@ static int opts_do_parse(QemuOpts *opts, const char *params,
>
> int qemu_opts_do_parse(QemuOpts *opts, const char *params, const char *firstname)
> {
> - return opts_do_parse(opts, params, firstname, false);
> + return opts_do_parse(opts, params, firstname, false, false);
> +}
> +
> +int qemu_opts_do_parse_replace(QemuOpts *opts, const char *params,
> + const char *firstname)
> +{
> + return opts_do_parse(opts, params, firstname, false, true);
> }
>
> +
> static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
> int permit_abbrev, bool defaults)
> {
> @@ -986,7 +1065,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
> return NULL;
> }
>
> - if (opts_do_parse(opts, params, firstname, defaults) != 0) {
> + if (opts_do_parse(opts, params, firstname, defaults, false) != 0) {
> qemu_opts_del(opts);
> return NULL;
> }
> --
> 1.7.11.7
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH V12 0/4] replace QEMUOptionParameter with QemuOpts parser
2013-03-05 9:06 ` [Qemu-devel] [PATCH V12 0/4] replace QEMUOptionParameter with QemuOpts parser Stefan Hajnoczi
@ 2013-03-07 7:36 ` Markus Armbruster
0 siblings, 0 replies; 10+ messages in thread
From: Markus Armbruster @ 2013-03-07 7:36 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: kwolf, Dong Xu Wang, qemu-devel
Stefan Hajnoczi <stefanha@redhat.com> writes:
> Looks fine at the high level. Markus had specific comments in the last
> version, please wait for his review before applying.
I intend to review it again. Unfortunately, I'm struggling with getting
my review queue under control. Please be patient.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH V12 0/4] replace QEMUOptionParameter with QemuOpts parser
[not found] <1362043236-4635-1-git-send-email-wdongxu@linux.vnet.ibm.com>
2013-03-05 9:06 ` [Qemu-devel] [PATCH V12 0/4] replace QEMUOptionParameter with QemuOpts parser Stefan Hajnoczi
[not found] ` <1362043236-4635-4-git-send-email-wdongxu@linux.vnet.ibm.com>
@ 2013-03-21 13:03 ` Markus Armbruster
2013-03-21 13:20 ` Kevin Wolf
[not found] ` <1362043236-4635-3-git-send-email-wdongxu@linux.vnet.ibm.com>
[not found] ` <1362043236-4635-2-git-send-email-wdongxu@linux.vnet.ibm.com>
4 siblings, 1 reply; 10+ messages in thread
From: Markus Armbruster @ 2013-03-21 13:03 UTC (permalink / raw)
To: Dong Xu Wang; +Cc: kwolf, qemu-devel, stefanha
Doesn't apply anymore. I'll review anyway; sorry for being late with
that.
Kevin, I haven't been able to follow your work on driver-specific
options; do you expect further conflicts with this work?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH V12 2/4] Create four QemuOptsList related functions
[not found] ` <1362043236-4635-3-git-send-email-wdongxu@linux.vnet.ibm.com>
@ 2013-03-21 13:05 ` Markus Armbruster
0 siblings, 0 replies; 10+ messages in thread
From: Markus Armbruster @ 2013-03-21 13:05 UTC (permalink / raw)
To: Dong Xu Wang; +Cc: kwolf, qemu-devel, stefanha
Dong Xu Wang <wdongxu@linux.vnet.ibm.com> writes:
> This patch will create 4 functions, count_opts_list, qemu_opts_append,
> qemu_opts_free and qemu_opts_print_help, they will be used in following
> commits.
>
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> ---
> v11->v12:
> 1) renmae functions.
> 2) fix loop styles and code styles.
> 3) qemu_opts_apend will not return NULL now.
> 4) merge_lists value is from arguments in qemu_opts_append.
>
> v6->v7:
> 1) Fix typo.
>
> v5->v6:
> 1) allocate enough space in append_opts_list function.
>
> include/qemu/option.h | 4 +++
> util/qemu-option.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 85 insertions(+)
>
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index 394170a..c58db43 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -156,4 +156,8 @@ int qemu_opts_print(QemuOpts *opts, void *dummy);
> int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void *opaque,
> int abort_on_failure);
>
> +QemuOptsList *qemu_opts_append(QemuOptsList *first,
> + QemuOptsList *second);
> +void qemu_opts_free(QemuOptsList *list);
> +void qemu_opts_print_help(QemuOptsList *list);
> #endif
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index dbb77b9..0e42452 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -1153,3 +1153,84 @@ int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void *opaque,
> loc_pop(&loc);
> return rc;
> }
> +
> +static size_t count_opts_list(QemuOptsList *list)
> +{
> + size_t i = 0;
> +
> + for (i = 0; list && list->desc[i].name; i++) {
"list &&" makes it accept null argument. See below.
> + ;
> + }
> +
> + return i;
> +}
> +
> +/* Create a new QemuOptsList and make its desc to the merge of first
> + * and second. It will allocate space for one new QemuOptsList plus
> + * enough space for QemuOptDesc in first and second QemuOptsList.
> + * First argument's QemuOptDesc members take precedence over second's.
> + * The result's name and implied_opt_name are not copied from them,
> + * merge_lists is set if first's or second's is set.
I still think we should simply assert that neither argument has
merge_lists set. Or maybe that their merge_lists match.
> + */
> +QemuOptsList *qemu_opts_append(QemuOptsList *first,
> + QemuOptsList *second)
> +{
> + size_t num_first_opts, num_second_opts;
> + QemuOptsList *dest = NULL;
> + int i = 0;
> + int index = 0;
> + QemuOptsList *p = first;
> +
> + num_first_opts = count_opts_list(first);
> + num_second_opts = count_opts_list(second);
> +
> + dest = g_malloc0(sizeof(QemuOptsList)
> + + (num_first_opts + num_second_opts + 1) * sizeof(QemuOptDesc));
> +
> + dest->name = "append_opts_list";
> + dest->implied_opt_name = NULL;
> + dest->merge_lists = first->merge_lists || second->merge_lists;
Note: in the previous version, this function accepted null arguments.
No more. We'll see in PATCH 3/4 whether the callers are happy with this
change.
If first and second can't be null, there's no need to make
count_opts_list() accept null argument anymore, and "list &&" there
could be dropped.
> + QTAILQ_INIT(&dest->head);
> +
> + for (i = 0; p && p->desc[i].name; i++) {
> + if (!find_desc_by_name(dest->desc, p->desc[i].name)) {
> + dest->desc[index].name = g_strdup(p->desc[i].name);
> + dest->desc[index].help = g_strdup(p->desc[i].help);
> + dest->desc[index].type = p->desc[i].type;
> + dest->desc[index].def_value_str =
> + g_strdup(p->desc[i].def_value_str);
> + index++;
> + }
> + if ((p == first) && p && !p->desc[i].name) {
Please drop superfluous paranthesesis around p == first.
> + p = second;
> + i = 0;
> + }
Not exactly squeaky clean loop control, but it should do :)
> + }
> + dest->desc[index].name = NULL;
> + return dest;
> +}
> +
> +/* free a QemuOptsList, can accept NULL as arguments */
> +void qemu_opts_free(QemuOptsList *list)
> +{
> + int i = 0;
> +
> + for (i = 0; list && list->desc[i].name; i++) {
> + g_free((char *)list->desc[i].name);
> + g_free((char *)list->desc[i].help);
> + g_free((char *)list->desc[i].def_value_str);
> + }
> +
> + g_free(list);
> +}
> +
> +void qemu_opts_print_help(QemuOptsList *list)
> +{
> + int i = 0;
> + printf("Supported options:\n");
> + for (i = 0; list && list->desc[i].name; i++) {
> + printf("%-16s %s\n", list->desc[i].name,
> + list->desc[i].help ?
> + list->desc[i].help : "No description available");
> + }
> +}
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH V12 0/4] replace QEMUOptionParameter with QemuOpts parser
2013-03-21 13:03 ` [Qemu-devel] [PATCH V12 0/4] replace QEMUOptionParameter with QemuOpts parser Markus Armbruster
@ 2013-03-21 13:20 ` Kevin Wolf
0 siblings, 0 replies; 10+ messages in thread
From: Kevin Wolf @ 2013-03-21 13:20 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Dong Xu Wang, qemu-devel, stefanha
Am 21.03.2013 um 14:03 hat Markus Armbruster geschrieben:
> Doesn't apply anymore. I'll review anyway; sorry for being late with
> that.
>
> Kevin, I haven't been able to follow your work on driver-specific
> options; do you expect further conflicts with this work?
No, QEMUOptionParameter is only used for bdrv_create(), i.e. creating
images, which I don't touch. In my work on bdrv_open() I've been using
QDict/QemuOpts from the beginning.
Kevin
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH V12 3/4] Use QemuOpts support in block layer
[not found] ` <1362043236-4635-4-git-send-email-wdongxu@linux.vnet.ibm.com>
2013-03-06 1:51 ` [Qemu-devel] [PATCH V12 3/4] Use QemuOpts support in block layer Dong Xu Wang
@ 2013-03-21 15:31 ` Markus Armbruster
2013-03-25 6:31 ` Dong Xu Wang
1 sibling, 1 reply; 10+ messages in thread
From: Markus Armbruster @ 2013-03-21 15:31 UTC (permalink / raw)
To: Dong Xu Wang; +Cc: kwolf, qemu-devel, stefanha
Dong Xu Wang <wdongxu@linux.vnet.ibm.com> writes:
> This patch will use QemuOpts related functions in block layer, add
> a member bdrv_create_opts to BlockDriver struct, it will return
> a QemuOptsList pointer, which includes the image format's create
> options.
>
> And create options's primary consumer is block creating related
> functions, so modify them together.
>
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> ---
>
> v11->v12:
> 1) create functions, such as qemu_opt_get_del and qemu_opt_replace_set.
> These functions works like origin code.
> 2) use QEMU_OPT_SIZE, not QEMU_OPT_NUMBER.
> 3) in bdrv_create, if opts is NULL, will create an empty one, so can
> discard if(opts) code safely.
>
> v10->v11:
> 1) qed.h move QED_DEFAULT_CLUSTER_SIZE from enum to macro, or
> qemu_opts_print produce un-expanded cluster_size.
> 2) In qcow2.c and qcow.c, bdrv_create_file(filename, NULL), NULL ->
> opts,
> or while using protocol, there will be an error.
>
> v8->v9:
> 1) add qemu_ prefix to gluster_create_opts.
> 2) fix bug: bdrv_gluster_unix and bdrv_gluster_rdma should also be
> converted.
>
> v7->v8:
> 1) rebase to upstream source tree.
> 2) add gluster.c, raw-win32.c, and rbd.c.
>
> v6->v7:
> 1) use osdep.h:stringify(), not redefining new macro.
> 2) preserve TODO comment.
> 3) fix typo. BLOCK_OPT_ENCRYPT->BLOCK_OPT_STATIC.
> 4) initialize disk_type even when opts is NULL.
>
> v5->v6:
> 1) judge if opts == NULL in block layer create functions.
> 2) use bdrv_create_file(filename, NULL) in qcow_create and cow_create
> funtion.
> 3) made more readable while using qemu_opt_get_number.
>
>
> block.c | 91 ++++++++++++------------
> block/cow.c | 46 ++++++------
> block/gluster.c | 37 +++++-----
> block/iscsi.c | 8 +--
> block/qcow.c | 61 ++++++++--------
> block/qcow2.c | 173 +++++++++++++++++++++++-----------------------
> block/qed.c | 86 +++++++++++------------
> block/qed.h | 2 +-
> block/raw-posix.c | 59 +++++++---------
> block/raw-win32.c | 31 +++++----
> block/raw.c | 30 ++++----
> block/rbd.c | 62 ++++++++---------
> block/sheepdog.c | 75 ++++++++++----------
> block/vdi.c | 70 +++++++++----------
> block/vmdk.c | 90 ++++++++++++------------
> block/vpc.c | 57 +++++++--------
> block/vvfat.c | 11 +--
> include/block/block.h | 4 +-
> include/block/block_int.h | 6 +-
> include/qemu/option.h | 13 +++-
> qemu-img.c | 61 ++++++++--------
> util/qemu-option.c | 93 +++++++++++++++++++++++--
> 22 files changed, 613 insertions(+), 553 deletions(-)
*Ouch*
Any chance to split this patch up some? Its size makes it really hard
to review...
> diff --git a/block.c b/block.c
> index 4582961..975c3d8 100644
> --- a/block.c
> +++ b/block.c
> @@ -357,7 +357,7 @@ BlockDriver *bdrv_find_whitelisted_format(const char *format_name)
> typedef struct CreateCo {
> BlockDriver *drv;
> char *filename;
> - QEMUOptionParameter *options;
> + QemuOpts *opts;
> int ret;
> } CreateCo;
>
> @@ -366,11 +366,11 @@ static void coroutine_fn bdrv_create_co_entry(void *opaque)
> CreateCo *cco = opaque;
> assert(cco->drv);
>
> - cco->ret = cco->drv->bdrv_create(cco->filename, cco->options);
> + cco->ret = cco->drv->bdrv_create(cco->filename, cco->opts);
> }
>
> int bdrv_create(BlockDriver *drv, const char* filename,
> - QEMUOptionParameter *options)
> + QemuOpts *opts)
Since you touch this anyway, consider unbreaking the line:
int bdrv_create(BlockDriver *drv, const char* filename, QemuOpts *opts)
> {
> int ret;
>
> @@ -378,7 +378,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
> CreateCo cco = {
> .drv = drv,
> .filename = g_strdup(filename),
> - .options = options,
> + .opts = opts ?: qemu_opts_create_nofail(drv->bdrv_create_opts),
> .ret = NOT_DONE,
> };
>
As discussed during review of v11, this avoids passing null opts to the
bdrv_create() method. Good.
> @@ -405,7 +405,7 @@ out:
out:
g_free(cco.filename);
> return ret;
> }
I suspect you need
if (!opts) {
qemu_opts_del(cco->opts);
}
to avoid leaking the empty cco->opts you create in the previous hunk.
>
> -int bdrv_create_file(const char* filename, QEMUOptionParameter *options)
> +int bdrv_create_file(const char *filename, QemuOpts *opts)
> {
> BlockDriver *drv;
>
> @@ -414,7 +414,7 @@ int bdrv_create_file(const char* filename, QEMUOptionParameter *options)
> return -ENOENT;
> }
>
> - return bdrv_create(drv, filename, options);
> + return bdrv_create(drv, filename, opts);
> }
>
> /*
> @@ -814,7 +814,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
> int64_t total_size;
> int is_protocol = 0;
> BlockDriver *bdrv_qcow2;
> - QEMUOptionParameter *options;
> + QemuOpts *opts;
> char backing_filename[PATH_MAX];
>
> /* if snapshot, we create a temporary backing file and open it
> @@ -847,17 +847,16 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
> return -errno;
>
> bdrv_qcow2 = bdrv_find_format("qcow2");
> - options = parse_option_parameters("", bdrv_qcow2->create_options, NULL);
> + opts = qemu_opts_create_nofail(bdrv_qcow2->bdrv_create_opts);
>
> - set_option_parameter_int(options, BLOCK_OPT_SIZE, total_size);
> - set_option_parameter(options, BLOCK_OPT_BACKING_FILE, backing_filename);
> + qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_size);
> + qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, backing_filename);
> if (drv) {
> - set_option_parameter(options, BLOCK_OPT_BACKING_FMT,
> - drv->format_name);
> + qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, drv->format_name);
> }
>
> - ret = bdrv_create(bdrv_qcow2, tmp_filename, options);
> - free_option_parameters(options);
> + ret = bdrv_create(bdrv_qcow2, tmp_filename, opts);
> + qemu_opts_del(opts);
> if (ret < 0) {
> return ret;
> }
> @@ -4502,8 +4501,10 @@ void bdrv_img_create(const char *filename, const char *fmt,
> char *options, uint64_t img_size, int flags,
> Error **errp, bool quiet)
> {
> - QEMUOptionParameter *param = NULL, *create_options = NULL;
> - QEMUOptionParameter *backing_fmt, *backing_file, *size;
> + QemuOpts *opts = NULL;
> + QemuOptsList *create_opts = NULL;
> + const char *backing_fmt, *backing_file;
> + int64_t size;
> BlockDriverState *bs = NULL;
> BlockDriver *drv, *proto_drv;
> BlockDriver *backing_drv = NULL;
> @@ -4521,28 +4522,23 @@ void bdrv_img_create(const char *filename, const char *fmt,
> error_setg(errp, "Unknown protocol '%s'", filename);
> return;
> }
> -
> - create_options = append_option_parameters(create_options,
> - drv->create_options);
> - create_options = append_option_parameters(create_options,
> - proto_drv->create_options);
> -
> + create_opts = qemu_opts_append(drv->bdrv_create_opts,
> + proto_drv->bdrv_create_opts);
qemu_opts_append() dereferences its arguments to compute
dest->merge_lists = first->merge_lists || second->merge_lists;
However, either of drv->create_opts and proto_drv->create_opts may be
null, as far as I can see. If I'm correct, you need a few more test
cases :)
Before: format's options get appended first, then protocol's options.
Because get_option_parameter() searches in order, format options shadow
protocol options.
After: append_opts_list() gives first argument's options precedence.
Thus, no change. Good.
Should bdrv_create_options option name clashes be avoided? Beyond the
scope of this patch.
> /* Create parameter list with default values */
> - param = parse_option_parameters("", create_options, param);
> + opts = qemu_opts_create_nofail(create_opts);
Before: param empty.
After: opts empty.
Good.
>
> - set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size);
> + qemu_opt_set_number(opts, BLOCK_OPT_SIZE, img_size);
Before: param contains exactly BLOCK_OPT_SIZE = img_size.
After: opts contains exactly BLOCK_OPT_SIZE = img_size.
Both old and new code seem to assume BLOCK_OPT_SIZE is always a valid
option. Beyond the scope of this patch.
Good.
>
> /* Parse -o options */
> if (options) {
> - param = parse_option_parameters(options, create_options, param);
> - if (param == NULL) {
> + if (qemu_opts_do_parse_replace(opts, options, NULL) != 0) {
> error_setg(errp, "Invalid options for file format '%s'.", fmt);
> goto out;
> }
> }
Before: values from -o replace whatever is in param already.
After: values from -o replace whatever is in opts already.
In particular, size from -o replaces img_size before and after.
Did you test it does?
$ qemu-img create -f raw -o size=1024 t.img 512
Formatting 't.img', fmt=raw size=1024
$ ls -l t.img
-rw-r--r--. 1 armbru armbru 1024 Mar 21 15:12 t.img
>
> if (base_filename) {
> - if (set_option_parameter(param, BLOCK_OPT_BACKING_FILE,
> + if (qemu_opt_replace_set(opts, BLOCK_OPT_BACKING_FILE,
> base_filename)) {
> error_setg(errp, "Backing file not supported for file format '%s'",
> fmt);
Before: base_filename replaces any backing_file from -o in param.
After: base_filename replaces any backing_file from -o in opts.
Did you test it does?
$ qemu-img create -f qcow2 -b t.img -o backing_file=/dev/null t.qcow2
Formatting 't.qcow2', fmt=qcow2 size=1024 backing_file='t.img' encryption=off cluster_size=65536 lazy_refcounts=off
$ qemu-img info t.qcow2image: t.qcow2
file format: qcow2
virtual size: 1.0K (1024 bytes)
disk size: 196K
cluster_size: 65536
backing file: t.img
> @@ -4551,39 +4547,37 @@ void bdrv_img_create(const char *filename, const char *fmt,
> }
>
> if (base_fmt) {
> - if (set_option_parameter(param, BLOCK_OPT_BACKING_FMT, base_fmt)) {
> + if (qemu_opt_replace_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt)) {
> error_setg(errp, "Backing file format not supported for file "
> "format '%s'", fmt);
> goto out;
> }
> }
Likewise.
Did you test?
$ qemu-img create -f qcow2 -b t.img -F file -o backing_file=/dev/null,backing_fmt=host_device t.qcow2
Formatting 't.qcow2', fmt=qcow2 size=1024 backing_file='t.img' backing_fmt='file' encryption=off cluster_size=65536 lazy_refcounts=off
$ qemu-img info t.qcow2image: t.qcow2
file format: qcow2
virtual size: 1.0K (1024 bytes)
disk size: 196K
cluster_size: 65536
backing file: t.img
backing file format: file
>
> - backing_file = get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
> - if (backing_file && backing_file->value.s) {
> - if (!strcmp(filename, backing_file->value.s)) {
> + 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);
> + 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->value.s);
> + 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 = get_option_parameter(param, BLOCK_OPT_SIZE);
> - if (size && size->value.n == -1) {
> - if (backing_file && backing_file->value.s) {
> + size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
> + if (size == -1) {
> + if (backing_file) {
> uint64_t size;
> - char buf[32];
> int back_flags;
>
> /* backing files always opened read-only */
> @@ -4592,17 +4586,16 @@ void bdrv_img_create(const char *filename, const char *fmt,
>
> bs = bdrv_new("");
>
> - ret = bdrv_open(bs, backing_file->value.s, back_flags, backing_drv);
> + ret = bdrv_open(bs, backing_file, back_flags, backing_drv);
> if (ret < 0) {
> error_setg_errno(errp, -ret, "Could not open '%s'",
> - backing_file->value.s);
> + backing_file);
> goto out;
> }
> bdrv_get_geometry(bs, &size);
> size *= 512;
>
> - snprintf(buf, sizeof(buf), "%" PRId64, size);
> - set_option_parameter(param, BLOCK_OPT_SIZE, buf);
> + qemu_opt_set_number(opts, BLOCK_OPT_SIZE, size);
We get here only when opts doesn't have BLOCK_OPT_SIZE.
Good.
> } else {
> error_setg(errp, "Image creation needs a size parameter");
> goto out;
> @@ -4611,10 +4604,10 @@ 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, NULL);
> puts("");
> }
Before:
param[] contains all valid options. If the option in param[i] hasn't
been set, param[i].value still has the default value defined in the
.create_options[] where we got this option.
print_option_parameters() iterates over all valid options, and prints
NAME=VALUE. Except it prints nothing for an option of type OPT_STRING
when the value is null. That's the case when the default is null and
the option hasn't been set.
After:
opts contains only the options that have been set. opts->list->desc[]
contains all valid options (or is empty, which means "accept all", but
that shouldn't happen here).
qemu_opts_print() in master prints only the options that have been set.
Differs from print_option_parameters() for unset options. That's why
you rewrite qemu_opts_print() in PATCH 1/4.
The rewritten qemu_opts_print() prints all all valid options, except
unset ones that have a null def_value_str.
Therefore:
* OPT_STRING parameters need to become QEMU_OPT_STRING options, and
their default value needs to go both into def_value_str *and* into
every caller's handling of qemu_opt_get() returning null.
* OPT_FLAG, OPT_NUMBER, OPT_STRING parameters need to become
QEMU_OPT_BOOL, QEMU_OPT_NUMBER, QEMU_OPT_SIZE options, and their
default value needs to go both into def_value_str *and* into the last
argument of every qemu_opt_bool() getting it.
I hate how the default value needs to go in multiple places now. More
on that in my forthcoming review of PATCH 1/4.
> - ret = bdrv_create(drv, filename, param);
> + ret = bdrv_create(drv, filename, opts);
> if (ret < 0) {
> if (ret == -ENOTSUP) {
> error_setg(errp,"Formatting or formatting option not supported for "
> @@ -4629,8 +4622,10 @@ void bdrv_img_create(const char *filename, const char *fmt,
> }
>
> out:
> - free_option_parameters(create_options);
> - free_option_parameters(param);
> + qemu_opts_free(create_opts);
> + if (opts) {
> + qemu_opts_del(opts);
> + }
>
> if (bs) {
> bdrv_delete(bs);
> diff --git a/block/cow.c b/block/cow.c
> index 4baf904..135fa33 100644
> --- a/block/cow.c
> +++ b/block/cow.c
> @@ -255,7 +255,7 @@ static void cow_close(BlockDriverState *bs)
> {
> }
>
> -static int cow_create(const char *filename, QEMUOptionParameter *options)
> +static int cow_create(const char *filename, QemuOpts *opts)
> {
> struct cow_header_v2 cow_header;
> struct stat st;
> @@ -264,17 +264,11 @@ static int cow_create(const char *filename, QEMUOptionParameter *options)
> int ret;
> BlockDriverState *cow_bs;
>
> - /* Read out options */
> - while (options && options->name) {
> - if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> - image_sectors = options->value.n / 512;
> - } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FILE)) {
> - image_filename = options->value.s;
> - }
> - options++;
> - }
> + /* Read out opts */
> + image_sectors = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0) / 512;
> + image_filename = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
Why _del?
>
> - ret = bdrv_create_file(filename, options);
> + ret = bdrv_create_file(filename, opts);
> if (ret < 0) {
> return ret;
> }
> @@ -318,18 +312,22 @@ exit:
> return ret;
> }
>
> -static QEMUOptionParameter cow_create_options[] = {
> - {
> - .name = BLOCK_OPT_SIZE,
> - .type = OPT_SIZE,
> - .help = "Virtual disk size"
> - },
> - {
> - .name = BLOCK_OPT_BACKING_FILE,
> - .type = OPT_STRING,
> - .help = "File name of a base image"
> - },
> - { NULL }
> +static QemuOptsList cow_create_opts = {
> + .name = "cow-create-opts",
> + .head = QTAILQ_HEAD_INITIALIZER(cow_create_opts.head),
> + .desc = {
> + {
> + .name = BLOCK_OPT_SIZE,
> + .type = QEMU_OPT_SIZE,
> + .help = "Virtual disk size"
> + },
> + {
> + .name = BLOCK_OPT_BACKING_FILE,
> + .type = QEMU_OPT_STRING,
> + .help = "File name of a base image"
> + },
> + { /* end of list */ }
> + }
> };
>
> static BlockDriver bdrv_cow = {
> @@ -345,7 +343,7 @@ static BlockDriver bdrv_cow = {
> .bdrv_write = cow_co_write,
> .bdrv_co_is_allocated = cow_co_is_allocated,
>
> - .create_options = cow_create_options,
> + .bdrv_create_opts = &cow_create_opts,
> };
>
> static void bdrv_cow_init(void)
[Boatload of drivers snipped; not reviewed in this round]
> diff --git a/include/block/block.h b/include/block/block.h
> index 0f750d7..a477b7a 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -126,8 +126,8 @@ BlockDriver *bdrv_find_protocol(const char *filename);
> BlockDriver *bdrv_find_format(const char *format_name);
> BlockDriver *bdrv_find_whitelisted_format(const char *format_name);
> int bdrv_create(BlockDriver *drv, const char* filename,
> - QEMUOptionParameter *options);
> -int bdrv_create_file(const char* filename, QEMUOptionParameter *options);
> + QemuOpts *options);
> +int bdrv_create_file(const char *filename, QemuOpts *options);
QemuOpts *opts
> BlockDriverState *bdrv_new(const char *device_name);
> void bdrv_make_anon(BlockDriverState *bs);
> void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old);
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index eaad53e..4f942ef 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -90,7 +90,7 @@ struct BlockDriver {
> const uint8_t *buf, int nb_sectors);
> void (*bdrv_close)(BlockDriverState *bs);
> void (*bdrv_rebind)(BlockDriverState *bs);
> - int (*bdrv_create)(const char *filename, QEMUOptionParameter *options);
> + int (*bdrv_create)(const char *filename, QemuOpts *options);
QemuOpts *opts
> int (*bdrv_set_key)(BlockDriverState *bs, const char *key);
> int (*bdrv_make_empty)(BlockDriverState *bs);
> /* aio */
> @@ -179,9 +179,7 @@ struct BlockDriver {
> unsigned long int req, void *buf,
> BlockDriverCompletionFunc *cb, void *opaque);
>
> - /* List of options for creating images, terminated by name == NULL */
> - QEMUOptionParameter *create_options;
> -
> + QemuOptsList *bdrv_create_opts;
>
> /*
> * Returns 0 for completed check, -errno for internal errors.
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index c58db43..0d5fb66 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -108,6 +108,7 @@ struct QemuOptsList {
> };
>
> const char *qemu_opt_get(QemuOpts *opts, const char *name);
> +const char *qemu_opt_get_del(QemuOpts *opts, const char *name);
> /**
> * qemu_opt_has_help_opt:
> * @opts: options to search for a help request
> @@ -121,9 +122,13 @@ const char *qemu_opt_get(QemuOpts *opts, const char *name);
> */
> bool qemu_opt_has_help_opt(QemuOpts *opts);
> bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval);
> +bool qemu_opt_get_bool_del(QemuOpts *opts, const char *name, bool defval);
> uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval);
> uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval);
> +uint64_t qemu_opt_get_size_del(QemuOpts *opts, const char *name,
> + uint64_t defval);
> int qemu_opt_set(QemuOpts *opts, const char *name, const char *value);
> +int qemu_opt_replace_set(QemuOpts *opts, const char *name, const char *value);
> void qemu_opt_set_err(QemuOpts *opts, const char *name, const char *value,
> Error **errp);
> int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val);
> @@ -144,7 +149,10 @@ const char *qemu_opts_id(QemuOpts *opts);
> void qemu_opts_del(QemuOpts *opts);
> void qemu_opts_validate(QemuOpts *opts, const QemuOptDesc *desc, Error **errp);
> int qemu_opts_do_parse(QemuOpts *opts, const char *params, const char *firstname);
> -QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params, int permit_abbrev);
> +int qemu_opts_do_parse_replace(QemuOpts *opts, const char *params,
> + const char *firstname);
> +QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params,
> + int permit_abbrev);
> void qemu_opts_set_defaults(QemuOptsList *list, const char *params,
> int permit_abbrev);
> QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict,
Putting these new qemu_opt_*() functions in their own patch would reduce
this patch's size a little.
> @@ -156,8 +164,7 @@ int qemu_opts_print(QemuOpts *opts, void *dummy);
> int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void *opaque,
> int abort_on_failure);
>
> -QemuOptsList *qemu_opts_append(QemuOptsList *first,
> - QemuOptsList *second);
> +QemuOptsList *qemu_opts_append(QemuOptsList *first, QemuOptsList *second);
> void qemu_opts_free(QemuOptsList *list);
> void qemu_opts_print_help(QemuOptsList *list);
> #endif
> diff --git a/qemu-img.c b/qemu-img.c
> index 471de7d..9339957 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -229,7 +229,7 @@ static int read_password(char *buf, int buf_size)
> static int print_block_option_help(const char *filename, const char *fmt)
> {
> BlockDriver *drv, *proto_drv;
> - QEMUOptionParameter *create_options = NULL;
> + QemuOptsList *create_opts = NULL;
>
> /* Find driver and parse its options */
> drv = bdrv_find_format(fmt);
> @@ -244,12 +244,10 @@ static int print_block_option_help(const char *filename, const char *fmt)
> return 1;
> }
>
> - create_options = append_option_parameters(create_options,
> - drv->create_options);
> - create_options = append_option_parameters(create_options,
> - proto_drv->create_options);
> - print_option_help(create_options);
> - free_option_parameters(create_options);
> + create_opts = qemu_opts_append(drv->bdrv_create_opts,
> + proto_drv->bdrv_create_opts);
> + qemu_opts_print_help(create_opts);
> + qemu_opts_free(create_opts);
> return 0;
> }
>
Must merge options exactly like bdrv_img_create(). It does. Good.
I suspect the merge code should be factored out. Beyond the scope of
this patch.
Running out of steam, review is becoming more superficial and less
reliable.
> @@ -301,19 +299,19 @@ fail:
> return NULL;
> }
>
> -static int add_old_style_options(const char *fmt, QEMUOptionParameter *list,
> +static int add_old_style_options(const char *fmt, QemuOpts *list,
> const char *base_filename,
> const char *base_fmt)
> {
> if (base_filename) {
> - if (set_option_parameter(list, BLOCK_OPT_BACKING_FILE, base_filename)) {
> + if (qemu_opt_set(list, BLOCK_OPT_BACKING_FILE, base_filename)) {
> error_report("Backing file not supported for file format '%s'",
> fmt);
> return -1;
> }
> }
> if (base_fmt) {
> - if (set_option_parameter(list, BLOCK_OPT_BACKING_FMT, base_fmt)) {
> + if (qemu_opt_set(list, BLOCK_OPT_BACKING_FMT, base_fmt)) {
> error_report("Backing file format not supported for file "
> "format '%s'", fmt);
> return -1;
Looks like this duplicates code in bdrv_img_create(). Cleanup beyond
the scope of this patch.
> @@ -1120,8 +1118,9 @@ static int img_convert(int argc, char **argv)
> uint8_t * buf = NULL;
> const uint8_t *buf1;
> BlockDriverInfo bdi;
> - QEMUOptionParameter *param = NULL, *create_options = NULL;
> - QEMUOptionParameter *out_baseimg_param;
> + QemuOpts *param = NULL;
> + QemuOptsList *create_opts = NULL;
> + const char *out_baseimg_param;
> char *options = NULL;
> const char *snapshot_name = NULL;
> float local_progress = 0;
> @@ -1265,40 +1264,36 @@ static int img_convert(int argc, char **argv)
> goto out;
> }
>
> - create_options = append_option_parameters(create_options,
> - drv->create_options);
> - create_options = append_option_parameters(create_options,
> - proto_drv->create_options);
> + create_opts = qemu_opts_append(drv->bdrv_create_opts,
> + proto_drv->bdrv_create_opts);
>
More duplicated option merge code. Cleanup beyond the scope of this
patch.
> if (options) {
> - param = parse_option_parameters(options, create_options, param);
> - if (param == NULL) {
> + if (qemu_opts_do_parse_replace(param, options, NULL) != 0) {
> error_report("Invalid options for file format '%s'.", out_fmt);
> ret = -1;
> goto out;
> }
> } else {
> - param = parse_option_parameters("", create_options, param);
> + param = qemu_opts_create_nofail(create_opts);
> }
> -
> - set_option_parameter_int(param, BLOCK_OPT_SIZE, total_sectors * 512);
> + qemu_opt_set_number(param, BLOCK_OPT_SIZE, total_sectors * 512);
> ret = add_old_style_options(out_fmt, param, out_baseimg, NULL);
> if (ret < 0) {
> goto out;
> }
>
> /* Get backing file name if -o backing_file was used */
> - out_baseimg_param = get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
> + out_baseimg_param = qemu_opt_get(param, BLOCK_OPT_BACKING_FILE);
> if (out_baseimg_param) {
> - out_baseimg = out_baseimg_param->value.s;
> + out_baseimg = out_baseimg_param;
> }
More copy/paste coding. Cleanup beyond the scope of this patch.
>
> /* Check if compression is supported */
> if (compress) {
> - QEMUOptionParameter *encryption =
> - get_option_parameter(param, BLOCK_OPT_ENCRYPT);
> - QEMUOptionParameter *preallocation =
> - get_option_parameter(param, BLOCK_OPT_PREALLOC);
> + bool encryption =
> + qemu_opt_get_bool(param, BLOCK_OPT_ENCRYPT, false);
> + const char *preallocation =
> + qemu_opt_get(param, BLOCK_OPT_PREALLOC);
>
> if (!drv->bdrv_write_compressed) {
> error_report("Compression not supported for this file format");
> @@ -1306,15 +1301,15 @@ static int img_convert(int argc, char **argv)
> goto out;
> }
>
> - if (encryption && encryption->value.n) {
> + if (encryption) {
> error_report("Compression and encryption not supported at "
> "the same time");
> ret = -1;
> goto out;
> }
>
> - if (preallocation && preallocation->value.s
> - && strcmp(preallocation->value.s, "off"))
> + if (preallocation
> + && strcmp(preallocation, "off"))
> {
> error_report("Compression and preallocation not supported at "
> "the same time");
> @@ -1532,8 +1527,10 @@ static int img_convert(int argc, char **argv)
> }
> out:
> qemu_progress_end();
> - free_option_parameters(create_options);
> - free_option_parameters(param);
> + qemu_opts_free(create_opts);
> + if (param) {
> + qemu_opts_del(param);
> + }
> qemu_vfree(buf);
> if (out_bs) {
> bdrv_delete(out_bs);
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 0e42452..dba82b4 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
Changes to this file not reviewed in this round.
> @@ -33,6 +33,8 @@
> #include "qapi/qmp/qerror.h"
> #include "qemu/option_int.h"
>
> +static void qemu_opt_del(QemuOpt *opt);
> +
> /*
> * Extracts the name of an option from the parameter string (p points at the
> * first byte of the option name)
> @@ -531,6 +533,16 @@ const char *qemu_opt_get(QemuOpts *opts, const char *name)
> return opt ? opt->str : NULL;
> }
>
> +const char *qemu_opt_get_del(QemuOpts *opts, const char *name)
> +{
> + QemuOpt *opt = qemu_opt_find(opts, name);
> + const char *str = opt ? opt->str : NULL;
> + if (opt) {
> + qemu_opt_del(opt);
> + }
> + return str;
> +}
> +
> bool qemu_opt_has_help_opt(QemuOpts *opts)
> {
> QemuOpt *opt;
> @@ -553,6 +565,22 @@ bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval)
> return opt->value.boolean;
> }
>
> +bool qemu_opt_get_bool_del(QemuOpts *opts, const char *name, bool defval)
> +{
> + QemuOpt *opt = qemu_opt_find(opts, name);
> + bool ret;
> +
> + if (opt == NULL) {
> + return defval;
> + }
> + ret = opt->value.boolean;
> + assert(opt->desc && opt->desc->type == QEMU_OPT_BOOL);
> + if (opt) {
> + qemu_opt_del(opt);
> + }
> + return ret;
> +}
> +
> uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval)
> {
> QemuOpt *opt = qemu_opt_find(opts, name);
> @@ -573,6 +601,23 @@ uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval)
> return opt->value.uint;
> }
>
> +uint64_t qemu_opt_get_size_del(QemuOpts *opts, const char *name,
> + uint64_t defval)
> +{
> + QemuOpt *opt = qemu_opt_find(opts, name);
> + uint64_t ret;
> +
> + if (opt == NULL) {
> + return defval;
> + }
> + ret = opt->value.uint;
> + assert(opt->desc && opt->desc->type == QEMU_OPT_SIZE);
> + if (opt) {
> + qemu_opt_del(opt);
> + }
> + return ret;
> +}
> +
> static void qemu_opt_parse(QemuOpt *opt, Error **errp)
> {
> if (opt->desc == NULL)
> @@ -624,7 +669,7 @@ static const QemuOptDesc *find_desc_by_name(const QemuOptDesc *desc,
> }
>
> static void opt_set(QemuOpts *opts, const char *name, const char *value,
> - bool prepend, Error **errp)
> + bool prepend, bool replace, Error **errp)
> {
> QemuOpt *opt;
> const QemuOptDesc *desc;
> @@ -636,6 +681,11 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value,
> return;
> }
>
> + opt = qemu_opt_find(opts, name);
> + if (replace && opt) {
> + qemu_opt_del(opt);
> + }
> +
> opt = g_malloc0(sizeof(*opt));
> opt->name = g_strdup(name);
> opt->opts = opts;
> @@ -658,7 +708,29 @@ int qemu_opt_set(QemuOpts *opts, const char *name, const char *value)
> {
> Error *local_err = NULL;
>
> - opt_set(opts, name, value, false, &local_err);
> + opt_set(opts, name, value, false, false, &local_err);
> + if (error_is_set(&local_err)) {
> + qerror_report_err(local_err);
> + error_free(local_err);
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * If name exists, the function will delete the opt first and then add a new
> + * one.
> + */
> +int qemu_opt_replace_set(QemuOpts *opts, const char *name, const char *value)
> +{
> + Error *local_err = NULL;
> + QemuOpt *opt = qemu_opt_find(opts, name);
> +
> + if (opt) {
> + qemu_opt_del(opt);
> + }
> + opt_set(opts, name, value, false, true, &local_err);
> if (error_is_set(&local_err)) {
> qerror_report_err(local_err);
> error_free(local_err);
> @@ -671,7 +743,7 @@ int qemu_opt_set(QemuOpts *opts, const char *name, const char *value)
> void qemu_opt_set_err(QemuOpts *opts, const char *name, const char *value,
> Error **errp)
> {
> - opt_set(opts, name, value, false, errp);
> + opt_set(opts, name, value, false, false, errp);
> }
>
> int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val)
> @@ -895,7 +967,7 @@ int qemu_opts_print(QemuOpts *opts, void *dummy)
> }
>
> static int opts_do_parse(QemuOpts *opts, const char *params,
> - const char *firstname, bool prepend)
> + const char *firstname, bool prepend, bool replace)
> {
> char option[128], value[1024];
> const char *p,*pe,*pc;
> @@ -931,7 +1003,7 @@ static int opts_do_parse(QemuOpts *opts, const char *params,
> }
> if (strcmp(option, "id") != 0) {
> /* store and parse */
> - opt_set(opts, option, value, prepend, &local_err);
> + opt_set(opts, option, value, prepend, replace, &local_err);
> if (error_is_set(&local_err)) {
> qerror_report_err(local_err);
> error_free(local_err);
> @@ -947,9 +1019,16 @@ static int opts_do_parse(QemuOpts *opts, const char *params,
>
> int qemu_opts_do_parse(QemuOpts *opts, const char *params, const char *firstname)
> {
> - return opts_do_parse(opts, params, firstname, false);
> + return opts_do_parse(opts, params, firstname, false, false);
> +}
> +
> +int qemu_opts_do_parse_replace(QemuOpts *opts, const char *params,
> + const char *firstname)
> +{
> + return opts_do_parse(opts, params, firstname, false, true);
> }
>
> +
> static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
> int permit_abbrev, bool defaults)
> {
> @@ -986,7 +1065,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
> return NULL;
> }
>
> - if (opts_do_parse(opts, params, firstname, defaults) != 0) {
> + if (opts_do_parse(opts, params, firstname, defaults, false) != 0) {
> qemu_opts_del(opts);
> return NULL;
> }
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH V12 1/4] add def_value_str and use it in qemu_opts_print
[not found] ` <1362043236-4635-2-git-send-email-wdongxu@linux.vnet.ibm.com>
@ 2013-03-21 15:59 ` Markus Armbruster
0 siblings, 0 replies; 10+ messages in thread
From: Markus Armbruster @ 2013-03-21 15:59 UTC (permalink / raw)
To: Dong Xu Wang; +Cc: kwolf, qemu-devel, stefanha
Dong Xu Wang <wdongxu@linux.vnet.ibm.com> writes:
> qemu_opts_print has no user now, so can re-write the function safely.
>
> qemu_opts_print will be used while using "qemu-img create", it will
> produce the same output as previous code.
>
> The behavior of this function has changed:
>
> 1. Print every possible option, whether a value has been set or not.
> 2. Option descriptors may provide a default value.
> 3. Print to stdout instead of stderr.
>
> Previously the behavior was to print every option that has been set.
> Options that have not been set would be skipped.
The commit message needs updating for v12: def_value_str isn't just
about qemu_opts_print() anymore.
More serious, the default value job is only half done. My fault,
because my description of what I want done in my review of v11 was
confused and misleading. I apologize for that. Let me try again.
You want qemu_opts_print() to print the effective value even for some
options that aren't set, because you don't want to change output of
qemu-img. Fair enough.
Currently, what to do when an option isn't set is left to
qemu_opt_get_FOO() callers:
* qemu_opt_get() returns NULL when option isn't set. Caller can fall
back to a default value, or do something else entirely.
* qemu_opt_get_{bool,number,size}() let the caller supply a default
value argument to be returned when the option isn't set.
Trouble is qemu_opts_print() can't know what these callers do, so it
can't show the effective value for options that aren't set.
Your v11 solved this problem by putting the default value in
QemuOptDesc, too, for use by qemu_opts_print(). Requires copying the
caller's real default value there.
You copied only the default values you actually need for your particular
use of qemu_opts_print().
Keeping the copies consistent is manual. It's impossible when different
callers use different default values, but that isn't the case for any of
the ones you copied, as far as I can tell.
I dislike having the default value in multiple places. Two ways to
avoid the duplication:
1. Keep the default value entirely in the callers
In cases where you want qemu_opts_print() to show the effective
value, change the caller to also set the value in opts when it falls
back to a default.
Keep qemu_opts_print() showing only options that are actually set.
This is fine, because the previous paragraph makes sure all the
values you want to see are actually set.
2. Move the default value entirely to QemuOptDesc
When getting the value of an option that hasn't been set, and
QemuOptDesc has a default value, return that. Else, behave as
before.
Example: qemu_opt_get_number(opts, "foo", 42)
If "foo" has been set in opts, return its value.
Else, if opt's QemuOptDesc has a default value for "foo", return
that.
Else, return 42.
Note that the last argument is useless when QemuOptDesc has a
default value. Ugly. If it bothers us, assert that the argument
equals the default from QemuOptDesc.
Example: qemu_opt_get(opts, "bar")
If "bar" has been set in opts, return its value.
Else, if opt's QemuOptDesc has a default value for "bar", return
that.
Else, return NULL.
Note that "no default in QemuOptDesc" does *not* mean "there is no
default". There may or may not be a default, depending on how the
option's users get the value.
Hope I'm making sense this time :)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH V12 3/4] Use QemuOpts support in block layer
2013-03-21 15:31 ` Markus Armbruster
@ 2013-03-25 6:31 ` Dong Xu Wang
2013-04-02 16:22 ` Markus Armbruster
0 siblings, 1 reply; 10+ messages in thread
From: Dong Xu Wang @ 2013-03-25 6:31 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
On Thu, Mar 21, 2013 at 11:31 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Dong Xu Wang <wdongxu@linux.vnet.ibm.com> writes:
>
>> This patch will use QemuOpts related functions in block layer, add
>> a member bdrv_create_opts to BlockDriver struct, it will return
>> a QemuOptsList pointer, which includes the image format's create
>> options.
>>
>> And create options's primary consumer is block creating related
>> functions, so modify them together.
>>
>> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
>> ---
>>
>> v11->v12:
>> 1) create functions, such as qemu_opt_get_del and qemu_opt_replace_set.
>> These functions works like origin code.
>> 2) use QEMU_OPT_SIZE, not QEMU_OPT_NUMBER.
>> 3) in bdrv_create, if opts is NULL, will create an empty one, so can
>> discard if(opts) code safely.
>>
>> v10->v11:
>> 1) qed.h move QED_DEFAULT_CLUSTER_SIZE from enum to macro, or
>> qemu_opts_print produce un-expanded cluster_size.
>> 2) In qcow2.c and qcow.c, bdrv_create_file(filename, NULL), NULL ->
>> opts,
>> or while using protocol, there will be an error.
>>
>> v8->v9:
>> 1) add qemu_ prefix to gluster_create_opts.
>> 2) fix bug: bdrv_gluster_unix and bdrv_gluster_rdma should also be
>> converted.
>>
>> v7->v8:
>> 1) rebase to upstream source tree.
>> 2) add gluster.c, raw-win32.c, and rbd.c.
>>
>> v6->v7:
>> 1) use osdep.h:stringify(), not redefining new macro.
>> 2) preserve TODO comment.
>> 3) fix typo. BLOCK_OPT_ENCRYPT->BLOCK_OPT_STATIC.
>> 4) initialize disk_type even when opts is NULL.
>>
>> v5->v6:
>> 1) judge if opts == NULL in block layer create functions.
>> 2) use bdrv_create_file(filename, NULL) in qcow_create and cow_create
>> funtion.
>> 3) made more readable while using qemu_opt_get_number.
>>
>>
>> block.c | 91 ++++++++++++------------
>> block/cow.c | 46 ++++++------
>> block/gluster.c | 37 +++++-----
>> block/iscsi.c | 8 +--
>> block/qcow.c | 61 ++++++++--------
>> block/qcow2.c | 173 +++++++++++++++++++++++-----------------------
>> block/qed.c | 86 +++++++++++------------
>> block/qed.h | 2 +-
>> block/raw-posix.c | 59 +++++++---------
>> block/raw-win32.c | 31 +++++----
>> block/raw.c | 30 ++++----
>> block/rbd.c | 62 ++++++++---------
>> block/sheepdog.c | 75 ++++++++++----------
>> block/vdi.c | 70 +++++++++----------
>> block/vmdk.c | 90 ++++++++++++------------
>> block/vpc.c | 57 +++++++--------
>> block/vvfat.c | 11 +--
>> include/block/block.h | 4 +-
>> include/block/block_int.h | 6 +-
>> include/qemu/option.h | 13 +++-
>> qemu-img.c | 61 ++++++++--------
>> util/qemu-option.c | 93 +++++++++++++++++++++++--
>> 22 files changed, 613 insertions(+), 553 deletions(-)
>
> *Ouch*
>
> Any chance to split this patch up some? Its size makes it really hard
> to review...
>
I will split this patch into some small patches in next version.
>> diff --git a/block.c b/block.c
>> index 4582961..975c3d8 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -357,7 +357,7 @@ BlockDriver *bdrv_find_whitelisted_format(const char *format_name)
>> typedef struct CreateCo {
>> BlockDriver *drv;
>> char *filename;
>> - QEMUOptionParameter *options;
>> + QemuOpts *opts;
>> int ret;
>> } CreateCo;
>>
>> @@ -366,11 +366,11 @@ static void coroutine_fn bdrv_create_co_entry(void *opaque)
>> CreateCo *cco = opaque;
>> assert(cco->drv);
>>
>> - cco->ret = cco->drv->bdrv_create(cco->filename, cco->options);
>> + cco->ret = cco->drv->bdrv_create(cco->filename, cco->opts);
>> }
>>
>> int bdrv_create(BlockDriver *drv, const char* filename,
>> - QEMUOptionParameter *options)
>> + QemuOpts *opts)
>
> Since you touch this anyway, consider unbreaking the line:
>
> int bdrv_create(BlockDriver *drv, const char* filename, QemuOpts *opts)
>
Okay.
>> {
>> int ret;
>>
>> @@ -378,7 +378,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
>> CreateCo cco = {
>> .drv = drv,
>> .filename = g_strdup(filename),
>> - .options = options,
>> + .opts = opts ?: qemu_opts_create_nofail(drv->bdrv_create_opts),
>> .ret = NOT_DONE,
>> };
>>
>
> As discussed during review of v11, this avoids passing null opts to the
> bdrv_create() method. Good.
>
>> @@ -405,7 +405,7 @@ out:
> out:
> g_free(cco.filename);
>> return ret;
>> }
>
> I suspect you need
>
> if (!opts) {
> qemu_opts_del(cco->opts);
> }
>
> to avoid leaking the empty cco->opts you create in the previous hunk.
>
Okay.
>>
>> -int bdrv_create_file(const char* filename, QEMUOptionParameter *options)
>> +int bdrv_create_file(const char *filename, QemuOpts *opts)
>> {
>> BlockDriver *drv;
>>
>> @@ -414,7 +414,7 @@ int bdrv_create_file(const char* filename, QEMUOptionParameter *options)
>> return -ENOENT;
>> }
>>
>> - return bdrv_create(drv, filename, options);
>> + return bdrv_create(drv, filename, opts);
>> }
>>
>> /*
>> @@ -814,7 +814,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
>> int64_t total_size;
>> int is_protocol = 0;
>> BlockDriver *bdrv_qcow2;
>> - QEMUOptionParameter *options;
>> + QemuOpts *opts;
>> char backing_filename[PATH_MAX];
>>
>> /* if snapshot, we create a temporary backing file and open it
>> @@ -847,17 +847,16 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
>> return -errno;
>>
>> bdrv_qcow2 = bdrv_find_format("qcow2");
>> - options = parse_option_parameters("", bdrv_qcow2->create_options, NULL);
>> + opts = qemu_opts_create_nofail(bdrv_qcow2->bdrv_create_opts);
>>
>> - set_option_parameter_int(options, BLOCK_OPT_SIZE, total_size);
>> - set_option_parameter(options, BLOCK_OPT_BACKING_FILE, backing_filename);
>> + qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_size);
>> + qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, backing_filename);
>> if (drv) {
>> - set_option_parameter(options, BLOCK_OPT_BACKING_FMT,
>> - drv->format_name);
>> + qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, drv->format_name);
>> }
>>
>> - ret = bdrv_create(bdrv_qcow2, tmp_filename, options);
>> - free_option_parameters(options);
>> + ret = bdrv_create(bdrv_qcow2, tmp_filename, opts);
>> + qemu_opts_del(opts);
>> if (ret < 0) {
>> return ret;
>> }
>> @@ -4502,8 +4501,10 @@ void bdrv_img_create(const char *filename, const char *fmt,
>> char *options, uint64_t img_size, int flags,
>> Error **errp, bool quiet)
>> {
>> - QEMUOptionParameter *param = NULL, *create_options = NULL;
>> - QEMUOptionParameter *backing_fmt, *backing_file, *size;
>> + QemuOpts *opts = NULL;
>> + QemuOptsList *create_opts = NULL;
>> + const char *backing_fmt, *backing_file;
>> + int64_t size;
>> BlockDriverState *bs = NULL;
>> BlockDriver *drv, *proto_drv;
>> BlockDriver *backing_drv = NULL;
>> @@ -4521,28 +4522,23 @@ void bdrv_img_create(const char *filename, const char *fmt,
>> error_setg(errp, "Unknown protocol '%s'", filename);
>> return;
>> }
>> -
>> - create_options = append_option_parameters(create_options,
>> - drv->create_options);
>> - create_options = append_option_parameters(create_options,
>> - proto_drv->create_options);
>> -
>> + create_opts = qemu_opts_append(drv->bdrv_create_opts,
>> + proto_drv->bdrv_create_opts);
>
> qemu_opts_append() dereferences its arguments to compute
>
> dest->merge_lists = first->merge_lists || second->merge_lists;
>
> However, either of drv->create_opts and proto_drv->create_opts may be
> null, as far as I can see. If I'm correct, you need a few more test
> cases :)
>
Okay, will check first and second.
> Before: format's options get appended first, then protocol's options.
> Because get_option_parameter() searches in order, format options shadow
> protocol options.
>
> After: append_opts_list() gives first argument's options precedence.
>
> Thus, no change. Good.
>
> Should bdrv_create_options option name clashes be avoided? Beyond the
> scope of this patch.
>
>> /* Create parameter list with default values */
>> - param = parse_option_parameters("", create_options, param);
>> + opts = qemu_opts_create_nofail(create_opts);
>
> Before: param empty.
>
> After: opts empty.
>
> Good.
>
>>
>> - set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size);
>> + qemu_opt_set_number(opts, BLOCK_OPT_SIZE, img_size);
>
> Before: param contains exactly BLOCK_OPT_SIZE = img_size.
>
> After: opts contains exactly BLOCK_OPT_SIZE = img_size.
>
> Both old and new code seem to assume BLOCK_OPT_SIZE is always a valid
> option. Beyond the scope of this patch.
>
> Good.
>
>>
>> /* Parse -o options */
>> if (options) {
>> - param = parse_option_parameters(options, create_options, param);
>> - if (param == NULL) {
>> + if (qemu_opts_do_parse_replace(opts, options, NULL) != 0) {
>> error_setg(errp, "Invalid options for file format '%s'.", fmt);
>> goto out;
>> }
>> }
>
> Before: values from -o replace whatever is in param already.
>
> After: values from -o replace whatever is in opts already.
>
> In particular, size from -o replaces img_size before and after.
>
> Did you test it does?
>
> $ qemu-img create -f raw -o size=1024 t.img 512
> Formatting 't.img', fmt=raw size=1024
> $ ls -l t.img
> -rw-r--r--. 1 armbru armbru 1024 Mar 21 15:12 t.img
>
>>
>> if (base_filename) {
>> - if (set_option_parameter(param, BLOCK_OPT_BACKING_FILE,
>> + if (qemu_opt_replace_set(opts, BLOCK_OPT_BACKING_FILE,
>> base_filename)) {
>> error_setg(errp, "Backing file not supported for file format '%s'",
>> fmt);
>
> Before: base_filename replaces any backing_file from -o in param.
>
> After: base_filename replaces any backing_file from -o in opts.
>
> Did you test it does?
>
Oh, I did not test command line like this, my patch will fail here.
Will fix in next version.
> $ qemu-img create -f qcow2 -b t.img -o backing_file=/dev/null t.qcow2
> Formatting 't.qcow2', fmt=qcow2 size=1024 backing_file='t.img' encryption=off cluster_size=65536 lazy_refcounts=off
> $ qemu-img info t.qcow2image: t.qcow2
> file format: qcow2
> virtual size: 1.0K (1024 bytes)
> disk size: 196K
> cluster_size: 65536
> backing file: t.img
>
I run this command using upstream QEMU code, but failed.
[wdongxu@13@VM:~]$ qemu-img info t.qcow2image: t.qcow2
qemu-img: Could not open 't.qcow2image:': No such file or directory
Or did I type something wrong?
>> @@ -4551,39 +4547,37 @@ void bdrv_img_create(const char *filename, const char *fmt,
>> }
>>
>> if (base_fmt) {
>> - if (set_option_parameter(param, BLOCK_OPT_BACKING_FMT, base_fmt)) {
>> + if (qemu_opt_replace_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt)) {
>> error_setg(errp, "Backing file format not supported for file "
>> "format '%s'", fmt);
>> goto out;
>> }
>> }
>
> Likewise.
>
> Did you test?
>
> $ qemu-img create -f qcow2 -b t.img -F file -o backing_file=/dev/null,backing_fmt=host_device t.qcow2
> Formatting 't.qcow2', fmt=qcow2 size=1024 backing_file='t.img' backing_fmt='file' encryption=off cluster_size=65536 lazy_refcounts=off
> $ qemu-img info t.qcow2image: t.qcow2
> file format: qcow2
> virtual size: 1.0K (1024 bytes)
> disk size: 196K
> cluster_size: 65536
> backing file: t.img
> backing file format: file
>
>>
>> - backing_file = get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
>> - if (backing_file && backing_file->value.s) {
>> - if (!strcmp(filename, backing_file->value.s)) {
>> + 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);
>> + 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->value.s);
>> + 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 = get_option_parameter(param, BLOCK_OPT_SIZE);
>> - if (size && size->value.n == -1) {
>> - if (backing_file && backing_file->value.s) {
>> + size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
>> + if (size == -1) {
>> + if (backing_file) {
>> uint64_t size;
>> - char buf[32];
>> int back_flags;
>>
>> /* backing files always opened read-only */
>> @@ -4592,17 +4586,16 @@ void bdrv_img_create(const char *filename, const char *fmt,
>>
>> bs = bdrv_new("");
>>
>> - ret = bdrv_open(bs, backing_file->value.s, back_flags, backing_drv);
>> + ret = bdrv_open(bs, backing_file, back_flags, backing_drv);
>> if (ret < 0) {
>> error_setg_errno(errp, -ret, "Could not open '%s'",
>> - backing_file->value.s);
>> + backing_file);
>> goto out;
>> }
>> bdrv_get_geometry(bs, &size);
>> size *= 512;
>>
>> - snprintf(buf, sizeof(buf), "%" PRId64, size);
>> - set_option_parameter(param, BLOCK_OPT_SIZE, buf);
>> + qemu_opt_set_number(opts, BLOCK_OPT_SIZE, size);
>
> We get here only when opts doesn't have BLOCK_OPT_SIZE.
>
> Good.
>
>> } else {
>> error_setg(errp, "Image creation needs a size parameter");
>> goto out;
>> @@ -4611,10 +4604,10 @@ 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, NULL);
>> puts("");
>> }
>
> Before:
>
> param[] contains all valid options. If the option in param[i] hasn't
> been set, param[i].value still has the default value defined in the
> .create_options[] where we got this option.
>
> print_option_parameters() iterates over all valid options, and prints
> NAME=VALUE. Except it prints nothing for an option of type OPT_STRING
> when the value is null. That's the case when the default is null and
> the option hasn't been set.
>
> After:
>
> opts contains only the options that have been set. opts->list->desc[]
> contains all valid options (or is empty, which means "accept all", but
> that shouldn't happen here).
>
> qemu_opts_print() in master prints only the options that have been set.
> Differs from print_option_parameters() for unset options. That's why
> you rewrite qemu_opts_print() in PATCH 1/4.
>
> The rewritten qemu_opts_print() prints all all valid options, except
> unset ones that have a null def_value_str.
>
> Therefore:
>
> * OPT_STRING parameters need to become QEMU_OPT_STRING options, and
> their default value needs to go both into def_value_str *and* into
> every caller's handling of qemu_opt_get() returning null.
>
> * OPT_FLAG, OPT_NUMBER, OPT_STRING parameters need to become
> QEMU_OPT_BOOL, QEMU_OPT_NUMBER, QEMU_OPT_SIZE options, and their
> default value needs to go both into def_value_str *and* into the last
> argument of every qemu_opt_bool() getting it.
>
> I hate how the default value needs to go in multiple places now. More
> on that in my forthcoming review of PATCH 1/4.
>
>> - ret = bdrv_create(drv, filename, param);
>> + ret = bdrv_create(drv, filename, opts);
>> if (ret < 0) {
>> if (ret == -ENOTSUP) {
>> error_setg(errp,"Formatting or formatting option not supported for "
>> @@ -4629,8 +4622,10 @@ void bdrv_img_create(const char *filename, const char *fmt,
>> }
>>
>> out:
>> - free_option_parameters(create_options);
>> - free_option_parameters(param);
>> + qemu_opts_free(create_opts);
>> + if (opts) {
>> + qemu_opts_del(opts);
>> + }
>>
>> if (bs) {
>> bdrv_delete(bs);
>> diff --git a/block/cow.c b/block/cow.c
>> index 4baf904..135fa33 100644
>> --- a/block/cow.c
>> +++ b/block/cow.c
>> @@ -255,7 +255,7 @@ static void cow_close(BlockDriverState *bs)
>> {
>> }
>>
>> -static int cow_create(const char *filename, QEMUOptionParameter *options)
>> +static int cow_create(const char *filename, QemuOpts *opts)
>> {
>> struct cow_header_v2 cow_header;
>> struct stat st;
>> @@ -264,17 +264,11 @@ static int cow_create(const char *filename, QEMUOptionParameter *options)
>> int ret;
>> BlockDriverState *cow_bs;
>>
>> - /* Read out options */
>> - while (options && options->name) {
>> - if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
>> - image_sectors = options->value.n / 512;
>> - } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FILE)) {
>> - image_filename = options->value.s;
>> - }
>> - options++;
>> - }
>> + /* Read out opts */
>> + image_sectors = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0) / 512;
>> + image_filename = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
>
> Why _del?
>
Before: after getting one parameter, then option++, so in my patches, I want to
delete the opt after using it. If I did not delete it, some parameters
will be passed
to "protocol", it will go wrong. Do you think it is acceptable?
>>
>> - ret = bdrv_create_file(filename, options);
>> + ret = bdrv_create_file(filename, opts);
>> if (ret < 0) {
>> return ret;
>> }
>> @@ -318,18 +312,22 @@ exit:
>> return ret;
>> }
>>
>> -static QEMUOptionParameter cow_create_options[] = {
>> - {
>> - .name = BLOCK_OPT_SIZE,
>> - .type = OPT_SIZE,
>> - .help = "Virtual disk size"
>> - },
>> - {
>> - .name = BLOCK_OPT_BACKING_FILE,
>> - .type = OPT_STRING,
>> - .help = "File name of a base image"
>> - },
>> - { NULL }
>> +static QemuOptsList cow_create_opts = {
>> + .name = "cow-create-opts",
>> + .head = QTAILQ_HEAD_INITIALIZER(cow_create_opts.head),
>> + .desc = {
>> + {
>> + .name = BLOCK_OPT_SIZE,
>> + .type = QEMU_OPT_SIZE,
>> + .help = "Virtual disk size"
>> + },
>> + {
>> + .name = BLOCK_OPT_BACKING_FILE,
>> + .type = QEMU_OPT_STRING,
>> + .help = "File name of a base image"
>> + },
>> + { /* end of list */ }
>> + }
>> };
>>
>> static BlockDriver bdrv_cow = {
>> @@ -345,7 +343,7 @@ static BlockDriver bdrv_cow = {
>> .bdrv_write = cow_co_write,
>> .bdrv_co_is_allocated = cow_co_is_allocated,
>>
>> - .create_options = cow_create_options,
>> + .bdrv_create_opts = &cow_create_opts,
>> };
>>
>> static void bdrv_cow_init(void)
> [Boatload of drivers snipped; not reviewed in this round]
>> diff --git a/include/block/block.h b/include/block/block.h
>> index 0f750d7..a477b7a 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -126,8 +126,8 @@ BlockDriver *bdrv_find_protocol(const char *filename);
>> BlockDriver *bdrv_find_format(const char *format_name);
>> BlockDriver *bdrv_find_whitelisted_format(const char *format_name);
>> int bdrv_create(BlockDriver *drv, const char* filename,
>> - QEMUOptionParameter *options);
>> -int bdrv_create_file(const char* filename, QEMUOptionParameter *options);
>> + QemuOpts *options);
>> +int bdrv_create_file(const char *filename, QemuOpts *options);
>
> QemuOpts *opts
>
Okay.
>> BlockDriverState *bdrv_new(const char *device_name);
>> void bdrv_make_anon(BlockDriverState *bs);
>> void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old);
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index eaad53e..4f942ef 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -90,7 +90,7 @@ struct BlockDriver {
>> const uint8_t *buf, int nb_sectors);
>> void (*bdrv_close)(BlockDriverState *bs);
>> void (*bdrv_rebind)(BlockDriverState *bs);
>> - int (*bdrv_create)(const char *filename, QEMUOptionParameter *options);
>> + int (*bdrv_create)(const char *filename, QemuOpts *options);
>
> QemuOpts *opts
>
>> int (*bdrv_set_key)(BlockDriverState *bs, const char *key);
>> int (*bdrv_make_empty)(BlockDriverState *bs);
>> /* aio */
>> @@ -179,9 +179,7 @@ struct BlockDriver {
>> unsigned long int req, void *buf,
>> BlockDriverCompletionFunc *cb, void *opaque);
>>
>> - /* List of options for creating images, terminated by name == NULL */
>> - QEMUOptionParameter *create_options;
>> -
>> + QemuOptsList *bdrv_create_opts;
>>
>> /*
>> * Returns 0 for completed check, -errno for internal errors.
>> diff --git a/include/qemu/option.h b/include/qemu/option.h
>> index c58db43..0d5fb66 100644
>> --- a/include/qemu/option.h
>> +++ b/include/qemu/option.h
>> @@ -108,6 +108,7 @@ struct QemuOptsList {
>> };
>>
>> const char *qemu_opt_get(QemuOpts *opts, const char *name);
>> +const char *qemu_opt_get_del(QemuOpts *opts, const char *name);
>> /**
>> * qemu_opt_has_help_opt:
>> * @opts: options to search for a help request
>> @@ -121,9 +122,13 @@ const char *qemu_opt_get(QemuOpts *opts, const char *name);
>> */
>> bool qemu_opt_has_help_opt(QemuOpts *opts);
>> bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval);
>> +bool qemu_opt_get_bool_del(QemuOpts *opts, const char *name, bool defval);
>> uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval);
>> uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval);
>> +uint64_t qemu_opt_get_size_del(QemuOpts *opts, const char *name,
>> + uint64_t defval);
>> int qemu_opt_set(QemuOpts *opts, const char *name, const char *value);
>> +int qemu_opt_replace_set(QemuOpts *opts, const char *name, const char *value);
>> void qemu_opt_set_err(QemuOpts *opts, const char *name, const char *value,
>> Error **errp);
>> int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val);
>> @@ -144,7 +149,10 @@ const char *qemu_opts_id(QemuOpts *opts);
>> void qemu_opts_del(QemuOpts *opts);
>> void qemu_opts_validate(QemuOpts *opts, const QemuOptDesc *desc, Error **errp);
>> int qemu_opts_do_parse(QemuOpts *opts, const char *params, const char *firstname);
>> -QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params, int permit_abbrev);
>> +int qemu_opts_do_parse_replace(QemuOpts *opts, const char *params,
>> + const char *firstname);
>> +QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params,
>> + int permit_abbrev);
>> void qemu_opts_set_defaults(QemuOptsList *list, const char *params,
>> int permit_abbrev);
>> QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict,
>
> Putting these new qemu_opt_*() functions in their own patch would reduce
> this patch's size a little.
>
>> @@ -156,8 +164,7 @@ int qemu_opts_print(QemuOpts *opts, void *dummy);
>> int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void *opaque,
>> int abort_on_failure);
>>
>> -QemuOptsList *qemu_opts_append(QemuOptsList *first,
>> - QemuOptsList *second);
>> +QemuOptsList *qemu_opts_append(QemuOptsList *first, QemuOptsList *second);
>> void qemu_opts_free(QemuOptsList *list);
>> void qemu_opts_print_help(QemuOptsList *list);
>> #endif
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 471de7d..9339957 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -229,7 +229,7 @@ static int read_password(char *buf, int buf_size)
>> static int print_block_option_help(const char *filename, const char *fmt)
>> {
>> BlockDriver *drv, *proto_drv;
>> - QEMUOptionParameter *create_options = NULL;
>> + QemuOptsList *create_opts = NULL;
>>
>> /* Find driver and parse its options */
>> drv = bdrv_find_format(fmt);
>> @@ -244,12 +244,10 @@ static int print_block_option_help(const char *filename, const char *fmt)
>> return 1;
>> }
>>
>> - create_options = append_option_parameters(create_options,
>> - drv->create_options);
>> - create_options = append_option_parameters(create_options,
>> - proto_drv->create_options);
>> - print_option_help(create_options);
>> - free_option_parameters(create_options);
>> + create_opts = qemu_opts_append(drv->bdrv_create_opts,
>> + proto_drv->bdrv_create_opts);
>> + qemu_opts_print_help(create_opts);
>> + qemu_opts_free(create_opts);
>> return 0;
>> }
>>
>
> Must merge options exactly like bdrv_img_create(). It does. Good.
>
> I suspect the merge code should be factored out. Beyond the scope of
> this patch.
>
> Running out of steam, review is becoming more superficial and less
> reliable.
>
>> @@ -301,19 +299,19 @@ fail:
>> return NULL;
>> }
>>
>> -static int add_old_style_options(const char *fmt, QEMUOptionParameter *list,
>> +static int add_old_style_options(const char *fmt, QemuOpts *list,
>> const char *base_filename,
>> const char *base_fmt)
>> {
>> if (base_filename) {
>> - if (set_option_parameter(list, BLOCK_OPT_BACKING_FILE, base_filename)) {
>> + if (qemu_opt_set(list, BLOCK_OPT_BACKING_FILE, base_filename)) {
>> error_report("Backing file not supported for file format '%s'",
>> fmt);
>> return -1;
>> }
>> }
>> if (base_fmt) {
>> - if (set_option_parameter(list, BLOCK_OPT_BACKING_FMT, base_fmt)) {
>> + if (qemu_opt_set(list, BLOCK_OPT_BACKING_FMT, base_fmt)) {
>> error_report("Backing file format not supported for file "
>> "format '%s'", fmt);
>> return -1;
>
> Looks like this duplicates code in bdrv_img_create(). Cleanup beyond
> the scope of this patch.
>
>> @@ -1120,8 +1118,9 @@ static int img_convert(int argc, char **argv)
>> uint8_t * buf = NULL;
>> const uint8_t *buf1;
>> BlockDriverInfo bdi;
>> - QEMUOptionParameter *param = NULL, *create_options = NULL;
>> - QEMUOptionParameter *out_baseimg_param;
>> + QemuOpts *param = NULL;
>> + QemuOptsList *create_opts = NULL;
>> + const char *out_baseimg_param;
>> char *options = NULL;
>> const char *snapshot_name = NULL;
>> float local_progress = 0;
>> @@ -1265,40 +1264,36 @@ static int img_convert(int argc, char **argv)
>> goto out;
>> }
>>
>> - create_options = append_option_parameters(create_options,
>> - drv->create_options);
>> - create_options = append_option_parameters(create_options,
>> - proto_drv->create_options);
>> + create_opts = qemu_opts_append(drv->bdrv_create_opts,
>> + proto_drv->bdrv_create_opts);
>>
>
> More duplicated option merge code. Cleanup beyond the scope of this
> patch.
>
>> if (options) {
>> - param = parse_option_parameters(options, create_options, param);
>> - if (param == NULL) {
>> + if (qemu_opts_do_parse_replace(param, options, NULL) != 0) {
>> error_report("Invalid options for file format '%s'.", out_fmt);
>> ret = -1;
>> goto out;
>> }
>> } else {
>> - param = parse_option_parameters("", create_options, param);
>> + param = qemu_opts_create_nofail(create_opts);
>> }
>> -
>> - set_option_parameter_int(param, BLOCK_OPT_SIZE, total_sectors * 512);
>> + qemu_opt_set_number(param, BLOCK_OPT_SIZE, total_sectors * 512);
>> ret = add_old_style_options(out_fmt, param, out_baseimg, NULL);
>> if (ret < 0) {
>> goto out;
>> }
>>
>> /* Get backing file name if -o backing_file was used */
>> - out_baseimg_param = get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
>> + out_baseimg_param = qemu_opt_get(param, BLOCK_OPT_BACKING_FILE);
>> if (out_baseimg_param) {
>> - out_baseimg = out_baseimg_param->value.s;
>> + out_baseimg = out_baseimg_param;
>> }
>
> More copy/paste coding. Cleanup beyond the scope of this patch.
>
>>
>> /* Check if compression is supported */
>> if (compress) {
>> - QEMUOptionParameter *encryption =
>> - get_option_parameter(param, BLOCK_OPT_ENCRYPT);
>> - QEMUOptionParameter *preallocation =
>> - get_option_parameter(param, BLOCK_OPT_PREALLOC);
>> + bool encryption =
>> + qemu_opt_get_bool(param, BLOCK_OPT_ENCRYPT, false);
>> + const char *preallocation =
>> + qemu_opt_get(param, BLOCK_OPT_PREALLOC);
>>
>> if (!drv->bdrv_write_compressed) {
>> error_report("Compression not supported for this file format");
>> @@ -1306,15 +1301,15 @@ static int img_convert(int argc, char **argv)
>> goto out;
>> }
>>
>> - if (encryption && encryption->value.n) {
>> + if (encryption) {
>> error_report("Compression and encryption not supported at "
>> "the same time");
>> ret = -1;
>> goto out;
>> }
>>
>> - if (preallocation && preallocation->value.s
>> - && strcmp(preallocation->value.s, "off"))
>> + if (preallocation
>> + && strcmp(preallocation, "off"))
>> {
>> error_report("Compression and preallocation not supported at "
>> "the same time");
>> @@ -1532,8 +1527,10 @@ static int img_convert(int argc, char **argv)
>> }
>> out:
>> qemu_progress_end();
>> - free_option_parameters(create_options);
>> - free_option_parameters(param);
>> + qemu_opts_free(create_opts);
>> + if (param) {
>> + qemu_opts_del(param);
>> + }
>> qemu_vfree(buf);
>> if (out_bs) {
>> bdrv_delete(out_bs);
>> diff --git a/util/qemu-option.c b/util/qemu-option.c
>> index 0e42452..dba82b4 100644
>> --- a/util/qemu-option.c
>> +++ b/util/qemu-option.c
>
> Changes to this file not reviewed in this round.
>
>> @@ -33,6 +33,8 @@
>> #include "qapi/qmp/qerror.h"
>> #include "qemu/option_int.h"
>>
>> +static void qemu_opt_del(QemuOpt *opt);
>> +
>> /*
>> * Extracts the name of an option from the parameter string (p points at the
>> * first byte of the option name)
>> @@ -531,6 +533,16 @@ const char *qemu_opt_get(QemuOpts *opts, const char *name)
>> return opt ? opt->str : NULL;
>> }
>>
>> +const char *qemu_opt_get_del(QemuOpts *opts, const char *name)
>> +{
>> + QemuOpt *opt = qemu_opt_find(opts, name);
>> + const char *str = opt ? opt->str : NULL;
>> + if (opt) {
>> + qemu_opt_del(opt);
>> + }
>> + return str;
>> +}
>> +
>> bool qemu_opt_has_help_opt(QemuOpts *opts)
>> {
>> QemuOpt *opt;
>> @@ -553,6 +565,22 @@ bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval)
>> return opt->value.boolean;
>> }
>>
>> +bool qemu_opt_get_bool_del(QemuOpts *opts, const char *name, bool defval)
>> +{
>> + QemuOpt *opt = qemu_opt_find(opts, name);
>> + bool ret;
>> +
>> + if (opt == NULL) {
>> + return defval;
>> + }
>> + ret = opt->value.boolean;
>> + assert(opt->desc && opt->desc->type == QEMU_OPT_BOOL);
>> + if (opt) {
>> + qemu_opt_del(opt);
>> + }
>> + return ret;
>> +}
>> +
>> uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval)
>> {
>> QemuOpt *opt = qemu_opt_find(opts, name);
>> @@ -573,6 +601,23 @@ uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval)
>> return opt->value.uint;
>> }
>>
>> +uint64_t qemu_opt_get_size_del(QemuOpts *opts, const char *name,
>> + uint64_t defval)
>> +{
>> + QemuOpt *opt = qemu_opt_find(opts, name);
>> + uint64_t ret;
>> +
>> + if (opt == NULL) {
>> + return defval;
>> + }
>> + ret = opt->value.uint;
>> + assert(opt->desc && opt->desc->type == QEMU_OPT_SIZE);
>> + if (opt) {
>> + qemu_opt_del(opt);
>> + }
>> + return ret;
>> +}
>> +
>> static void qemu_opt_parse(QemuOpt *opt, Error **errp)
>> {
>> if (opt->desc == NULL)
>> @@ -624,7 +669,7 @@ static const QemuOptDesc *find_desc_by_name(const QemuOptDesc *desc,
>> }
>>
>> static void opt_set(QemuOpts *opts, const char *name, const char *value,
>> - bool prepend, Error **errp)
>> + bool prepend, bool replace, Error **errp)
>> {
>> QemuOpt *opt;
>> const QemuOptDesc *desc;
>> @@ -636,6 +681,11 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value,
>> return;
>> }
>>
>> + opt = qemu_opt_find(opts, name);
>> + if (replace && opt) {
>> + qemu_opt_del(opt);
>> + }
>> +
>> opt = g_malloc0(sizeof(*opt));
>> opt->name = g_strdup(name);
>> opt->opts = opts;
>> @@ -658,7 +708,29 @@ int qemu_opt_set(QemuOpts *opts, const char *name, const char *value)
>> {
>> Error *local_err = NULL;
>>
>> - opt_set(opts, name, value, false, &local_err);
>> + opt_set(opts, name, value, false, false, &local_err);
>> + if (error_is_set(&local_err)) {
>> + qerror_report_err(local_err);
>> + error_free(local_err);
>> + return -1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> + * If name exists, the function will delete the opt first and then add a new
>> + * one.
>> + */
>> +int qemu_opt_replace_set(QemuOpts *opts, const char *name, const char *value)
>> +{
>> + Error *local_err = NULL;
>> + QemuOpt *opt = qemu_opt_find(opts, name);
>> +
>> + if (opt) {
>> + qemu_opt_del(opt);
>> + }
>> + opt_set(opts, name, value, false, true, &local_err);
>> if (error_is_set(&local_err)) {
>> qerror_report_err(local_err);
>> error_free(local_err);
>> @@ -671,7 +743,7 @@ int qemu_opt_set(QemuOpts *opts, const char *name, const char *value)
>> void qemu_opt_set_err(QemuOpts *opts, const char *name, const char *value,
>> Error **errp)
>> {
>> - opt_set(opts, name, value, false, errp);
>> + opt_set(opts, name, value, false, false, errp);
>> }
>>
>> int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val)
>> @@ -895,7 +967,7 @@ int qemu_opts_print(QemuOpts *opts, void *dummy)
>> }
>>
>> static int opts_do_parse(QemuOpts *opts, const char *params,
>> - const char *firstname, bool prepend)
>> + const char *firstname, bool prepend, bool replace)
>> {
>> char option[128], value[1024];
>> const char *p,*pe,*pc;
>> @@ -931,7 +1003,7 @@ static int opts_do_parse(QemuOpts *opts, const char *params,
>> }
>> if (strcmp(option, "id") != 0) {
>> /* store and parse */
>> - opt_set(opts, option, value, prepend, &local_err);
>> + opt_set(opts, option, value, prepend, replace, &local_err);
>> if (error_is_set(&local_err)) {
>> qerror_report_err(local_err);
>> error_free(local_err);
>> @@ -947,9 +1019,16 @@ static int opts_do_parse(QemuOpts *opts, const char *params,
>>
>> int qemu_opts_do_parse(QemuOpts *opts, const char *params, const char *firstname)
>> {
>> - return opts_do_parse(opts, params, firstname, false);
>> + return opts_do_parse(opts, params, firstname, false, false);
>> +}
>> +
>> +int qemu_opts_do_parse_replace(QemuOpts *opts, const char *params,
>> + const char *firstname)
>> +{
>> + return opts_do_parse(opts, params, firstname, false, true);
>> }
>>
>> +
>> static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
>> int permit_abbrev, bool defaults)
>> {
>> @@ -986,7 +1065,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
>> return NULL;
>> }
>>
>> - if (opts_do_parse(opts, params, firstname, defaults) != 0) {
>> + if (opts_do_parse(opts, params, firstname, defaults, false) != 0) {
>> qemu_opts_del(opts);
>> return NULL;
>> }
>
Really thanks for your reviewing, Markus.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH V12 3/4] Use QemuOpts support in block layer
2013-03-25 6:31 ` Dong Xu Wang
@ 2013-04-02 16:22 ` Markus Armbruster
0 siblings, 0 replies; 10+ messages in thread
From: Markus Armbruster @ 2013-04-02 16:22 UTC (permalink / raw)
To: Dong Xu Wang; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
Dong Xu Wang <wdongxu@linux.vnet.ibm.com> writes:
> On Thu, Mar 21, 2013 at 11:31 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> Dong Xu Wang <wdongxu@linux.vnet.ibm.com> writes:
[...]
>>> diff --git a/block.c b/block.c
>>> index 4582961..975c3d8 100644
>>> --- a/block.c
>>> +++ b/block.c
[...]
>>> @@ -4521,28 +4522,23 @@ void bdrv_img_create(const char *filename, const char *fmt,
>>> error_setg(errp, "Unknown protocol '%s'", filename);
>>> return;
>>> }
>>> -
>>> - create_options = append_option_parameters(create_options,
>>> - drv->create_options);
>>> - create_options = append_option_parameters(create_options,
>>> - proto_drv->create_options);
>>> -
>>> + create_opts = qemu_opts_append(drv->bdrv_create_opts,
>>> + proto_drv->bdrv_create_opts);
>>
>> qemu_opts_append() dereferences its arguments to compute
>>
>> dest->merge_lists = first->merge_lists || second->merge_lists;
>>
>> However, either of drv->create_opts and proto_drv->create_opts may be
>> null, as far as I can see. If I'm correct, you need a few more test
>> cases :)
>>
> Okay, will check first and second.
>> Before: format's options get appended first, then protocol's options.
>> Because get_option_parameter() searches in order, format options shadow
>> protocol options.
>>
>> After: append_opts_list() gives first argument's options precedence.
>>
>> Thus, no change. Good.
>>
>> Should bdrv_create_options option name clashes be avoided? Beyond the
>> scope of this patch.
>>
>>> /* Create parameter list with default values */
>>> - param = parse_option_parameters("", create_options, param);
>>> + opts = qemu_opts_create_nofail(create_opts);
>>
>> Before: param empty.
>>
>> After: opts empty.
>>
>> Good.
>>
>>>
>>> - set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size);
>>> + qemu_opt_set_number(opts, BLOCK_OPT_SIZE, img_size);
>>
>> Before: param contains exactly BLOCK_OPT_SIZE = img_size.
>>
>> After: opts contains exactly BLOCK_OPT_SIZE = img_size.
>>
>> Both old and new code seem to assume BLOCK_OPT_SIZE is always a valid
>> option. Beyond the scope of this patch.
>>
>> Good.
>>
>>>
>>> /* Parse -o options */
>>> if (options) {
>>> - param = parse_option_parameters(options, create_options, param);
>>> - if (param == NULL) {
>>> + if (qemu_opts_do_parse_replace(opts, options, NULL) != 0) {
>>> error_setg(errp, "Invalid options for file format '%s'.", fmt);
>>> goto out;
>>> }
>>> }
>>
>> Before: values from -o replace whatever is in param already.
>>
>> After: values from -o replace whatever is in opts already.
>>
>> In particular, size from -o replaces img_size before and after.
>>
>> Did you test it does?
>>
>> $ qemu-img create -f raw -o size=1024 t.img 512
>> Formatting 't.img', fmt=raw size=1024
>> $ ls -l t.img
>> -rw-r--r--. 1 armbru armbru 1024 Mar 21 15:12 t.img
>>
>>>
>>> if (base_filename) {
>>> - if (set_option_parameter(param, BLOCK_OPT_BACKING_FILE,
>>> + if (qemu_opt_replace_set(opts, BLOCK_OPT_BACKING_FILE,
>>> base_filename)) {
>>> error_setg(errp, "Backing file not supported for file format '%s'",
>>> fmt);
>>
>> Before: base_filename replaces any backing_file from -o in param.
>>
>> After: base_filename replaces any backing_file from -o in opts.
>>
>> Did you test it does?
>>
> Oh, I did not test command line like this, my patch will fail here.
> Will fix in next version.
>
>> $ qemu-img create -f qcow2 -b t.img -o backing_file=/dev/null t.qcow2
>> Formatting 't.qcow2', fmt=qcow2 size=1024 backing_file='t.img' encryption=off cluster_size=65536 lazy_refcounts=off
>> $ qemu-img info t.qcow2image: t.qcow2
>> file format: qcow2
>> virtual size: 1.0K (1024 bytes)
>> disk size: 196K
>> cluster_size: 65536
>> backing file: t.img
>>
>
> I run this command using upstream QEMU code, but failed.
> [wdongxu@13@VM:~]$ qemu-img info t.qcow2image: t.qcow2
> qemu-img: Could not open 't.qcow2image:': No such file or directory
>
> Or did I type something wrong?
Looks like I pastoed. Please try "qemu-img info t.qcow2".
[...]
>>> diff --git a/block/cow.c b/block/cow.c
>>> index 4baf904..135fa33 100644
>>> --- a/block/cow.c
>>> +++ b/block/cow.c
>>> @@ -255,7 +255,7 @@ static void cow_close(BlockDriverState *bs)
>>> {
>>> }
>>>
>>> -static int cow_create(const char *filename, QEMUOptionParameter *options)
>>> +static int cow_create(const char *filename, QemuOpts *opts)
>>> {
>>> struct cow_header_v2 cow_header;
>>> struct stat st;
>>> @@ -264,17 +264,11 @@ static int cow_create(const char *filename, QEMUOptionParameter *options)
>>> int ret;
>>> BlockDriverState *cow_bs;
>>>
>>> - /* Read out options */
>>> - while (options && options->name) {
>>> - if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
>>> - image_sectors = options->value.n / 512;
>>> - } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FILE)) {
>>> - image_filename = options->value.s;
>>> - }
>>> - options++;
>>> - }
>>> + /* Read out opts */
>>> + image_sectors = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0) / 512;
>>> + image_filename = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
>>
>> Why _del?
>>
>
> Before: after getting one parameter, then option++, so in my patches, I want to
> delete the opt after using it. If I did not delete it, some parameters
> will be passed
> to "protocol", it will go wrong. Do you think it is acceptable?
I missed the fact that the old code changes its options parameter before
it passes it to bdrv_create_file(). Let's examine how it works.
options is either null an array of QEMUOptionParameter terminated by an
sentinel element with a null name.
The loop steps through options until it hits the sentinel.
Postcondition: !options || !options->name. In words: either no options,
or an empty array of options.
Then:
>>>
>>> - ret = bdrv_create_file(filename, options);
>>> + ret = bdrv_create_file(filename, opts);
>>> if (ret < 0) {
>>> return ret;
>>> }
We pass either no options or an empty array of options to
bdrv_create_file(). I suspect we could just as well pass NULL.
If I'm right, you should pass NULL instead of opts. No reason to change
opts then.
>>> @@ -318,18 +312,22 @@ exit:
>>> return ret;
>>> }
[...]
>
> Really thanks for your reviewing, Markus.
You're welcome!
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-04-02 16:24 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1362043236-4635-1-git-send-email-wdongxu@linux.vnet.ibm.com>
2013-03-05 9:06 ` [Qemu-devel] [PATCH V12 0/4] replace QEMUOptionParameter with QemuOpts parser Stefan Hajnoczi
2013-03-07 7:36 ` Markus Armbruster
[not found] ` <1362043236-4635-4-git-send-email-wdongxu@linux.vnet.ibm.com>
2013-03-06 1:51 ` [Qemu-devel] [PATCH V12 3/4] Use QemuOpts support in block layer Dong Xu Wang
2013-03-21 15:31 ` Markus Armbruster
2013-03-25 6:31 ` Dong Xu Wang
2013-04-02 16:22 ` Markus Armbruster
2013-03-21 13:03 ` [Qemu-devel] [PATCH V12 0/4] replace QEMUOptionParameter with QemuOpts parser Markus Armbruster
2013-03-21 13:20 ` Kevin Wolf
[not found] ` <1362043236-4635-3-git-send-email-wdongxu@linux.vnet.ibm.com>
2013-03-21 13:05 ` [Qemu-devel] [PATCH V12 2/4] Create four QemuOptsList related functions Markus Armbruster
[not found] ` <1362043236-4635-2-git-send-email-wdongxu@linux.vnet.ibm.com>
2013-03-21 15:59 ` [Qemu-devel] [PATCH V12 1/4] add def_value_str and use it in qemu_opts_print Markus Armbruster
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).