Re: [Qemu-devel] [PULL 06/25] xen: create xenstore areas for XenDevice-s

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

Hi Anthony, Paul.

Cc'ing Markus too.

On 1/14/19 2:51 PM, Anthony PERARD wrote:
> From: Paul Durrant <paul.durrant@citrix.com>
> 
> This patch adds a new source module, xen-bus-helper.c, which builds on
> basic libxenstore primitives to provide functions to create (setting
> permissions appropriately) and destroy xenstore areas, and functions to
> 'printf' and 'scanf' nodes therein. The main xen-bus code then uses
> these primitives [1] to initialize and destroy the frontend and backend
> areas for a XenDevice during realize and unrealize respectively.
> 
> The 'xen-block' implementation is extended with a 'get_name' method that
> returns the VBD number. This number is required to 'name' the xenstore
> areas.
> 
> NOTE: An exit handler is also added to make sure the xenstore areas are
>        cleaned up if QEMU terminates without devices being unrealized.
> 
> [1] The 'scanf' functions are actually not yet needed, but they will be
>      needed by code delivered in subsequent patches.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Reviewed-by: Anthony Perard <anthony.perard@citrix.com>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>   hw/block/xen-block.c            |   9 +
>   hw/xen/Makefile.objs            |   2 +-
>   hw/xen/trace-events             |  12 +-
>   hw/xen/xen-bus-helper.c         | 150 +++++++++++++++
>   hw/xen/xen-bus.c                | 321 +++++++++++++++++++++++++++++++-
>   include/hw/xen/xen-bus-helper.h |  39 ++++
>   include/hw/xen/xen-bus.h        |  12 ++
>   7 files changed, 540 insertions(+), 5 deletions(-)
>   create mode 100644 hw/xen/xen-bus-helper.c
>   create mode 100644 include/hw/xen/xen-bus-helper.h
> 
[...]
> +static void xen_device_exit(Notifier *n, void *data)
> +{
> +    XenDevice *xendev = container_of(n, XenDevice, exit);
> +
> +    xen_device_unrealize(DEVICE(xendev), &error_abort);
>   }
>   
>   static void xen_device_realize(DeviceState *dev, Error **errp)
>   {
>       XenDevice *xendev = XEN_DEVICE(dev);
>       XenDeviceClass *xendev_class = XEN_DEVICE_GET_CLASS(xendev);
> +    XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
>       const char *type = object_get_typename(OBJECT(xendev));
>       Error *local_err = NULL;
>   
> -    trace_xen_device_realize(type);
> +    if (xendev->frontend_id == DOMID_INVALID) {
> +        xendev->frontend_id = xen_domid;
> +    }
> +
> +    if (xendev->frontend_id >= DOMID_FIRST_RESERVED) {
> +        error_setg(errp, "invalid frontend-id");
> +        goto unrealize;
> +    }
> +
> +    if (!xendev_class->get_name) {
> +        error_setg(errp, "get_name method not implemented");
> +        goto unrealize;
> +    }
> +
> +    xendev->name = xendev_class->get_name(xendev, &local_err);
> +    if (local_err) {
> +        error_propagate_prepend(errp, local_err,
> +                                "failed to get device name: ");
> +        goto unrealize;
> +    }
> +
> +    trace_xen_device_realize(type, xendev->name);
> +
> +    xen_device_backend_create(xendev, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        goto unrealize;
> +    }
> +
> +    xen_device_frontend_create(xendev, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        goto unrealize;
> +    }
>   
>       if (xendev_class->realize) {
>           xendev_class->realize(xendev, &local_err);
> @@ -72,18 +364,43 @@ static void xen_device_realize(DeviceState *dev, Error **errp)
>           }
>       }
>   
> +    xen_device_backend_printf(xendev, "frontend", "%s",
> +                              xendev->frontend_path);
> +    xen_device_backend_printf(xendev, "frontend-id", "%u",
> +                              xendev->frontend_id);
> +    xen_device_backend_printf(xendev, "online", "%u", 1);
> +    xen_device_backend_printf(xendev, "hotplug-status", "connected");
> +
> +    xen_device_backend_set_state(xendev, XenbusStateInitWait);
> +
> +    xen_device_frontend_printf(xendev, "backend", "%s",
> +                               xendev->backend_path);
> +    xen_device_frontend_printf(xendev, "backend-id", "%u",
> +                               xenbus->backend_id);
> +
> +    xen_device_frontend_set_state(xendev, XenbusStateInitialising);
> +
> +    xendev->exit.notify = xen_device_exit;
> +    qemu_add_exit_notifier(&xendev->exit);
>       return;
>   
>   unrealize:
>       xen_device_unrealize(dev, &error_abort);

It seems if unrealize() fails, the error stored in &local_err is never 
reported. Not sure if this can be improved although.

>   }
>   
> +static Property xen_device_props[] = {
> +    DEFINE_PROP_UINT16("frontend-id", XenDevice, frontend_id,
> +                       DOMID_INVALID),
> +    DEFINE_PROP_END_OF_LIST()
> +};
> +
>   static void xen_device_class_init(ObjectClass *class, void *data)
>   {
>       DeviceClass *dev_class = DEVICE_CLASS(class);
>   
>       dev_class->realize = xen_device_realize;
>       dev_class->unrealize = xen_device_unrealize;
> +    dev_class->props = xen_device_props;
>       dev_class->bus_type = TYPE_XEN_BUS;
>   }
>   
[...]