Re: [Qemu-devel] [PATCH v6 2/9] qdev: add to BusState "hotplug-handler" link

[Igor Mammedov]

On Wed, 05 Feb 2014 16:52:27 +0100
Andreas Färber <address@hidden> wrote:

> Am 05.02.2014 16:36, schrieb Igor Mammedov:
> > It will allow to reuse field with different BUSes,
> > reducing code duplication. Field is intended for
> > replacing 'hotplug_qdev' field in PCIBus and also
> > will allow to avoid adding equivalent field to
> > DimmBus with possiblitity to refactor other BUSes
> > to use it instead of custom field.
> > In addition once all users of allow_hotplug field
> > are converted to new API, link could replace
> > allow_hotplug field in qdev hotplug code.
> > 
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> >  hw/core/qdev.c         |    4 ++++
> >  include/hw/qdev-core.h |   12 ++++++++++++
> >  2 files changed, 16 insertions(+), 0 deletions(-)
> > 
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index 82a9123..c9f0c33 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -32,6 +32,7 @@
> >  #include "qapi/visitor.h"
> >  #include "qapi/qmp/qjson.h"
> >  #include "monitor/monitor.h"
> > +#include "hw/hotplug.h"
> >  
> >  int qdev_hotplug = 0;
> >  static bool qdev_hot_added = false;
> > @@ -870,6 +871,9 @@ static void qbus_initfn(Object *obj)
> >      BusState *bus = BUS(obj);
> >  
> >      QTAILQ_INIT(&bus->children);
> > +    object_property_add_link(obj, QDEV_HOTPLUG_HANDLER_PROPERTY,
> > +                             TYPE_HOTPLUG_HANDLER,
> > +                             (Object **)&bus->hotplug_handler, NULL);
> 
> Will/should the user ever change that property? If not, we could drop
> this hunk and change the inline link-setting below to just do it the C
> way. Otherwise it should probably be using &error_abort instead of NULL.
So far user is expected to set property only one time.
But why deviate from QOM an allow user to poke directly into arbitrary bus
internals even with help of inline helper below? 
Link also will allow to keep pointer safe, i.e. hotplug_handler won't
disappear suddenly.

As for using &error_abort in initfn(), this function might be called during
hotplug and crash running guest.
One way to handle such errors could be passing errp to initfn(),
another is shown is this patch: i.e. ignore error in initfn() as it's done
in other initfn()-s and handle error at set time in qbus_set_hotplug_handler(),
then caller can decide whether do abort or report error up the call stack, i.e.
to libvirt via QMP.

> 
> Regards,
> Andreas
> 
> >  }
> >  
> >  static char *default_bus_get_fw_dev_path(DeviceState *dev)
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index 2c4f140..41ec533 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -8,6 +8,7 @@
> >  #include "qom/object.h"
> >  #include "hw/irq.h"
> >  #include "qapi/error.h"
> > +#include "hw/hotplug.h"
> >  
> >  enum {
> >      DEV_NVECTORS_UNSPECIFIED = -1,
> > @@ -180,14 +181,18 @@ typedef struct BusChild {
> >      QTAILQ_ENTRY(BusChild) sibling;
> >  } BusChild;
> >  
> > +#define QDEV_HOTPLUG_HANDLER_PROPERTY "hotplug-handler"
> > +
> >  /**
> >   * BusState:
> > + * @hotplug_device: link to a hotplug device associated with bus.
> >   */
> >  struct BusState {
> >      Object obj;
> >      DeviceState *parent;
> >      const char *name;
> >      int allow_hotplug;
> > +    HotplugHandler *hotplug_handler;
> >      int max_index;
> >      QTAILQ_HEAD(ChildrenHead, BusChild) children;
> >      QLIST_ENTRY(BusState) sibling;
> > @@ -321,4 +326,11 @@ extern int qdev_hotplug;
> >  
> >  char *qdev_get_dev_path(DeviceState *dev);
> >  
> > +static inline void qbus_set_hotplug_handler(BusState *bus, DeviceState 
> > *handler,
> > +                                            Error **errp)
> > +{
> > +    object_property_set_link(OBJECT(bus), OBJECT(handler),
> > +                             QDEV_HOTPLUG_HANDLER_PROPERTY, errp);
> > +    bus->allow_hotplug = 1;
> > +}
> >  #endif
> > 
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg


-- 
Regards,
  Igor