Re: [Qemu-devel] [PATCH 03/11] define hotplug interface

[Igor Mammedov]

On Sat, 14 Dec 2013 17:03:34 +1000
Peter Crosthwaite <address@hidden> wrote:

> On Fri, Dec 13, 2013 at 10:44 PM, Igor Mammedov <address@hidden> wrote:
> > Provide generic hotplug interface for devices.
> 
> "Provide a". s/devices/hotplug handlers would be cleaner too, to match
> your v2 changes.
sure

> 
> > Intended for replacing hotplug mechanism used by
> > PCI/PCIE/SHPC code and will be used for memory hotplug.
> >
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> > v2:
> > * s/device/handler/
> > * add hotplug_handler_plug/hotplug_handler_unplug API
> > v1:
> > 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     | 48 +++++++++++++++++++++++++++++++++
> >  include/hw/hotplug.h  | 75 
> > +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 124 insertions(+)
> >  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..9e324be 100644
> > --- a/hw/core/Makefile.objs
> > +++ b/hw/core/Makefile.objs
> > @@ -2,6 +2,7 @@
> >  common-obj-y += qdev.o qdev-properties.o
> >  # irq.o needed for qdev GPIO handling:
> >  common-obj-y += irq.o
> > +common-obj-y += hotplug.o
> >
> >  common-obj-$(CONFIG_EMPTY_SLOT) += empty_slot.o
> >  common-obj-$(CONFIG_XILINX_AXI) += stream.o
> > diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
> > new file mode 100644
> > index 0000000..5c3b5c9
> > --- /dev/null
> > +++ b/hw/core/hotplug.c
> > @@ -0,0 +1,48 @@
> > +/*
> > + * Hotplug handler 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"
> > +#include "qemu/module.h"
> > +
> > +void hotplug_handler_plug(HotplugHandler *plug_handler,
> > +                          DeviceState *plugged_dev,
> > +                          Error **errp)
> > +{
> > +    HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler);
> > +
> > +    if (hdc->plug) {
> > +        hdc->plug(plug_handler, plugged_dev, errp);
> > +    }
> 
> So does it mean anything to have a hotplug handler device that can't
> plug? Im thinking this API definition should be compusorly and
> throw-errors/assert rather than silent fail.
It doesn't have "fail" semantic but rather "handler doesn't need to do
anything for plug".
And although in current usage we always have plug/unplug pair,
not asserting would be more flexible from POV of generic API.

> 
> > +}
> > +
> > +void hotplug_handler_unplug(HotplugHandler *plug_handler,
> > +                            DeviceState *plugged_dev,
> > +                            Error **errp)
> > +{
> > +    HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler);
> > +
> > +    if (hdc->unplug) {
> > +        hdc->unplug(plug_handler, plugged_dev, errp);
> > +    }
> > +}
> > +
> > +static const TypeInfo hotplug_handler_info = {
> > +    .name          = TYPE_HOTPLUG_HANDLER,
> > +    .parent        = TYPE_INTERFACE,
> > +    .class_size = sizeof(HotplugHandlerClass),
> > +};
> > +
> > +static void hotplug_handler_register_types(void)
> > +{
> > +    type_register_static(&hotplug_handler_info);
> > +}
> > +
> > +type_init(hotplug_handler_register_types)
> > diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
> > new file mode 100644
> > index 0000000..64ae6f7
> > --- /dev/null
> > +++ b/include/hw/hotplug.h
> > @@ -0,0 +1,75 @@
> > +/*
> > + * Hotplug handler 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 "qom/object.h"
> > +#include "qemu/typedefs.h"
> > +
> > +#define TYPE_HOTPLUG_HANDLER "hotplug-handler"
> > +
> > +#define HOTPLUG_HANDLER_CLASS(klass) \
> > +     OBJECT_CLASS_CHECK(HotplugHandlerClass, (klass), TYPE_HOTPLUG_HANDLER)
> > +#define HOTPLUG_HANDLER_GET_CLASS(obj) \
> > +     OBJECT_GET_CLASS(HotplugHandlerClass, (obj), TYPE_HOTPLUG_HANDLER)
> > +#define HOTPLUG_HANDLER(obj) \
> > +     INTERFACE_CHECK(HotplugHandler, (obj), TYPE_HOTPLUG_HANDLER)
> > +
> > +
> > +typedef struct HotplugHandler {
> 
> /* <private> */
> 
> > +    Object Parent;
> > +} HotplugHandler;
> > +
> > +/**
> > + * hotplug_fn:
> > + * @plug_handler: a device performing plug/uplug action
> > + * @plugged_dev: a device that has been (un)plugged
> > + * @errp: returns an error if this function fails
> > + */
> > +typedef void (*hotplug_fn)(HotplugHandler *plug_handler,
> > +                           DeviceState *plugged_dev, Error **errp);
> > +
> > +/**
> > + * HotplugDeviceClass:
> > + *
> > + * Interface to be implemented by a device performing
> > + * hardware (un)plug functions.
> > + *
> > + * @parent: Opaque parent interface.
> 
> Not sure if it makes sense to publicly document the QOM parent
> first-field. It should not be used in code from this context.
I'd be glad to, but I think Andreas asked me once to do so for "parent"
in DimmDevice class. I also no sure that is necessary to document this
field in every child.

> 
> > + * @plug: plug callback.
> > + * @unplug: unplug callback.
> > + */
> > +typedef struct HotplugHandlerClass {
> 
> /* <private> */
> 
> > +    InterfaceClass parent;
> > +
> 
> /* <public> */
> 
> > +    hotplug_fn plug;
> > +    hotplug_fn unplug;
> > +} HotplugHandlerClass;
> > +
> > +/**
> > + * hotplug_handler_plug:
> > + *
> > + * Call #HotplugHandlerClass.plug callback of @plug_handler.
> > + */
> > +void hotplug_handler_plug(HotplugHandler *plug_handler,
> > +                          DeviceState *plugged_dev,
> > +                          Error **errp);
> > +
> > +/**
> > + * hotplug_handler_unplug:
> > + *
> > + * Call #HotplugHandlerClass.unplug callback of @plug_handler.
> > + */
> > +void hotplug_handler_unplug(HotplugHandler *plug_handler,
> > +                            DeviceState *plugged_dev,
> > +                            Error **errp);
> > +#endif
> > --
> > 1.8.3.1
> >
> >

I'll ally the rest of changes on next respin, thanks for review!