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 <address@hidden> 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 <address@hidden>  
> > 
> > Reviewed-by: David Gibson <address@hidden>
> > 
> > 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