On 31.05.2018 16:13, Igor Mammedov wrote: > On Wed, 30 May 2018 16:13:32 +0200 > David Hildenbrand <da...@redhat.com> wrote: > >> On 30.05.2018 15:08, Igor Mammedov wrote: >>> On Thu, 17 May 2018 10:15:17 +0200 >>> David Hildenbrand <da...@redhat.com> 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). >>> I'd split out introducing local error into separate patch >>> so patch would do a single thing.
I can do that if it makes review easier. >>> >>>> Also, add code to pass control to the final stage hotplug handler at the >>>> parent bus. >>> But I don't agree with generic >>> "} else if ("dev->parent_bus && dev->parent_bus->hotplug_handler) {" >>> forwarding, it's done by 3/14 for generic case and in case of >>> special device that needs bus handler called from machine one, >>> I'd suggest to do forwarding explicitly for that device only >>> like we do with acpi_dev. >> >> I decided to do it that way because it is generic and results in nicer >> recovery handling (e.g. in case pc_dimm plug fails, we can simply >> rollback all (for now MemoryDevice) previous plug operations). > rollback should be managed by the caller of pc_dimm plug > directly, so it's not relevant here. > >> IMHO, the resulting code is easier to read. >> >> From this handling it is clear that >> "if we reach the hotplug handler, and it is not some special device >> plugged by the machine (CPU, PC_DIMM), pass it on to the actual hotplug >> handler if any exists" > I strongly disagree with that it's easier to deal with. > You are basically duplicating already generalized code > from qdev_get_hotplug_handler() back into boards. > > If a device doesn't have to be handled by machine handler, > than qdev_get_hotplug_handler() must return its bus handler > if any directly. So branch in question that your are copying > is a dead one, pls drop it. We forward selected (pc_get_hotpug_handler()) devices to the right hotplug handler. Nothing wrong about that. I don't agree with "basically duplicating already generalized code" wrong. We have to forward at some place. Your idea simply places that code at some other place. But I think we have to get the general idea sorted out first. What you have in mind (e.g. plug): if (TYPE_MEMORY_DEVICE) { memory_device_plug(); if (local_err) { goto out; } if (TYPE_PC_DIMM) { pc_dimm_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); } if (local_err) { memory_device_unplug() goto out; } } else if (TYPE_CPU) ... What I have in mind (and implemented in this series): if (TYPE_MEMORY_DEVICE) { memory_device_plug(); } /* possibly other interfaces */ if (local_err) { error_handling(); return; } if (TYPE_PC_DIMM) { pc_dimm_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); } if (local_err) { if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) { memory_device_unplug() } /* possibly other interfaces */ } ... I claim that my variant is more generic because: - it easily supports multiple interfaces (like MemoryDevice) per Device that need a hotplug handler call - there is only one call to hotplug_handler_plug() in case we add similar handling for another device Apart from that they do exactly the same thing. -- Thanks, David / dhildenb