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

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

On 4/2/20 11:49 AM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Sent: 01 April 2020 17:14
>> To: Anthony PERARD <anthony.perard@citrix.com>; qemu-devel@nongnu.org
>> Cc: xen-devel@lists.xenproject.org; Peter Maydell <peter.maydell@linaro.org>; Paul Durrant
>> <paul@xen.org>; Markus Armbruster <armbru@redhat.com>
>> Subject: Re: [Qemu-devel] [PULL 06/25] xen: create xenstore areas for XenDevice-s
>>
>> 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.
> 
> In this case that's essentially a "don't care". We want to know why the realize failed but if the unrealize fails something is probably pretty seriously screwed (hence the error_abort).

OK. I was surprised by the singular pattern (which confuses Coccinelle 
semantic transformations), but it works.

Thanks for the quick feedback Paul!

Regards,

Phil.

> 
>    Paul
>