diff mbox

Fix 'name' option to work with -readconfig

Message ID 1399374955-29082-1-git-send-email-dgilbert@redhat.com
State New
Headers show

Commit Message

Dr. David Alan Gilbert May 6, 2014, 11:15 a.m. UTC 
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(-)

Comments

William Dauchy May 6, 2014, 2:13 p.m. UTC | #1 
On Tue, May 6, 2014 at 1:15 PM, Dr. David Alan Gilbert (git)
<dgilbert@redhat.com> wrote:
> 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>

Please use:

Reported-by: William Dauchy <william@gandi.net>

I have also successfully tested the patch
Tested-by: William Dauchy <william@gandi.net>

Thanks,
Markus Armbruster May 14, 2014, 9:20 a.m. UTC | #2 
"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.

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

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Dr. David Alan Gilbert May 14, 2014, 9:34 a.m. UTC | #3 
* 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
Markus Armbruster May 14, 2014, 11:02 a.m. UTC | #4 
"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * 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?

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 <armbru@redhat.com>
>
> Thanks.

My pleasure :)
Michael Tokarev May 23, 2014, 8:43 p.m. UTC | #5 
06.05.2014 15:15, Dr. David Alan Gilbert (git) wrote:
> 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>

Applied to -trivial, after:

 1. Changing Reported/Tested-by as asked by William Dauchy
 2. Adding a subject prefix (vl)
 3. Adding commit message as suggested by Marcus
 4. Adding Marcus's Reviewed-by.

Oh ma... ;)

Thank you!  And please excuse me for it took so long.  It'd be
faster if I hadn't do all th e above ;)

/mjt
Dr. David Alan Gilbert May 27, 2014, 7:51 a.m. UTC | #6 
* Michael Tokarev (mjt@tls.msk.ru) wrote:
> 06.05.2014 15:15, Dr. David Alan Gilbert (git) wrote:
> > 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>
> 
> Applied to -trivial, after:
> 
>  1. Changing Reported/Tested-by as asked by William Dauchy
>  2. Adding a subject prefix (vl)
>  3. Adding commit message as suggested by Marcus
>  4. Adding Marcus's Reviewed-by.
> 
> Oh ma... ;)
> 
> Thank you!  And please excuse me for it took so long.  It'd be
> faster if I hadn't do all th e above ;)

Thanks, and apologies for not fixing those up.

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

Patch

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);
+    }
+
 #ifndef _WIN32
     if (qemu_opts_foreach(qemu_find_opts("add-fd"), parse_add_fd, NULL, 1)) {
         exit(1);