[PATCH v2 05/15] libxl: change libxl__ao_device_remove to libxl__ao_device

[Roger Pau Monne <roger.pau@citrix.com>]

Introduce a new structure to track state of device backends, that will
be used in following patches on this series.

This structure if used for both device creation and device
destruction and removes libxl__ao_device_remove.

Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
---
 tools/libxl/libxl.c          |  192 +++++++++++++++++++++++++++++------------
 tools/libxl/libxl.h          |   15 +++-
 tools/libxl/libxl_device.c   |  107 ++++++++++++++++--------
 tools/libxl/libxl_internal.h |   45 ++++++++--
 4 files changed, 257 insertions(+), 102 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2a09192..b13de4f 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1271,6 +1271,26 @@ int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass)
 
 /******************************************************************************/
 
+/* generic callback for devices that only need to set ao_complete */
+static void libxl__device_cb(libxl__egc *egc, libxl__ao_device *aorm)
+{
+    STATE_AO_GC(aorm->ao);
+
+    if (aorm->rc) {
+        LOGE(ERROR, "unable to %s %s with id %u",
+                    aorm->action == DEVICE_CONNECT ? "add" : "remove",
+                    libxl__device_kind_to_string(aorm->dev->kind),
+                    aorm->dev->devid);
+        goto out;
+    }
+
+out:
+    libxl__ao_complete(egc, ao, aorm->rc);
+    return;
+}
+
+/******************************************************************************/
+
 int libxl__device_disk_setdefault(libxl__gc *gc, libxl_device_disk *disk)
 {
     int rc;
@@ -1445,35 +1465,50 @@ int libxl_device_disk_remove(libxl_ctx *ctx, uint32_t domid,
                              const libxl_asyncop_how *ao_how)
 {
     AO_CREATE(ctx, domid, ao_how);
-    libxl__device device;
+    libxl__device *device;
+    libxl__ao_device *aorm;
     int rc;
 
-    rc = libxl__device_from_disk(gc, domid, disk, &device);
+    GCNEW(device);
+    rc = libxl__device_from_disk(gc, domid, disk, device);
     if (rc != 0) goto out;
 
-    rc = libxl__initiate_device_remove(egc, ao, &device);
-    if (rc) goto out;
-
-    return AO_INPROGRESS;
+    GCNEW(aorm);
+    libxl__init_ao_device(aorm, ao, NULL);
+    aorm->action = DEVICE_DISCONNECT;
+    aorm->dev = device;
+    aorm->callback = libxl__device_cb;
+    libxl__initiate_device_remove(egc, aorm);
 
 out:
-    return AO_ABORT(rc);
+    if (rc) return AO_ABORT(rc);
+    return AO_INPROGRESS;
 }
 
 int libxl_device_disk_destroy(libxl_ctx *ctx, uint32_t domid,
-                              libxl_device_disk *disk)
+                              libxl_device_disk *disk,
+                              const libxl_asyncop_how *ao_how)
 {
-    GC_INIT(ctx);
-    libxl__device device;
+    AO_CREATE(ctx, domid, ao_how);
+    libxl__device *device;
+    libxl__ao_device *aorm;
     int rc;
 
-    rc = libxl__device_from_disk(gc, domid, disk, &device);
+    GCNEW(device);
+    rc = libxl__device_from_disk(gc, domid, disk, device);
     if (rc != 0) goto out;
 
-    rc = libxl__device_destroy(gc, &device);
+    GCNEW(aorm);
+    libxl__init_ao_device(aorm, ao, NULL);
+    aorm->action = DEVICE_DISCONNECT;
+    aorm->force = 1;
+    aorm->dev = device;
+    aorm->callback = libxl__device_cb;
+    libxl__initiate_device_remove(egc, aorm);
+
 out:
-    GC_FREE;
-    return rc;
+    if (rc) return AO_ABORT(rc);
+    return AO_INPROGRESS;
 }
 
 static void libxl__device_disk_from_xs_be(libxl__gc *gc,
@@ -1923,35 +1958,50 @@ int libxl_device_nic_remove(libxl_ctx *ctx, uint32_t domid,
                             const libxl_asyncop_how *ao_how)
 {
     AO_CREATE(ctx, domid, ao_how);
-    libxl__device device;
+    libxl__device *device;
+    libxl__ao_device *aorm;
     int rc;
 
-    rc = libxl__device_from_nic(gc, domid, nic, &device);
+    GCNEW(device);
+    rc = libxl__device_from_nic(gc, domid, nic, device);
     if (rc != 0) goto out;
 
-    rc = libxl__initiate_device_remove(egc, ao, &device);
-    if (rc) goto out;
-
-    return AO_INPROGRESS;
+    GCNEW(aorm);
+    libxl__init_ao_device(aorm, ao, NULL);
+    aorm->action = DEVICE_DISCONNECT;
+    aorm->dev = device;
+    aorm->callback = libxl__device_cb;
+    libxl__initiate_device_remove(egc, aorm);
 
 out:
-    return AO_ABORT(rc);
+    if (rc) return AO_ABORT(rc);
+    return AO_INPROGRESS;
 }
 
 int libxl_device_nic_destroy(libxl_ctx *ctx, uint32_t domid,
-                                  libxl_device_nic *nic)
+                             libxl_device_nic *nic,
+                             const libxl_asyncop_how *ao_how)
 {
-    GC_INIT(ctx);
-    libxl__device device;
+    AO_CREATE(ctx, domid, ao_how);
+    libxl__device *device;
+    libxl__ao_device *aorm;
     int rc;
 
-    rc = libxl__device_from_nic(gc, domid, nic, &device);
+    GCNEW(device);
+    rc = libxl__device_from_nic(gc, domid, nic, device);
     if (rc != 0) goto out;
 
-    rc = libxl__device_destroy(gc, &device);
+    GCNEW(aorm);
+    libxl__init_ao_device(aorm, ao, NULL);
+    aorm->action = DEVICE_DISCONNECT;
+    aorm->force = 1;
+    aorm->dev = device;
+    aorm->callback = libxl__device_cb;
+    libxl__initiate_device_remove(egc, aorm);
+
 out:
-    GC_FREE;
-    return rc;
+    if (rc) return AO_ABORT(rc);
+    return AO_INPROGRESS;
 }
 
 static void libxl__device_nic_from_xs_be(libxl__gc *gc,
@@ -2285,35 +2335,50 @@ int libxl_device_vkb_remove(libxl_ctx *ctx, uint32_t domid,
                             const libxl_asyncop_how *ao_how)
 {
     AO_CREATE(ctx, domid, ao_how);
-    libxl__device device;
+    libxl__device *device;
+    libxl__ao_device *aorm;
     int rc;
 
-    rc = libxl__device_from_vkb(gc, domid, vkb, &device);
+    GCNEW(device);
+    rc = libxl__device_from_vkb(gc, domid, vkb, device);
     if (rc != 0) goto out;
 
-    rc = libxl__initiate_device_remove(egc, ao, &device);
-    if (rc) goto out;
-
-    return AO_INPROGRESS;
+    GCNEW(aorm);
+    libxl__init_ao_device(aorm, ao, NULL);
+    aorm->action = DEVICE_DISCONNECT;
+    aorm->dev = device;
+    aorm->callback = libxl__device_cb;
+    libxl__initiate_device_remove(egc, aorm);
 
 out:
-    return AO_ABORT(rc);
+    if (rc) return AO_ABORT(rc);
+    return AO_INPROGRESS;
 }
 
 int libxl_device_vkb_destroy(libxl_ctx *ctx, uint32_t domid,
-                                  libxl_device_vkb *vkb)
+                             libxl_device_vkb *vkb,
+                             const libxl_asyncop_how *ao_how)
 {
-    GC_INIT(ctx);
-    libxl__device device;
+    AO_CREATE(ctx, domid, ao_how);
+    libxl__device *device;
+    libxl__ao_device *aorm;
     int rc;
 
-    rc = libxl__device_from_vkb(gc, domid, vkb, &device);
+    GCNEW(device);
+    rc = libxl__device_from_vkb(gc, domid, vkb, device);
     if (rc != 0) goto out;
 
-    rc = libxl__device_destroy(gc, &device);
+    GCNEW(aorm);
+    libxl__init_ao_device(aorm, ao, NULL);
+    aorm->action = DEVICE_DISCONNECT;
+    aorm->force = 1;
+    aorm->dev = device;
+    aorm->callback = libxl__device_cb;
+    libxl__initiate_device_remove(egc, aorm);
+
 out:
-    GC_FREE;
-    return rc;
+    if (rc) return AO_ABORT(rc);
+    return AO_INPROGRESS;
 }
 
 /******************************************************************************/
