|
[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
On Thu, 2012-07-26 at 20:12 +0100, Roger Pau Monne wrote:
> Don't have a fixed number of devices in the aodevs array, and instead
> size it depending on the devices present in xenstore. Also change the
> console destroy path to a switch instead of an if.
>
> Signed-off-by: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Ian Campbell <ian.campbell@xxxxxxxxxxxxx>
> ---
> tools/libxl/libxl_device.c | 123
> ++++++++++++++++++------------------------
> tools/libxl/libxl_internal.h | 10 +++-
> 2 files changed, 62 insertions(+), 71 deletions(-)
>
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index da0c3ea..4667261 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> int libxl__nic_type(libxl__gc *gc, libxl__device *dev, libxl_nic_type
> *nictype)
> {
> char *snictype, *be_path;
> @@ -451,9 +407,15 @@ void libxl__prepare_ao_devices(libxl__ao *ao,
> libxl__ao_devices *aodevs)
>
> 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]);
> + GCNEW(aodevs->array[i]);
> + aodevs->array[i]->aodevs = aodevs;
> + libxl__prepare_ao_device(ao, aodevs->array[i]);
> }
> + /*
> + * Since we have already added all the aodevs, and marked them
> + * as active, we can mark the addition operation as finished.
> + */
> + aodevs->in_addition = 0;
> }
>
> void libxl__ao_devices_callback(libxl__egc *egc, libxl__ao_device *aodev)
>
> - 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);
> + /*
> + * Initialize values of the aodevs structure manually,
> + * since we don't know the number of devices we are going
> + * to unplug yet.
> + */
> + aodevs->in_addition = 1;
> + aodevs->size = 0;
> + aodevs->array = NULL;
This suggests to me that libxl__prepare_ao_devices should take the size
as an argument and initialise that too, setting the array to NULL or
allocating it as appropriate.
> aodevs->callback = devices_remove_callback;
>
> path = libxl__sprintf(gc, "/local/domain/%d/device", domid);
> @@ -588,21 +553,28 @@ void libxl__devices_destroy(libxl__egc *egc,
> libxl__devices_remove_state *drs)
> dev->domid = domid;
> dev->kind = kind;
> dev->devid = atoi(devs[j]);
> - if (dev->backend_kind == LIBXL__DEVICE_KIND_CONSOLE) {
> + switch (dev->backend_kind) {
> + case LIBXL__DEVICE_KIND_CONSOLE:
> /* Currently console devices can be destroyed
> * synchronously by just removing xenstore entries,
> * this is what libxl__device_destroy does.
> */
> libxl__device_destroy(gc, dev);
> - continue;
> + break;
> + default:
> + GCREALLOC_ARRAY(aodevs->array, aodevs->size + 1);
> + GCNEW(aodev);
> + aodevs->array[aodevs->size] = aodev;
> + libxl__prepare_ao_device(ao, aodev);
You could make this return a new aodev,
aodev = libxl__prepare_ao_device(ao);
?
You might even encapsulate the array realloc into
aodev = libxl__ao_devices_append(aodevs)
?
> + aodev->aodevs = aodevs;
> + aodev->action = DEVICE_DISCONNECT;
> + aodev->dev = dev;
> + aodev->callback = libxl__ao_devices_callback;
> + aodev->force = drs->force;
> + libxl__initiate_device_remove(egc, aodev);
> + aodevs->size++;
> + break;
> }
> - aodev = &aodevs->array[numdev];
> - 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,7 +596,18 @@ void libxl__devices_destroy(libxl__egc *egc,
> libxl__devices_remove_state *drs)
> }
>
> out:
> - if (!numdev) drs->callback(egc, drs, rc);
> + if (!aodevs->size) drs->callback(egc, drs, rc);
> + else {
> + /*
> + * Create a dummy aodev to check if all the other aodevs are
> finished,
> + * and call the callback.
> + */
> + GCNEW(aodev);
> + libxl__prepare_ao_device(ao, aodev);
> + aodev->aodevs = aodevs;
> + aodevs->in_addition = 0;
> + libxl__ao_devices_callback(egc, aodev);
> + }
This dummy event strikes me as a bit odd.
Would it not be possible to loop over xenstore creating the aodevs array
and only then have a second loop which calls
libxl__initiate_device_remove(egc, aodev) for each in turn? Probably you
would encapsulate this into a helper libxl__as_devices_initiate which
would call the callback if size == 0.
That would do away with this whole "in_addition" phase since you
wouldn't actually initiate anything until you had created the entire
list.
> return;
> }
>
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 3e91529..3ee3a09 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1808,6 +1808,8 @@ _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.
> + * Once this function is called, the aodevs->size value cannot be changed,
> + * and no further aodevs can be added to the array.
> */
> _hidden void libxl__prepare_ao_devices(libxl__ao *ao,
> libxl__ao_devices *aodevs);
> @@ -1849,9 +1851,15 @@ struct libxl__ao_device {
> */
> typedef void libxl__devices_callback(libxl__egc*, libxl__ao_devices*, int
> rc);
> struct libxl__ao_devices {
> - libxl__ao_device *array;
> + libxl__ao_device **array;
> int size;
> libxl__devices_callback *callback;
> + /*
> + * if in_addition == 1 means we are currently adding devices
> + * to the array, if in_addition == 0, means we have added all
> + * the devices.
> + */
> + int in_addition;
> };
>
> /*
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |