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

[David Hildenbrand]

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);
}

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