Re: [Qemu-devel] [PATCH 1/7] define hotplug interface

[Paolo Bonzini]

Il 09/12/2013 13:42, Igor Mammedov ha scritto:
> On Fri, 06 Dec 2013 18:26:37 +0100
> Paolo Bonzini <address@hidden> wrote:
> 
>> Il 06/12/2013 18:03, Igor Mammedov ha scritto:
>>> Provide generic hotplug interface for devices.
>>> Intended for replacing hotplug mechanism used by
>>> PCI/PCIE/SHPC code.
>>>
>>> Signed-off-by: Igor Mammedov <address@hidden>
>>> ---
>>> it's scsi-bus like interface, but abstracted from bus altogether
>>> since all current users care about in hotplug handlers, it's
>>> hotplug device and hotplugged device and bus only serves
>>> as a means to get access to hotplug device and it's callbacks.
>>> ---
>>>  hw/core/Makefile.objs |    1 +
>>>  hw/core/hotplug.c     |   25 ++++++++++++++++++++++++
>>>  include/hw/hotplug.h  |   50 
>>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 76 insertions(+), 0 deletions(-)
>>>  create mode 100644 hw/core/hotplug.c
>>>  create mode 100644 include/hw/hotplug.h
>>>
>>> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
>>> index 950146c..47f6555 100644
>>> --- a/hw/core/Makefile.objs
>>> +++ b/hw/core/Makefile.objs
>>> @@ -10,4 +10,5 @@ common-obj-$(CONFIG_SOFTMMU) += sysbus.o
>>>  common-obj-$(CONFIG_SOFTMMU) += null-machine.o
>>>  common-obj-$(CONFIG_SOFTMMU) += loader.o
>>>  common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
>>> +common-obj-$(CONFIG_SOFTMMU) += hotplug.o
>>>  
>>> diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
>>> new file mode 100644
>>> index 0000000..3e84d9c
>>> --- /dev/null
>>> +++ b/hw/core/hotplug.c
>>> @@ -0,0 +1,25 @@
>>> +/*
>>> + * Hotplug device interface.
>>> + *
>>> + * Copyright (c) 2013 Red Hat Inc.
>>> + *
>>> + * Authors:
>>> + *  Igor Mammedov <address@hidden>,
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2 or 
>>> later.
>>> + * See the COPYING file in the top-level directory.
>>> + */
>>> +#include "hw/hotplug.h"
>>> +
>>> +static const TypeInfo hotplug_device_info = {
>>> +    .name          = TYPE_HOTPLUG_DEVICE,
>>
>> Perhaps replace "device" with "handler" throughout?
> Marcel and Li suggested s/device/interface/ what do you think about it?

I think "handler" and "interface" are two different things.

This is the interface that makes a device able to handle hotplug.

So, "Interface" would be something to use instead of Class in the struct
name if you wanted (e.g. HotplugDeviceInterface or
HotplugHandlerInterface).  As you pointed out, however, the convention
seems to be to stick with "Class".  If you want to change
StreamSlaveClass to StreamSlaveInterface, and use HotplugXYZInterface
here, that would be okay.  But I don't think this is necessary; I just
made it as an example to explain why my request was a bit different from
Marcel and Li's.

>>> +#define TYPE_HOTPLUG_DEVICE "hotplug-device"
>>> +
>>> +#define HOTPLUG_DEVICE_CLASS(klass) \
>>> +     OBJECT_CLASS_CHECK(HotplugDeviceClass, (klass), TYPE_HOTPLUG_DEVICE)
>>> +#define HOTPLUG_DEVICE_GET_CLASS(obj) \
>>> +    OBJECT_GET_CLASS(HotplugDeviceClass, (obj), TYPE_HOTPLUG_DEVICE)
>>
>> Missing HOTPLUG_DEVICE macro, see include/hw/stream.h.
> 
> Since there were no users for it, I've dropped it. But I can sure add it.

I think the other request below will add a user.

Paolo


>>> +/**
>>> + * hotplug_fn:
>>> + * @hotplug_dev: a device performing hotplug/uplug action
>>> + * @hotplugged_dev: a device that has been hotplugged
>>> + * @errp: returns an error if this function fails
>>> + */
>>> +typedef void (*hotplug_fn)(DeviceState *hotplug_dev,
>>
>> First argument should be HotplugDevice (which is a struct containing a
>> single Object field; again see include/hw/stream.h).
>>
>>> +                           DeviceState *hotplugged_dev, Error **errp);
>>> +
>>> +/**
>>> + * HotplugDeviceClass:
>>> + *
>>> + * Interface to be implemented by a device performing
>>> + * hardware hotplug/unplug functions.
>>> + *
>>> + * @parent: Opaque parent interface.
>>> + * @hotplug: hotplug callback.
>>> + * @hot_unplug: hot unplug callback.
>>
>> Just plug/unplug.  IIRC it is used for cold-plug too.
>>
>> Otherwise looks great.
>>
>> Paolo
>>
>>> + */
>>> +typedef struct HotplugDeviceClass {
>>> +    InterfaceClass parent;
>>> +
>>> +    hotplug_fn hotplug;
>>> +    hotplug_fn hot_unplug;
>>> +} HotplugDeviceClass;
>>> +
>>> +#endif
>>>
>>
> 
> 
>