@@ -2418,35 +2483,50 @@ int libxl_device_vfb_remove(libxl_ctx *ctx, uint32_t domid,
                             const libxl_asyncop_how *ao_how)
 {
     AO_CREATE(ctx, domid, ao_how);
-    libxl__device device;
+    libxl__device *device;
+    libxl__ao_device *aorm;
     int rc;
 
-    rc = libxl__device_from_vfb(gc, domid, vfb, &device);
+    GCNEW(device);
+    rc = libxl__device_from_vfb(gc, domid, vfb, device);
     if (rc != 0) goto out;
 
-    rc = libxl__initiate_device_remove(egc, ao, &device);
-    if (rc) goto out;
-
-    return AO_INPROGRESS;
+    GCNEW(aorm);
+    libxl__init_ao_device(aorm, ao, NULL);
+    aorm->action = DEVICE_DISCONNECT;
+    aorm->dev = device;
+    aorm->callback = libxl__device_cb;
+    libxl__initiate_device_remove(egc, aorm);
 
 out:
-    return AO_ABORT(rc);
+    if (rc) return AO_ABORT(rc);
+    return AO_INPROGRESS;
 }
 
 int libxl_device_vfb_destroy(libxl_ctx *ctx, uint32_t domid,
-                                  libxl_device_vfb *vfb)
+                             libxl_device_vfb *vfb,
+                             const libxl_asyncop_how *ao_how)
 {
-    GC_INIT(ctx);
-    libxl__device device;
+    AO_CREATE(ctx, domid, ao_how);
+    libxl__device *device;
+    libxl__ao_device *aorm;
     int rc;
 
-    rc = libxl__device_from_vfb(gc, domid, vfb, &device);
+    GCNEW(device);
+    rc = libxl__device_from_vfb(gc, domid, vfb, device);
     if (rc != 0) goto out;
 
-    rc = libxl__device_destroy(gc, &device);
+    GCNEW(aorm);
+    libxl__init_ao_device(aorm, ao, NULL);
+    aorm->action = DEVICE_DISCONNECT;
+    aorm->force = 1;
+    aorm->dev = device;
+    aorm->callback = libxl__device_cb;
+    libxl__initiate_device_remove(egc, aorm);
+
 out:
-    GC_FREE;
-    return rc;
+    if (rc) return AO_ABORT(rc);
+    return AO_INPROGRESS;
 }
 
 /******************************************************************************/
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 979940a..c3115a9 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -667,7 +667,8 @@ int libxl_device_disk_remove(libxl_ctx *ctx, uint32_t domid,
                              libxl_device_disk *disk,
                              const libxl_asyncop_how *ao_how);
 int libxl_device_disk_destroy(libxl_ctx *ctx, uint32_t domid,
-                              libxl_device_disk *disk);
+                              libxl_device_disk *disk,
+                              const libxl_asyncop_how *ao_how);
 
 libxl_device_disk *libxl_device_disk_list(libxl_ctx *ctx, uint32_t domid, int *num);
 int libxl_device_disk_getinfo(libxl_ctx *ctx, uint32_t domid,
@@ -691,7 +692,9 @@ int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic);
 int libxl_device_nic_remove(libxl_ctx *ctx, uint32_t domid,
                             libxl_device_nic *nic,
                             const libxl_asyncop_how *ao_how);
