From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50419) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WDBt4-0005Z7-3T for qemu-devel@nongnu.org; Tue, 11 Feb 2014 06:52:32 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WDBsy-0007nr-3R for qemu-devel@nongnu.org; Tue, 11 Feb 2014 06:52:26 -0500 Received: from mx1.redhat.com ([209.132.183.28]:19437) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WDBsx-0007nj-RT for qemu-devel@nongnu.org; Tue, 11 Feb 2014 06:52:20 -0500 Date: Tue, 11 Feb 2014 12:52:12 +0100 From: Igor Mammedov Message-ID: <20140211125212.2c296aaf@nial.usersys.redhat.com> In-Reply-To: <52F905B7.5030207@redhat.com> References: <1391674564-3783-1-git-send-email-imammedo@redhat.com> <1391674564-3783-2-git-send-email-imammedo@redhat.com> <52F905B7.5030207@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII 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: Laszlo Ersek 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 On Mon, 10 Feb 2014 18:00:39 +0100 Laszlo Ersek wrote: > 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.) I'd guess function qemu_find_opts_singleton() is called so to express common idiom and shouldn't have an expectation for merge_lists == true being granted. So it could be reused with other options as well. > > > + > > 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. looking at possible call paths qemu_opts_create() with given arguments should never fail so assert like behavior here seems to be just fine, so we would notice for certain if/when it breaks. > > Anyway I don't care enough to insist on a respin. > > Reviewed-by: Laszlo Ersek >