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

[Igor Mammedov]

On Mon, 09 Dec 2013 10:58:04 +0200
Marcel Apfelbaum <address@hidden> wrote:

> On Fri, 2013-12-06 at 18:03 +0100, Igor Mammedov wrote:
> > 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,
> > +    .parent        = TYPE_INTERFACE,
> > +    .class_size = sizeof(HotplugDeviceClass),
> > +};
> > +
> > +static void hotplug_device_register_types(void)
> > +{
> > +    type_register_static(&hotplug_device_info);
> > +}
> > +
> > +type_init(hotplug_device_register_types)
> > diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
> > new file mode 100644
> > index 0000000..cfa79bb
> > --- /dev/null
> > +++ b/include/hw/hotplug.h
> > @@ -0,0 +1,50 @@
> > +/*
> > + * 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.
> > + */
> > +#ifndef HOTPLUG_H
> > +#define HOTPLUG_H
> > +
> > +#include "hw/qdev-core.h"
> > +
> > +#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)
> > +
> > +/**
> > + * hotplug_fn:
> > + * @hotplug_dev: a device performing hotplug/uplug action
> /s/uplug/unplug  :)
> 
> > + * @hotplugged_dev: a device that has been hotplugged
> > + * @errp: returns an error if this function fails
> > + */
> > +typedef void (*hotplug_fn)(DeviceState *hotplug_dev,
> > +                           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.
> > + */
> > +typedef struct HotplugDeviceClass {
> May I ask why do we call it a class if it is an interface?
> I would call it HotplugDeviceInterface, so I will not think
> that it is part of the inheritance chain every time I see it in code.
> Did I miss something?

We have InterfaceClass as a parent, so it's just consistent to follow
the same pattern.
And naming is in consistence with what I've seen in object.h
and include/hw/stream.h after which I've modeled it.

> 
> Thanks,
> Marcel
> 
> 
> > +    InterfaceClass parent;
> > +
> > +    hotplug_fn hotplug;
> > +    hotplug_fn hot_unplug;
> > +} HotplugDeviceClass;
> > +
> > +#endif
> 
> 
> 
>