From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:34778) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TBFjk-0006Ds-Hk for qemu-devel@nongnu.org; Mon, 10 Sep 2012 21:58:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TBFjf-0000mW-6w for qemu-devel@nongnu.org; Mon, 10 Sep 2012 21:58:00 -0400 Received: from mail-wi0-f175.google.com ([209.85.212.175]:57320) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TBFje-0000mJ-Kx for qemu-devel@nongnu.org; Mon, 10 Sep 2012 21:57:55 -0400 Received: by wibhm2 with SMTP id hm2so2217358wib.10 for ; Mon, 10 Sep 2012 18:57:53 -0700 (PDT) MIME-Version: 1.0 Sender: donald.open@gmail.com In-Reply-To: <20120910143821.143c6f48@doriath.home> References: <1346901211-8314-1-git-send-email-wdongxu@linux.vnet.ibm.com> <87392u8brf.fsf@blackfin.pond.sub.org> <20120910143821.143c6f48@doriath.home> From: Dong Xu Wang Date: Tue, 11 Sep 2012 09:57:12 +0800 Message-ID: Content-Type: text/plain; charset=ISO-8859-1 Subject: Re: [Qemu-devel] [PATCH RFC] remove QEMUOptionParameter List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: kwolf@redhat.com, Markus Armbruster , qemu-devel@nongnu.org On Tue, Sep 11, 2012 at 1:38 AM, Luiz Capitulino wrote: > On Fri, 07 Sep 2012 10:42:28 +0200 > Markus Armbruster 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 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 >> > --- >> > 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? > > I'm not going to respin the whole series, but the patch you mention > and the following two seem worth it to have on master. > > Dong, do you want me to respin or can you add them to this series? I can add them to this series, thank you Luiz. > >> >> > + 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 : ""); >> > - 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 >> > >