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 <wdon...@linux.vnet.ibm.com> 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 <wdon...@linux.vnet.ibm.com>
> ---
> 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.
> 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.
>
> - 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 *.
> + 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.
> + 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?
> 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.
> {
> 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?
> + 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?
> + 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
