comments below
On 02/06/14 09:16, Igor Mammedov wrote:
> From: Paolo Bonzini <pbonz...@redhat.com>
>
> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> Signed-off-by: Igor Mammedov <imamm...@redhat.com>
> ---
> include/qemu/config-file.h | 2 ++
> util/qemu-config.c | 14 ++++++++++++++
> vl.c | 11 +----------
> 3 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/include/qemu/config-file.h b/include/qemu/config-file.h
> index dbd97c4..d4ba20e 100644
> --- a/include/qemu/config-file.h
> +++ b/include/qemu/config-file.h
> @@ -8,6 +8,8 @@
>
> QemuOptsList *qemu_find_opts(const char *group);
> QemuOptsList *qemu_find_opts_err(const char *group, Error **errp);
> +QemuOpts *qemu_find_opts_singleton(const char *group);
> +
> void qemu_add_opts(QemuOptsList *list);
> void qemu_add_drive_opts(QemuOptsList *list);
> int qemu_set_option(const char *str);
> diff --git a/util/qemu-config.c b/util/qemu-config.c
> index 9298f55..9dfacbc 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -39,6 +39,20 @@ QemuOptsList *qemu_find_opts(const char *group)
> return ret;
> }
>
> +QemuOpts *qemu_find_opts_singleton(const char *group)
> +{
> + QemuOptsList *list;
> + QemuOpts *opts;
> +
> + list = qemu_find_opts(group);
> + assert(list);
> + opts = qemu_opts_find(list, NULL);
> + if (!opts) {
> + opts = qemu_opts_create(list, NULL, 0, &error_abort);
> + }
> + return opts;
> +}
First of all, the commit message body is empty, and the subject line
doesn't really say much, plus there are no comments, so I have no clue
what we're trying to implement here *in general*.
For example, if you look into qemu_opts_create(), you'll see that when
(id==NULL), then you don't actually need the
qemu_opts_find(list, NULL)
call, because qemu_opts_create() calls that itself anyway. Of course,
this behavior *also* depends on merge_lists being "true", but in case of
"machine", that *is* a given.
Hence a specification for the function would help deciding if we can
take merge_lists for granted, or if we want something more general. In
the former case, the code above is unnecessarily pessimistic.
(Basically the qemu_find_opts_singleton() should be a *very* thin
wrapper around qemu_opts_create(), because the latter already has this
"O_CREAT without O_EXCL" semantics.)
> +
> static CommandLineParameterInfoList *query_option_descs(const QemuOptDesc
> *desc)
> {
> CommandLineParameterInfoList *param_list = NULL, *entry;
> diff --git a/vl.c b/vl.c
> index 383be1b..7f2595c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -539,16 +539,7 @@ static QemuOptsList qemu_msg_opts = {
> */
> QemuOpts *qemu_get_machine_opts(void)
> {
> - QemuOptsList *list;
> - QemuOpts *opts;
> -
> - list = qemu_find_opts("machine");
> - assert(list);
> - opts = qemu_opts_find(list, NULL);
> - if (!opts) {
> - opts = qemu_opts_create(list, NULL, 0, &error_abort);
> - }
> - return opts;
> + return qemu_find_opts_singleton("machine");
> }
>
> const char *qemu_get_vm_name(void)
>
I also kinda hate that "error_abort" -- while used in several
option-specific parser functions (balloon_parse, virtcon_parse,
QEMU_OPTION_virtfs, QEMU_OPTION_virtfs_synth) -- now makes it into a
generic-looking options function. qemu_find_opts_singleton() should take
an errp.
Anyway I don't care enough to insist on a respin.
Reviewed-by: Laszlo Ersek <ler...@redhat.com>
