From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54857) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WCuE0-0007eS-Tt for qemu-devel@nongnu.org; Mon, 10 Feb 2014 12:01:01 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WCuDu-0002vG-RC for qemu-devel@nongnu.org; Mon, 10 Feb 2014 12:00:52 -0500 Received: from mx1.redhat.com ([209.132.183.28]:33360) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WCuDu-0002v6-Ai for qemu-devel@nongnu.org; Mon, 10 Feb 2014 12:00:46 -0500 Message-ID: <52F905B7.5030207@redhat.com> Date: Mon, 10 Feb 2014 18:00:39 +0100 From: Laszlo Ersek MIME-Version: 1.0 References: <1391674564-3783-1-git-send-email-imammedo@redhat.com> <1391674564-3783-2-git-send-email-imammedo@redhat.com> In-Reply-To: <1391674564-3783-2-git-send-email-imammedo@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/2] QemuOpts: introduce qemu_find_opts_singleton List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: kwolf@redhat.com, akong@redhat.com, stefanha@redhat.com, armbru@redhat.com, mjt@tls.msk.ru, qemu-devel@nongnu.org, mreitz@redhat.com, aliguori@amazon.com, pbonzini@redhat.com, lcapitulino@redhat.com comments below On 02/06/14 09:16, Igor Mammedov wrote: > From: Paolo Bonzini > > Signed-off-by: Paolo Bonzini > Signed-off-by: Igor Mammedov > --- > 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