| Message ID | 1386938688-22433-5-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Dec 13, 2013 at 10:44 PM, Igor Mammedov <imammedo@redhat.com> wrote: > It will allow to reuse field with different BUSes, reducing code duplication. > Field is intended fot 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 in qdev hotplug code. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > hw/core/qdev.c | 4 ++++ > include/hw/qdev-core.h | 5 +++++ > 2 files changed, 9 insertions(+) > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index e374a93..25c2d2c 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; > @@ -868,6 +869,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); I think failure of this is fatal. When the patches go through, probably want to &error_abort. > } > > 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 f2043a6..684a5da 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, > @@ -169,14 +170,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; > -- > 1.8.3.1 > >
On Sat, 14 Dec 2013 17:05:57 +1000 Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Fri, Dec 13, 2013 at 10:44 PM, Igor Mammedov <imammedo@redhat.com> wrote: > > It will allow to reuse field with different BUSes, reducing code duplication. > > Field is intended fot 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 in qdev hotplug code. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > hw/core/qdev.c | 4 ++++ > > include/hw/qdev-core.h | 5 +++++ > > 2 files changed, 9 insertions(+) > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > > index e374a93..25c2d2c 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; > > @@ -868,6 +869,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); > > I think failure of this is fatal. When the patches go through, > probably want to &error_abort. That's just another example of we don't care about error that gave birth to &error_abort topic. But I think is a drawback if error-less initfn. We have a lot of such usage without a way to report error, that's why "we don't care about error" was invented I guess. But more to the point, bus could be created during hotplug, and we do not want to abort guest, instead hotplug operation should fail. In this case following setting of QDEV_HOTPLUG_HANDLER_PROPERTY will fail if object_property_add_link() failed before. > > > } > > > > 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 f2043a6..684a5da 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, > > @@ -169,14 +170,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; > > -- > > 1.8.3.1 > > > >
On Mon, 16 Dec 2013 16:26:25 +0100 Igor Mammedov <imammedo@redhat.com> wrote: > On Sat, 14 Dec 2013 17:05:57 +1000 > Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > [...] > > > > > > QTAILQ_INIT(&bus->children); > > > + object_property_add_link(obj, QDEV_HOTPLUG_HANDLER_PROPERTY, > > > + TYPE_HOTPLUG_HANDLER, > > > + (Object **)&bus->hotplug_handler, NULL); > > > > I think failure of this is fatal. When the patches go through, > > probably want to &error_abort. [...] > > But more to the point, bus could be created during hotplug, and > we do not want to abort guest, instead hotplug operation should fail. > In this case following setting of QDEV_HOTPLUG_HANDLER_PROPERTY > will fail if object_property_add_link() failed before. In following patches there is a couple places asking for being replaced with &error_abort, so if your patches get in before respin, I'll gladly use &error_abort there.
diff --git a/hw/core/qdev.c b/hw/core/qdev.c index e374a93..25c2d2c 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; @@ -868,6 +869,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); } 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 f2043a6..684a5da 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, @@ -169,14 +170,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;
It will allow to reuse field with different BUSes, reducing code duplication. Field is intended fot 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 in qdev hotplug code. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- hw/core/qdev.c | 4 ++++ include/hw/qdev-core.h | 5 +++++ 2 files changed, 9 insertions(+)