-int libxl_device_nic_destroy(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic);
+int libxl_device_nic_destroy(libxl_ctx *ctx, uint32_t domid,
+                             libxl_device_nic *nic,
+                             const libxl_asyncop_how *ao_how);
 
 libxl_device_nic *libxl_device_nic_list(libxl_ctx *ctx, uint32_t domid, int *num);
 int libxl_device_nic_getinfo(libxl_ctx *ctx, uint32_t domid,
@@ -702,14 +705,18 @@ int libxl_device_vkb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vkb *vkb);
 int libxl_device_vkb_remove(libxl_ctx *ctx, uint32_t domid,
                             libxl_device_vkb *vkb,
                             const libxl_asyncop_how *ao_how);
-int libxl_device_vkb_destroy(libxl_ctx *ctx, uint32_t domid, libxl_device_vkb *vkb);
+int libxl_device_vkb_destroy(libxl_ctx *ctx, uint32_t domid,
+                             libxl_device_vkb *vkb,
+                             const libxl_asyncop_how *ao_how);
 
 /* Framebuffer */
 int libxl_device_vfb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vfb *vfb);
 int libxl_device_vfb_remove(libxl_ctx *ctx, uint32_t domid,
                             libxl_device_vfb *vfb,
                             const libxl_asyncop_how *ao_how);
