[Qemu-devel] [PATCH v3 0/2] improve error handling of global properties

[Qemu-devel] [PATCH v3 0/2] improve error handling of global properties

[Greg Kurz]

As suggested by Eduardo, this series split the error handling of global
properties in two separate patches.

---

Greg Kurz (2):
      qdev: ignore GlobalProperty.errp for hotplugged devices
      vl: exit if a bad property value is passed to -global


 hw/core/qdev-properties.c |    4 ++--
 include/hw/qdev-core.h    |    4 +++-
 vl.c                      |    1 +
 3 files changed, 6 insertions(+), 3 deletions(-)

--
Greg

[Qemu-devel] [PATCH v3 1/2] qdev: ignore GlobalProperty.errp for hotplugged devices

[Greg Kurz]

This patch ensures QEMU won't terminate while hotplugging a device if the
global property cannot be set and errp points to error_fatal or error_abort.

While here, it also fixes indentation of the typename argument.

Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/core/qdev-properties.c |    4 ++--
 include/hw/qdev-core.h    |    4 +++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 14e544ab17d2..311af6da7684 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1084,7 +1084,7 @@ int qdev_prop_check_globals(void)
 }
 
 static void qdev_prop_set_globals_for_type(DeviceState *dev,
-                                const char *typename)
+                                           const char *typename)
 {
     GList *l;
 
@@ -1100,7 +1100,7 @@ static void qdev_prop_set_globals_for_type(DeviceState *dev,
         if (err != NULL) {
             error_prepend(&err, "can't apply global %s.%s=%s: ",
                           prop->driver, prop->property, prop->value);
-            if (prop->errp) {
+            if (!dev->hotplugged && prop->errp) {
                 error_propagate(prop->errp, err);
             } else {
                 assert(prop->user_provided);
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 1d1f8612a9b8..4b4b33bec885 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -261,7 +261,9 @@ struct PropertyInfo {
  * @used: Set to true if property was used when initializing a device.
  * @errp: Error destination, used like first argument of error_setg()
  *        in case property setting fails later. If @errp is NULL, we
- *        print warnings instead of ignoring errors silently.
+ *        print warnings instead of ignoring errors silently. For
+ *        hotplugged devices, errp is always ignored and warnings are
+ *        printed instead.
  */
 typedef struct GlobalProperty {
     const char *driver;

Re: [Qemu-devel] [PATCH v3 1/2] qdev: ignore GlobalProperty.errp for hotplugged devices

[David Gibson]

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

On Fri, Jul 22, 2016 at 01:01:26AM +0200, Greg Kurz wrote:
> This patch ensures QEMU won't terminate while hotplugging a device if the
> global property cannot be set and errp points to error_fatal or error_abort.
> 
> While here, it also fixes indentation of the typename argument.
> 
> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Greg Kurz <groug@kaod.org>

This seems kind of bogus to me - we have this whole infrastructure for
handling errors, and here we throw it away.

It seems like the right solution would be to make the caller in the
hotplug case *not* use error_abort or error_fatal, and instead get the
error propagated back to the monitor which will display it.

> ---
>  hw/core/qdev-properties.c |    4 ++--
>  include/hw/qdev-core.h    |    4 +++-
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 14e544ab17d2..311af6da7684 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1084,7 +1084,7 @@ int qdev_prop_check_globals(void)
>  }
>  
>  static void qdev_prop_set_globals_for_type(DeviceState *dev,
> -                                const char *typename)
> +                                           const char *typename)
>  {
>      GList *l;
>  
> @@ -1100,7 +1100,7 @@ static void qdev_prop_set_globals_for_type(DeviceState *dev,
>          if (err != NULL) {
>              error_prepend(&err, "can't apply global %s.%s=%s: ",
>                            prop->driver, prop->property, prop->value);
> -            if (prop->errp) {
> +            if (!dev->hotplugged && prop->errp) {
>                  error_propagate(prop->errp, err);
>              } else {
>                  assert(prop->user_provided);
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 1d1f8612a9b8..4b4b33bec885 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -261,7 +261,9 @@ struct PropertyInfo {
>   * @used: Set to true if property was used when initializing a device.
>   * @errp: Error destination, used like first argument of error_setg()
>   *        in case property setting fails later. If @errp is NULL, we
> - *        print warnings instead of ignoring errors silently.
> + *        print warnings instead of ignoring errors silently. For
> + *        hotplugged devices, errp is always ignored and warnings are
> + *        printed instead.
>   */
>  typedef struct GlobalProperty {
>      const char *driver;
> 

-- 
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 v3 1/2] qdev: ignore GlobalProperty.errp for hotplugged devices

[Greg Kurz]

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

On Fri, 22 Jul 2016 11:28:48 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Jul 22, 2016 at 01:01:26AM +0200, Greg Kurz wrote:
> > This patch ensures QEMU won't terminate while hotplugging a device if the
> > global property cannot be set and errp points to error_fatal or error_abort.
> > 
> > While here, it also fixes indentation of the typename argument.
> > 
> > Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> > Signed-off-by: Greg Kurz <groug@kaod.org>  
> 
> This seems kind of bogus to me - we have this whole infrastructure for
> handling errors, and here we throw it away.
> 
> It seems like the right solution would be to make the caller in the
> hotplug case *not* use error_abort or error_fatal, and instead get the
> error propagated back to the monitor which will display it.
> 

The caller is QOM initialization here. Are you asking to add an errp argument
to object_initialize() and friends ?

> > ---
> >  hw/core/qdev-properties.c |    4 ++--
> >  include/hw/qdev-core.h    |    4 +++-
> >  2 files changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> > index 14e544ab17d2..311af6da7684 100644
> > --- a/hw/core/qdev-properties.c
> > +++ b/hw/core/qdev-properties.c
> > @@ -1084,7 +1084,7 @@ int qdev_prop_check_globals(void)
> >  }
> >  
> >  static void qdev_prop_set_globals_for_type(DeviceState *dev,
> > -                                const char *typename)
> > +                                           const char *typename)
> >  {
> >      GList *l;
> >  
> > @@ -1100,7 +1100,7 @@ static void qdev_prop_set_globals_for_type(DeviceState *dev,
> >          if (err != NULL) {
> >              error_prepend(&err, "can't apply global %s.%s=%s: ",
> >                            prop->driver, prop->property, prop->value);
> > -            if (prop->errp) {
> > +            if (!dev->hotplugged && prop->errp) {
> >                  error_propagate(prop->errp, err);
> >              } else {
> >                  assert(prop->user_provided);
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index 1d1f8612a9b8..4b4b33bec885 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -261,7 +261,9 @@ struct PropertyInfo {
> >   * @used: Set to true if property was used when initializing a device.
> >   * @errp: Error destination, used like first argument of error_setg()
> >   *        in case property setting fails later. If @errp is NULL, we
> > - *        print warnings instead of ignoring errors silently.
> > + *        print warnings instead of ignoring errors silently. For
> > + *        hotplugged devices, errp is always ignored and warnings are
> > + *        printed instead.
> >   */
> >  typedef struct GlobalProperty {
> >      const char *driver;
> >   
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH v3 1/2] qdev: ignore GlobalProperty.errp for hotplugged devices

[David Gibson]

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

On Fri, Jul 22, 2016 at 09:16:57AM +0200, Greg Kurz wrote:
> On Fri, 22 Jul 2016 11:28:48 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Fri, Jul 22, 2016 at 01:01:26AM +0200, Greg Kurz wrote:
> > > This patch ensures QEMU won't terminate while hotplugging a device if the
> > > global property cannot be set and errp points to error_fatal or error_abort.
> > > 
> > > While here, it also fixes indentation of the typename argument.
> > > 
> > > Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> > > Signed-off-by: Greg Kurz <groug@kaod.org>  
> > 
> > This seems kind of bogus to me - we have this whole infrastructure for
> > handling errors, and here we throw it away.
> > 
> > It seems like the right solution would be to make the caller in the
> > hotplug case *not* use error_abort or error_fatal, and instead get the
> > error propagated back to the monitor which will display it.
> 
> The caller is QOM initialization here. Are you asking to add an errp argument
> to object_initialize() and friends ?

Ugh.  I guess I am.  I can see why you'd want to avoid that.  On the
other hand, it really does seem like the right approach.

> 
> > > ---
> > >  hw/core/qdev-properties.c |    4 ++--
> > >  include/hw/qdev-core.h    |    4 +++-
> > >  2 files changed, 5 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> > > index 14e544ab17d2..311af6da7684 100644
> > > --- a/hw/core/qdev-properties.c
> > > +++ b/hw/core/qdev-properties.c
> > > @@ -1084,7 +1084,7 @@ int qdev_prop_check_globals(void)
> > >  }
> > >  
> > >  static void qdev_prop_set_globals_for_type(DeviceState *dev,
> > > -                                const char *typename)
> > > +                                           const char *typename)
> > >  {
> > >      GList *l;
> > >  
> > > @@ -1100,7 +1100,7 @@ static void qdev_prop_set_globals_for_type(DeviceState *dev,
> > >          if (err != NULL) {
> > >              error_prepend(&err, "can't apply global %s.%s=%s: ",
> > >                            prop->driver, prop->property, prop->value);
> > > -            if (prop->errp) {
> > > +            if (!dev->hotplugged && prop->errp) {
> > >                  error_propagate(prop->errp, err);
> > >              } else {
> > >                  assert(prop->user_provided);
> > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > > index 1d1f8612a9b8..4b4b33bec885 100644
> > > --- a/include/hw/qdev-core.h
> > > +++ b/include/hw/qdev-core.h
> > > @@ -261,7 +261,9 @@ struct PropertyInfo {
> > >   * @used: Set to true if property was used when initializing a device.
> > >   * @errp: Error destination, used like first argument of error_setg()
> > >   *        in case property setting fails later. If @errp is NULL, we
> > > - *        print warnings instead of ignoring errors silently.
> > > + *        print warnings instead of ignoring errors silently. For
> > > + *        hotplugged devices, errp is always ignored and warnings are
> > > + *        printed instead.
> > >   */
> > >  typedef struct GlobalProperty {
> > >      const char *driver;
> > >   
> > 
> 



-- 
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 v3 1/2] qdev: ignore GlobalProperty.errp for hotplugged devices

[Eduardo Habkost]

On Fri, Jul 22, 2016 at 11:28:48AM +1000, David Gibson wrote:
> On Fri, Jul 22, 2016 at 01:01:26AM +0200, Greg Kurz wrote:
> > This patch ensures QEMU won't terminate while hotplugging a device if the
> > global property cannot be set and errp points to error_fatal or error_abort.
> > 
> > While here, it also fixes indentation of the typename argument.
> > 
> > Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> 
> This seems kind of bogus to me - we have this whole infrastructure for
> handling errors, and here we throw it away.

What is this patch throwing away? We have never been able to use
the error infrastructure properly while applying global
properties.

> 
> It seems like the right solution would be to make the caller in the
> hotplug case *not* use error_abort or error_fatal, and instead get the
> error propagated back to the monitor which will display it.

GlobalProperty::errp is a workaround to the fact that
ObjectClass::instance_post_init() can't report errors at all (and
that's because object_new() and object_initialize_with_type()
can't report errors. Do you have any suggestions to fix it?

I have suggested saving global property errors in a DeviceState
field and reporting then later on device_realize(). Maybe I
should implement it and send as RFC.

> 
> > ---
> >  hw/core/qdev-properties.c |    4 ++--
> >  include/hw/qdev-core.h    |    4 +++-
> >  2 files changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> > index 14e544ab17d2..311af6da7684 100644
> > --- a/hw/core/qdev-properties.c
> > +++ b/hw/core/qdev-properties.c
> > @@ -1084,7 +1084,7 @@ int qdev_prop_check_globals(void)
> >  }
> >  
> >  static void qdev_prop_set_globals_for_type(DeviceState *dev,
> > -                                const char *typename)
> > +                                           const char *typename)
> >  {
> >      GList *l;
> >  
> > @@ -1100,7 +1100,7 @@ static void qdev_prop_set_globals_for_type(DeviceState *dev,
> >          if (err != NULL) {
> >              error_prepend(&err, "can't apply global %s.%s=%s: ",
> >                            prop->driver, prop->property, prop->value);
> > -            if (prop->errp) {
> > +            if (!dev->hotplugged && prop->errp) {
> >                  error_propagate(prop->errp, err);
> >              } else {
> >                  assert(prop->user_provided);
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index 1d1f8612a9b8..4b4b33bec885 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -261,7 +261,9 @@ struct PropertyInfo {
> >   * @used: Set to true if property was used when initializing a device.
> >   * @errp: Error destination, used like first argument of error_setg()
> >   *        in case property setting fails later. If @errp is NULL, we
> > - *        print warnings instead of ignoring errors silently.
> > + *        print warnings instead of ignoring errors silently. For
> > + *        hotplugged devices, errp is always ignored and warnings are
> > + *        printed instead.
> >   */
> >  typedef struct GlobalProperty {
> >      const char *driver;
> > 
> 
> -- 
> 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



-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 1/2] qdev: ignore GlobalProperty.errp for hotplugged devices

[David Gibson]

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

On Fri, Jul 22, 2016 at 10:56:31AM -0300, Eduardo Habkost wrote:
> On Fri, Jul 22, 2016 at 11:28:48AM +1000, David Gibson wrote:
> > On Fri, Jul 22, 2016 at 01:01:26AM +0200, Greg Kurz wrote:
> > > This patch ensures QEMU won't terminate while hotplugging a device if the
> > > global property cannot be set and errp points to error_fatal or error_abort.
> > > 
> > > While here, it also fixes indentation of the typename argument.
> > > 
> > > Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > 
> > This seems kind of bogus to me - we have this whole infrastructure for
> > handling errors, and here we throw it away.
> 
> What is this patch throwing away? We have never been able to use
> the error infrastructure properly while applying global
> properties.

"throwing away" was a bit too strong.

But, it seems a shame that we have this error infrastructure which
supposedly let's you report errors in a consistent way whether they be
fatal or non-fatal, but here we're not able to use it to report a
non-fatal error.

> > It seems like the right solution would be to make the caller in the
> > hotplug case *not* use error_abort or error_fatal, and instead get the
> > error propagated back to the monitor which will display it.
> 
> GlobalProperty::errp is a workaround to the fact that
> ObjectClass::instance_post_init() can't report errors at all (and
> that's because object_new() and object_initialize_with_type()
> can't report errors. Do you have any suggestions to fix it?

Is there an inherent reason object_initialize() and object_new() can't
report errors?  Or just that it hasn't been implemented yet?

> I have suggested saving global property errors in a DeviceState
> field and reporting then later on device_realize(). Maybe I
> should implement it and send as RFC.

Maybe.

In any case my initial objection was because I hadn't realized the
difficulty of implementing this in the error API, so I withdraw it.

-- 
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 --]

[Qemu-devel] [PATCH v3 2/2] 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.

The previous behavior is kept for hotplugged devices since we don't want
QEMU to exit when doing device_add.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
v3: - set directly the global property errp to &error_fatal
---
 vl.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/vl.c b/vl.c
index a455947b4f32..e7c2c628de29 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 = &error_fatal;
     qdev_prop_register_global(g);
     return 0;
 }

Re: [Qemu-devel] [PATCH v3 0/2] improve error handling of global properties

[Eduardo Habkost]

On Fri, Jul 22, 2016 at 01:01:10AM +0200, Greg Kurz wrote:
> As suggested by Eduardo, this series split the error handling of global
> properties in two separate patches.

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

Applied to machine-next, thanks.

-- 
Eduardo