On 07.06.2018 15:44, Igor Mammedov wrote:
> On Mon, 4 Jun 2018 13:27:01 +0200
> David Hildenbrand <da...@redhat.com> wrote:
>
>> 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);
>> }
> 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 */
> }
That will result in a lot of duplicate code - for every machine we
support (dimm/virtio-mem/virtio-pmem/*add more memory devices here*) -
virtio-mem and virtio-pmem could most probably share the code.
>
>> 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.
>>
>
--
Thanks,
David / dhildenb
