Re: [Qemu-trivial] [Qemu-devel] [PATCH] Fix 'name' option to work with -readconfig

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > The 'name' option silently failed when used in config files
> > ( http://lists.gnu.org/archive/html/qemu-devel/2014-04/msg00378.html )
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Reported-by: William Dauchy <wdauchy@gmail.com>
> > ---
> >  vl.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/vl.c b/vl.c
> > index 8411a4a..47c199a 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -965,7 +965,7 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
> >      return 0;
> >  }
> >  
> > -static void parse_name(QemuOpts *opts)
> > +static int parse_name(QemuOpts *opts, void *opaque)
> >  {
> >      const char *proc_name;
> >  
> > @@ -978,6 +978,8 @@ static void parse_name(QemuOpts *opts)
> >      if (proc_name) {
> >          os_set_proc_name(proc_name);
> >      }
> > +
> > +    return 0;
> >  }
> >  
> >  bool usb_enabled(bool default_usb)
> > @@ -3780,7 +3782,6 @@ int main(int argc, char **argv, char **envp)
> >                  if (!opts) {
> >                      exit(1);
> >                  }
> > -                parse_name(opts);
> >                  break;
> >              case QEMU_OPTION_prom_env:
> >                  if (nb_prom_envs >= MAX_PROM_ENVS) {
> > @@ -3955,6 +3956,10 @@ int main(int argc, char **argv, char **envp)
> >          exit(1);
> >      }
> >  
> > +    if (qemu_opts_foreach(qemu_find_opts("name"), parse_name, NULL, 1)) {
> > +        exit(1);
> > +    }
> > +
> 
> This will never exit, but that's okay.

Ah, because my parse_name currently never fails?

> >  #ifndef _WIN32
> >      if (qemu_opts_foreach(qemu_find_opts("add-fd"), parse_add_fd, NULL, 1)) {
> >          exit(1);
> 
> -readconfig stores the configuration read in QemuOpts.  Command line
> option parsing should do the same, and no more.  In particular it should
> not act upon the option.  That needs to be done separately, where both
> command line and -readconfig settings are visible in QemuOpts.
> 
> Your patch does exactly that.  I think amending the commit message with
> the previous paragraph would improve it.
> 
> Have you checked command line option parsing (the big switch) for
> similar problems?

I hadn't, other than fixing up -name; tbh It's taken me a while to understand
how QemuOpts is supposed to work (and hence why I didn't get this in the 1st
patch); I'd seen the qemu_opts_foreach uses at the bottom of the switch, but
since they looked like a loop, I'd assumed they were only for options with
multiple sets of values and not looked any further.

The big switch seems to be a mix of a lot of different ways of doing things;
A quick scan shows rtc, option_rom, usb_device, and others all use qemu_opts_parse
in the big switch but also taking an action in the switch.

> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks.

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK