| Message ID | 146914207969.16527.15568547267031224728.stgit@bahia.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; >
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; > > >
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; > > > > > >
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
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.
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;
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(-)