On Tue, 5 Jun 2018 09:51:26 +0200 David Hildenbrand <da...@redhat.com> wrote:
> On 05.06.2018 03:08, David Gibson wrote: > > On Thu, May 17, 2018 at 10:15:19AM +0200, David Hildenbrand wrote: > >> For multi stage hotplug handlers, we'll have to do some error handling > >> in some hotplug functions, so let's use a local error variable (except > >> for unplug requests). > >> > >> Also, add code to pass control to the final stage hotplug handler at the > >> parent bus. > >> > >> Signed-off-by: David Hildenbrand <da...@redhat.com> > >> --- > >> hw/ppc/spapr.c | 54 +++++++++++++++++++++++++++++++++++++++++++----------- > >> 1 file changed, 43 insertions(+), 11 deletions(-) > >> > >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > >> index ebf30dd60b..b7c5c95f7a 100644 > >> --- a/hw/ppc/spapr.c > >> +++ b/hw/ppc/spapr.c > >> @@ -3571,27 +3571,48 @@ static void > >> spapr_machine_device_plug(HotplugHandler *hotplug_dev, > >> { > >> MachineState *ms = MACHINE(hotplug_dev); > >> sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms); > >> + Error *local_err = NULL; > >> > >> + /* final stage hotplug handler */ > >> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > >> int node; > >> > >> if (!smc->dr_lmb_enabled) { > >> - error_setg(errp, "Memory hotplug not supported for this > >> machine"); > >> - return; > >> + error_setg(&local_err, > >> + "Memory hotplug not supported for this machine"); > >> + goto out; > >> } > >> - node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, > >> errp); > >> - if (*errp) { > >> - return; > >> + node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, > >> + &local_err); > >> + if (local_err) { > >> + goto out; > >> } > >> if (node < 0 || node >= MAX_NODES) { > >> - error_setg(errp, "Invaild node %d", node); > >> - return; > >> + error_setg(&local_err, "Invaild node %d", node); > >> + goto out; > >> } > >> > >> - spapr_memory_plug(hotplug_dev, dev, node, errp); > >> + spapr_memory_plug(hotplug_dev, dev, node, &local_err); > >> } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) { > >> - spapr_core_plug(hotplug_dev, dev, errp); > >> + spapr_core_plug(hotplug_dev, dev, &local_err); > >> + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > >> + hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, > >> &local_err); > >> + } > >> +out: > >> + error_propagate(errp, local_err); > >> +} > >> + > >> +static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev, > >> + DeviceState *dev, Error **errp) > >> +{ > >> + Error *local_err = NULL; > >> + > >> + /* final stage hotplug handler */ > >> + if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > > > > As I think Igor said on the equivalent PC patch, I don't quite get > > this. Isn't this already handled by the generic hotplug code picking > > up the bus's hotplug handler if the machine doesn't supply one? > > See my reply to patch nr 4. > > What we do is, we install the machine hotplug handler as an > "intermediate" hotplug handler. > > E.g. if we have a VIRTIO based MemoryDevice, we have to initialize the > MemoryDevice specific stuff in the machine hotplug handler, but then > pass the device onto the last stage hotplug handler (which will > eventually attach it to a bus and notify the guest). as said in v4, pls don't do this implicit routing as it's hard to read and maintain. Do explicit routing within concrete device helper (virtio_mem_[un|pre]plug()) keeping un/pre/plug handlers simple. And then you won't need if check as well, just call to bus handler directly. [...]