[Qemu-devel] [PATCH] vl: exit if a bad property value is passed to -global

[Qemu-devel] [PATCH] vl: exit if a bad property value is passed to -global

[Greg Kurz]

When passing '-global driver=host-powerpc64-cpu,property=compat,value=foo'
on the command line, without this patch, we get the following warning per
device (which means many lines if the guests has many cpus):

qemu-system-ppc64: Warning: can't apply global host-powerpc64-cpu.compat=foo:
    Invalid compatibility mode "foo"

... and QEMU continues execution, ignoring the property.

With this patch, we get a single line:

qemu-system-ppc64: can't apply global host-powerpc64-cpu.compat=foo:
    Invalid compatibility mode "foo"

... and QEMU exits.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 vl.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index 356713ea075c..a7cc92781687 100644
--- a/vl.c
+++ b/vl.c
@@ -2922,6 +2922,7 @@ static int global_init_func(void *opaque, QemuOpts *opts, Error **errp)
     g->property = qemu_opt_get(opts, "property");
     g->value    = qemu_opt_get(opts, "value");
     g->user_provided = true;
+    g->errp = errp;
     qdev_prop_register_global(g);
     return 0;
 }
@@ -4451,7 +4452,7 @@ int main(int argc, char **argv, char **envp)
     machine_register_compat_props(current_machine);
 
     qemu_opts_foreach(qemu_find_opts("global"),
-                      global_init_func, NULL, NULL);
+                      global_init_func, NULL, &error_fatal);
 
     /* This checkpoint is required by replay to separate prior clock
        reading from the other reads, because timer polling functions query

Re: [Qemu-devel] [PATCH] vl: exit if a bad property value is passed to -global

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2062 bytes --]

On Wed, Jul 13, 2016 at 08:05:03PM +0200, Greg Kurz wrote:
> When passing '-global driver=host-powerpc64-cpu,property=compat,value=foo'
> on the command line, without this patch, we get the following warning per
> device (which means many lines if the guests has many cpus):
> 
> qemu-system-ppc64: Warning: can't apply global host-powerpc64-cpu.compat=foo:
>     Invalid compatibility mode "foo"
> 
> ... and QEMU continues execution, ignoring the property.
> 
> With this patch, we get a single line:
> 
> qemu-system-ppc64: can't apply global host-powerpc64-cpu.compat=foo:
>     Invalid compatibility mode "foo"
> 
> ... and QEMU exits.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

Not really up to me to decide if this is the right approach for global
option handling, but it seems like the way to go to me.

> ---
>  vl.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/vl.c b/vl.c
> index 356713ea075c..a7cc92781687 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2922,6 +2922,7 @@ static int global_init_func(void *opaque, QemuOpts *opts, Error **errp)
>      g->property = qemu_opt_get(opts, "property");
>      g->value    = qemu_opt_get(opts, "value");
>      g->user_provided = true;
> +    g->errp = errp;
>      qdev_prop_register_global(g);
>      return 0;
>  }
> @@ -4451,7 +4452,7 @@ int main(int argc, char **argv, char **envp)
>      machine_register_compat_props(current_machine);
>  
>      qemu_opts_foreach(qemu_find_opts("global"),
> -                      global_init_func, NULL, NULL);
> +                      global_init_func, NULL, &error_fatal);
>  
>      /* This checkpoint is required by replay to separate prior clock
>         reading from the other reads, because timer polling functions query
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH] vl: exit if a bad property value is passed to -global

[Igor Mammedov]

On Thu, 14 Jul 2016 11:42:40 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Jul 13, 2016 at 08:05:03PM +0200, Greg Kurz wrote:
> > When passing '-global driver=host-powerpc64-cpu,property=compat,value=foo'
> > on the command line, without this patch, we get the following warning per
> > device (which means many lines if the guests has many cpus):
> > 
> > qemu-system-ppc64: Warning: can't apply global host-powerpc64-cpu.compat=foo:
> >     Invalid compatibility mode "foo"
> > 
> > ... and QEMU continues execution, ignoring the property.
> > 
> > With this patch, we get a single line:
> > 
> > qemu-system-ppc64: can't apply global host-powerpc64-cpu.compat=foo:
> >     Invalid compatibility mode "foo"
> > 
> > ... and QEMU exits.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>  
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Not really up to me to decide if this is the right approach for global
> option handling, but it seems like the way to go to me.
CCing Eduardo
 as he basically rewrote that path several times and might know why it's this way now.

> 
> > ---
> >  vl.c |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/vl.c b/vl.c
> > index 356713ea075c..a7cc92781687 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -2922,6 +2922,7 @@ static int global_init_func(void *opaque, QemuOpts *opts, Error **errp)
> >      g->property = qemu_opt_get(opts, "property");
> >      g->value    = qemu_opt_get(opts, "value");
> >      g->user_provided = true;
> > +    g->errp = errp;
> >      qdev_prop_register_global(g);
> >      return 0;
> >  }
> > @@ -4451,7 +4452,7 @@ int main(int argc, char **argv, char **envp)
> >      machine_register_compat_props(current_machine);
> >  
> >      qemu_opts_foreach(qemu_find_opts("global"),
> > -                      global_init_func, NULL, NULL);
> > +                      global_init_func, NULL, &error_fatal);
> >  
> >      /* This checkpoint is required by replay to separate prior clock
> >         reading from the other reads, because timer polling functions query
> >   
> 

Re: [Qemu-devel] [PATCH] vl: exit if a bad property value is passed to -global

[Eduardo Habkost]

Sorry for the long delay:

On Thu, Jul 14, 2016 at 10:29:57AM +0200, Igor Mammedov wrote:
> On Thu, 14 Jul 2016 11:42:40 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Wed, Jul 13, 2016 at 08:05:03PM +0200, Greg Kurz wrote:
> > > When passing '-global driver=host-powerpc64-cpu,property=compat,value=foo'
> > > on the command line, without this patch, we get the following warning per
> > > device (which means many lines if the guests has many cpus):
> > > 
> > > qemu-system-ppc64: Warning: can't apply global host-powerpc64-cpu.compat=foo:
> > >     Invalid compatibility mode "foo"
> > > 
> > > ... and QEMU continues execution, ignoring the property.
> > > 
> > > With this patch, we get a single line:
> > > 
> > > qemu-system-ppc64: can't apply global host-powerpc64-cpu.compat=foo:
> > >     Invalid compatibility mode "foo"
> > > 
> > > ... and QEMU exits.
> > > 
> > > Signed-off-by: Greg Kurz <groug@kaod.org>  
> > 
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > 
> > Not really up to me to decide if this is the right approach for global
> > option handling, but it seems like the way to go to me.
> CCing Eduardo
>  as he basically rewrote that path several times and might know why it's this way now.
> 
> > 
> > > ---
> > >  vl.c |    3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/vl.c b/vl.c
> > > index 356713ea075c..a7cc92781687 100644
> > > --- a/vl.c
> > > +++ b/vl.c
> > > @@ -2922,6 +2922,7 @@ static int global_init_func(void *opaque, QemuOpts *opts, Error **errp)
> > >      g->property = qemu_opt_get(opts, "property");
> > >      g->value    = qemu_opt_get(opts, "value");
> > >      g->user_provided = true;
> > > +    g->errp = errp;

Exiting QEMU like you suggest is valid, but we can't set errp to
error_fatal here, because it could make QEMU abort when using
device_add.

Probably the simplest solution is to ignore GlobalProperty.errp
in qdev_prop_set_globals_for_type() if dev->hotplugged is set.
Then we can safely set errp here.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] vl: exit if a bad property value is passed to -global

[Greg Kurz]

On Wed, 20 Jul 2016 17:18:26 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> Sorry for the long delay:
> 
> On Thu, Jul 14, 2016 at 10:29:57AM +0200, Igor Mammedov wrote:
> > On Thu, 14 Jul 2016 11:42:40 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Wed, Jul 13, 2016 at 08:05:03PM +0200, Greg Kurz wrote:  
> > > > When passing '-global driver=host-powerpc64-cpu,property=compat,value=foo'
> > > > on the command line, without this patch, we get the following warning per
> > > > device (which means many lines if the guests has many cpus):
> > > > 
> > > > qemu-system-ppc64: Warning: can't apply global host-powerpc64-cpu.compat=foo:
> > > >     Invalid compatibility mode "foo"
> > > > 
> > > > ... and QEMU continues execution, ignoring the property.
> > > > 
> > > > With this patch, we get a single line:
> > > > 
> > > > qemu-system-ppc64: can't apply global host-powerpc64-cpu.compat=foo:
> > > >     Invalid compatibility mode "foo"
> > > > 
> > > > ... and QEMU exits.
> > > > 
> > > > Signed-off-by: Greg Kurz <groug@kaod.org>    
> > > 
> > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > > 
> > > Not really up to me to decide if this is the right approach for global
> > > option handling, but it seems like the way to go to me.  
> > CCing Eduardo
> >  as he basically rewrote that path several times and might know why it's this way now.
> >   
> > >   
> > > > ---
> > > >  vl.c |    3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/vl.c b/vl.c
> > > > index 356713ea075c..a7cc92781687 100644
> > > > --- a/vl.c
> > > > +++ b/vl.c
> > > > @@ -2922,6 +2922,7 @@ static int global_init_func(void *opaque, QemuOpts *opts, Error **errp)
> > > >      g->property = qemu_opt_get(opts, "property");
> > > >      g->value    = qemu_opt_get(opts, "value");
> > > >      g->user_provided = true;
> > > > +    g->errp = errp;  
> 
> Exiting QEMU like you suggest is valid, but we can't set errp to
> error_fatal here, because it could make QEMU abort when using
> device_add.
> 

Indeed QEMU exits :\

> Probably the simplest solution is to ignore GlobalProperty.errp
> in qdev_prop_set_globals_for_type() if dev->hotplugged is set.
> Then we can safely set errp here.
> 

That should do the trick. I'll send a v2.

Thanks !

--
Greg