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

[Roger Pau Monnà <roger.pau@xxxxxxxxxxxxx>]

2012/2/2 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.

The Sequence is the following, but I'm not really sure this is the
most correct way. All states can go to HOTPLUG_DEVICE_ERROR, and the
following state changes:

HOTPLUG_DEVICE_INIT no state change
HOTPLUG_DEVICE_CONNECT -> HOTPLUG_DEVICE_CONNECTED (on success)
HOTPLUG_DEVICE_CONNECTED no change
HOTPLUG_DEVICE_DISCONNECT -> HOTPLUG_DEVICE_DISCONNECTED (on success)
HOTPLUG_DEVICE_FORCE_DISCONNECT -> HOTPLUG_DEVICE_DISCONNECTED (on success)


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

I guess so, when Ian Jackson event API is all in I will probably have
to rewrite some parts of the code.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel