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

[Peter Crosthwaite]

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.

> 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.

> +}
> +
> +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.

> + * @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
>
>