[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFC] remove QEMUOptionParameter
|
From: |
Dong Xu Wang |
|
Subject: |
Re: [Qemu-devel] [PATCH RFC] remove QEMUOptionParameter |
|
Date: |
Mon, 10 Sep 2012 10:10:56 +0800 |
On Fri, Sep 7, 2012 at 4:42 PM, Markus Armbruster <address@hidden> wrote:
> Some overlap with what I'm working on, but since you have code to show,
> and I don't, I'll review yours as if mine didn't exist.
>
>
> Dong Xu Wang <address@hidden> writes:
>
>> QEMU now has QEMUOptionParameter and QemuOpts to parse parameters, so we can
>> remove one safely. This RFC removed QEMUOptionParameter and use QemuOpts. But
>> the patch is not completed, I think it is better to send a RFC first. In the
>> RFC, I only allow qcow2 and raw file format and did not test it completly, if
>> you have any concerns and suggestions about the main idea, please let me
>> know,
>> that wil be very grateful.
>>
>> Signed-off-by: Dong Xu Wang <address@hidden>
>> ---
>> block.c | 106 ++++++++-----
>> block.h | 8 +-
>> block/Makefile.objs | 9 +-
>> block/qcow2.c | 123 ++++----------
>> block/raw-posix.c | 45 +----
>> block/raw.c | 12 +--
>> block_int.h | 5 +-
>> qemu-config.c | 85 ++++++++++
>> qemu-img.c | 67 ++++----
>> qemu-option.c | 456
>> +++++++++++++++++++--------------------------------
>> qemu-option.h | 47 +-----
>> 11 files changed, 417 insertions(+), 546 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 470bdcc..8e973ff 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -351,7 +351,7 @@ BlockDriver *bdrv_find_whitelisted_format(const char
>> *format_name)
>> typedef struct CreateCo {
>> BlockDriver *drv;
>> char *filename;
>> - QEMUOptionParameter *options;
>> + QemuOpts *options;
>
> Consider renaming to opts, because that's the commonly used name. You
> did it already elsewhere.
Okay.
>
>> int ret;
>> } CreateCo;
>>
>> @@ -364,7 +364,7 @@ static void coroutine_fn bdrv_create_co_entry(void
>> *opaque)
>> }
>>
>> int bdrv_create(BlockDriver *drv, const char* filename,
>> - QEMUOptionParameter *options)
>> + QemuOpts *opts)
>> {
>> int ret;
>>
>> @@ -372,7 +372,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
>> CreateCo cco = {
>> .drv = drv,
>> .filename = g_strdup(filename),
>> - .options = options,
>> + .options = opts,
>> .ret = NOT_DONE,
>> };
>>
>> @@ -397,7 +397,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
>> return ret;
>> }
>>
>> -int bdrv_create_file(const char* filename, QEMUOptionParameter *options)
>> +int bdrv_create_file(const char *filename, QemuOpts *opts)
>> {
>> BlockDriver *drv;
>>
>> @@ -406,7 +406,7 @@ int bdrv_create_file(const char* filename,
>> QEMUOptionParameter *options)
>> return -ENOENT;
>> }
>>
>> - return bdrv_create(drv, filename, options);
>> + return bdrv_create(drv, filename, opts);
>> }
>>
>> /*
>> @@ -742,8 +742,9 @@ int bdrv_open(BlockDriverState *bs, const char
>> *filename, int flags,
>> int64_t total_size;
>> int is_protocol = 0;
>> BlockDriver *bdrv_qcow2;
>> - QEMUOptionParameter *options;
>> + QemuOpts *options;
>> char backing_filename[PATH_MAX];
>> + char buf_total_size[1024];
>>
>> /* if snapshot, we create a temporary backing file and open it
>> instead of opening 'filename' directly */
>> @@ -775,17 +776,19 @@ 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);
>> + options = qemu_opts_create(qemu_find_opts("qcow2_create_options"),
>> + NULL, 0, NULL);
>
> I'm afraid you named them "qcow2_create_opts" in qemu-config.c.
Okay.
>
>>
>> - set_option_parameter_int(options, BLOCK_OPT_SIZE, total_size);
>> - set_option_parameter(options, BLOCK_OPT_BACKING_FILE,
>> backing_filename);
>> + snprintf(buf_total_size, sizeof(buf_total_size),
>> + "%" PRId64, total_size);
>> + qemu_opt_set(options, BLOCK_OPT_SIZE, buf_total_size);
>
> This is a bit awkward.
>
> We could have qemu_opt_set_number(), like qemu_opt_set_bool(). Except
> qemu_opt_set_bool() has issues. Luiz's fix is discussed here:
> http://lists.nongnu.org/archive/html/qemu-devel/2012-07/msg02716.html
>
> Luiz, do you plan to respin?
>
>> + qemu_opt_set(options, BLOCK_OPT_BACKING_FILE, backing_filename);
>> if (drv) {
>> - set_option_parameter(options, BLOCK_OPT_BACKING_FMT,
>> + qemu_opt_set(options, BLOCK_OPT_BACKING_FMT,
>> drv->format_name);
>> }
>>
>> ret = bdrv_create(bdrv_qcow2, tmp_filename, options);
>> - free_option_parameters(options);
>> if (ret < 0) {
>> return ret;
>> }
>
> This means ownership of options now passes to bdrv_create(). Which
> doesn't free it either. Should it?
>
>> @@ -3901,12 +3904,16 @@ int bdrv_img_create(const char *filename, const char
>> *fmt,
>> const char *base_filename, const char *base_fmt,
>> char *options, uint64_t img_size, int flags)
>> {
>> - QEMUOptionParameter *param = NULL, *create_options = NULL;
>> - QEMUOptionParameter *backing_fmt, *backing_file, *size;
>> + QemuOpts *param = NULL;
>> + QemuOptsList *create_options = NULL;
>> + const char *backing_fmt, *backing_file, *size;
>> BlockDriverState *bs = NULL;
>> BlockDriver *drv, *proto_drv;
>> BlockDriver *backing_drv = NULL;
>> int ret = 0;
>> + char buf_img_size[1024];
>> + char create_buff[1024];
>> + char proto_buff[1024];
>>
>> /* Find driver and parse its options */
>> drv = bdrv_find_format(fmt);
>> @@ -3922,20 +3929,19 @@ int bdrv_img_create(const char *filename, const char
>> *fmt,
>> ret = -EINVAL;
>> goto out;
>> }
>> -
>> - create_options = append_option_parameters(create_options,
>> - drv->create_options);
>> - create_options = append_option_parameters(create_options,
>> - proto_drv->create_options);
>> -
>> + get_format_create_options(create_buff, sizeof(create_buff), drv);
>> + get_proto_create_options(proto_buff, sizeof(proto_buff), proto_drv);
>
> These two functions don't get options, they get a "group name" to pass
> to qemu_find_opts().
>
> That's the only use of the value. What about making the functions
> return the QemuOptsList instead?
>
> Or even a function that maps BlockDriverState * to QemuOptsList *.
Okay.
>
>> + create_options = append_opts_list(qemu_find_opts(create_buff),
>> + qemu_find_opts(proto_buff));
>> /* Create parameter list with default values */
>> - param = parse_option_parameters("", create_options, param);
>> + param = qemu_opts_create(create_options, NULL, 0, NULL);
>
> Ignoring errors is fine, because qemu_opts_create() can't fail when id
> is null.
>
> Aside: this is a common pattern. Maybe we should have a
> qemu_opts_create_nofail(QemuOptsList *list) for the purpose.
>
>>
>> - set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size);
>> + snprintf(buf_img_size, sizeof(buf_img_size), "%" PRId64, img_size);
>> + qemu_opt_set(param, BLOCK_OPT_SIZE, buf_img_size);
>
> Another possible user of qemu_opt_set_number().
>
>>
>> /* Parse -o options */
>> if (options) {
>> - param = parse_option_parameters(options, create_options, param);
>> + param = parse_opts_list(options, create_options, param);
>> if (param == NULL) {
>> error_report("Invalid options for file format '%s'.", fmt);
>> ret = -EINVAL;
>> @@ -3944,7 +3950,7 @@ int bdrv_img_create(const char *filename, const char
>> *fmt,
>> }
>>
>> if (base_filename) {
>> - if (set_option_parameter(param, BLOCK_OPT_BACKING_FILE,
>> + if (qemu_opt_set(param, BLOCK_OPT_BACKING_FILE,
>> base_filename)) {
>> error_report("Backing file not supported for file format '%s'",
>> fmt);
>> @@ -3954,7 +3960,7 @@ int 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_set(param, BLOCK_OPT_BACKING_FMT, base_fmt)) {
>> error_report("Backing file format not supported for file "
>> "format '%s'", fmt);
>> ret = -EINVAL;
>> @@ -3962,9 +3968,9 @@ int bdrv_img_create(const char *filename, const char
>> *fmt,
>> }
>> }
>>
>> - 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(param, BLOCK_OPT_BACKING_FILE);
>> + if (backing_file) {
>> + if (!strcmp(filename, backing_file)) {
>> error_report("Error: Trying to create an image with the "
>> "same filename as the backing file");
>> ret = -EINVAL;
>> @@ -3972,12 +3978,12 @@ int bdrv_img_create(const char *filename, const char
>> *fmt,
>> }
>> }
>>
>> - 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(param, BLOCK_OPT_BACKING_FMT);
>> + if (backing_fmt) {
>> + backing_drv = bdrv_find_format(backing_fmt);
>> if (!backing_drv) {
>> error_report("Unknown backing file format '%s'",
>> - backing_fmt->value.s);
>> + backing_fmt);
>> ret = -EINVAL;
>> goto out;
>> }
>> @@ -3985,9 +3991,9 @@ int bdrv_img_create(const char *filename, const char
>> *fmt,
>>
>> // 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) {
>
> Can you explain how this code works? I don't get it.
>
>> + size = qemu_opt_get(param, BLOCK_OPT_SIZE);
>> + if (size && !strcmp(size, "-1")) {
>
> What about qemu_opt_get_number()?
>
> Beware, QemuOpts support only *unsigned* numbers.
Okay.
>
>> + if (backing_file) {
>> uint64_t size;
>> char buf[32];
>> int back_flags;
>> @@ -3998,16 +4004,16 @@ int 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_report("Could not open '%s'", backing_file->value.s);
>> + error_report("Could not open '%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(param, BLOCK_OPT_SIZE, buf);
>
> Another one for qemu_opt_set_number().
>
>> } else {
>> error_report("Image creation needs a size parameter");
>> ret = -EINVAL;
>> @@ -4016,7 +4022,7 @@ int bdrv_img_create(const char *filename, const char
>> *fmt,
>> }
>>
>> printf("Formatting '%s', fmt=%s ", filename, fmt);
>> - print_option_parameters(param);
>> + qemu_opts_print(param, NULL);
>> puts("");
>>
>> ret = bdrv_create(drv, filename, param);
>> @@ -4035,8 +4041,7 @@ int bdrv_img_create(const char *filename, const char
>> *fmt,
>> }
>>
>> out:
>> - free_option_parameters(create_options);
>> - free_option_parameters(param);
>> + free_opts_list(create_options);
>
> Why isn't param freed with qemu_opts_del()?
>
>> if (bs) {
>> bdrv_delete(bs);
>> @@ -4171,3 +4176,24 @@ void block_job_sleep_ns(BlockJob *job, QEMUClock
>> *clock, int64_t ns)
>> job->busy = true;
>> }
>> }
>> +
>> +void get_proto_create_options(char *proto_buff,
>> + int buf_len, BlockDriver *proto_drv)
>> +{
>> + if (strcmp(proto_drv->format_name, "host_device") ||
>> + strcmp(proto_drv->format_name, "host_floppy") ||
>> + strcmp(proto_drv->format_name, "host_cdrom")) {
>> + snprintf(proto_buff, buf_len,
>> + "%s", "file_proto_create_opts");
>
> This function knows that certain drivers share options. Shouldn't that
> knowledge be encapsulated in drivers?
>
> I suspect the proper solution is a BlockDriver method returning the
> create option list.
>
>> + } else {
>> + snprintf(proto_buff, buf_len,
>> + "%s""_proto_create_opts", proto_drv->format_name);
>> + }
>> +}
>> +
>> +void get_format_create_options(char *create_buff, int buf_len, BlockDriver*
>> drv)
>> +{
>> + snprintf(create_buff, buf_len,
>> + "%s""_create_opts", drv->format_name);
>> +}
>> +
>> diff --git a/block.h b/block.h
>> index 2e2be11..04ec173 100644
>> --- a/block.h
>> +++ b/block.h
>> @@ -119,8 +119,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);
>> @@ -405,4 +405,8 @@ typedef enum {
>> #define BLKDBG_EVENT(bs, evt) bdrv_debug_event(bs, evt)
>> void bdrv_debug_event(BlockDriverState *bs, BlkDebugEvent event);
>>
>> +void get_proto_create_options(char *proto_buff,
>> + int buf_len, BlockDriver *proto_drv);
>> +void get_format_create_options(char *create_buff,
>> + int buf_len, BlockDriver *drv);
>> #endif
>> diff --git a/block/Makefile.objs b/block/Makefile.objs
>> index b5754d3..19de020 100644
>> --- a/block/Makefile.objs
>> +++ b/block/Makefile.objs
>> @@ -1,8 +1,9 @@
>> -block-obj-y += raw.o cow.o qcow.o vdi.o vmdk.o cloop.o dmg.o bochs.o vpc.o
>> vvfat.o
>> +#block-obj-y += raw.o cow.o qcow.o vdi.o vmdk.o cloop.o dmg.o bochs.o vpc.o
>> vvfat.o
>> block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o
>> qcow2-cache.o
>> -block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
>> -block-obj-y += qed-check.o
>> -block-obj-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o
>> +#block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
>> +#block-obj-y += qed-check.o
>> +#block-obj-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o
>> +block-obj-y += raw.o
>> block-obj-y += stream.o
>> block-obj-$(CONFIG_WIN32) += raw-win32.o
>> block-obj-$(CONFIG_POSIX) += raw-posix.o
>
> Lots of stuff cut to simplify exploring, I guess.
>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 8f183f1..c0f3764 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1170,7 +1170,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 *options, int version)
>> {
>> /* Calculate cluster_bits */
>> int cluster_bits;
>> @@ -1201,6 +1201,7 @@ static int qcow2_create2(const char *filename, int64_t
>> total_size,
>> uint8_t* refcount_table;
>> int ret;
>>
>> + qemu_opt_set(options, BLOCK_OPT_SIZE, "0");
>
> Why do you need this?
If it doesnot exist, raw-posix.c:raw_create will get a core, because
of no size option.
>
>> ret = bdrv_create_file(filename, options);
>> if (ret < 0) {
>> return ret;
>> @@ -1304,7 +1305,7 @@ out:
>> return ret;
>> }
>>
>> -static int qcow2_create(const char *filename, QEMUOptionParameter *options)
>> +static int qcow2_create(const char *filename, QemuOpts *param)
>
> Rename to opts instead of param, please.
Okay.
>
>> {
>> const char *backing_file = NULL;
>> const char *backing_fmt = NULL;
>> @@ -1313,45 +1314,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(param, BLOCK_OPT_SIZE, 0) / 512;
>> + backing_file = qemu_opt_get(param, BLOCK_OPT_BACKING_FILE);
>> + backing_fmt = qemu_opt_get(param, BLOCK_OPT_BACKING_FMT);
>> + if (qemu_opt_get_bool(param, BLOCK_OPT_ENCRYPT, 0)) {
>> + flags |= BLOCK_FLAG_ENCRYPT;
>> + }
>> + cluster_size =
>> + qemu_opt_get_size(param, BLOCK_OPT_CLUSTER_SIZE,
>> DEFAULT_CLUSTER_SIZE);
>> + buf = qemu_opt_get(param, 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(param, 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(param, BLOCK_OPT_LAZY_REFCOUNTS, 0)) {
>> + flags |= BLOCK_FLAG_LAZY_REFCOUNTS;
>> }
>>
>> if (backing_file && prealloc) {
>> @@ -1367,7 +1364,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, param, version);
>> }
>>
>> static int qcow2_make_empty(BlockDriverState *bs)
>> @@ -1628,51 +1625,6 @@ 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 }
>> -};
>> -
>
> Replacement moves to qemu-config.c. Not sure that's an improvement, but
> it's how QemuOpts generally work. For an exception, see QemuOptsList
> use in blkdebug.c.
>
>> static BlockDriver bdrv_qcow2 = {
>> .format_name = "qcow2",
>> .instance_size = sizeof(BDRVQcowState),
>> @@ -1707,7 +1659,6 @@ static BlockDriver bdrv_qcow2 = {
>>
>> .bdrv_invalidate_cache = qcow2_invalidate_cache,
>>
>> - .create_options = qcow2_create_options,
>> .bdrv_check = qcow2_check,
>> };
>>
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index 6be20b1..bcce7ed 100644
>> --- a/block/raw-posix.c
>> +++ b/block/raw-posix.c
>> @@ -558,19 +558,13 @@ 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(opts, BLOCK_OPT_SIZE, 0) /
>> BDRV_SECTOR_SIZE;
>>
>> fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
>> 0644);
>> @@ -720,15 +714,6 @@ static coroutine_fn int raw_co_discard(BlockDriverState
>> *bs,
>> return 0;
>> }
>>
>> -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",
>> @@ -748,8 +733,6 @@ static BlockDriver bdrv_file = {
>> .bdrv_getlength = raw_getlength,
>> .bdrv_get_allocated_file_size
>> = raw_get_allocated_file_size,
>> -
>> - .create_options = raw_create_options,
>> };
>>
>> /***********************************************/
>> @@ -962,20 +945,14 @@ static int fd_open(BlockDriverState *bs)
>>
>> #endif /* !linux && !FreeBSD */
>>
>> -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(opts, BLOCK_OPT_SIZE, 0) /
>> BDRV_SECTOR_SIZE;
>>
>> fd = qemu_open(filename, O_WRONLY | O_BINARY);
>> if (fd < 0)
>> @@ -999,21 +976,20 @@ 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,
>> .bdrv_close = raw_close,
>> .bdrv_create = hdev_create,
>> - .create_options = raw_create_options,
>> .bdrv_has_zero_init = hdev_has_zero_init,
>>
>> - .bdrv_aio_readv = raw_aio_readv,
>> - .bdrv_aio_writev = raw_aio_writev,
>> - .bdrv_aio_flush = raw_aio_flush,
>> + .bdrv_aio_readv = raw_aio_readv,
>> + .bdrv_aio_writev = raw_aio_writev,
>> + .bdrv_aio_flush = raw_aio_flush,
>>
>> .bdrv_truncate = raw_truncate,
>> - .bdrv_getlength = raw_getlength,
>> + .bdrv_getlength = raw_getlength,
>> .bdrv_get_allocated_file_size
>> = raw_get_allocated_file_size,
>>
>> @@ -1126,7 +1102,6 @@ static BlockDriver bdrv_host_floppy = {
>> .bdrv_file_open = floppy_open,
>> .bdrv_close = raw_close,
>> .bdrv_create = hdev_create,
>> - .create_options = raw_create_options,
>> .bdrv_has_zero_init = hdev_has_zero_init,
>>
>> .bdrv_aio_readv = raw_aio_readv,
>> @@ -1225,7 +1200,6 @@ static BlockDriver bdrv_host_cdrom = {
>> .bdrv_file_open = cdrom_open,
>> .bdrv_close = raw_close,
>> .bdrv_create = hdev_create,
>> - .create_options = raw_create_options,
>> .bdrv_has_zero_init = hdev_has_zero_init,
>>
>> .bdrv_aio_readv = raw_aio_readv,
>> @@ -1344,7 +1318,6 @@ static BlockDriver bdrv_host_cdrom = {
>> .bdrv_file_open = cdrom_open,
>> .bdrv_close = raw_close,
>> .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.c b/block/raw.c
>> index ff34ea4..a6a6758 100644
>> --- a/block/raw.c
>> +++ b/block/raw.c
>> @@ -87,20 +87,11 @@ 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)
>> +static int raw_create(const char *filename, QemuOpts *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_has_zero_init(BlockDriverState *bs)
>> {
>> return bdrv_has_zero_init(bs->file);
>> @@ -133,7 +124,6 @@ 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,
>> };
>>
>> diff --git a/block_int.h b/block_int.h
>> index 4452f6f..5eea367 100644
>> --- a/block_int.h
>> +++ b/block_int.h
>> @@ -147,7 +147,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 */
>> @@ -236,9 +236,6 @@ 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;
>> -
>>
>> /*
>> * Returns 0 for completed check, -errno for internal errors.
>> diff --git a/qemu-config.c b/qemu-config.c
>> index c05ffbc..0376c92 100644
>> --- a/qemu-config.c
>> +++ b/qemu-config.c
>> @@ -5,6 +5,87 @@
>> #include "hw/qdev.h"
>> #include "error.h"
>>
>> +#include "block_int.h"
>> +#define QCOW2_DEFAULT_CLUSTER_SIZE 65536
>> +
>> +static QemuOptsList file_proto_create_opts = {
>> + .name = "file_proto_create_opts",
>
> Please use '-' instead of '_' in name strings. More of the same below.
>
>> + .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 */ }
>> + }
>> +};
>> +
>> +/************************************************************/
>> +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 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"
>> + },
>> + {
>> + .name = BLOCK_OPT_CLUSTER_SIZE,
>> + .type = QEMU_OPT_SIZE,
>> + .help = "qcow2 cluster size",
>> + .def_value = QCOW2_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",
>> + },
>> + { /* end of list */ }
>> + }
>> +};
>> +
>> +/*******************************************************************/
>> +
>> static QemuOptsList qemu_drive_opts = {
>> .name = "drive",
>> .head = QTAILQ_HEAD_INITIALIZER(qemu_drive_opts.head),
>> @@ -659,6 +740,10 @@ static QemuOptsList *vm_config_groups[32] = {
>> &qemu_boot_opts,
>> &qemu_iscsi_opts,
>> &qemu_sandbox_opts,
>> +
>> + &file_proto_create_opts,
>> + &raw_create_opts,
>> + &qcow2_create_opts,
>> NULL,
>> };
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index b41e670..a442b5e 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -195,7 +195,9 @@ 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_options = NULL;
>> + char create_buff[1024];
>> + char proto_buff[1024];
>>
>> /* Find driver and parse its options */
>> drv = bdrv_find_format(fmt);
>> @@ -209,13 +211,12 @@ static int print_block_option_help(const char
>> *filename, const char *fmt)
>> error_report("Unknown protocol '%s'", filename);
>> 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);
>> + get_format_create_options(create_buff, sizeof(create_buff), drv);
>> + get_proto_create_options(proto_buff, sizeof(proto_buff), proto_drv);
>> + create_options = append_opts_list(qemu_find_opts(create_buff),
>> + qemu_find_opts(proto_buff));
>> + print_opts_list(create_options);
>> + free_opts_list(create_options);
>> return 0;
>> }
>>
>> @@ -265,19 +266,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;
>> @@ -664,12 +665,15 @@ 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_options = NULL;
>> + const char *out_baseimg_param;
>> char *options = NULL;
>> const char *snapshot_name = NULL;
>> float local_progress;
>> int min_sparse = 8; /* Need at least 4k of zeros for sparse detection */
>> + char buf_size[1024];
>> + char create_buff[1024], proto_buff[1024];
>>
>> fmt = NULL;
>> out_fmt = "raw";
>> @@ -800,40 +804,40 @@ 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);
>> + get_format_create_options(create_buff, sizeof(create_buff), drv);
>> + get_proto_create_options(proto_buff, sizeof(proto_buff), proto_drv);
>> + create_options = append_opts_list(qemu_find_opts(create_buff),
>> + qemu_find_opts(proto_buff));
>>
>> if (options) {
>> - param = parse_option_parameters(options, create_options, param);
>> + param = parse_opts_list(options, create_options, param);
>> if (param == NULL) {
>> error_report("Invalid options for file format '%s'.", out_fmt);
>> ret = -1;
>> goto out;
>> }
>> } else {
>> - param = parse_option_parameters("", create_options, param);
>> + param = qemu_opts_create(create_options, NULL, 0, NULL);
>> }
>> -
>> - set_option_parameter_int(param, BLOCK_OPT_SIZE, total_sectors * 512);
>> + snprintf(buf_size, sizeof(buf_size), "%" PRId64, total_sectors * 512);
>> + qemu_opt_set(param, BLOCK_OPT_SIZE, buf_size);
>> 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);
>> + const char *encryption =
>> + qemu_opt_get(param, BLOCK_OPT_ENCRYPT);
>> + const char *preallocation =
>> + qemu_opt_get(param, BLOCK_OPT_PREALLOC);
>>
>> if (!drv->bdrv_write_compressed) {
>> error_report("Compression not supported for this file format");
>> @@ -841,15 +845,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");
>> @@ -1063,8 +1067,7 @@ static int img_convert(int argc, char **argv)
>> }
>> out:
>> qemu_progress_end();
>> - free_option_parameters(create_options);
>> - free_option_parameters(param);
>> + free_opts_list(create_options);
>> qemu_vfree(buf);
>> if (out_bs) {
>> bdrv_delete(out_bs);
>> diff --git a/qemu-option.c b/qemu-option.c
>> index 27891e7..b83ca52 100644
>> --- a/qemu-option.c
>> +++ b/qemu-option.c
>> @@ -153,22 +153,6 @@ int check_params(char *buf, int buf_size,
>> return 0;
>> }
>>
>> -/*
>> - * Searches an option list for an option with the given name
>> - */
>> -QEMUOptionParameter *get_option_parameter(QEMUOptionParameter *list,
>> - const char *name)
>> -{
>> - while (list && list->name) {
>> - if (!strcmp(list->name, name)) {
>> - return list;
>> - }
>> - list++;
>> - }
>> -
>> - return NULL;
>> -}
>> -
>> static void parse_option_bool(const char *name, const char *value, bool
>> *ret,
>> Error **errp)
>> {
>> @@ -240,275 +224,6 @@ static void parse_option_size(const char *name, const
>> char *value,
>> }
>> }
>>
>> -/*
>> - * Sets the value of a parameter in a given option list. The parsing of the
>> - * value depends on the type of option:
>> - *
>> - * OPT_FLAG (uses value.n):
>> - * If no value is given, the flag is set to 1.
>> - * Otherwise the value must be "on" (set to 1) or "off" (set to 0)
>> - *
>> - * OPT_STRING (uses value.s):
>> - * value is strdup()ed and assigned as option value
>> - *
>> - * OPT_SIZE (uses value.n):
>> - * The value is converted to an integer. Suffixes for kilobytes etc.
>> are
>> - * allowed (powers of 1024).
>> - *
>> - * Returns 0 on succes, -1 in error cases
>> - */
>> -int set_option_parameter(QEMUOptionParameter *list, const char *name,
>> - const char *value)
>> -{
>> - bool flag;
>> - Error *local_err = NULL;
>> -
>> - // Find a matching parameter
>> - list = get_option_parameter(list, name);
>> - if (list == NULL) {
>> - fprintf(stderr, "Unknown option '%s'\n", name);
>> - return -1;
>> - }
>> -
>> - // Process parameter
>> - switch (list->type) {
>> - case OPT_FLAG:
>> - parse_option_bool(name, value, &flag, &local_err);
>> - if (!error_is_set(&local_err)) {
>> - list->value.n = flag;
>> - }
>> - break;
>> -
>> - case OPT_STRING:
>> - if (value != NULL) {
>> - list->value.s = g_strdup(value);
>> - } else {
>> - fprintf(stderr, "Option '%s' needs a parameter\n", name);
>> - return -1;
>> - }
>> - break;
>> -
>> - case OPT_SIZE:
>> - parse_option_size(name, value, &list->value.n, &local_err);
>> - break;
>> -
>> - default:
>> - fprintf(stderr, "Bug: Option '%s' has an unknown type\n", name);
>> - return -1;
>> - }
>> -
>> - if (error_is_set(&local_err)) {
>> - qerror_report_err(local_err);
>> - error_free(local_err);
>> - return -1;
>> - }
>> -
>> - return 0;
>> -}
>> -
>> -/*
>> - * Sets the given parameter to an integer instead of a string.
>> - * This function cannot be used to set string options.
>> - *
>> - * Returns 0 on success, -1 in error cases
>> - */
>> -int set_option_parameter_int(QEMUOptionParameter *list, const char *name,
>> - uint64_t value)
>> -{
>> - // Find a matching parameter
>> - list = get_option_parameter(list, name);
>> - if (list == NULL) {
>> - fprintf(stderr, "Unknown option '%s'\n", name);
>> - return -1;
>> - }
>> -
>> - // Process parameter
>> - switch (list->type) {
>> - case OPT_FLAG:
>> - case OPT_NUMBER:
>> - case OPT_SIZE:
>> - list->value.n = value;
>> - break;
>> -
>> - default:
>> - return -1;
>> - }
>> -
>> - return 0;
>> -}
>> -
>> -/*
>> - * Frees a option list. If it contains strings, the strings are freed as
>> well.
>> - */
>> -void free_option_parameters(QEMUOptionParameter *list)
>> -{
>> - QEMUOptionParameter *cur = list;
>> -
>> - while (cur && cur->name) {
>> - if (cur->type == OPT_STRING) {
>> - g_free(cur->value.s);
>> - }
>> - cur++;
>> - }
>> -
>> - g_free(list);
>> -}
>> -
>> -/*
>> - * Count valid options in list
>> - */
>> -static size_t count_option_parameters(QEMUOptionParameter *list)
>> -{
>> - size_t num_options = 0;
>> -
>> - while (list && list->name) {
>> - num_options++;
>> - list++;
>> - }
>> -
>> - return num_options;
>> -}
>> -
>> -/*
>> - * Append an option list (list) to an option list (dest).
>> - *
>> - * If dest is NULL, a new copy of list is created.
>> - *
>> - * Returns a pointer to the first element of dest (or the newly allocated
>> copy)
>> - */
>> -QEMUOptionParameter *append_option_parameters(QEMUOptionParameter *dest,
>> - QEMUOptionParameter *list)
>> -{
>> - size_t num_options, num_dest_options;
>> -
>> - num_options = count_option_parameters(dest);
>> - num_dest_options = num_options;
>> -
>> - num_options += count_option_parameters(list);
>> -
>> - dest = g_realloc(dest, (num_options + 1) * sizeof(QEMUOptionParameter));
>> - dest[num_dest_options].name = NULL;
>> -
>> - while (list && list->name) {
>> - if (get_option_parameter(dest, list->name) == NULL) {
>> - dest[num_dest_options++] = *list;
>> - dest[num_dest_options].name = NULL;
>> - }
>> - list++;
>> - }
>> -
>> - return dest;
>> -}
>> -
>> -/*
>> - * Parses a parameter string (param) into an option list (dest).
>> - *
>> - * list is the template option list. If dest is NULL, a new copy of list is
>> - * created. If list is NULL, this function fails.
>> - *
>> - * A parameter string consists of one or more parameters, separated by
>> commas.
>> - * Each parameter consists of its name and possibly of a value. In the
>> latter
>> - * case, the value is delimited by an = character. To specify a value which
>> - * contains commas, double each comma so it won't be recognized as the end
>> of
>> - * the parameter.
>> - *
>> - * For more details of the parsing see above.
>> - *
>> - * Returns a pointer to the first element of dest (or the newly allocated
>> copy)
>> - * or NULL in error cases
>> - */
>> -QEMUOptionParameter *parse_option_parameters(const char *param,
>> - QEMUOptionParameter *list, QEMUOptionParameter *dest)
>> -{
>> - QEMUOptionParameter *allocated = NULL;
>> - char name[256];
>> - char value[256];
>> - char *param_delim, *value_delim;
>> - char next_delim;
>> -
>> - if (list == NULL) {
>> - return NULL;
>> - }
>> -
>> - if (dest == NULL) {
>> - dest = allocated = append_option_parameters(NULL, list);
>> - }
>> -
>> - while (*param) {
>> -
>> - // Find parameter name and value in the string
>> - param_delim = strchr(param, ',');
>> - value_delim = strchr(param, '=');
>> -
>> - if (value_delim && (value_delim < param_delim || !param_delim)) {
>> - next_delim = '=';
>> - } else {
>> - next_delim = ',';
>> - value_delim = NULL;
>> - }
>> -
>> - param = get_opt_name(name, sizeof(name), param, next_delim);
>> - if (value_delim) {
>> - param = get_opt_value(value, sizeof(value), param + 1);
>> - }
>> - if (*param != '\0') {
>> - param++;
>> - }
>> -
>> - // Set the parameter
>> - if (set_option_parameter(dest, name, value_delim ? value : NULL)) {
>> - goto fail;
>> - }
>> - }
>> -
>> - return dest;
>> -
>> -fail:
>> - // Only free the list if it was newly allocated
>> - free_option_parameters(allocated);
>> - return NULL;
>> -}
>> -
>> -/*
>> - * Prints all options of a list that have a value to stdout
>> - */
>> -void print_option_parameters(QEMUOptionParameter *list)
>> -{
>> - while (list && list->name) {
>> - switch (list->type) {
>> - case OPT_STRING:
>> - if (list->value.s != NULL) {
>> - printf("%s='%s' ", list->name, list->value.s);
>> - }
>> - break;
>> - case OPT_FLAG:
>> - printf("%s=%s ", list->name, list->value.n ? "on" : "off");
>> - break;
>> - case OPT_SIZE:
>> - case OPT_NUMBER:
>> - printf("%s=%" PRId64 " ", list->name, list->value.n);
>> - break;
>> - default:
>> - printf("%s=(unknown type) ", list->name);
>> - break;
>> - }
>> - list++;
>> - }
>> -}
>> -
>> -/*
>> - * Prints an overview of all available options
>> - */
>> -void print_option_help(QEMUOptionParameter *list)
>> -{
>> - printf("Supported options:\n");
>> - while (list && list->name) {
>> - printf("%-16s %s\n", list->name,
>> - list->help ? list->help : "No description available");
>> - list++;
>> - }
>> -}
>> -
>> /* ------------------------------------------------------------------ */
>>
>> static QemuOpt *qemu_opt_find(QemuOpts *opts, const char *name)
>> @@ -832,14 +547,36 @@ void qemu_opts_del(QemuOpts *opts)
>>
>> int qemu_opts_print(QemuOpts *opts, void *dummy)
>> {
>> - QemuOpt *opt;
>> + QemuOpt *opt = NULL;
>> + QemuOptDesc *desc = opts->list->desc;
>>
>> - fprintf(stderr, "%s: %s:", opts->list->name,
>> - opts->id ? opts->id : "<noid>");
>> - QTAILQ_FOREACH(opt, &opts->head, next) {
>> - fprintf(stderr, " %s=\"%s\"", opt->name, opt->str);
>> + while (desc && desc->name) {
>> + opt = qemu_opt_find(opts, desc->name);
>> + switch (desc->type) {
>> + case QEMU_OPT_STRING:
>> + if (opt != NULL) {
>> + printf("%s='%s' ", opt->name, opt->str);
>> + }
>> + break;
>> + case QEMU_OPT_BOOL:
>> + printf("%s=%s ", desc->name, (opt && opt->str) ? "on" : "off");
>> + break;
>> + case QEMU_OPT_NUMBER:
>> + case QEMU_OPT_SIZE:
>> + if (strcmp(desc->name, "cluster_size")) {
>> + printf("%s=%" PRId64 " ", desc->name,
>> + (opt && opt->value.uint) ? opt->value.uint : 0);
>> + } else {
>> + printf("%s=%" PRId64 " ", desc->name,
>> + (opt && opt->value.uint) ? opt->value.uint :
>> desc->def_value);
>> + }
>> + break;
>> + default:
>> + printf("%s=(unknown type) ", desc->name);
>> + break;
>> + }
>> + desc++;
>> }
>> - fprintf(stderr, "\n");
>> return 0;
>> }
>>
>
> Why do you need to change this function?
>
>> @@ -1110,3 +847,140 @@ 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;
>> +
>> + while (list && list->desc[i].name) {
>> + i++;
>> + }
>> +
>> + return i;
>> +}
>> +
>> +static bool opts_desc_find(QemuOptsList *list, const char *name)
>> +{
>> + const QemuOptDesc *desc = list->desc;
>> + int i;
>> +
>> + for (i = 0; desc[i].name != NULL; i++) {
>> + if (strcmp(desc[i].name, name) == 0) {
>> + return true;;
>
> Extra ;
>
>> + }
>> + }
>> + return false;
>> +}
>
> Duplicates the loop searching list->desc[] yet again. What about
> factoring out all the copies into a function? Separate cleanup patch
> preceding this one.
>
>> +
>> +/* merge two QemuOptsLists to one and return it. */
>> +QemuOptsList *append_opts_list(QemuOptsList *first,
>> + QemuOptsList *second)
>> +{
>> + size_t num_first_options, num_second_options;
>> + QemuOptsList *dest = NULL;
>> + int i = 0;
>> + int index = 0;
>> +
>> + num_first_options = count_opts_list(first);
>> + num_second_options = count_opts_list(second);
>> + if (num_first_options + num_second_options == 0) {
>> + return NULL;
>> + }
>> +
>> + dest = g_malloc0(sizeof(QemuOptsList)
>> + + (num_first_options + num_second_options) * sizeof(QemuOptDesc));
>
> Allocate space for both first and second.
>
>> +
>> + if (first) {
>> + memcpy(dest, first, sizeof(QemuOptsList));
>> + } else if (second) {
>> + memcpy(dest, second, sizeof(QemuOptsList));
>> + }
>
> Copy either first or second.
>
> If both are non-null, the space for second remains blank.
>
> Why copy at all? The loops below copy again, don't they?
>
>> +
>> + while (first && (first->desc[i].name)) {
>> + if (!opts_desc_find(dest, first->desc[i].name)) {
>> + dest->desc[index].name = strdup(first->desc[i].name);
>> + dest->desc[index].help = strdup(first->desc[i].help);
>
> g_strdup()
>
> Why do you need to copy name and help?
desc is a set of flexible array, I want to resize the array, so I have
to malloc a new block of address and copy the name and help field.
>
>> + dest->desc[index].type = first->desc[i].type;
>> + dest->desc[index].def_value = first->desc[i].def_value;
>> + dest->desc[++index].name = NULL;
>
> I'd terminate dest->desc[] after the loop, not in every iteration.
>
>> + }
>> + i++;
>> + }
>> + i = 0;
>> + while (second && (second->desc[i].name)) {
>> + if (!opts_desc_find(dest, second->desc[i].name)) {
>> + dest->desc[index].name = strdup(first->desc[i].name);
>> + dest->desc[index].help = strdup(first->desc[i].help);
>> + dest->desc[index].type = second->desc[i].type;
>> + dest->desc[index].def_value = second->desc[i].def_value;
>> + dest->desc[++i].name = NULL;
>> + }
>> + i++;
>> + }
>
> Please avoid duplicating the loop.
>
> What if first and second both contain the same parameter name?
>
Okay.
>> + return dest;
>> +}
>> +
>> +void free_opts_list(QemuOptsList *list)
>> +{
>> + int i = 0;
>> +
>> + while (list && list->desc[i].name) {
>> + g_free((char *)list->desc[i].name);
>> + g_free((char *)list->desc[i].help);
>> + i++;
>> + }
>> +
>> + g_free(list);
>> +}
>> +
>> +void print_opts_list(QemuOptsList *list)
>> +{
>> + int i = 0;
>> + printf("Supported options:\n");
>> + while (list && list->desc[i].name) {
>> + printf("%-16s %s\n", list->desc[i].name,
>> + list->desc[i].help ? list->desc[i].help : "No description
>> available");
>> + i++;
>> + }
>> +}
>> +
>> +QemuOpts *parse_opts_list(const char *param,
>> + QemuOptsList *list, QemuOpts *dest)
>> +{
>> + char name[256];
>> + char value[256];
>> + char *param_delim, *value_delim;
>> + char next_delim;
>> +
>> + if (list == NULL) {
>> + return NULL;
>> + }
>> + while (*param) {
>> + param_delim = strchr(param, ',');
>> + value_delim = strchr(param, '=');
>> +
>> + if (value_delim && (value_delim < param_delim || !param_delim)) {
>> + next_delim = '=';
>> + } else {
>> + next_delim = ',';
>> + value_delim = NULL;
>> + }
>> +
>> + param = get_opt_name(name, sizeof(name), param, next_delim);
>> + if (value_delim) {
>> + param = get_opt_value(value, sizeof(value), param + 1);
>> + }
>> + if (*param != '\0') {
>> + param++;
>> + }
>> +
>> + if (qemu_opt_set(dest, name, value_delim ? value : NULL)) {
>> + goto fail;
>> + }
>> + }
>> +
>> + return dest;
>> +
>> +fail:
>> + return NULL;
>> +}
>
> Can you explain why the existing QemuOpts parsers won't do?
>
>> diff --git a/qemu-option.h b/qemu-option.h
>> index ca72986..75718fe 100644
>> --- a/qemu-option.h
>> +++ b/qemu-option.h
>> @@ -31,24 +31,6 @@
>> #include "error.h"
>> #include "qdict.h"
>>
>> -enum QEMUOptionParType {
>> - OPT_FLAG,
>> - OPT_NUMBER,
>> - OPT_SIZE,
>> - OPT_STRING,
>> -};
>> -
>> -typedef struct QEMUOptionParameter {
>> - const char *name;
>> - enum QEMUOptionParType type;
>> - union {
>> - uint64_t n;
>> - char* s;
>> - } value;
>> - const char *help;
>> -} QEMUOptionParameter;
>> -
>> -
>> const char *get_opt_name(char *buf, int buf_size, const char *p, char
>> delim);
>> const char *get_opt_value(char *buf, int buf_size, const char *p);
>> int get_next_param_value(char *buf, int buf_size,
>> @@ -58,27 +40,6 @@ int get_param_value(char *buf, int buf_size,
>> int check_params(char *buf, int buf_size,
>> const char * const *params, const char *str);
>>
>> -
>> -/*
>> - * The following functions take a parameter list as input. This is a
>> pointer to
>> - * the first element of a QEMUOptionParameter array which is terminated by
>> an
>> - * entry with entry->name == NULL.
>> - */
>> -
>> -QEMUOptionParameter *get_option_parameter(QEMUOptionParameter *list,
>> - const char *name);
>> -int set_option_parameter(QEMUOptionParameter *list, const char *name,
>> - const char *value);
>> -int set_option_parameter_int(QEMUOptionParameter *list, const char *name,
>> - uint64_t value);
>> -QEMUOptionParameter *append_option_parameters(QEMUOptionParameter *dest,
>> - QEMUOptionParameter *list);
>> -QEMUOptionParameter *parse_option_parameters(const char *param,
>> - QEMUOptionParameter *list, QEMUOptionParameter *dest);
>> -void free_option_parameters(QEMUOptionParameter *list);
>> -void print_option_parameters(QEMUOptionParameter *list);
>> -void print_option_help(QEMUOptionParameter *list);
>> -
>> /* ------------------------------------------------------------------ */
>>
>> typedef struct QemuOpt QemuOpt;
>> @@ -96,6 +57,7 @@ typedef struct QemuOptDesc {
>> const char *name;
>> enum QemuOptType type;
>> const char *help;
>> + uint64_t def_value;
>
> The only user I can see is qemu_opts_print(), which can't be right.
>
>> } QemuOptDesc;
>>
>> struct QemuOptsList {
>> @@ -152,5 +114,10 @@ typedef int (*qemu_opts_loopfunc)(QemuOpts *opts, void
>> *opaque);
>> 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 *append_opts_list(QemuOptsList *dest,
>> + QemuOptsList *list);
>> +void free_opts_list(QemuOptsList *list);
>> +void print_opts_list(QemuOptsList *list);
>> +QemuOpts *parse_opts_list(const char *param,
>> + QemuOptsList *list, QemuOpts *dest);
>> #endif
>