| Message ID | 146843310345.22044.7846135644864365198.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
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 >
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 > > >
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.
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
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
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(-)