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

[Markus Armbruster]

"Dr. David Alan Gilbert" <address@hidden> writes:

> * Markus Armbruster (address@hidden) wrote:
>> "Dr. David Alan Gilbert (git)" <address@hidden> writes:
>> 
>> > From: "Dr. David Alan Gilbert" <address@hidden>
>> >
>> > 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 <address@hidden>
>> > Reported-by: William Dauchy <address@hidden>
>> > ---
>> >  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?

Yes.  But that's a non-local argument, and handling the impossible
failure here doesn't hurt.

>> >  #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.

Okay.  Looks like we better audit this some time.

>> Reviewed-by: Markus Armbruster <address@hidden>
>
> Thanks.

My pleasure :)