Re: [Qemu-devel] [PATCH 1/2] QemuOpts: introduce qemu_find_opts_singleton

[Igor Mammedov]

On Mon, 10 Feb 2014 18:00:39 +0100
Laszlo Ersek <address@hidden> wrote:

> comments below
> 
> On 02/06/14 09:16, Igor Mammedov wrote:
> > From: Paolo Bonzini <address@hidden>
> > 
> > Signed-off-by: Paolo Bonzini <address@hidden>
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> >  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 <address@hidden>
>