Re: [Qemu-ppc] [PATCH v4 04/14] pc: prepare for multi stage hotplug handlers

[Igor Mammedov]

On Mon, 4 Jun 2018 13:27:01 +0200
David Hildenbrand <address@hidden> wrote:

> On 31.05.2018 16:13, Igor Mammedov wrote:
> > On Wed, 30 May 2018 16:13:32 +0200
> > David Hildenbrand <address@hidden> wrote:
> >   
> >> On 30.05.2018 15:08, Igor Mammedov wrote:  
> >>> On Thu, 17 May 2018 10:15:17 +0200
> >>> David Hildenbrand <address@hidden> 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);
> }
I don't like this implicit wiring, where reader of this part of code
has no idea that TYPE_MEMORY_DEVICE might be also bus device that needs
request forwarding.
I'd prefer [pre/un]plug handlers be concrete and explicit spaghetti code,
something like this:

if (TYPE_PC_DIMM) {
    pc_dimm_plug()
    /* do here additional concrete machine specific things */
} else if (TYPE_VIRTIO_MEM) {
    virtio_mem_plug() <- do forwarding in there
    /* and do here additional concrete machine specific things */
} else if (TYPE_CPU) {
    cpu_plug()
    /* do here additional concrete machine specific things */
}

> 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
As someone said "one more layer of indirection would solve problem".
But then one would have a clue how it works after a while (including
author of the feature).
I don't think we have a problem here and need generalization.

> 
> Apart from that they do exactly the same thing.
>