|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 1/2] libxl: fix race in libxl__devices_destroy
Ian Jackson writes ("Re: [PATCH 1/2] libxl: fix race in
libxl__devices_destroy"):
> How about making libxl__prepare_ao_devices automatically create the
> "we are still searching for stuff to do" aodev ?
Here is what I came up with. I have compiled it but not tested it
yet. Despite me introducing some new functions the number of lines of
code is unchanged - indeed, it shrinks if you don't count comments
:-).
Ian.
From: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Subject: [PATCH RFC] 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. Perhaps eventually we will rename the structs.
Signed-off-by: Roger Pau Monne <roger.pau@xxxxxxxxxx>
Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Cc: Ian Campbell <ian.campbell@xxxxxxxxxxxxx>
-
Changes in v3:
* New multidev interfaces - extensive changes.
---
tools/libxl/libxl_create.c | 8 +-
tools/libxl/libxl_device.c | 128 +++++++++++++++++++-----------------------
tools/libxl/libxl_dm.c | 8 +-
tools/libxl/libxl_internal.h | 66 +++++++++++++---------
4 files changed, 105 insertions(+), 105 deletions(-)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index aafacd8..3265d69 100644
--- a/tools/libxl/libxl_create.c
+++ b/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;
}
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index da0c3ea..d3dfdbc 100644
--- a/tools/libxl/libxl_device.c
+++ b/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,80 @@ 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);
+}
+
+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;
}
-void libxl__ao_devices_callback(libxl__egc *egc, libxl__ao_device *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)
+{
+ 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 +491,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);
\
}
\
}
@@ -545,20 +541,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);
@@ -596,13 +585,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++;
}
}
}
@@ -624,8 +611,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 */
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index f2e9572..177642b 100644
--- a/tools/libxl/libxl_dm.c
+++ b/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;
}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index f0c94e8..7072b09 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1810,20 +1810,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;
@@ -1831,32 +1817,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 multidevs */
+ 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.
- *
- * This structure holds several devices, and the callback is only called
- * when all the devices inside of the array have finished.
- */
+/*
+ * Multiple devices "multidevs" handling.
+ *
+ * Firstly, you should
+ * libxl__multidev_begin
+ * multidevs->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;
};
/*
--
tg: (45b79a7..) t/xen/xl.devnum-race (depends on: t/xen/xl.pty-pollhup)
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |