[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V12 3/4] Use QemuOpts support in block layer
|
From: |
Dong Xu Wang |
|
Subject: |
Re: [Qemu-devel] [PATCH V12 3/4] Use QemuOpts support in block layer |
|
Date: |
Mon, 25 Mar 2013 14:31:17 +0800 |
On Thu, Mar 21, 2013 at 11:31 PM, Markus Armbruster <address@hidden> wrote:
> Dong Xu Wang <address@hidden> 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 <address@hidden>
>> ---
>>
>> 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.
address@hidden@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.