-int libxl_device_vfb_destroy(libxl_ctx *ctx, uint32_t domid, libxl_device_vfb *vfb);
+int libxl_device_vfb_destroy(libxl_ctx *ctx, uint32_t domid,
+                             libxl_device_vfb *vfb,
+                             const libxl_asyncop_how *ao_how);
 
 /* PCI Passthrough */
 int libxl_device_pci_add(libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev);
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 2006406..e572d4d 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -356,11 +356,16 @@ int libxl__device_disk_dev_number(const char *virtpath, int *pdisk,
     return -1;
 }
 
+/* Device AO operations */
 
-typedef struct {
-    libxl__ao *ao;
-    libxl__ev_devstate ds;
-} libxl__ao_device_remove;
+void libxl__init_ao_device(libxl__ao_device *aorm, libxl__ao *ao,
+                           libxl__ao_device **base)
+{
+    aorm->ao = ao;
+    aorm->active = 1;
+    aorm->rc = 0;
+    aorm->base = base;
+}
 
 int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
 {
@@ -436,34 +441,35 @@ out:
 
 /* Callbacks for device related operations */
 
-static void device_remove_callback(libxl__egc *egc, libxl__ev_devstate *ds,
+static void device_backend_callback(libxl__egc *egc, libxl__ev_devstate *ds,
                                    int rc);
 
-static void device_remove_cleanup(libxl__gc *gc,
-                                  libxl__ao_device_remove *aorm);
+static void device_backend_cleanup(libxl__gc *gc,
+                                   libxl__ao_device *aorm);
 
-int libxl__initiate_device_remove(libxl__egc *egc, libxl__ao *ao,
-                                  libxl__device *dev)
+void libxl__initiate_device_remove(libxl__egc *egc,
+                                   libxl__ao_device *aorm)
 {
-    AO_GC;
+    STATE_AO_GC(aorm->ao);
     libxl_ctx *ctx = libxl__gc_owner(gc);
-    xs_transaction_t t;
-    char *be_path = libxl__device_backend_path(gc, dev);
+    xs_transaction_t t = 0;
+    char *be_path = libxl__device_backend_path(gc, aorm->dev);
     char *state_path = libxl__sprintf(gc, "%s/state", be_path);
-    char *state = libxl__xs_read(gc, XBT_NULL, state_path);
+    char *state;
     int rc = 0;
-    libxl__ao_device_remove *aorm = 0;
 
+retry_transaction:
+    t = xs_transaction_start(ctx->xsh);
+    if (aorm->force)
+        libxl__xs_path_cleanup(gc, t,
+                               libxl__device_frontend_path(gc, aorm->dev));
+    state = libxl__xs_read(gc, t, state_path);
     if (!state)
         goto out_ok;
-    if (atoi(state) != 4) {
+    if (atoi(state) == XenbusStateClosed) {
         libxl__device_destroy_tapdisk(gc, be_path);
-        xs_rm(ctx->xsh, XBT_NULL, be_path);
         goto out_ok;
     }
-
-retry_transaction:
-    t = xs_transaction_start(ctx->xsh);
     xs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/online", be_path), "0", strlen("0"));
     xs_write(ctx->xsh, t, state_path, "5", strlen("5"));
     if (!xs_transaction_end(ctx->xsh, t, 0)) {
@@ -474,40 +480,71 @@ retry_transaction:
             goto out_fail;
         }
     }
+    /* mark transaction as ended, to prevent double closing it on out_ok */
+    t = 0;
 
     libxl__device_destroy_tapdisk(gc, be_path);
 
-    aorm = libxl__zalloc(gc, sizeof(*aorm));
-    aorm->ao = ao;
     libxl__ev_devstate_init(&aorm->ds);
 
-    rc = libxl__ev_devstate_wait(gc, &aorm->ds, device_remove_callback,
+    rc = libxl__ev_devstate_wait(gc, &aorm->ds, device_backend_callback,
                                  state_path, XenbusStateClosed,
                                  LIBXL_DESTROY_TIMEOUT * 1000);
-    if (rc) goto out_fail;
+    if (rc) {
+        LOG(ERROR, "unable to remove device %s", be_path);
+        goto out_fail;
+    }
 
-    return 0;
+    return;
 
  out_fail:
     assert(rc);
-    device_remove_cleanup(gc, aorm);
-    return rc;
+    aorm->rc = rc;
+    aorm->callback(egc, aorm);
+    return;
 
  out_ok:
-    libxl__ao_complete(egc, ao, 0);
-    return 0;
+    if (t) xs_transaction_end(ctx->xsh, t, 0);
+    aorm->callback(egc, aorm);
+    return;
 }
 
-static void device_remove_callback(libxl__egc *egc, libxl__ev_devstate *ds,
+static void device_backend_callback(libxl__egc *egc, libxl__ev_devstate *ds,
                                    int rc) {
-    libxl__ao_device_remove *aorm = CONTAINER_OF(ds, *aorm, ds);
-    libxl__gc *gc = &aorm->ao->gc;
-    libxl__ao_complete(egc, aorm->ao, rc);
-    device_remove_cleanup(gc, aorm);
+    libxl__ao_device *aorm = CONTAINER_OF(ds, *aorm, ds);
+    STATE_AO_GC(aorm->ao);
+
+    device_backend_cleanup(gc, aorm);
+
+    if (rc == ERROR_TIMEDOUT && aorm->action == DEVICE_DISCONNECT &&
+        !aorm->force) {
+        aorm->force = 1;
+        libxl__initiate_device_remove(egc, aorm);
+        return;
+    }
+
+    /* Some devices (vkbd) fail to disconnect properly,
+     * but we shouldn't alarm the user if it's during
+     * domain destruction.
+     */
+    if (rc && aorm->action == DEVICE_CONNECT) {
+        LOG(ERROR, "unable to connect device with path %s",
+                   libxl__device_backend_path(gc, aorm->dev));
+        goto out;
+    } else if(rc) {
+        LOG(DEBUG, "unable to disconnect device with path %s",
+                   libxl__device_backend_path(gc, aorm->dev));
+        goto out;
+    }
+
+out:
+    aorm->rc = rc;
+    aorm->callback(egc, aorm);
+    return;
 }
 
-static void device_remove_cleanup(libxl__gc *gc,
-                                  libxl__ao_device_remove *aorm) {
+static void device_backend_cleanup(libxl__gc *gc, libxl__ao_device *aorm)
+{
     if (!aorm) return;
     libxl__ev_devstate_cancel(gc, &aorm->ds);
 }
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index ae5527b..b62110a 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -827,13 +827,6 @@ _hidden const char *libxl__device_nic_devname(libxl__gc *gc,
                                               uint32_t devid,
                                               libxl_nic_type type);
 
-/* Arranges that dev will be removed from its guest.  When
- * this is done, the ao will be completed.  An error
- * return from libxl__initiate_device_remove means that the ao
- * will _not_ be completed and the caller must do so. */
-_hidden int libxl__initiate_device_remove(libxl__egc*, libxl__ao*,
-                                          libxl__device *dev);
-
 /*
  * libxl__ev_devstate - waits a given time for a device to
  * reach a given state.  Follows the libxl_ev_* conventions.
@@ -1802,6 +1795,44 @@ _hidden void libxl__bootloader_init(libxl__bootloader_state *bl);
  * If callback is passed rc==0, will have updated st->info appropriately */
 _hidden void libxl__bootloader_run(libxl__egc*, libxl__bootloader_state *st);
 
+/*----- device addition/removal -----*/
+
+/* Action to perform (either connect or disconnect) */
+typedef enum {
+    DEVICE_CONNECT,
+    DEVICE_DISCONNECT
+} libxl__device_action;
+
+typedef struct libxl__ao_device libxl__ao_device;
+typedef void libxl__device_callback(libxl__egc*, libxl__ao_device*);
+
+_hidden void libxl__init_ao_device(libxl__ao_device *aorm, libxl__ao *ao,
+                                   libxl__ao_device **base);
+
+struct libxl__ao_device {
+    /* filled in by user */
+    libxl__ao *ao;
+    /* action being performed */
+    libxl__device_action action;
+    libxl__device *dev;
+    int force;
+    libxl__device_callback *callback;
+    /* private for implementation */
+    /* State in which the device is */
+    int active;
+    int rc;
+    libxl__ev_devstate ds;
+    void *base;
+};
+
+/* Arranges that dev will be removed to the guest, and the
+ * hotplug scripts will be executed (if necessary). When
+ * this is done (or an error happens), the callback in
+ * aorm->callback will be called.
+ */
+_hidden void libxl__initiate_device_remove(libxl__egc *egc,
+                                           libxl__ao_device *aorm);
+
 /*----- Domain creation -----*/
 
 typedef struct libxl__domain_create_state libxl__domain_create_state;
-- 
1.7.7.5 (Apple Git-26)