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 <address@hidden>
> > Signed-off-by: Greg Kurz <address@hidden>
> 
> 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