libxl: fix device counting race in libxl__devices_destroy

Don't have a fixed number of devices in the aodevs array, and instead
size it depending on the devices present in xenstore.  Somewhat
formalise the multiple device addition/removal machinery to make this
clearer and easier to do.

As a side-effect we fix a few "lost thread of control" bug which would
occur if there were no devices of a particular kind.  (Various if
statements which checked for there being no devices have become
redundant, but are retained to avoid making the patch bigger.)

Specifically:

 * Users of libxl__ao_devices are no longer expected to know in
   advance how many device operations they are going to do.  Instead
   they can initiate them one at a time, between bracketing calls to
   "begin" and "prepared".

 * The array of aodevs used for this is dynamically sized; to support
   this it's an array of pointers rather than of structs.

 * Users of libxl__ao_devices are presented with a more opaque interface.
   They are are no longer expected to, themselves,
      - look into the array of aodevs (this is now private)
      - know that the individual addition/removal completions are
        handled by libxl__ao_devices_callback (this callback function
        is now a private function for the multidev machinery)
      - ever deal with populating the contents of an aodevs

 * The doc comments relating to some of the members of
   libxl__ao_device are clarified.  (And the member `aodevs' is moved
   to put it with the other members with the same status.)

 * The multidev machinery allocates an aodev to represent the
   operation of preparing all of the other operations.  See
   the comment in libxl__multidev_begin.

A wrinkle is that the functions are called "multidev" but the structs
are called "libxl__ao_devices" and "aodevs".  I have given these
functions this name to distinguish them from "libxl__ao_device" and
"aodev" and so forth by more than just the use of the plural "s"
suffix.

In the next patch we will rename the structs.

Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@eu.citrix.com>
Committed-by: Ian Campbell <ian.campbell@citrix.com>

tools/libxl/libxl_create.c

@@ -909,10 +909,10 @@ static void domcreate_rebuild_done(libxl__egc *egc,
 
     store_libxl_entry(gc, domid, &d_config->b_info);
 
-    dcs->aodevs.size = d_config->num_disks;
+    libxl__multidev_begin(ao, &dcs->aodevs);
     dcs->aodevs.callback = domcreate_launch_dm;
-    libxl__prepare_ao_devices(ao, &dcs->aodevs);
     libxl__add_disks(egc, ao, domid, 0, d_config, &dcs->aodevs);
+    libxl__multidev_prepared(egc, &dcs->aodevs, 0);
 
     return;
 
@@ -1039,10 +1039,10 @@ static void domcreate_devmodel_started(libxl__egc *egc,
     /* Plug nic interfaces */
     if (d_config->num_nics > 0) {
         /* Attach nics */
-        dcs->aodevs.size = d_config->num_nics;
+        libxl__multidev_begin(ao, &dcs->aodevs);
         dcs->aodevs.callback = domcreate_attach_pci;
-        libxl__prepare_ao_devices(ao, &dcs->aodevs);
         libxl__add_nics(egc, ao, domid, 0, d_config, &dcs->aodevs);
+        libxl__multidev_prepared(egc, &dcs->aodevs, 0);
         return;
     }
 

tools/libxl/libxl_device.c

@@ -58,50 +58,6 @@ int libxl__parse_backend_path(libxl__gc *gc,
     return libxl__device_kind_from_string(strkind, &dev->backend_kind);
 }
 
-static int libxl__num_devices(libxl__gc *gc, uint32_t domid)
-{
-    char *path;
-    unsigned int num_kinds, num_devs;
-    char **kinds = NULL, **devs = NULL;
-    int i, j, rc = 0;
-    libxl__device dev;
-    libxl__device_kind kind;
-    int numdevs = 0;
-
-    path = GCSPRINTF("/local/domain/%d/device", domid);
-    kinds = libxl__xs_directory(gc, XBT_NULL, path, &num_kinds);
-    if (!kinds) {
-        if (errno != ENOENT) {
-            LOGE(ERROR, "unable to get xenstore device listing %s", path);
-            rc = ERROR_FAIL;
-            goto out;
-        }
-        num_kinds = 0;
-    }
-    for (i = 0; i < num_kinds; i++) {
-        if (libxl__device_kind_from_string(kinds[i], &kind))
-            continue;
-        if (kind == LIBXL__DEVICE_KIND_CONSOLE)
-            continue;
-
-        path = GCSPRINTF("/local/domain/%d/device/%s", domid, kinds[i]);
-        devs = libxl__xs_directory(gc, XBT_NULL, path, &num_devs);
-        if (!devs)
-            continue;
-        for (j = 0; j < num_devs; j++) {
-            path = GCSPRINTF("/local/domain/%d/device/%s/%s/backend",
-                             domid, kinds[i], devs[j]);
-            path = libxl__xs_read(gc, XBT_NULL, path);
-            if (path && libxl__parse_backend_path(gc, path, &dev) == 0) {
-                numdevs++;
-            }
-        }
-    }
-out:
-    if (rc) return rc;
-    return numdevs;
-}
-
 int libxl__nic_type(libxl__gc *gc, libxl__device *dev, libxl_nic_type *nictype)
 {
     char *snictype, *be_path;
@@ -445,40 +401,81 @@ void libxl__prepare_ao_device(libxl__ao *ao, libxl__ao_device *aodev)
     libxl__ev_child_init(&aodev->child);
 }
 
-void libxl__prepare_ao_devices(libxl__ao *ao, libxl__ao_devices *aodevs)
+/* multidev */
+
+void libxl__multidev_begin(libxl__ao *ao, libxl__ao_devices *aodevs)
 {
     AO_GC;
 
-    GCNEW_ARRAY(aodevs->array, aodevs->size);
-    for (int i = 0; i < aodevs->size; i++) {
-        aodevs->array[i].aodevs = aodevs;
-        libxl__prepare_ao_device(ao, &aodevs->array[i]);
-    }
+    aodevs->ao = ao;
+    aodevs->array = 0;
+    aodevs->used = aodevs->allocd = 0;
+
+    /* We allocate an aodev to represent the operation of preparing
+     * all of the other operations.  This operation is completed when
+     * we have started all the others (ie, when the user calls
+     * _prepared).  That arranges automatically that
+     *  (i) we do not think we have finished even if one of the
+     *      operations completes while we are still preparing
+     *  (ii) if we are starting zero operations, we do still
+     *      make the callback as soon as we know this fact
+     *  (iii) we have a nice consistent way to deal with any
+     *      error that might occur while deciding what to initiate
+     */
+    aodevs->preparation = libxl__multidev_prepare(aodevs);
 }
 
-void libxl__ao_devices_callback(libxl__egc *egc, libxl__ao_device *aodev)
+static void multidev_one_callback(libxl__egc *egc, libxl__ao_device *aodev);
+
+libxl__ao_device *libxl__multidev_prepare(libxl__ao_devices *aodevs) {
+    STATE_AO_GC(aodevs->ao);
+    libxl__ao_device *aodev;
+
+    GCNEW(aodev);
+    aodev->aodevs = aodevs;
+    aodev->callback = multidev_one_callback;
+    libxl__prepare_ao_device(ao, aodev);
+
+    if (aodevs->used >= aodevs->allocd) {
+        aodevs->allocd = aodevs->used * 2 + 5;
+        GCREALLOC_ARRAY(aodevs->array, aodevs->allocd);
+    }
+    aodevs->array[aodevs->used++] = aodev;
+
+    return aodev;
+}
+
+static void multidev_one_callback(libxl__egc *egc, libxl__ao_device *aodev)
 {
     STATE_AO_GC(aodev->ao);
     libxl__ao_devices *aodevs = aodev->aodevs;
     int i, error = 0;
 
     aodev->active = 0;
-    for (i = 0; i < aodevs->size; i++) {
-        if (aodevs->array[i].active)
+
+    for (i = 0; i < aodevs->used; i++) {
+        if (aodevs->array[i]->active)
             return;
 
-        if (aodevs->array[i].rc)
-            error = aodevs->array[i].rc;
+        if (aodevs->array[i]->rc)
+            error = aodevs->array[i]->rc;
     }
 
     aodevs->callback(egc, aodevs, error);
     return;
 }
 
+void libxl__multidev_prepared(libxl__egc *egc, libxl__ao_devices *aodevs,
+                              int rc)
+{
+    aodevs->preparation->rc = rc;
+    multidev_one_callback(egc, aodevs->preparation);
+}
+
 /******************************************************************************/
 
 /* Macro for defining the functions that will add a bunch of disks when
- * inside an async op.
+ * inside an async op with multidev.
  * This macro is added to prevent repetition of code.
  *
  * The following functions are defined:
@@ -495,9 +492,9 @@ void libxl__ao_devices_callback(libxl__egc *egc, libxl__ao_device *aodev)
         int i;                                                                 \
         int end = start + d_config->num_##type##s;                             \
         for (i = start; i < end; i++) {                                        \
-            aodevs->array[i].callback = libxl__ao_devices_callback;            \
+            libxl__ao_device *aodev = libxl__multidev_prepare(aodevs);         \
             libxl__device_##type##_add(egc, domid, &d_config->type##s[i-start],\
-                                       &aodevs->array[i]);                     \
+                                       aodev);                                 \
         }                                                                      \
     }
 
@@ -547,20 +544,13 @@ void libxl__devices_destroy(libxl__egc *egc, libxl__devices_remove_state *drs)
     char *path;
     unsigned int num_kinds, num_dev_xsentries;
     char **kinds = NULL, **devs = NULL;
-    int i, j, numdev = 0, rc = 0;
+    int i, j, rc = 0;
     libxl__device *dev;
     libxl__ao_devices *aodevs = &drs->aodevs;
     libxl__ao_device *aodev;
     libxl__device_kind kind;
 
-    aodevs->size = libxl__num_devices(gc, drs->domid);
-    if (aodevs->size < 0) {
-        LOG(ERROR, "unable to get number of devices for domain %u", drs->domid);
-        rc = aodevs->size;
-        goto out;
-    }
-
-    libxl__prepare_ao_devices(drs->ao, aodevs);
+    libxl__multidev_begin(ao, aodevs);
     aodevs->callback = devices_remove_callback;
 
     path = libxl__sprintf(gc, "/local/domain/%d/device", domid);
@@ -598,13 +588,11 @@ void libxl__devices_destroy(libxl__egc *egc, libxl__devices_remove_state *drs)
                     libxl__device_destroy(gc, dev);
                     continue;
                 }
-                aodev = &aodevs->array[numdev];
+                aodev = libxl__multidev_prepare(aodevs);
                 aodev->action = DEVICE_DISCONNECT;
                 aodev->dev = dev;
-                aodev->callback = libxl__ao_devices_callback;
                 aodev->force = drs->force;
                 libxl__initiate_device_remove(egc, aodev);
-                numdev++;
             }
         }
     }
@@ -626,8 +614,7 @@ void libxl__devices_destroy(libxl__egc *egc, libxl__devices_remove_state *drs)
     }
 
 out:
-    if (!numdev) drs->callback(egc, drs, rc);
-    return;
+    libxl__multidev_prepared(egc, aodevs, rc);
 }
 
 /* Callbacks for device related operations */

tools/libxl/libxl_dm.c

@@ -856,10 +856,10 @@ retry_transaction:
         if (errno == EAGAIN)
             goto retry_transaction;
 
-    sdss->aodevs.size = dm_config->num_disks;
+    libxl__multidev_begin(ao, &sdss->aodevs);
     sdss->aodevs.callback = spawn_stub_launch_dm;
-    libxl__prepare_ao_devices(ao, &sdss->aodevs);
     libxl__add_disks(egc, ao, dm_domid, 0, dm_config, &sdss->aodevs);
+    libxl__multidev_prepared(egc, &sdss->aodevs, 0);
 
     free(args);
     return;
@@ -982,10 +982,10 @@ static void spawn_stubdom_pvqemu_cb(libxl__egc *egc,
     if (rc) goto out;
 
     if (d_config->num_nics > 0) {
-        sdss->aodevs.size = d_config->num_nics;
+        libxl__multidev_begin(ao, &sdss->aodevs);
         sdss->aodevs.callback = stubdom_pvqemu_cb;
-        libxl__prepare_ao_devices(ao, &sdss->aodevs);
         libxl__add_nics(egc, ao, dm_domid, 0, d_config, &sdss->aodevs);
+        libxl__multidev_prepared(egc, &sdss->aodevs, 0);
         return;
     }
 

tools/libxl/libxl_internal.h

@@ -1816,20 +1816,6 @@ typedef void libxl__device_callback(libxl__egc*, libxl__ao_device*);
  */
 _hidden void libxl__prepare_ao_device(libxl__ao *ao, libxl__ao_device *aodev);
 
-/* Prepare a bunch of devices for addition/removal. Every ao_device in
- * ao_devices is set to 'active', and the ao_device 'base' field is set to
- * the one pointed by aodevs.
- */
-_hidden void libxl__prepare_ao_devices(libxl__ao *ao,
-                                       libxl__ao_devices *aodevs);
-
-/* Generic callback to use when adding/removing several devices, this will
- * check if the given aodev is the last one, and call the callback in the
- * parent libxl__ao_devices struct, passing the appropriate error if found.
- */
-_hidden void libxl__ao_devices_callback(libxl__egc *egc,
-                                        libxl__ao_device *aodev);
-
 struct libxl__ao_device {
     /* filled in by user */
     libxl__ao *ao;
@@ -1837,32 +1823,60 @@ struct libxl__ao_device {
     libxl__device *dev;
     int force;
     libxl__device_callback *callback;
-    /* private for implementation */
-    int active;
+    /* return value, zeroed by user on entry, is valid on callback */
     int rc;
+    /* private for multidev */
+    int active;
+    libxl__ao_devices *aodevs; /* reference to the containing multidev */
+    /* private for add/remove implementation */
     libxl__ev_devstate backend_ds;
     /* Bodge for Qemu devices, also used for timeout of hotplug execution */
     libxl__ev_time timeout;
-    /* Used internally to have a reference to the upper libxl__ao_devices
-     * struct when present */
-    libxl__ao_devices *aodevs;
     /* device hotplug execution */
     const char *what;
     int num_exec;
     libxl__ev_child child;
 };
 
-/* Helper struct to simply the plug/unplug of multiple devices at the same
- * time.
+/*
+ * Multiple devices "multidev" handling.
  *
- * This structure holds several devices, and the callback is only called
- * when all the devices inside of the array have finished.
+ * Firstly, you should
+ *    libxl__multidev_begin
+ *    multidev->callback = ...
+ * Then zero or more times
+ *    libxl__multidev_prepare
+ *    libal__initiate_device_{remove/addition}.
+ * Finally, once
+ *    libxl__multidev_prepared
+ * which will result (perhaps reentrantly) in one call to callback().
  */
+
+/* Starts preparing to add/remove a bunch of devices. */
+_hidden void libxl__multidev_begin(libxl__ao *ao, libxl__ao_devices*);
+
+/* Prepares to add/remove one of many devices.  Returns a libxl__ao_device
+ * which has had libxl__prepare_ao_device called, and which has also
+ * had ->callback set.  The user should not mess with aodev->callback. */
+_hidden libxl__ao_device *libxl__multidev_prepare(libxl__ao_devices*);
+
+/* Notifies the multidev machinery that we have now finished preparing
+ * and initiating devices.  multidev->callback may then be called as
+ * soon as there are no prepared but not completed operations
+ * outstanding, perhaps reentrantly.  If rc!=0 (error should have been
+ * logged) multidev->callback will get a non-zero rc.
+ * callback may be set by the user at any point before prepared. */
+_hidden void libxl__multidev_prepared(libxl__egc*, libxl__ao_devices*, int rc);
+
 typedef void libxl__devices_callback(libxl__egc*, libxl__ao_devices*, int rc);
 struct libxl__ao_devices {
-    libxl__ao_device *array;
-    int size;
+    /* set by user: */
     libxl__devices_callback *callback;
+    /* for private use by libxl__...ao_devices... machinery: */
+    libxl__ao *ao;
+    libxl__ao_device **array;
+    int used, allocd;
+    libxl__ao_device *preparation;
 };
 
 /*
@@ -2372,10 +2386,11 @@ _hidden void libxl__devices_destroy(libxl__egc *egc,
                                     libxl__devices_remove_state *drs);
 
 /* Helper function to add a bunch of disks. This should be used when
- * the caller is inside an async op. "devices" will NOT be prepared by this
- * function, so the caller must make sure to call _prepare before calling this
- * function. The start parameter contains the position inside the aodevs array
- * that should be used to store the state of this devices.
+ * the caller is inside an async op. "devices" will NOT be prepared by
+ * this function, so the caller must make sure to call
+ * libxl__multidev_begin before calling this function. The start
+ * parameter contains the position inside the aodevs array that should
+ * be used to store the state of this devices.
  *
  * The "callback" will be called for each device, and the user is responsible
  * for calling libxl__ao_device_check_last on the callback.