From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58852) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vq0gD-0006tS-NW for qemu-devel@nongnu.org; Mon, 09 Dec 2013 08:15:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vq0g5-0003xv-AK for qemu-devel@nongnu.org; Mon, 09 Dec 2013 08:15:21 -0500 Received: from mail-qc0-x22a.google.com ([2607:f8b0:400d:c01::22a]:54185) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vq0g5-0003xq-57 for qemu-devel@nongnu.org; Mon, 09 Dec 2013 08:15:13 -0500 Received: by mail-qc0-f170.google.com with SMTP id x13so2680662qcv.15 for ; Mon, 09 Dec 2013 05:15:12 -0800 (PST) Sender: Paolo Bonzini Message-ID: <52A5C259.2050603@redhat.com> Date: Mon, 09 Dec 2013 14:15:05 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1386349395-5710-1-git-send-email-imammedo@redhat.com> <1386349395-5710-2-git-send-email-imammedo@redhat.com> <52A208CD.4020403@redhat.com> <20131209134220.1434b669@nial.usersys.redhat.com> In-Reply-To: <20131209134220.1434b669@nial.usersys.redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/7] define hotplug interface List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: peter.maydell@linaro.org, ehabkost@redhat.com, marcel.a@redhat.com, mst@redhat.com, qemu-devel@nongnu.org, blauwirbel@gmail.com, alex.williamson@redhat.com, anthony@codemonkey.ws, afaerber@suse.de Il 09/12/2013 13:42, Igor Mammedov ha scritto: > On Fri, 06 Dec 2013 18:26:37 +0100 > Paolo Bonzini 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 >>> --- >>> 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 , >>> + * >>> + * 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 >>> >> > > >