Re: [Xen-devel] [PATCH 25 of 29 RFC] libxl: add libxl_hotplug_dispatch

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 2 Feb 2012, Roger Pau Monne wrote:
> # HG changeset patch
> # User Roger Pau Monne <roger.pau@xxxxxxxxxxxxx>
> # Date 1328179686 -3600
> # Node ID 94532199f647fc03816fc5ae50ab53c8c5b80cd8
> # Parent  1506b1f2ab0fe5f4cd5bcd86a566d5a7be5f838b
> libxl: add libxl_hotplug_dispatch
> 
> Added a new function to libxl API to handle device hotplug. Actions to
> execute upon hotplug device state changes are handled using the
> libxl_device_disk_hotplug_actions and libxl_device_nic_hotplug_actions
> depending on the type of device. Currently only VIF and VBD devices
> are handled by the hotplug mechanism.
> 
> Signed-off-by: Roger Pau Monne <roger.pau@xxxxxxxxxxxxx>
> 
> diff -r 1506b1f2ab0f -r 94532199f647 tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c       Thu Feb 02 11:45:03 2012 +0100
> +++ b/tools/libxl/libxl.c       Thu Feb 02 11:48:06 2012 +0100
> @@ -982,6 +982,188 @@ out:
>      return rc;
>  }
> 
> +static int libxl__hotplug_generic_dispatcher(libxl__gc *gc, char *path,
> +                                uint32_t domid,
> +                                libxl__hotplug_status state,
> +                                libxl__device_generic_hotplug_actions 
> *action,
> +                                void *device)
> +{
> +    libxl_ctx *ctx = libxl__gc_owner(gc);
> +    char *spath = libxl__sprintf(gc, "%s/state", path);
> +    int rc = 0;
> +
> +    LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "processing device %s with state %d",
> +                                      path, state);
> +    switch(state) {
> +    case HOTPLUG_DEVICE_INIT:
> +        if (action->init) {
> +            rc = action->init(ctx, domid, device);
> +            if (rc < 0) {
> +                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to init"
> +                                                  " device %s", path);
> +                libxl__xs_write(gc, XBT_NULL, spath, "%d",
> +                                HOTPLUG_DEVICE_ERROR);
> +                break;
> +            }
> +        }
> +        libxl__xs_write(gc, XBT_NULL, spath, "%d", HOTPLUG_DEVICE_CONNECT);
> +        break;
> +    case HOTPLUG_DEVICE_CONNECT:
> +        if (action->connect) {
> +            rc = action->connect(ctx, domid, device);
> +            if (rc < 0) {
> +                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to connect"
> +                                                  " device %s", path);
> +                libxl__xs_write(gc, XBT_NULL, spath, "%d",
> +                                HOTPLUG_DEVICE_ERROR);
> +                break;
> +            }
> +        }
> +        libxl__xs_write(gc, XBT_NULL, spath, "%d", HOTPLUG_DEVICE_CONNECTED);
> +        break;
> +    case HOTPLUG_DEVICE_CONNECTED:
> +        if (action->connected) {
> +            rc = action->connected(ctx, domid, device);
> +            if (rc < 0) {
> +                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to execute "
> +                                                  "connected action on"
> +                                                  " device %s", path);
> +                libxl__xs_write(gc, XBT_NULL, spath, "%d",
> +                                HOTPLUG_DEVICE_ERROR);
> +                break;
> +            }
> +        }
> +        break;
> +    case HOTPLUG_DEVICE_DISCONNECT:
> +        if (action->disconnect) {
> +            rc = action->disconnect(ctx, domid, device);
> +            if (rc < 0) {
> +                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to unplug"
> +                                                  " device %s", path);
> +                libxl__xs_write(gc, XBT_NULL, spath, "%d",
> +                                HOTPLUG_DEVICE_ERROR);
> +                break;
> +            }
> +        }
> +        libxl__xs_write(gc, XBT_NULL, spath, "%d",
> +                        HOTPLUG_DEVICE_DISCONNECTED);
> +        break;
> +    case HOTPLUG_DEVICE_DISCONNECTED:
> +        if (action->disconnected) {
> +            rc = action->disconnected(ctx, domid, device);
> +            if (rc < 0) {
> +                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to execute "
> +                                                  "unpluged action on"
> +                                                  " device %s", path);
> +                libxl__xs_write(gc, XBT_NULL, spath, "%d",
> +                                HOTPLUG_DEVICE_ERROR);
> +                break;
> +            }
> +        }
> +        break;
> +    case HOTPLUG_DEVICE_FORCE_DISCONNECT:
> +        if (action->force_disconnect) {
> +            rc = action->force_disconnect(ctx, domid, device);
> +            if (rc < 0) {
> +                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to force "
> +                                                  "disconnect device "
> +                                                  "%s", path);
> +                libxl__xs_write(gc, XBT_NULL, spath, "%d",
> +                                HOTPLUG_DEVICE_ERROR);
> +                break;
> +            }
> +        }
> +        libxl__xs_write(gc, XBT_NULL, spath, "%d",
> +                        HOTPLUG_DEVICE_DISCONNECTED);
> +        break;
> +    case HOTPLUG_DEVICE_ERROR:
> +        if (action->error)
> +            rc = action->error(ctx, domid, device);
> +        break;
> +    }
> +    return rc;

I can see that we are writing an error code to xenstore in case of
errors, but I fail to see where we are writing back a positive response.
Of course the caller of libxl_device_disk/nic_hotplug_add could watch
the backend state waiting for it to come up, but I think that having an
explicit ACK under /hotplug would be much better.

I haven't reviewed all the patches in detail but I think that this
series is going in the right thing direction, from the architectural
point of view.
The main requests for changes will probably be regarding event handling
and the usage of the new APIs.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel