* [PATCH v4 01/10] libxl: change libxl__ao_device_remove to libxl__ao_device
2012-05-24 10:23 [PATCH v4 0/10] execute hotplug scripts from libxl Roger Pau Monne
@ 2012-05-24 10:23 ` Roger Pau Monne
2012-05-29 13:32 ` Ian Jackson
2012-05-24 10:23 ` [PATCH v4 02/10] libxl: move device model creation prototypes Roger Pau Monne
` (8 subsequent siblings)
9 siblings, 1 reply; 26+ messages in thread
From: Roger Pau Monne @ 2012-05-24 10:23 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Roger Pau Monne
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.
Changes since v2:
* Remove unnecessary comments in libxl__ao_device.
* Change libxl__device_cb to device_addrm_aocomplete.
* Rename aorm parameter in device_addrm_aocomplete to aodev.
* Use a macro to define {nic,disk,vkb,vfb}_{remove,destroy}
functions.
* Rename libxl__init_ao_device to libxl__prepare_ao_device and add a
comment explaining why we need to set active to 1.
* Replace al uses of aorm with aodev.
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
---
tools/libxl/libxl.c | 211 ++++++++++++++----------------------------
tools/libxl/libxl.h | 15 ++-
tools/libxl/libxl_device.c | 113 +++++++++++++++--------
tools/libxl/libxl_internal.h | 49 +++++++++--
4 files changed, 197 insertions(+), 191 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index e3d05c2..7fdecf1 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1317,6 +1317,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 device_addrm_aocomplete(libxl__egc *egc, libxl__ao_device *aodev)
+{
+ STATE_AO_GC(aodev->ao);
+
+ if (aodev->rc) {
+ LOGE(ERROR, "unable to %s %s with id %u",
+ aodev->action == DEVICE_CONNECT ? "add" : "remove",
+ libxl__device_kind_to_string(aodev->dev->kind),
+ aodev->dev->devid);
+ goto out;
+ }
+
+out:
+ libxl__ao_complete(egc, ao, aodev->rc);
+ return;
+}
+
+/******************************************************************************/
+
int libxl__device_disk_setdefault(libxl__gc *gc, libxl_device_disk *disk)
{
int rc;
@@ -1486,42 +1506,6 @@ out:
return rc;
}
-int libxl_device_disk_remove(libxl_ctx *ctx, uint32_t domid,
- libxl_device_disk *disk,
- const libxl_asyncop_how *ao_how)
-{
- AO_CREATE(ctx, domid, ao_how);
- libxl__device device;
- int rc;
-
- 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;
-
-out:
- return AO_ABORT(rc);
-}
-
-int libxl_device_disk_destroy(libxl_ctx *ctx, uint32_t domid,
- libxl_device_disk *disk)
-{
- GC_INIT(ctx);
- libxl__device device;
- int rc;
-
- rc = libxl__device_from_disk(gc, domid, disk, &device);
- if (rc != 0) goto out;
-
- rc = libxl__device_destroy(gc, &device);
-out:
- GC_FREE;
- return rc;
-}
-
static void libxl__device_disk_from_xs_be(libxl__gc *gc,
const char *be_path,
libxl_device_disk *disk)
@@ -1964,42 +1948,6 @@ out:
return rc;
}
-int libxl_device_nic_remove(libxl_ctx *ctx, uint32_t domid,
- libxl_device_nic *nic,
- const libxl_asyncop_how *ao_how)
-{
- AO_CREATE(ctx, domid, ao_how);
- libxl__device device;
- int rc;
-
- 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;
-
-out:
- return AO_ABORT(rc);
-}
-
-int libxl_device_nic_destroy(libxl_ctx *ctx, uint32_t domid,
- libxl_device_nic *nic)
-{
- GC_INIT(ctx);
- libxl__device device;
- int rc;
-
- rc = libxl__device_from_nic(gc, domid, nic, &device);
- if (rc != 0) goto out;
-
- rc = libxl__device_destroy(gc, &device);
-out:
- GC_FREE;
- return rc;
-}
-
static void libxl__device_nic_from_xs_be(libxl__gc *gc,
const char *be_path,
libxl_device_nic *nic)
@@ -2326,42 +2274,6 @@ out:
return rc;
}
-int libxl_device_vkb_remove(libxl_ctx *ctx, uint32_t domid,
- libxl_device_vkb *vkb,
- const libxl_asyncop_how *ao_how)
-{
- AO_CREATE(ctx, domid, ao_how);
- libxl__device device;
- int rc;
-
- 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;
-
-out:
- return AO_ABORT(rc);
-}
-
-int libxl_device_vkb_destroy(libxl_ctx *ctx, uint32_t domid,
- libxl_device_vkb *vkb)
-{
- GC_INIT(ctx);
- libxl__device device;
- int rc;
-
- rc = libxl__device_from_vkb(gc, domid, vkb, &device);
- if (rc != 0) goto out;
-
- rc = libxl__device_destroy(gc, &device);
-out:
- GC_FREE;
- return rc;
-}
-
/******************************************************************************/
int libxl__device_vfb_setdefault(libxl__gc *gc, libxl_device_vfb *vfb)
@@ -2459,41 +2371,56 @@ out:
return rc;
}
-int libxl_device_vfb_remove(libxl_ctx *ctx, uint32_t domid,
- libxl_device_vfb *vfb,
- const libxl_asyncop_how *ao_how)
-{
- AO_CREATE(ctx, domid, ao_how);
- libxl__device device;
- int rc;
-
- 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;
-
-out:
- return AO_ABORT(rc);
-}
-
-int libxl_device_vfb_destroy(libxl_ctx *ctx, uint32_t domid,
- libxl_device_vfb *vfb)
-{
- GC_INIT(ctx);
- libxl__device device;
- int rc;
-
- rc = libxl__device_from_vfb(gc, domid, vfb, &device);
- if (rc != 0) goto out;
+/******************************************************************************/
- rc = libxl__device_destroy(gc, &device);
-out:
- GC_FREE;
- return rc;
-}
+/* Macro for defining device remove/destroy functions in a compact way */
+#define DEFINE_DEVICE_REMOVE(type, removedestroy, f) \
+ int libxl_device_##type##_##removedestroy(libxl_ctx *ctx, \
+ uint32_t domid, libxl_device_##type *type, \
+ const libxl_asyncop_how *ao_how) \
+ { \
+ AO_CREATE(ctx, domid, ao_how); \
+ libxl__device *device; \
+ libxl__ao_device *aodev; \
+ int rc; \
+ \
+ GCNEW(device); \
+ rc = libxl__device_from_##type(gc, domid, type, device); \
+ if (rc != 0) goto out; \
+ \
+ GCNEW(aodev); \
+ libxl__prepare_ao_device(aodev, ao, NULL); \
+ aodev->action = DEVICE_DISCONNECT; \
+ aodev->dev = device; \
+ aodev->callback = device_addrm_aocomplete; \
+ aodev->force = f; \
+ libxl__initiate_device_remove(egc, aodev); \
+ \
+ out: \
+ if (rc) return AO_ABORT(rc); \
+ return AO_INPROGRESS; \
+ }
+
+/* Define all remove/destroy functions and undef the macro */
+
+/* disk */
+DEFINE_DEVICE_REMOVE(disk, remove, 0)
+DEFINE_DEVICE_REMOVE(disk, destroy, 1)
+
+/* nic */
+DEFINE_DEVICE_REMOVE(nic, remove, 0)
+DEFINE_DEVICE_REMOVE(nic, destroy, 1)
+
+/* vkb */
+DEFINE_DEVICE_REMOVE(vkb, remove, 0)
+DEFINE_DEVICE_REMOVE(vkb, destroy, 1)
+
+/* vfb */
+
+DEFINE_DEVICE_REMOVE(vfb, remove, 0)
+DEFINE_DEVICE_REMOVE(vfb, destroy, 1)
+
+#undef DEFINE_DEVICE_REMOVE
/******************************************************************************/
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 316d290..24e4f43 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -674,7 +674,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,
@@ -698,7 +699,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,
@@ -709,14 +712,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..48f05f9 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__prepare_ao_device(libxl__ao_device *aodev, libxl__ao *ao,
+ libxl__ao_device **base)
+{
+ aodev->ao = ao;
+ aodev->active = 1;
+ aodev->rc = 0;
+ aodev->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 *aodev);
-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 *aodev)
{
- AO_GC;
+ STATE_AO_GC(aodev->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, aodev->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 (aodev->force)
+ libxl__xs_path_cleanup(gc, t,
+ libxl__device_frontend_path(gc, aodev->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,42 +480,73 @@ 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);
+ libxl__ev_devstate_init(&aodev->ds);
- rc = libxl__ev_devstate_wait(gc, &aorm->ds, device_remove_callback,
+ rc = libxl__ev_devstate_wait(gc, &aodev->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;
+ aodev->rc = rc;
+ aodev->callback(egc, aodev);
+ return;
out_ok:
- libxl__ao_complete(egc, ao, 0);
- return 0;
+ if (t) xs_transaction_end(ctx->xsh, t, 0);
+ aodev->callback(egc, aodev);
+ 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 *aodev = CONTAINER_OF(ds, *aodev, ds);
+ STATE_AO_GC(aodev->ao);
+
+ device_backend_cleanup(gc, aodev);
+
+ if (rc == ERROR_TIMEDOUT && aodev->action == DEVICE_DISCONNECT &&
+ !aodev->force) {
+ aodev->force = 1;
+ libxl__initiate_device_remove(egc, aodev);
+ return;
+ }
+
+ /* Some devices (vkbd) fail to disconnect properly,
+ * but we shouldn't alarm the user if it's during
+ * domain destruction.
+ */
+ if (rc && aodev->action == DEVICE_CONNECT) {
+ LOG(ERROR, "unable to connect device with path %s",
+ libxl__device_backend_path(gc, aodev->dev));
+ goto out;
+ } else if(rc) {
+ LOG(DEBUG, "unable to disconnect device with path %s",
+ libxl__device_backend_path(gc, aodev->dev));
+ goto out;
+ }
+
+out:
+ aodev->rc = rc;
+ aodev->callback(egc, aodev);
+ return;
}
-static void device_remove_cleanup(libxl__gc *gc,
- libxl__ao_device_remove *aorm) {
- if (!aorm) return;
- libxl__ev_devstate_cancel(gc, &aorm->ds);
+static void device_backend_cleanup(libxl__gc *gc, libxl__ao_device *aodev)
+{
+ if (!aodev) return;
+ libxl__ev_devstate_cancel(gc, &aodev->ds);
}
int libxl__wait_for_device_model(libxl__gc *gc,
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 52f5435..670a17b 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -830,13 +830,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.
@@ -1814,6 +1807,48 @@ _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*);
+
+/* This functions sets the necessary libxl__ao_device struct values to use
+ * safely inside functions. It marks the operation as "active"
+ * since we need to be sure that all device status structs are set
+ * to active before start queueing events, or we might call
+ * ao_complete before all devices had finished */
+_hidden void libxl__prepare_ao_device(libxl__ao_device *aodev, 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 */
+ 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
+ * aodev->callback will be called.
+ */
+_hidden void libxl__initiate_device_remove(libxl__egc *egc,
+ libxl__ao_device *aodev);
+
/*----- Domain creation -----*/
typedef struct libxl__domain_create_state libxl__domain_create_state;
--
1.7.7.5 (Apple Git-26)
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v4 01/10] libxl: change libxl__ao_device_remove to libxl__ao_device
2012-05-24 10:23 ` [PATCH v4 01/10] libxl: change libxl__ao_device_remove to libxl__ao_device Roger Pau Monne
@ 2012-05-29 13:32 ` Ian Jackson
2012-05-30 8:43 ` Roger Pau Monne
0 siblings, 1 reply; 26+ messages in thread
From: Ian Jackson @ 2012-05-29 13:32 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel@lists.xen.org
Roger Pau Monne writes ("[PATCH v4 01/10] libxl: change libxl__ao_device_remove to libxl__ao_device"):
> 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.
...
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index e3d05c2..7fdecf1 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
...
> +/* Macro for defining device remove/destroy functions in a compact way */
> +#define DEFINE_DEVICE_REMOVE(type, removedestroy, f) \
Looks reasonable to me.
> +/* Define all remove/destroy functions and undef the macro */
> +
> +/* disk */
> +DEFINE_DEVICE_REMOVE(disk, remove, 0)
> +DEFINE_DEVICE_REMOVE(disk, destroy, 1)
I'm not sure what purpose the comment serves but I don't really object
to it :-).
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 52f5435..670a17b 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
...
> @@ -830,13 +830,6 @@ _hidden const char *libxl__device_nic_devname(libxl__gc *
> +/* This functions sets the necessary libxl__ao_device struct values to use
> + * safely inside functions. It marks the operation as "active"
> + * since we need to be sure that all device status structs are set
> + * to active before start queueing events, or we might call
> + * ao_complete before all devices had finished */
> +_hidden void libxl__prepare_ao_device(libxl__ao_device *aodev, libxl__ao *ao,
> + libxl__ao_device **base);
You need to clearly explain what the constraints are on the order of
calling _prepare and _initiate_..._remove.
Questions whose answers should be clear from your text include:
- is it legal to call _initiate_ without having previously called
_prepare ?
- is it legal to call _prepare and then simply throw away the
aodev ?
- is it legal to call _prepare multiple times ? (If the answer
to the previous question is `yes' then it must be, I think.)
> @@ -436,34 +441,35 @@ out:
> -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 *aodev)
> {
...
> +retry_transaction:
> + t = xs_transaction_start(ctx->xsh);
> + if (aodev->force)
> + libxl__xs_path_cleanup(gc, t,
> + libxl__device_frontend_path(gc, aodev->dev));
> + state = libxl__xs_read(gc, t, state_path);
Didn't I previously comment adversely on having this check here ?
Ah yes, I commented on the corresponding code in add (v2 08/15):
[...], I think all of this is done for you by
libxl__ev_devstate_wait. You don't need to pre-check the state path
at all.
And:
Do you really need to do the xenstore state read here ? Surely
libxl__ev_devstate_wait will do it for you.
> + /* Some devices (vkbd) fail to disconnect properly,
> + * but we shouldn't alarm the user if it's during
> + * domain destruction.
> + */
> + if (rc && aodev->action == DEVICE_CONNECT) {
> + LOG(ERROR, "unable to connect device with path %s",
> + libxl__device_backend_path(gc, aodev->dev));
> + goto out;
> + } else if(rc) {
Missing space after `if'.
> + LOG(DEBUG, "unable to disconnect device with path %s",
> + libxl__device_backend_path(gc, aodev->dev));
> + goto out;
> + }
Last time I wrote:
Perhaps we should have a separate flag which controls error reporting
so that a user-requested graceful device disconnect gets full error
logging ?
Did you miss the fact that email suggesting the macro for generating
the device remove functions also had these other comments in ?
Thanks,
Ian.
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v4 01/10] libxl: change libxl__ao_device_remove to libxl__ao_device
2012-05-29 13:32 ` Ian Jackson
@ 2012-05-30 8:43 ` Roger Pau Monne
2012-05-31 11:13 ` Ian Jackson
0 siblings, 1 reply; 26+ messages in thread
From: Roger Pau Monne @ 2012-05-30 8:43 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel@lists.xen.org
Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH v4 01/10] libxl: change libxl__ao_device_remove to libxl__ao_device"):
>> 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.
> ...
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index e3d05c2..7fdecf1 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
> ...
>> +/* Macro for defining device remove/destroy functions in a compact way */
>> +#define DEFINE_DEVICE_REMOVE(type, removedestroy, f) \
>
> Looks reasonable to me.
>
>> +/* Define all remove/destroy functions and undef the macro */
>> +
>> +/* disk */
>> +DEFINE_DEVICE_REMOVE(disk, remove, 0)
>> +DEFINE_DEVICE_REMOVE(disk, destroy, 1)
>
> I'm not sure what purpose the comment serves but I don't really object
> to it :-).
>
>> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
>> index 52f5435..670a17b 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
> ...
>> @@ -830,13 +830,6 @@ _hidden const char *libxl__device_nic_devname(libxl__gc *
>> +/* This functions sets the necessary libxl__ao_device struct values to use
>> + * safely inside functions. It marks the operation as "active"
>> + * since we need to be sure that all device status structs are set
>> + * to active before start queueing events, or we might call
>> + * ao_complete before all devices had finished */
>> +_hidden void libxl__prepare_ao_device(libxl__ao_device *aodev, libxl__ao *ao,
>> + libxl__ao_device **base);
>
> You need to clearly explain what the constraints are on the order of
> calling _prepare and _initiate_..._remove.
>
> Questions whose answers should be clear from your text include:
> - is it legal to call _initiate_ without having previously called
> _prepare ?
> - is it legal to call _prepare and then simply throw away the
> aodev ?
> - is it legal to call _prepare multiple times ? (If the answer
> to the previous question is `yes' then it must be, I think.)
I've added a more detailed comment about _prepare.
>> @@ -436,34 +441,35 @@ out:
>> -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 *aodev)
>> {
> ...
>> +retry_transaction:
>> + t = xs_transaction_start(ctx->xsh);
>> + if (aodev->force)
>> + libxl__xs_path_cleanup(gc, t,
>> + libxl__device_frontend_path(gc, aodev->dev));
>> + state = libxl__xs_read(gc, t, state_path);
>
> Didn't I previously comment adversely on having this check here ?
>
> Ah yes, I commented on the corresponding code in add (v2 08/15):
>
> [...], I think all of this is done for you by
> libxl__ev_devstate_wait. You don't need to pre-check the state path
> at all.
>
> And:
>
> Do you really need to do the xenstore state read here ? Surely
> libxl__ev_devstate_wait will do it for you.
Done.
>> + /* Some devices (vkbd) fail to disconnect properly,
>> + * but we shouldn't alarm the user if it's during
>> + * domain destruction.
>> + */
>> + if (rc&& aodev->action == DEVICE_CONNECT) {
>> + LOG(ERROR, "unable to connect device with path %s",
>> + libxl__device_backend_path(gc, aodev->dev));
>> + goto out;
>> + } else if(rc) {
>
> Missing space after `if'.
Done.
>> + LOG(DEBUG, "unable to disconnect device with path %s",
>> + libxl__device_backend_path(gc, aodev->dev));
>> + goto out;
>> + }
>
> Last time I wrote:
>
> Perhaps we should have a separate flag which controls error reporting
> so that a user-requested graceful device disconnect gets full error
> logging ?
>
> Did you miss the fact that email suggesting the macro for generating
> the device remove functions also had these other comments in ?
I've replied to this email, the response is here:
http://marc.info/?l=xen-devel&m=133770109429587&w=2
I would like to leave this for future work, since the error checking we
have now is not better that what I'm proposing here.
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v4 01/10] libxl: change libxl__ao_device_remove to libxl__ao_device
2012-05-30 8:43 ` Roger Pau Monne
@ 2012-05-31 11:13 ` Ian Jackson
0 siblings, 0 replies; 26+ messages in thread
From: Ian Jackson @ 2012-05-31 11:13 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel@lists.xen.org
Roger Pau Monne writes ("Re: [PATCH v4 01/10] libxl: change libxl__ao_device_remove to libxl__ao_device"):
> Ian Jackson wrote:
> > Questions whose answers should be clear from your text include:
...
> I've added a more detailed comment about _prepare.
Thanks.
> > Did you miss the fact that email suggesting the macro for generating
> > the device remove functions also had these other comments in ?
>
> I've replied to this email, the response is here:
>
> http://marc.info/?l=xen-devel&m=133770109429587&w=2
Sorry, I seem to have dropped that mail. My fault.
> I would like to leave this for future work, since the error checking we
> have now is not better that what I'm proposing here.
OK.
Ian.
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v4 02/10] libxl: move device model creation prototypes
2012-05-24 10:23 [PATCH v4 0/10] execute hotplug scripts from libxl Roger Pau Monne
2012-05-24 10:23 ` [PATCH v4 01/10] libxl: change libxl__ao_device_remove to libxl__ao_device Roger Pau Monne
@ 2012-05-24 10:23 ` Roger Pau Monne
2012-05-24 10:23 ` [PATCH v4 03/10] libxl: convert libxl_domain_destroy to an async op Roger Pau Monne
` (7 subsequent siblings)
9 siblings, 0 replies; 26+ messages in thread
From: Roger Pau Monne @ 2012-05-24 10:23 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Roger Pau Monne
Move prototypes regarding device model creation, since they will
depend on domain destruction in future patches.
This patch is pure code motion.
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
---
tools/libxl/libxl_internal.h | 75 ++++++++++++++++++++---------------------
1 files changed, 37 insertions(+), 38 deletions(-)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 670a17b..bd71844 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1070,44 +1070,6 @@ static inline int libxl__spawn_inuse(libxl__spawn_state *ss)
_hidden int libxl__spawn_record_pid(libxl__gc*, libxl__spawn_state*,
pid_t innerchild);
-/*----- device model creation -----*/
-
-/* First layer; wraps libxl__spawn_spawn. */
-
-typedef struct libxl__dm_spawn_state libxl__dm_spawn_state;
-
-typedef void libxl__dm_spawn_cb(libxl__egc *egc, libxl__dm_spawn_state*,
- int rc /* if !0, error was logged */);
-
-struct libxl__dm_spawn_state {
- /* mixed - spawn.ao must be initialised by user; rest is private: */
- libxl__spawn_state spawn;
- /* filled in by user, must remain valid: */
- uint32_t guest_domid; /* domain being served */
- libxl_domain_config *guest_config;
- libxl__domain_build_state *build_state; /* relates to guest_domid */
- libxl__dm_spawn_cb *callback;
-};
-
-_hidden void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state*);
-
-/* Stubdom device models. */
-
-typedef struct {
- /* Mixed - user must fill in public parts EXCEPT callback,
- * which may be undefined on entry. (See above for details) */
- libxl__dm_spawn_state dm; /* the stub domain device model */
- /* filled in by user, must remain valid: */
- libxl__dm_spawn_cb *callback; /* called as callback(,&sdss->dm,) */
- /* private to libxl__spawn_stub_dm: */
- libxl_domain_config dm_config;
- libxl__domain_build_state dm_state;
- libxl__dm_spawn_state pvqemu;
-} libxl__stub_dm_spawn_state;
-
-_hidden void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state*);
-
-
/*
* libxl__wait_for_offspring - Wait for child state
* gc: allocation pool
@@ -1849,6 +1811,43 @@ struct libxl__ao_device {
_hidden void libxl__initiate_device_remove(libxl__egc *egc,
libxl__ao_device *aodev);
+/*----- device model creation -----*/
+
+/* First layer; wraps libxl__spawn_spawn. */
+
+typedef struct libxl__dm_spawn_state libxl__dm_spawn_state;
+
+typedef void libxl__dm_spawn_cb(libxl__egc *egc, libxl__dm_spawn_state*,
+ int rc /* if !0, error was logged */);
+
+struct libxl__dm_spawn_state {
+ /* mixed - spawn.ao must be initialised by user; rest is private: */
+ libxl__spawn_state spawn;
+ /* filled in by user, must remain valid: */
+ uint32_t guest_domid; /* domain being served */
+ libxl_domain_config *guest_config;
+ libxl__domain_build_state *build_state; /* relates to guest_domid */
+ libxl__dm_spawn_cb *callback;
+};
+
+_hidden void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state*);
+
+/* Stubdom device models. */
+
+typedef struct {
+ /* Mixed - user must fill in public parts EXCEPT callback,
+ * which may be undefined on entry. (See above for details) */
+ libxl__dm_spawn_state dm; /* the stub domain device model */
+ /* filled in by user, must remain valid: */
+ libxl__dm_spawn_cb *callback; /* called as callback(,&sdss->dm,) */
+ /* private to libxl__spawn_stub_dm: */
+ libxl_domain_config dm_config;
+ libxl__domain_build_state dm_state;
+ libxl__dm_spawn_state pvqemu;
+} libxl__stub_dm_spawn_state;
+
+_hidden void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state*);
+
/*----- Domain creation -----*/
typedef struct libxl__domain_create_state libxl__domain_create_state;
--
1.7.7.5 (Apple Git-26)
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v4 03/10] libxl: convert libxl_domain_destroy to an async op
2012-05-24 10:23 [PATCH v4 0/10] execute hotplug scripts from libxl Roger Pau Monne
2012-05-24 10:23 ` [PATCH v4 01/10] libxl: change libxl__ao_device_remove to libxl__ao_device Roger Pau Monne
2012-05-24 10:23 ` [PATCH v4 02/10] libxl: move device model creation prototypes Roger Pau Monne
@ 2012-05-24 10:23 ` Roger Pau Monne
2012-05-29 14:04 ` Ian Jackson
2012-05-24 10:23 ` [PATCH v4 04/10] libxl: convert libxl_device_disk_add to an asyn op Roger Pau Monne
` (6 subsequent siblings)
9 siblings, 1 reply; 26+ messages in thread
From: Roger Pau Monne @ 2012-05-24 10:23 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Roger Pau Monne
This change introduces some new structures, and breaks the mutual
dependency that libxl_domain_destroy and libxl__destroy_device_model
had. This is done by checking if the domid passed to
libxl_domain_destroy has a stubdom, and then having the bulk of the
destroy machinery in a separate function (libxl__destroy_domid) that
doesn't check for stubdom presence, since we check for it in the upper
level function. The reason behind this change is the need to use
structures for ao operations, and it was impossible to have two
different self-referencing structs.
All uses of libxl_domain_destroy have been changed, and either
replaced by the new libxl_domain_destroy ao function or by the
internal libxl__domain_destroy that can be used inside an already
running ao.
Changes since v3:
* Fixed python bindings.
Changes since v2:
* Remove printfs.
* Replace aorm with aodev.
* Define an auxiliary libxl__ao_device *aodev to avoid using the long
expression: drs->aorm[numdev]...
* Added a common callback for both domain and stubdomain destruction
that checks if both domains are finished and handles errors
correctly.
* Change libxl__ao_device_check_last logic a bit and add a comment
describing how does it work.
* Fixed spelling mistakes.
* Use a do-while for xs transaction in device_remove_callback.
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
---
tools/libxl/libxl.c | 167 +++++++++++++++++++++++++++++++++++--
tools/libxl/libxl.h | 3 +-
tools/libxl/libxl_create.c | 29 ++++++-
tools/libxl/libxl_device.c | 149 +++++++++++++++++++++++++++++----
tools/libxl/libxl_dm.c | 85 ++++++++++---------
tools/libxl/libxl_internal.h | 95 +++++++++++++++++++++-
tools/libxl/xl_cmdimpl.c | 12 ++--
tools/python/xen/lowlevel/xl/xl.c | 2 +-
8 files changed, 465 insertions(+), 77 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 7fdecf1..0b3eb48 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1112,11 +1112,133 @@ void libxl_evdisable_disk_eject(libxl_ctx *ctx, libxl_evgen_disk_eject *evg) {
GC_FREE;
}
-int libxl_domain_destroy(libxl_ctx *ctx, uint32_t domid)
+/* Callbacks for libxl_domain_destroy */
+
+static void domain_destroy_cb(libxl__egc *egc, libxl__domain_destroy_state *dds,
+ int rc);
+
+int libxl_domain_destroy(libxl_ctx *ctx, uint32_t domid,
+ const libxl_asyncop_how *ao_how)
{
- GC_INIT(ctx);
+ AO_CREATE(ctx, domid, ao_how);
+ libxl__domain_destroy_state *dds;
+
+ GCNEW(dds);
+ dds->ao = ao;
+ dds->domid = domid;
+ dds->callback = domain_destroy_cb;
+ libxl__domain_destroy(egc, dds);
+
+ return AO_INPROGRESS;
+}
+
+static void domain_destroy_cb(libxl__egc *egc, libxl__domain_destroy_state *dds,
+ int rc)
+{
+ STATE_AO_GC(dds->ao);
+
+ if (rc)
+ LOG(ERROR, "destruction of domain %u failed", dds->domid);
+
+ libxl__ao_complete(egc, ao, rc);
+}
+
+/* Callbacks for libxl__domain_destroy */
+
+static void stubdom_callback(libxl__egc *egc, libxl__destroy_domid_state *dis,
+ int rc);
+
+static void domain_callback(libxl__egc *egc, libxl__destroy_domid_state *dis,
+ int rc);
+
+static void destroy_finish_check(libxl__egc *egc,
+ libxl__domain_destroy_state *dds);
+
+void libxl__domain_destroy(libxl__egc *egc, libxl__domain_destroy_state *dds)
+{
+ STATE_AO_GC(dds->ao);
+ uint32_t stubdomid = libxl_get_stubdom_id(CTX, dds->domid);
+
+ if (stubdomid) {
+ dds->stubdom.ao = ao;
+ dds->stubdom.domid = stubdomid;
+ dds->stubdom.callback = stubdom_callback;
+ dds->stubdom.force = 1;
+ libxl__destroy_domid(egc, &dds->stubdom);
+ } else {
+ dds->stubdom_finished = 1;
+ }
+
+ dds->domain.ao = ao;
+ dds->domain.domid = dds->domid;
+ dds->domain.callback = domain_callback;
+ dds->domain.force = 1;
+ libxl__destroy_domid(egc, &dds->domain);
+}
+
+static void stubdom_callback(libxl__egc *egc, libxl__destroy_domid_state *dis,
+ int rc)
+{
+ STATE_AO_GC(dis->ao);
+ libxl__domain_destroy_state *dds = CONTAINER_OF(dis, *dds, stubdom);
+ const char *savefile;
+
+ if (rc) {
+ LOG(ERROR, "unable to destroy stubdom with domid %u", dis->domid);
+ dds->rc = rc;
+ }
+
+ dds->stubdom_finished = 1;
+ savefile = libxl__device_model_savefile(gc, dis->domid);
+ rc = unlink(savefile);
+ /*
+ * On suspend libxl__domain_save_device_model will have already
+ * unlinked the save file.
+ */
+ if (rc && errno == ENOENT) rc = 0;
+ if (rc) {
+ LOGEV(ERROR, errno, "failed to remove device-model savefile %s",
+ savefile);
+ }
+
+ destroy_finish_check(egc, dds);
+}
+
+static void domain_callback(libxl__egc *egc, libxl__destroy_domid_state *dis,
+ int rc)
+{
+ STATE_AO_GC(dis->ao);
+ libxl__domain_destroy_state *dds = CONTAINER_OF(dis, *dds, domain);
+
+ if (rc) {
+ LOG(ERROR, "unable to destroy guest with domid %u", dis->domid);
+ dds->rc = rc;
+ }
+
+ dds->domain_finished = 1;
+ destroy_finish_check(egc, dds);
+}
+
+static void destroy_finish_check(libxl__egc *egc,
+ libxl__domain_destroy_state *dds)
+{
+ if (!(dds->domain_finished && dds->stubdom_finished))
+ return;
+
+ dds->callback(egc, dds, dds->rc);
+}
+
+/* Callbacks for libxl__destroy_domid */
+static void devices_destroy_cb(libxl__egc *egc,
+ libxl__devices_remove_state *drs,
+ int rc);
+
+void libxl__destroy_domid(libxl__egc *egc, libxl__destroy_domid_state *dis)
+{
+ STATE_AO_GC(dis->ao);
+ libxl_ctx *ctx = CTX;
+ uint32_t domid = dis->domid;
char *dom_path;
- char *vm_path;
char *pid;
int rc, dm_present;
@@ -1127,7 +1249,7 @@ int libxl_domain_destroy(libxl_ctx *ctx, uint32_t domid)
case ERROR_INVAL:
LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "non-existant domain %d", domid);
default:
- return rc;
+ goto out;
}
switch (libxl__domain_type(gc, domid)) {
@@ -1160,7 +1282,37 @@ int libxl_domain_destroy(libxl_ctx *ctx, uint32_t domid)
libxl__qmp_cleanup(gc, domid);
}
- if (libxl__devices_destroy(gc, domid) < 0)
+ dis->drs.ao = ao;
+ dis->drs.domid = domid;
+ dis->drs.callback = devices_destroy_cb;
+ dis->drs.force = dis->force;
+ libxl__devices_destroy(egc, &dis->drs);
+ return;
+
+out:
+ assert(rc);
+ dis->callback(egc, dis, rc);
+ return;
+}
+
+static void devices_destroy_cb(libxl__egc *egc,
+ libxl__devices_remove_state *drs,
+ int rc)
+{
+ STATE_AO_GC(drs->ao);
+ libxl__destroy_domid_state *dis = CONTAINER_OF(drs, *dis, drs);
+ libxl_ctx *ctx = CTX;
+ uint32_t domid = dis->domid;
+ char *dom_path;
+ char *vm_path;
+
+ dom_path = libxl__xs_get_dompath(gc, domid);
+ if (!dom_path) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ if (rc < 0)
LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
"libxl__devices_destroy failed for %d", domid);
@@ -1183,9 +1335,10 @@ int libxl_domain_destroy(libxl_ctx *ctx, uint32_t domid)
goto out;
}
rc = 0;
+
out:
- GC_FREE;
- return rc;
+ dis->callback(egc, dis, rc);
+ return;
}
int libxl_console_exec(libxl_ctx *ctx, uint32_t domid, int cons_num, libxl_console_type type)
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 24e4f43..7933809 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -537,7 +537,8 @@ int libxl_domain_suspend(libxl_ctx *ctx, libxl_domain_suspend_info *info,
int libxl_domain_resume(libxl_ctx *ctx, uint32_t domid, int suspend_cancel);
int libxl_domain_shutdown(libxl_ctx *ctx, uint32_t domid);
int libxl_domain_reboot(libxl_ctx *ctx, uint32_t domid);
-int libxl_domain_destroy(libxl_ctx *ctx, uint32_t domid);
+int libxl_domain_destroy(libxl_ctx *ctx, uint32_t domid,
+ const libxl_asyncop_how *ao_how);
int libxl_domain_preserve(libxl_ctx *ctx, uint32_t domid, libxl_domain_create_info *info, const char *name_suffix, libxl_uuid new_uuid);
/* get max. number of cpus supported by hypervisor */
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 14721eb..4e88551 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -584,6 +584,12 @@ static void domcreate_complete(libxl__egc *egc,
libxl__domain_create_state *dcs,
int rc);
+/* If creation is not successful, this callback will be executed
+ * when domain destruction is finished */
+static void domcreate_destruction_cb(libxl__egc *egc,
+ libxl__domain_destroy_state *dds,
+ int rc);
+
static void initiate_domain_create(libxl__egc *egc,
libxl__domain_create_state *dcs)
{
@@ -848,16 +854,31 @@ static void domcreate_complete(libxl__egc *egc,
if (rc) {
if (dcs->guest_domid) {
- int rc2 = libxl_domain_destroy(CTX, dcs->guest_domid);
- if (rc2)
- LOG(ERROR, "unable to destroy domain %d following"
- " failed creation", dcs->guest_domid);
+ dcs->dds.ao = ao;
+ dcs->dds.domid = dcs->guest_domid;
+ dcs->dds.callback = domcreate_destruction_cb;
+ libxl__domain_destroy(egc, &dcs->dds);
+ return;
}
dcs->guest_domid = -1;
}
dcs->callback(egc, dcs, rc, dcs->guest_domid);
}
+static void domcreate_destruction_cb(libxl__egc *egc,
+ libxl__domain_destroy_state *dds,
+ int rc)
+{
+ STATE_AO_GC(dds->ao);
+ libxl__domain_create_state *dcs = CONTAINER_OF(dds, *dcs, dds);
+
+ if (rc)
+ LOG(ERROR, "unable to destroy domain %u following failed creation",
+ dds->domid);
+
+ dcs->callback(egc, dcs, ERROR_FAIL, dcs->guest_domid);
+}
+
/*----- application-facing domain creation interface -----*/
typedef struct {
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 48f05f9..413b98f 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -58,6 +58,48 @@ 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;
+
+ 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__device_generic_add(libxl__gc *gc, libxl__device *device,
char **bents, char **fents)
{
@@ -367,6 +409,23 @@ void libxl__prepare_ao_device(libxl__ao_device *aodev, libxl__ao *ao,
aodev->base = base;
}
+int libxl__ao_device_check_last(libxl__gc *gc, libxl__ao_device *device,
+ libxl__ao_device *list, int num)
+{
+ int i, pending = 0, error = 0;
+
+ device->active = 0;
+ for (i = 0; i < num; i++) {
+ if (list[i].active)
+ pending++;
+
+ if (list[i].rc)
+ error = list[i].rc;
+ }
+
+ return pending == 0 ? error : 1;
+}
+
int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
{
libxl_ctx *ctx = libxl__gc_owner(gc);
@@ -381,16 +440,35 @@ int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
return 0;
}
-int libxl__devices_destroy(libxl__gc *gc, uint32_t domid)
+/* Callback for device destruction */
+
+static void device_remove_callback(libxl__egc *egc, libxl__ao_device *aodev);
+
+void libxl__devices_destroy(libxl__egc *egc, libxl__devices_remove_state *drs)
{
+ STATE_AO_GC(drs->ao);
libxl_ctx *ctx = libxl__gc_owner(gc);
+ uint32_t domid = drs->domid;
char *path;
unsigned int num_kinds, num_devs;
char **kinds = NULL, **devs = NULL;
- int i, j;
- libxl__device dev;
+ int i, j, numdev = 0, rc = 0;
+ libxl__device *dev;
+ libxl__ao_device *aodev;
libxl__device_kind kind;
+ drs->num_devices = libxl__num_devices(gc, drs->domid);
+ if (drs->num_devices < 0) {
+ LOG(ERROR, "unable to get number of devices for domain %u", drs->domid);
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ GCNEW_ARRAY(drs->aodev, drs->num_devices);
+ for (i = 0; i < drs->num_devices; i++) {
+ libxl__prepare_ao_device(&drs->aodev[i], drs->ao, &drs->aodev);
+ }
+
path = libxl__sprintf(gc, "/local/domain/%d/device", domid);
kinds = libxl__xs_directory(gc, XBT_NULL, path, &num_kinds);
if (!kinds) {
@@ -413,12 +491,18 @@ int libxl__devices_destroy(libxl__gc *gc, uint32_t domid)
path = libxl__sprintf(gc, "/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) {
- dev.domid = domid;
- dev.kind = kind;
- dev.devid = atoi(devs[j]);
-
- libxl__device_destroy(gc, &dev);
+ GCNEW(dev);
+ if (path && libxl__parse_backend_path(gc, path, dev) == 0) {
+ aodev = &drs->aodev[numdev];
+ dev->domid = domid;
+ dev->kind = kind;
+ dev->devid = atoi(devs[j]);
+ aodev->action = DEVICE_DISCONNECT;
+ aodev->dev = dev;
+ aodev->callback = device_remove_callback;
+ aodev->force = drs->force;
+ libxl__initiate_device_remove(egc, aodev);
+ numdev++;
}
}
}
@@ -426,17 +510,19 @@ int libxl__devices_destroy(libxl__gc *gc, uint32_t domid)
/* console 0 frontend directory is not under /local/domain/<domid>/device */
path = libxl__sprintf(gc, "/local/domain/%d/console/backend", domid);
path = libxl__xs_read(gc, XBT_NULL, path);
+ GCNEW(dev);
if (path && strcmp(path, "") &&
- libxl__parse_backend_path(gc, path, &dev) == 0) {
- dev.domid = domid;
- dev.kind = LIBXL__DEVICE_KIND_CONSOLE;
- dev.devid = 0;
+ libxl__parse_backend_path(gc, path, dev) == 0) {
+ dev->domid = domid;
+ dev->kind = LIBXL__DEVICE_KIND_CONSOLE;
+ dev->devid = 0;
- libxl__device_destroy(gc, &dev);
+ libxl__device_destroy(gc, dev);
}
out:
- return 0;
+ if (!numdev) drs->callback(egc, drs, rc);
+ return;
}
/* Callbacks for device related operations */
@@ -549,6 +635,39 @@ static void device_backend_cleanup(libxl__gc *gc, libxl__ao_device *aodev)
libxl__ev_devstate_cancel(gc, &aodev->ds);
}
+static void device_remove_callback(libxl__egc *egc, libxl__ao_device *aodev)
+{
+ STATE_AO_GC(aodev->ao);
+ libxl__devices_remove_state *drs = CONTAINER_OF(aodev->base, *drs, aodev);
+ char *be_path = libxl__device_backend_path(gc, aodev->dev);
+ char *fe_path = libxl__device_frontend_path(gc, aodev->dev);
+ xs_transaction_t t = 0;
+ int rc = 0;
+
+ if (aodev->action == DEVICE_DISCONNECT) {
+ do {
+ t = xs_transaction_start(CTX->xsh);
+ libxl__xs_path_cleanup(gc, t, fe_path);
+ libxl__xs_path_cleanup(gc, t, be_path);
+ rc = !xs_transaction_end(CTX->xsh, t, 0);
+ } while (rc && errno == EAGAIN);
+
+ if (rc) {
+ LOGE(ERROR, "unable to finish transaction");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ }
+
+out:
+ rc = libxl__ao_device_check_last(gc, aodev, drs->aodev,
+ drs->num_devices);
+ if (rc > 0) return;
+
+ drs->callback(egc, drs, rc);
+ return;
+}
+
int libxl__wait_for_device_model(libxl__gc *gc,
uint32_t domid, char *state,
libxl__spawn_starting *spawning,
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 5bf9a0b..a403b4c 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -673,6 +673,10 @@ static void spawn_stubdom_pvqemu_cb(libxl__egc *egc,
libxl__dm_spawn_state *stubdom_dmss,
int rc);
+static void spaw_stubdom_pvqemu_destroy_cb(libxl__egc *egc,
+ libxl__destroy_domid_state *dis,
+ int rc);
+
void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
{
STATE_AO_GC(sdss->dm.spawn.ao);
@@ -892,12 +896,32 @@ static void spawn_stubdom_pvqemu_cb(libxl__egc *egc,
out:
if (rc) {
- if (dm_domid)
- libxl_domain_destroy(CTX, dm_domid);
+ if (dm_domid) {
+ sdss->dis.ao = ao;
+ sdss->dis.domid = dm_domid;
+ sdss->dis.force = 1;
+ sdss->dis.callback = spaw_stubdom_pvqemu_destroy_cb;
+ libxl__destroy_domid(egc, &sdss->dis);
+ return;
+ }
}
sdss->callback(egc, &sdss->dm, rc);
}
+static void spaw_stubdom_pvqemu_destroy_cb(libxl__egc *egc,
+ libxl__destroy_domid_state *dis,
+ int rc)
+{
+ libxl__stub_dm_spawn_state *sdss = CONTAINER_OF(dis, *sdss, dis);
+ STATE_AO_GC(sdss->dis.ao);
+
+ if (rc)
+ LOG(ERROR, "destruction of domain %u after failed creation failed",
+ sdss->pvqemu.guest_domid);
+
+ sdss->callback(egc, &sdss->dm, rc);
+}
+
/* callbacks passed to libxl__spawn_spawn */
static void device_model_confirm(libxl__egc *egc, libxl__spawn_state *spawn,
const char *xsdata);
@@ -1090,48 +1114,25 @@ int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
int ret;
pid = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "/local/domain/%d/image/device-model-pid", domid));
- if (!pid) {
- int stubdomid = libxl_get_stubdom_id(ctx, domid);
- const char *savefile;
-
- if (!stubdomid) {
- LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Couldn't find device model's pid");
- ret = ERROR_INVAL;
- goto out;
- }
- LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Device model is a stubdom, domid=%d", stubdomid);
- ret = libxl_domain_destroy(ctx, stubdomid);
- if (ret)
- goto out;
-
- savefile = libxl__device_model_savefile(gc, domid);
- ret = unlink(savefile);
- /*
- * On suspend libxl__domain_save_device_model will have already
- * unlinked the save file.
- */
- if (ret && errno == ENOENT) ret = 0;
- if (ret) {
- LIBXL__LOG_ERRNO(ctx, XTL_ERROR,
- "failed to remove device-model savefile %s\n",
- savefile);
- goto out;
- }
+ if (!pid || !atoi(pid)) {
+ LOG(ERROR, "could not find device-model's pid for dom %u", domid);
+ ret = ERROR_FAIL;
+ goto out;
+ }
+ ret = kill(atoi(pid), SIGHUP);
+ if (ret < 0 && errno == ESRCH) {
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Device Model already exited");
+ ret = 0;
+ } else if (ret == 0) {
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Device Model signaled");
+ ret = 0;
} else {
- ret = kill(atoi(pid), SIGHUP);
- if (ret < 0 && errno == ESRCH) {
- LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Device Model already exited");
- ret = 0;
- } else if (ret == 0) {
- LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Device Model signaled");
- ret = 0;
- } else {
- LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "failed to kill Device Model [%d]",
- atoi(pid));
- ret = ERROR_FAIL;
- goto out;
- }
+ LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "failed to kill Device Model [%d]",
+ atoi(pid));
+ ret = ERROR_FAIL;
+ goto out;
}
+
xs_rm(ctx->xsh, XBT_NULL, libxl__sprintf(gc, "/local/domain/0/device-model/%d", domid));
xs_rm(ctx->xsh, XBT_NULL, libxl__sprintf(gc, "/local/domain/%d/hvmloader", domid));
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index bd71844..9003583 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -799,7 +799,6 @@ _hidden char *libxl__device_frontend_path(libxl__gc *gc, libxl__device *device);
_hidden int libxl__parse_backend_path(libxl__gc *gc, const char *path,
libxl__device *dev);
_hidden int libxl__device_destroy(libxl__gc *gc, libxl__device *dev);
-_hidden int libxl__devices_destroy(libxl__gc *gc, uint32_t domid);
_hidden int libxl__wait_for_backend(libxl__gc *gc, char *be_path, char *state);
/*
@@ -1788,6 +1787,16 @@ typedef void libxl__device_callback(libxl__egc*, libxl__ao_device*);
_hidden void libxl__prepare_ao_device(libxl__ao_device *aodev, libxl__ao *ao,
libxl__ao_device **base);
+/* Check if there are devices that have pending events in the array
+ * pointed to by the "list" parameter. Return values can be:
+ * < 0: All done, but error(s) found.
+ * 0: All done
+ * > 0: Not all done
+ * The passed libxl__ao_device struct in "device" is marked as finished.
+ */
+_hidden int libxl__ao_device_check_last(libxl__gc *gc, libxl__ao_device *device,
+ libxl__ao_device *list, int num);
+
struct libxl__ao_device {
/* filled in by user */
libxl__ao *ao;
@@ -1811,6 +1820,87 @@ struct libxl__ao_device {
_hidden void libxl__initiate_device_remove(libxl__egc *egc,
libxl__ao_device *aodev);
+/*----- Domain destruction -----*/
+
+/* Domain destruction has been split into two functions:
+ *
+ * libxl__domain_destroy is the main destroy function, which detects
+ * stubdoms and calls libxl__destroy_domid on the domain and its
+ * stubdom if present, creating a different libxl__destroy_domid_state
+ * for each one of them.
+ *
+ * libxl__destroy_domid actually destroys the domain, but it
+ * doesn't check for stubdomains, since that would involve
+ * recursion, which we want to avoid.
+ */
+
+typedef struct libxl__domain_destroy_state libxl__domain_destroy_state;
+typedef struct libxl__destroy_domid_state libxl__destroy_domid_state;
+typedef struct libxl__devices_remove_state libxl__devices_remove_state;
+
+typedef void libxl__domain_destroy_cb(libxl__egc *egc,
+ libxl__domain_destroy_state *dds,
+ int rc);
+
+typedef void libxl__domid_destroy_cb(libxl__egc *egc,
+ libxl__destroy_domid_state *dis,
+ int rc);
+
+typedef void libxl__devices_remove_callback(libxl__egc *egc,
+ libxl__devices_remove_state *drs,
+ int rc);
+
+struct libxl__devices_remove_state {
+ /* filled in by user */
+ libxl__ao *ao;
+ uint32_t domid;
+ libxl__devices_remove_callback *callback;
+ int force; /* libxl_device_TYPE_destroy rather than _remove */
+ /* private */
+ libxl__ao_device *aodev;
+ int num_devices;
+};
+
+struct libxl__destroy_domid_state {
+ /* filled in by user */
+ libxl__ao *ao;
+ uint32_t domid;
+ libxl__domid_destroy_cb *callback;
+ int force; /* libxl_device_TYPE_destroy rather than _remove */
+ /* private to implementation */
+ libxl__devices_remove_state drs;
+};
+
+struct libxl__domain_destroy_state {
+ /* filled by the user */
+ libxl__ao *ao;
+ uint32_t domid;
+ libxl__domain_destroy_cb *callback;
+ /* Private */
+ int rc;
+ uint32_t stubdomid;
+ libxl__destroy_domid_state stubdom;
+ int stubdom_finished;
+ libxl__destroy_domid_state domain;
+ int domain_finished;
+};
+
+/*
+ * Entry point for domain destruction
+ * This function checks for stubdom presence and then calls
+ * libxl__destroy_domid on the passed domain and it's stubdom if found.
+ */
+_hidden void libxl__domain_destroy(libxl__egc *egc,
+ libxl__domain_destroy_state *dds);
+
+/* Used to destroy a domain with the passed id (it doesn't check for stubs) */
+_hidden void libxl__destroy_domid(libxl__egc *egc,
+ libxl__destroy_domid_state *dis);
+
+/* Entry point for devices destruction */
+_hidden void libxl__devices_destroy(libxl__egc *egc,
+ libxl__devices_remove_state *drs);
+
/*----- device model creation -----*/
/* First layer; wraps libxl__spawn_spawn. */
@@ -1844,6 +1934,7 @@ typedef struct {
libxl_domain_config dm_config;
libxl__domain_build_state dm_state;
libxl__dm_spawn_state pvqemu;
+ libxl__destroy_domid_state dis;
} libxl__stub_dm_spawn_state;
_hidden void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state*);
@@ -1870,6 +1961,8 @@ struct libxl__domain_create_state {
libxl__stub_dm_spawn_state dmss;
/* If we're not doing stubdom, we use only dmss.dm,
* for the non-stubdom device model. */
+ /* necessary if the domain creation failed and we have to destroy it */
+ libxl__domain_destroy_state dds;
};
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 3c55a69..ecd7222 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1388,7 +1388,7 @@ static int handle_domain_death(libxl_ctx *ctx, uint32_t domid,
/* fall-through */
case LIBXL_ACTION_ON_SHUTDOWN_DESTROY:
LOG("Domain %d needs to be cleaned up: destroying the domain", domid);
- libxl_domain_destroy(ctx, domid);
+ libxl_domain_destroy(ctx, domid, 0);
break;
case LIBXL_ACTION_ON_SHUTDOWN_COREDUMP_DESTROY:
@@ -1976,7 +1976,7 @@ start:
error_out:
release_lock();
if (libxl_domid_valid_guest(domid))
- libxl_domain_destroy(ctx, domid);
+ libxl_domain_destroy(ctx, domid, 0);
out:
if (logfile != 2)
@@ -2542,7 +2542,7 @@ static void destroy_domain(const char *p)
fprintf(stderr, "Cannot destroy privileged domain 0.\n\n");
exit(-1);
}
- rc = libxl_domain_destroy(ctx, domid);
+ rc = libxl_domain_destroy(ctx, domid, 0);
if (rc) { fprintf(stderr,"destroy failed (rc=%d)\n",rc); exit(-1); }
}
@@ -2816,7 +2816,7 @@ static int save_domain(const char *p, const char *filename, int checkpoint,
if (checkpoint)
libxl_domain_resume(ctx, domid, 1);
else
- libxl_domain_destroy(ctx, domid);
+ libxl_domain_destroy(ctx, domid, 0);
exit(0);
}
@@ -3077,7 +3077,7 @@ static void migrate_domain(const char *domain_spec, const char *rune,
}
fprintf(stderr, "migration sender: Target reports successful startup.\n");
- libxl_domain_destroy(ctx, domid); /* bang! */
+ libxl_domain_destroy(ctx, domid, 0); /* bang! */
fprintf(stderr, "Migration successful.\n");
exit(0);
@@ -3230,7 +3230,7 @@ static void migrate_receive(int debug, int daemonize, int monitor,
if (rc) {
fprintf(stderr, "migration target: Failure, destroying our copy.\n");
- rc2 = libxl_domain_destroy(ctx, domid);
+ rc2 = libxl_domain_destroy(ctx, domid, 0);
if (rc2) {
fprintf(stderr, "migration target: Failed to destroy our copy"
" (code %d).\n", rc2);
diff --git a/tools/python/xen/lowlevel/xl/xl.c b/tools/python/xen/lowlevel/xl/xl.c
index a729e04..2c21d70 100644
--- a/tools/python/xen/lowlevel/xl/xl.c
+++ b/tools/python/xen/lowlevel/xl/xl.c
@@ -447,7 +447,7 @@ static PyObject *pyxl_domain_destroy(XlObject *self, PyObject *args)
int domid;
if ( !PyArg_ParseTuple(args, "i", &domid) )
return NULL;
- if ( libxl_domain_destroy(self->ctx, domid) ) {
+ if ( libxl_domain_destroy(self->ctx, domid, 0) ) {
PyErr_SetString(xl_error_obj, "cannot destroy domain");
return NULL;
}
--
1.7.7.5 (Apple Git-26)
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v4 03/10] libxl: convert libxl_domain_destroy to an async op
2012-05-24 10:23 ` [PATCH v4 03/10] libxl: convert libxl_domain_destroy to an async op Roger Pau Monne
@ 2012-05-29 14:04 ` Ian Jackson
0 siblings, 0 replies; 26+ messages in thread
From: Ian Jackson @ 2012-05-29 14:04 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel@lists.xen.org
Roger Pau Monne writes ("[PATCH v4 03/10] libxl: convert libxl_domain_destroy to an async op"):
> This change introduces some new structures, and breaks the mutual
> dependency that libxl_domain_destroy and libxl__destroy_device_model
> had. This is done by checking if the domid passed to
> libxl_domain_destroy has a stubdom, and then having the bulk of the
> destroy machinery in a separate function (libxl__destroy_domid) that
> doesn't check for stubdom presence, since we check for it in the upper
> level function. The reason behind this change is the need to use
> structures for ao operations, and it was impossible to have two
> different self-referencing structs.
Thanks. I have a series of largely cosmetic comments which it would
be nice if you felt like addressing, but:
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index bd71844..9003583 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
...
> +/* Check if there are devices that have pending events in the array
> + * pointed to by the "list" parameter. Return values can be:
> + * < 0: All done, but error(s) found.
> + * 0: All done
> + * > 0: Not all done
> + * The passed libxl__ao_device struct in "device" is marked as finished.
> + */
Good.
> +/*
> + * Entry point for domain destruction
> + * This function checks for stubdom presence and then calls
> + * libxl__destroy_domid on the passed domain and it's stubdom if found.
"its" not "it's"
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 7fdecf1..0b3eb48 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> +void libxl__domain_destroy(libxl__egc *egc, libxl__domain_destroy_state *dds)
> +{
> + STATE_AO_GC(dds->ao);
> + uint32_t stubdomid = libxl_get_stubdom_id(CTX, dds->domid);
> +
> + if (stubdomid) {
> + dds->stubdom.ao = ao;
> + dds->stubdom.domid = stubdomid;
> + dds->stubdom.callback = stubdom_callback;
> + dds->stubdom.force = 1;
This variable seems never to be set to anything but 1. We aren't
going to have libxl_domain_remove or indeed libxl__remove_domid, are
we ?
Perhaps the domain destroy functions should lose the force flag and
instead simply always pass 1 into the device remove functions.
> +static void stubdom_callback(libxl__egc *egc, libxl__destroy_domid_state *dis,
> + int rc)
> +{
Can this function please have `destroy' in its name somewhere ?
libxl.c contains code for doing other things than destruction. The
same goes for `domain_callback'.
> + STATE_AO_GC(dis->ao);
> + libxl__domain_destroy_state *dds = CONTAINER_OF(dis, *dds, stubdom);
> + const char *savefile;
> +
> + if (rc) {
> + LOG(ERROR, "unable to destroy stubdom with domid %u", dis->domid);
> + dds->rc = rc;
> + }
> +
> + dds->stubdom_finished = 1;
> + savefile = libxl__device_model_savefile(gc, dis->domid);
> + rc = unlink(savefile);
Please use `rc' only for libxl error codes. This is a system call
return value.
> + /*
> + * On suspend libxl__domain_save_device_model will have already
> + * unlinked the save file.
> + */
> + if (rc && errno == ENOENT) rc = 0;
> + if (rc) {
> + LOGEV(ERROR, errno, "failed to remove device-model savefile %s",
> + savefile);
> + }
This is one possible reason why the savefile might not exist. To be
honest I'm not really convinced this warrants an explanation. Domain
destruction inherently wants to be idempotent.
And, why not use libxl__remove_file which already contains the
relevant logic including a log message. ?
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index 48f05f9..413b98f 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -58,6 +58,48 @@ int libxl__parse_backend_path(libxl__gc *gc,
...
> + 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;
Having two variables `num_devs' and `numdevs' seems unnecessarily
opaque :-). Perhaps the one from libxl__xs_directory should be called
`num_dev_xsentries' or something.
> @@ -367,6 +409,23 @@ void libxl__prepare_ao_device(libxl__ao_device *aodev, libxl__ao *ao,
...
> +int libxl__ao_device_check_last(libxl__gc *gc, libxl__ao_device *device,
> + libxl__ao_device *list, int num)
> +{
> + int i, pending = 0, error = 0;
> +
> + device->active = 0;
> + for (i = 0; i < num; i++) {
> + if (list[i].active)
> + pending++;
Why not `return +1' ? That would do away with the variable `pending'.
> +
> + if (list[i].rc)
> + error = list[i].rc;
> + }
> +
> + return pending == 0 ? error : 1;
And it would simplify this to `return error;'.
> -int libxl__devices_destroy(libxl__gc *gc, uint32_t domid)
> +/* Callback for device destruction */
> +
> +static void device_remove_callback(libxl__egc *egc, libxl__ao_device *aodev);
> +
> +void libxl__devices_destroy(libxl__egc *egc, libxl__devices_remove_state *drs)
...
> + if (drs->num_devices < 0) {
> + LOG(ERROR, "unable to get number of devices for domain %u", drs->domid);
> + rc = ERROR_FAIL;
> + goto out;
A minor point but this should be `rc = drs->num_devices' surely ? A
minor point since it can currently only fail with ERROR_FAIL but at
some point we might want to invent more error codes and then they
should propagate nicely.
> @@ -426,17 +510,19 @@ int libxl__devices_destroy(libxl__gc *gc, uint32_t domid)
> /* console 0 frontend directory is not under /local/domain/<domid>/device */
> path = libxl__sprintf(gc, "/local/domain/%d/console/backend", domid);
> path = libxl__xs_read(gc, XBT_NULL, path);
> + GCNEW(dev);
> if (path && strcmp(path, "") &&
> - libxl__parse_backend_path(gc, path, &dev) == 0) {
> - dev.domid = domid;
> - dev.kind = LIBXL__DEVICE_KIND_CONSOLE;
> - dev.devid = 0;
> + libxl__parse_backend_path(gc, path, dev) == 0) {
> + dev->domid = domid;
> + dev->kind = LIBXL__DEVICE_KIND_CONSOLE;
> + dev->devid = 0;
>
> - libxl__device_destroy(gc, &dev);
> + libxl__device_destroy(gc, dev);
This is quite confusing, because libxl__device_destroy sounds like it
should be a synchronous version of libxl__initiate_device_destroy or
to put it another way an internal version of something called
libxl_device_destroy (which doesn't exist); of course such a function
couldn't exist either.
Perhaps libxl__device_destroy should be renamed somehow (perhaps in
another pre-patch).
Or perhaps there should be a comment to note that (currently) consoles
can be simply synchronously destroyed in xenstore.
> @@ -549,6 +635,39 @@ static void device_backend_cleanup(libxl__gc *gc, libxl__ao_device *aodev)
...
> + do {
> + t = xs_transaction_start(CTX->xsh);
> + libxl__xs_path_cleanup(gc, t, fe_path);
> + libxl__xs_path_cleanup(gc, t, be_path);
> + rc = !xs_transaction_end(CTX->xsh, t, 0);
> + } while (rc && errno == EAGAIN);
Again, can we reserve `rc' for libxl error codes ?
> @@ -1090,48 +1114,25 @@ int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
...
> + if (!pid || !atoi(pid)) {
> + LOG(ERROR, "could not find device-model's pid for dom %u", domid);
> + ret = ERROR_FAIL;
> + goto out;
> + }
> + ret = kill(atoi(pid), SIGHUP);
> + if (ret < 0 && errno == ESRCH) {
> + LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Device Model already exited");
> + ret = 0;
Of course if this happens we have risked sending SIGHUP to an innocent
process. It would be nice to arrange for that not to happen but I
don't think we currently have a way to prevent it.
> + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "failed to kill Device Model [%d]",
Would you care to use LOG(ERROR, ...) ? You might want to use LOG for
the other cases too.
Thanks,
Ian.
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v4 04/10] libxl: convert libxl_device_disk_add to an asyn op
2012-05-24 10:23 [PATCH v4 0/10] execute hotplug scripts from libxl Roger Pau Monne
` (2 preceding siblings ...)
2012-05-24 10:23 ` [PATCH v4 03/10] libxl: convert libxl_domain_destroy to an async op Roger Pau Monne
@ 2012-05-24 10:23 ` Roger Pau Monne
2012-05-29 14:26 ` Ian Jackson
2012-05-24 10:24 ` [PATCH v4 05/10] libxl: convert libxl_device_nic_add to an async operation Roger Pau Monne
` (5 subsequent siblings)
9 siblings, 1 reply; 26+ messages in thread
From: Roger Pau Monne @ 2012-05-24 10:23 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Roger Pau Monne
This patch converts libxl_device_disk_add to an ao operation that
waits for device backend to reach state XenbusStateInitWait and then
marks the operation as completed. This is not really useful now, but
will be used by latter patches that will launch hotplug scripts after
we reached the desired xenbus state.
As usual, libxl_device_disk_add callers have been modified, and the
internal function libxl__device_disk_add has been used if the call was
inside an already running ao.
Changes since v2:
* Remove state read from libxl__initiate_device_add
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
---
tools/libxl/Makefile | 2 +-
tools/libxl/libxl.c | 50 ++++++++++++++++++++++++++--------
tools/libxl/libxl.h | 4 ++-
tools/libxl/libxl_create.c | 41 ++++++++++++++++++++++------
tools/libxl/libxl_device.c | 48 +++++++++++++++++++++++++++++++++
tools/libxl/libxl_dm.c | 61 +++++++++++++++++++++++++++++++----------
tools/libxl/libxl_internal.h | 26 ++++++++++++++++++
tools/libxl/xl_cmdimpl.c | 2 +-
8 files changed, 195 insertions(+), 39 deletions(-)
diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 5d9227e..81b467e 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -13,7 +13,7 @@ XLUMINOR = 0
CFLAGS += -Werror -Wno-format-zero-length -Wmissing-declarations \
-Wno-declaration-after-statement -Wformat-nonliteral
-CFLAGS += -I. -fPIC
+CFLAGS += -I. -fPIC -O0
ifeq ($(CONFIG_Linux),y)
LIBUUID_LIBS += -luuid
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 0b3eb48..7147869 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1540,13 +1540,31 @@ static int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
return 0;
}
-int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk)
+int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid,
+ libxl_device_disk *disk,
+ const libxl_asyncop_how *ao_how)
{
- GC_INIT(ctx);
+ AO_CREATE(ctx, domid, ao_how);
+ libxl__ao_device *device;
+
+ GCNEW(device);
+ libxl__prepare_ao_device(device, ao, NULL);
+ device->callback = device_addrm_aocomplete;
+ libxl__device_disk_add(egc, domid, disk, device);
+
+ return AO_INPROGRESS;
+}
+
+void libxl__device_disk_add(libxl__egc *egc, uint32_t domid,
+ libxl_device_disk *disk,
+ libxl__ao_device *aodev)
+{
+ STATE_AO_GC(aodev->ao);
+ libxl_ctx *ctx = CTX;
flexarray_t *front;
flexarray_t *back;
char *dev;
- libxl__device device;
+ libxl__device *device;
int major, minor, rc;
rc = libxl__device_disk_setdefault(gc, disk);
@@ -1570,7 +1588,8 @@ int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *dis
goto out_free;
}
- rc = libxl__device_from_disk(gc, domid, disk, &device);
+ GCNEW(device);
+ rc = libxl__device_from_disk(gc, domid, disk, device);
if (rc != 0) {
LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Invalid or unsupported"
" virtual disk identifier %s", disk->vdev);
@@ -1588,7 +1607,7 @@ int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *dis
flexarray_append(back, "params");
flexarray_append(back, dev);
- assert(device.backend_kind == LIBXL__DEVICE_KIND_VBD);
+ assert(device->backend_kind == LIBXL__DEVICE_KIND_VBD);
break;
case LIBXL_DISK_BACKEND_TAP:
dev = libxl__blktap_devpath(gc, disk->pdev_path, disk->format);
@@ -1609,7 +1628,7 @@ int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *dis
flexarray_append(back, "params");
flexarray_append(back, libxl__sprintf(gc, "%s:%s",
libxl__device_disk_string_of_format(disk->format), disk->pdev_path));
- assert(device.backend_kind == LIBXL__DEVICE_KIND_QDISK);
+ assert(device->backend_kind == LIBXL__DEVICE_KIND_QDISK);
break;
default:
LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend type: %d\n", disk->backend);
@@ -1641,22 +1660,27 @@ int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *dis
flexarray_append(front, "state");
flexarray_append(front, libxl__sprintf(gc, "%d", 1));
flexarray_append(front, "virtual-device");
- flexarray_append(front, libxl__sprintf(gc, "%d", device.devid));
+ flexarray_append(front, libxl__sprintf(gc, "%d", device->devid));
flexarray_append(front, "device-type");
flexarray_append(front, disk->is_cdrom ? "cdrom" : "disk");
- libxl__device_generic_add(gc, &device,
+ libxl__device_generic_add(gc, device,
libxl__xs_kvs_of_flexarray(gc, back, back->count),
libxl__xs_kvs_of_flexarray(gc, front, front->count));
+ aodev->dev = device;
+ aodev->action = DEVICE_CONNECT;
+ libxl__initiate_device_add(egc, aodev);
+
rc = 0;
out_free:
flexarray_free(back);
flexarray_free(front);
out:
- GC_FREE;
- return rc;
+ aodev->rc = rc;
+ if (rc) aodev->callback(egc, aodev);
+ return;
}
static void libxl__device_disk_from_xs_be(libxl__gc *gc,
@@ -1859,11 +1883,13 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk)
ret = 0;
libxl_device_disk_remove(ctx, domid, disks + i, 0);
- libxl_device_disk_add(ctx, domid, disk);
+ /* fixme-ao */
+ libxl_device_disk_add(ctx, domid, disk, 0);
stubdomid = libxl_get_stubdom_id(ctx, domid);
if (stubdomid) {
libxl_device_disk_remove(ctx, stubdomid, disks + i, 0);
- libxl_device_disk_add(ctx, stubdomid, disk);
+ /* fixme-ao */
+ libxl_device_disk_add(ctx, stubdomid, disk, 0);
}
out:
for (i = 0; i < num; i++)
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 7933809..44c3949 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -670,7 +670,9 @@ void libxl_vminfo_list_free(libxl_vminfo *list, int nr);
*/
/* Disks */
-int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk);
+int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid,
+ libxl_device_disk *disk,
+ const libxl_asyncop_how *ao_how);
int libxl_device_disk_remove(libxl_ctx *ctx, uint32_t domid,
libxl_device_disk *disk,
const libxl_asyncop_how *ao_how);
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 4e88551..d1a4016 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -575,6 +575,8 @@ static void domcreate_bootloader_done(libxl__egc *egc,
libxl__bootloader_state *bl,
int rc);
+static void domcreate_disk_connected(libxl__egc *egc, libxl__ao_device *aodev);
+
static void domcreate_console_available(libxl__egc *egc,
libxl__domain_create_state *dcs);
@@ -670,7 +672,6 @@ static void domcreate_bootloader_done(libxl__egc *egc,
{
libxl__domain_create_state *dcs = CONTAINER_OF(bl, *dcs, bl);
STATE_AO_GC(bl->ao);
- int i;
/* convenience aliases */
const uint32_t domid = dcs->guest_domid;
@@ -704,15 +705,37 @@ static void domcreate_bootloader_done(libxl__egc *egc,
store_libxl_entry(gc, domid, &d_config->b_info);
- for (i = 0; i < d_config->num_disks; i++) {
- ret = libxl_device_disk_add(ctx, domid, &d_config->disks[i]);
- if (ret) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
- "cannot add disk %d to domain: %d", i, ret);
- ret = ERROR_FAIL;
- goto error_out;
- }
+ dcs->num_devices = d_config->num_disks;
+ libxl__add_disks(egc, ao, domid, d_config, &dcs->devices,
+ domcreate_disk_connected);
+
+ return;
+
+ error_out:
+ assert(ret);
+ domcreate_complete(egc, dcs, ret);
+}
+
+static void domcreate_disk_connected(libxl__egc *egc, libxl__ao_device *aodev)
+{
+ STATE_AO_GC(aodev->ao);
+ libxl__domain_create_state *dcs = CONTAINER_OF(aodev->base, *dcs, devices);
+ int i, ret = 0;
+
+ /* convenience aliases */
+ const uint32_t domid = dcs->guest_domid;
+ libxl_domain_config *const d_config = dcs->guest_config;
+ libxl__domain_build_state *const state = &dcs->build_state;
+ libxl_ctx *const ctx = CTX;
+
+ ret = libxl__ao_device_check_last(gc, aodev, dcs->devices,
+ dcs->num_devices);
+ if (ret > 0) return;
+ if (ret) {
+ LOG(ERROR, "error connecting disk devices");
+ goto error_out;
}
+
for (i = 0; i < d_config->num_vifs; i++) {
ret = libxl_device_nic_add(ctx, domid, &d_config->vifs[i]);
if (ret) {
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 413b98f..6c9bbc2 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -426,6 +426,24 @@ int libxl__ao_device_check_last(libxl__gc *gc, libxl__ao_device *device,
return pending == 0 ? error : 1;
}
+void libxl__add_disks(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
+ libxl_domain_config *d_config,
+ libxl__ao_device **devices,
+ libxl__device_callback callback)
+{
+ AO_GC;
+ GCNEW_ARRAY(*devices, d_config->num_disks);
+ libxl__ao_device *aodev = *devices;
+ int i;
+
+ for (i = 0; i < d_config->num_disks; i++) {
+ libxl__prepare_ao_device(&aodev[i], ao, devices);
+ aodev[i].callback = callback;
+ libxl__device_disk_add(egc, domid, &d_config->disks[i],
+ &aodev[i]);
+ }
+}
+
int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
{
libxl_ctx *ctx = libxl__gc_owner(gc);
@@ -533,6 +551,36 @@ static void device_backend_callback(libxl__egc *egc, libxl__ev_devstate *ds,
static void device_backend_cleanup(libxl__gc *gc,
libxl__ao_device *aodev);
+void libxl__initiate_device_add(libxl__egc *egc, libxl__ao_device *aodev)
+{
+ STATE_AO_GC(aodev->ao);
+ char *be_path = libxl__device_backend_path(gc, aodev->dev);
+ char *state_path = libxl__sprintf(gc, "%s/state", be_path);
+ int rc = 0;
+
+ if (aodev->dev->backend_kind == LIBXL__DEVICE_KIND_QDISK) {
+ aodev->callback(egc, aodev);
+ return;
+ }
+
+ libxl__ev_devstate_init(&aodev->ds);
+ rc = libxl__ev_devstate_wait(gc, &aodev->ds, device_backend_callback,
+ state_path, XenbusStateInitWait,
+ LIBXL_INIT_TIMEOUT * 1000);
+ if (rc) {
+ LOGE(ERROR, "unable to initialize device %s", be_path);
+ goto out_fail;
+ }
+
+ return;
+
+out_fail:
+ assert(rc);
+ aodev->rc = rc;
+ aodev->callback(egc, aodev);
+ return;
+}
+
void libxl__initiate_device_remove(libxl__egc *egc,
libxl__ao_device *aodev)
{
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index a403b4c..9903fb7 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -673,6 +673,8 @@ static void spawn_stubdom_pvqemu_cb(libxl__egc *egc,
libxl__dm_spawn_state *stubdom_dmss,
int rc);
+static void spawn_stub_disk_connected(libxl__egc *egc, libxl__ao_device *aodev);
+
static void spaw_stubdom_pvqemu_destroy_cb(libxl__egc *egc,
libxl__destroy_domid_state *dis,
int rc);
@@ -681,8 +683,7 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
{
STATE_AO_GC(sdss->dm.spawn.ao);
libxl_ctx *ctx = libxl__gc_owner(gc);
- int i, num_console = STUBDOM_SPECIAL_CONSOLES, ret;
- libxl__device_console *console;
+ int ret;
libxl_device_vfb vfb;
libxl_device_vkb vkb;
char **args;
@@ -800,22 +801,55 @@ retry_transaction:
if (errno == EAGAIN)
goto retry_transaction;
- for (i = 0; i < dm_config->num_disks; i++) {
- ret = libxl_device_disk_add(ctx, dm_domid, &dm_config->disks[i]);
- if (ret)
- goto out_free;
- }
+ sdss->num_devices = dm_config->num_disks;
+ libxl__add_disks(egc, ao, dm_domid, dm_config, &sdss->devices,
+ spawn_stub_disk_connected);
+
+ free(args);
+ return;
+
+out_free:
+ free(args);
+out:
+ assert(ret);
+ spawn_stubdom_pvqemu_cb(egc, &sdss->pvqemu, ret);
+}
+
+static void spawn_stub_disk_connected(libxl__egc *egc, libxl__ao_device *aodev)
+{
+ STATE_AO_GC(aodev->ao);
+ libxl_ctx *ctx = libxl__gc_owner(gc);
+ libxl__stub_dm_spawn_state *sdss = CONTAINER_OF(aodev->base, *sdss, devices);
+ int i, num_console = STUBDOM_SPECIAL_CONSOLES, ret = 0;
+ libxl__device_console *console;
+
+ /* convenience aliases */
+ libxl_domain_config *const dm_config = &sdss->dm_config;
+ libxl_domain_config *const guest_config = sdss->dm.guest_config;
+ const int guest_domid = sdss->dm.guest_domid;
+ libxl__domain_build_state *const d_state = sdss->dm.build_state;
+ libxl__domain_build_state *const stubdom_state = &sdss->dm_state;
+ uint32_t dm_domid = sdss->pvqemu.guest_domid;
+
+ ret = libxl__ao_device_check_last(gc, aodev, sdss->devices,
+ sdss->num_devices);
+ if (ret > 0) return;
+ if (ret) {
+ LOG(ERROR, "error connecting disk devices");
+ goto out;
+ }
+
for (i = 0; i < dm_config->num_vifs; i++) {
ret = libxl_device_nic_add(ctx, dm_domid, &dm_config->vifs[i]);
if (ret)
- goto out_free;
+ goto out;
}
ret = libxl_device_vfb_add(ctx, dm_domid, &dm_config->vfbs[0]);
if (ret)
- goto out_free;
+ goto out;
ret = libxl_device_vkb_add(ctx, dm_domid, &dm_config->vkbs[0]);
if (ret)
- goto out_free;
+ goto out;
if (guest_config->b_info.u.hvm.serial)
num_console++;
@@ -823,7 +857,7 @@ retry_transaction:
console = libxl__calloc(gc, num_console, sizeof(libxl__device_console));
if (!console) {
ret = ERROR_NOMEM;
- goto out_free;
+ goto out;
}
for (i = 0; i < num_console; i++) {
@@ -859,7 +893,7 @@ retry_transaction:
ret = libxl__device_console_add(gc, dm_domid, &console[i],
i == STUBDOM_CONSOLE_LOGGING ? stubdom_state : NULL);
if (ret)
- goto out_free;
+ goto out;
}
sdss->pvqemu.spawn.ao = ao;
@@ -870,11 +904,8 @@ retry_transaction:
libxl__spawn_local_dm(egc, &sdss->pvqemu);
- free(args);
return;
-out_free:
- free(args);
out:
assert(ret);
spawn_stubdom_pvqemu_cb(egc, &sdss->pvqemu, ret);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 9003583..963e734 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -70,6 +70,7 @@
#include "_libxl_types_internal.h"
#include "_libxl_types_internal_json.h"
+#define LIBXL_INIT_TIMEOUT 10
#define LIBXL_DESTROY_TIMEOUT 10
#define LIBXL_DEVICE_MODEL_START_TIMEOUT 10
#define LIBXL_XENCONSOLE_LIMIT 1048576
@@ -1812,6 +1813,19 @@ struct libxl__ao_device {
void *base;
};
+/* Internal AO operation to connect a disk device */
+_hidden void libxl__device_disk_add(libxl__egc *egc, uint32_t domid,
+ libxl_device_disk *disk,
+ libxl__ao_device *aodev);
+
+/* Arranges that dev will be added to the guest, and the
+ * hotplug scripts will be executed (if necessary). When
+ * this is done (or an error happens), the callback in
+ * aodev->callback will be called.
+ */
+_hidden void libxl__initiate_device_add(libxl__egc*, libxl__ao_device *aodev);
+
+
/* 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
@@ -1901,6 +1915,12 @@ _hidden void libxl__destroy_domid(libxl__egc *egc,
_hidden void libxl__devices_destroy(libxl__egc *egc,
libxl__devices_remove_state *drs);
+/* Helper function to add a bunch of disks */
+void libxl__add_disks(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
+ libxl_domain_config *d_config,
+ libxl__ao_device **devices,
+ libxl__device_callback callback);
+
/*----- device model creation -----*/
/* First layer; wraps libxl__spawn_spawn. */
@@ -1935,6 +1955,9 @@ typedef struct {
libxl__domain_build_state dm_state;
libxl__dm_spawn_state pvqemu;
libxl__destroy_domid_state dis;
+ /* used to store the state of devices being connected */
+ libxl__ao_device *devices;
+ int num_devices;
} libxl__stub_dm_spawn_state;
_hidden void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state*);
@@ -1963,6 +1986,9 @@ struct libxl__domain_create_state {
* for the non-stubdom device model. */
/* necessary if the domain creation failed and we have to destroy it */
libxl__domain_destroy_state dds;
+ /* used to store the state of devices being connected */
+ libxl__ao_device *devices;
+ int num_devices;
};
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index ecd7222..5f15b4c 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5334,7 +5334,7 @@ int main_blockattach(int argc, char **argv)
return 0;
}
- if (libxl_device_disk_add(ctx, fe_domid, &disk)) {
+ if (libxl_device_disk_add(ctx, fe_domid, &disk, 0)) {
fprintf(stderr, "libxl_device_disk_add failed.\n");
}
return 0;
--
1.7.7.5 (Apple Git-26)
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v4 04/10] libxl: convert libxl_device_disk_add to an asyn op
2012-05-24 10:23 ` [PATCH v4 04/10] libxl: convert libxl_device_disk_add to an asyn op Roger Pau Monne
@ 2012-05-29 14:26 ` Ian Jackson
2012-05-29 14:40 ` Ian Campbell
0 siblings, 1 reply; 26+ messages in thread
From: Ian Jackson @ 2012-05-29 14:26 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel@lists.xen.org
Roger Pau Monne writes ("[PATCH v4 04/10] libxl: convert libxl_device_disk_add to an asyn op"):
Subject line needs to say `async' not `asyn'.
> This patch converts libxl_device_disk_add to an ao operation that
> waits for device backend to reach state XenbusStateInitWait and then
> marks the operation as completed. This is not really useful now, but
> will be used by latter patches that will launch hotplug scripts after
> we reached the desired xenbus state.
>
> As usual, libxl_device_disk_add callers have been modified, and the
> internal function libxl__device_disk_add has been used if the call was
> inside an already running ao.
This looks good to me. I have some very minor comments.
...
> +/* Internal AO operation to connect a disk device */
> +_hidden void libxl__device_disk_add(libxl__egc *egc, uint32_t domid,
> + libxl_device_disk *disk,
> + libxl__ao_device *aodev);
> +
> +/* Arranges that dev will be added to the guest, and the
> + * hotplug scripts will be executed (if necessary). When
> + * this is done (or an error happens), the callback in
> + * aodev->callback will be called.
> + */
> +_hidden void libxl__initiate_device_add(libxl__egc*, libxl__ao_device *aodev);
>From the comments I'm a bit confused by the relationship between
these. I guess libxl__device_disk_add is called by
libxl__initiate_device_add and not vice versa, and that only
libxl__initiate_device_add is allowed to call libxl__device_disk_add.
Is that true ? If so it would be nice to say so.
> +/* Helper function to add a bunch of disks */
> +void libxl__add_disks(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
> + libxl_domain_config *d_config,
> + libxl__ao_device **devices,
> + libxl__device_callback callback);
> +
This comment should make it clear that the callback will be called
_once for each device_, and that that callback is therefore
responsible for using libxl__ao_device_check_last.
Doesn't this define `callback' as a parameter of function type?
Is that legal in C89/C99 ?
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index 5d9227e..81b467e 100644
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -13,7 +13,7 @@ XLUMINOR = 0
>
> CFLAGS += -Werror -Wno-format-zero-length -Wmissing-declarations \
> -Wno-declaration-after-statement -Wformat-nonliteral
> -CFLAGS += -I. -fPIC
> +CFLAGS += -I. -fPIC -O0
Stray hunk I think.
> @@ -1859,11 +1883,13 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk)
> ret = 0;
>
> libxl_device_disk_remove(ctx, domid, disks + i, 0);
> - libxl_device_disk_add(ctx, domid, disk);
> + /* fixme-ao */
We need to fix this in the API, at least by making libxl_cdrom_insert
take an ao_how at some point. But not in this patch, indeed.
> +void libxl__add_disks(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
> + libxl_domain_config *d_config,
> + libxl__ao_device **devices,
> + libxl__device_callback callback)
> +{
> + AO_GC;
> + GCNEW_ARRAY(*devices, d_config->num_disks);
> + libxl__ao_device *aodev = *devices;
> + int i;
> +
> + for (i = 0; i < d_config->num_disks; i++) {
> + libxl__prepare_ao_device(&aodev[i], ao, devices);
> + aodev[i].callback = callback;
> + libxl__device_disk_add(egc, domid, &d_config->disks[i],
> + &aodev[i]);
> + }
> +}
If libxl__device_disk_add ever becomes capable of reentrantly calling
the completion callback, this will go wrong because the others haven't
been prepared yet. Perhaps this should be in two loops ?
Ian.
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v4 04/10] libxl: convert libxl_device_disk_add to an asyn op
2012-05-29 14:26 ` Ian Jackson
@ 2012-05-29 14:40 ` Ian Campbell
0 siblings, 0 replies; 26+ messages in thread
From: Ian Campbell @ 2012-05-29 14:40 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel@lists.xen.org, Roger Pau Monne
On Tue, 2012-05-29 at 15:26 +0100, Ian Jackson wrote:
>
> > @@ -1859,11 +1883,13 @@ int libxl_cdrom_insert(libxl_ctx *ctx,
> uint32_t domid, libxl_device_disk *disk)
> > ret = 0;
> >
> > libxl_device_disk_remove(ctx, domid, disks + i, 0);
> > - libxl_device_disk_add(ctx, domid, disk);
> > + /* fixme-ao */
>
> We need to fix this in the API, at least by making libxl_cdrom_insert
> take an ao_how at some point. But not in this patch, indeed.
In yesterday's TODO I said:
* libxl_cdrom_insert. Should be easy once
disk_{add,remove} done. Nobody is looking at this? This
is basically a helper function and its functionality can
be implemented in terms of the libxl_disk_*
interfaces. We could therefore consider marking this
function as substandard and subject to change in the
future. We could also consider simply moving it into xl
as a helper function and revisting for 4.3.
I'm quite interested in thoughts on that ;-)
Ian.
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v4 05/10] libxl: convert libxl_device_nic_add to an async operation
2012-05-24 10:23 [PATCH v4 0/10] execute hotplug scripts from libxl Roger Pau Monne
` (3 preceding siblings ...)
2012-05-24 10:23 ` [PATCH v4 04/10] libxl: convert libxl_device_disk_add to an asyn op Roger Pau Monne
@ 2012-05-24 10:24 ` Roger Pau Monne
2012-05-29 14:36 ` Ian Jackson
2012-05-24 10:24 ` [PATCH v4 06/10] libxl: add option to choose who executes hotplug scripts Roger Pau Monne
` (4 subsequent siblings)
9 siblings, 1 reply; 26+ messages in thread
From: Roger Pau Monne @ 2012-05-24 10:24 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Roger Pau Monne
This patch converts libxl_device_nic_add to an ao operation that
waits for device backend to reach state XenbusStateInitWait and then
marks the operation as completed. This is not really useful now, but
will be used by latter patches that will launch hotplug scripts after
we reached the desired xenbus state.
Calls to libxl_device_nic_add have also been moved to occur after the
device model has been launched, so when hotplug scripts are called
from this functions the interfaces already exists.
As usual, libxl_device_nic_add callers have been modified, and the
internal function libxl__device_disk_add has been used if the call was
inside an already running ao.
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
---
tools/libxl/libxl.c | 39 ++++++++++++++++++-----
tools/libxl/libxl.h | 3 +-
tools/libxl/libxl_create.c | 70 +++++++++++++++++++++++++++++++++++++-----
tools/libxl/libxl_device.c | 18 +++++++++++
tools/libxl/libxl_dm.c | 54 ++++++++++++++++++++++++++++++--
tools/libxl/libxl_internal.h | 11 ++++++
tools/libxl/xl_cmdimpl.c | 2 +-
7 files changed, 176 insertions(+), 21 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 7147869..a5fc42d 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2026,12 +2026,27 @@ static int libxl__device_from_nic(libxl__gc *gc, uint32_t domid,
return 0;
}
-int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic)
+int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic,
+ const libxl_asyncop_how *ao_how)
{
- GC_INIT(ctx);
+ AO_CREATE(ctx, domid, ao_how);
+ libxl__ao_device *device;
+
+ GCNEW(device);
+ libxl__prepare_ao_device(device, ao, NULL);
+ device->callback = device_addrm_aocomplete;
+ libxl__device_nic_add(egc, domid, nic, device);
+
+ return AO_INPROGRESS;
+}
+
+void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
+ libxl_device_nic *nic, libxl__ao_device *aodev)
+{
+ STATE_AO_GC(aodev->ao);
flexarray_t *front;
flexarray_t *back;
- libxl__device device;
+ libxl__device *device;
char *dompath, **l;
unsigned int nb, rc;
@@ -2062,7 +2077,8 @@ int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic)
}
}
- 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_free;
flexarray_append(back, "frontend-id");
@@ -2103,6 +2119,9 @@ int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic)
flexarray_append(back, libxl__strdup(gc, nic->bridge));
flexarray_append(back, "handle");
flexarray_append(back, libxl__sprintf(gc, "%d", nic->devid));
+ flexarray_append(back, "type");
+ flexarray_append(back, libxl__strdup(gc,
+ libxl_nic_type_to_string(nic->nictype)));
flexarray_append(front, "backend-id");
flexarray_append(front, libxl__sprintf(gc, "%d", nic->backend_domid));
@@ -2113,18 +2132,22 @@ int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic)
flexarray_append(front, "mac");
flexarray_append(front, libxl__sprintf(gc,
LIBXL_MAC_FMT, LIBXL_MAC_BYTES(nic->mac)));
- libxl__device_generic_add(gc, &device,
+ libxl__device_generic_add(gc, device,
libxl__xs_kvs_of_flexarray(gc, back, back->count),
libxl__xs_kvs_of_flexarray(gc, front, front->count));
- /* FIXME: wait for plug */
+ aodev->dev = device;
+ aodev->action = DEVICE_CONNECT;
+ libxl__initiate_device_add(egc, aodev);
+
rc = 0;
out_free:
flexarray_free(back);
flexarray_free(front);
out:
- GC_FREE;
- return rc;
+ aodev->rc = rc;
+ if (rc) aodev->callback(egc, aodev);
+ return;
}
static void libxl__device_nic_from_xs_be(libxl__gc *gc,
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 44c3949..35aa03f 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -698,7 +698,8 @@ char * libxl_device_disk_local_attach(libxl_ctx *ctx, libxl_device_disk *disk);
int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk);
/* Network Interfaces */
-int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic);
+int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic,
+ const libxl_asyncop_how *ao_how);
int libxl_device_nic_remove(libxl_ctx *ctx, uint32_t domid,
libxl_device_nic *nic,
const libxl_asyncop_how *ao_how);
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index d1a4016..7466498 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -577,6 +577,11 @@ static void domcreate_bootloader_done(libxl__egc *egc,
static void domcreate_disk_connected(libxl__egc *egc, libxl__ao_device *aodev);
+static void domcreate_nic_connected(libxl__egc *egc, libxl__ao_device *aodev);
+
+static void domcreate_attach_pci(libxl__egc *egc,
+ libxl__domain_create_state *dcs);
+
static void domcreate_console_available(libxl__egc *egc,
libxl__domain_create_state *dcs);
@@ -737,13 +742,11 @@ static void domcreate_disk_connected(libxl__egc *egc, libxl__ao_device *aodev)
}
for (i = 0; i < d_config->num_vifs; i++) {
- ret = libxl_device_nic_add(ctx, domid, &d_config->vifs[i]);
- if (ret) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
- "cannot add nic %d to domain: %d", i, ret);
- ret = ERROR_FAIL;
- goto error_out;
- }
+ /* We have to init the nic here, because we still haven't
+ * called libxl_device_nic_add at this point, but qemu needs
+ * the nic information to be complete.
+ */
+ libxl__device_nic_setdefault(gc, &d_config->vifs[i]);
}
switch (d_config->c_info.type) {
case LIBXL_DOMAIN_TYPE_HVM:
@@ -816,7 +819,6 @@ static void domcreate_devmodel_started(libxl__egc *egc,
{
libxl__domain_create_state *dcs = CONTAINER_OF(dmss, *dcs, dmss.dm);
STATE_AO_GC(dmss->spawn.ao);
- int i;
libxl_ctx *ctx = CTX;
int domid = dcs->guest_domid;
@@ -836,6 +838,58 @@ static void domcreate_devmodel_started(libxl__egc *egc,
}
}
+ /* Plug nic interfaces */
+ if (!ret && d_config->num_vifs > 0) {
+ /* Attach nics */
+ dcs->num_devices = d_config->num_vifs;
+ libxl__add_nics(egc, ao, domid, d_config, &dcs->devices,
+ domcreate_nic_connected);
+ return;
+ }
+
+ domcreate_attach_pci(egc, dcs);
+ return;
+
+error_out:
+ assert(ret);
+ domcreate_complete(egc, dcs, ret);
+}
+
+static void domcreate_nic_connected(libxl__egc *egc, libxl__ao_device *aodev)
+{
+ STATE_AO_GC(aodev->ao);
+ libxl__domain_create_state *dcs = CONTAINER_OF(aodev->base, *dcs, devices);
+ int rc = 0;
+
+ rc = libxl__ao_device_check_last(gc, aodev, dcs->devices,
+ dcs->num_devices);
+
+ if (rc > 0) return;
+ if (rc) {
+ LOGE(ERROR, "error connecting nics devices");
+ goto error_out;
+ }
+
+ domcreate_attach_pci(egc, dcs);
+ return;
+
+error_out:
+ assert(rc);
+ domcreate_complete(egc, dcs, rc);
+}
+
+static void domcreate_attach_pci(libxl__egc *egc,
+ libxl__domain_create_state *dcs)
+{
+ STATE_AO_GC(dcs->ao);
+ int i, ret = 0;
+ libxl_ctx *ctx = CTX;
+ int domid = dcs->guest_domid;
+
+ /* convenience aliases */
+ libxl_domain_config *const d_config = dcs->guest_config;
+
+
for (i = 0; i < d_config->num_pcidevs; i++)
libxl__device_pci_add(gc, domid, &d_config->pcidevs[i], 1);
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 6c9bbc2..9933cc2 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -444,6 +444,24 @@ void libxl__add_disks(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
}
}
+void libxl__add_nics(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
+ libxl_domain_config *d_config,
+ libxl__ao_device **devices,
+ libxl__device_callback callback)
+{
+ AO_GC;
+ GCNEW_ARRAY(*devices, d_config->num_vifs);
+ libxl__ao_device *aodev = *devices;
+ int i;
+
+ for (i = 0; i < d_config->num_vifs; i++) {
+ libxl__prepare_ao_device(&aodev[i], ao, devices);
+ aodev[i].callback = callback;
+ libxl__device_nic_add(egc, domid, &d_config->vifs[i],
+ &aodev[i]);
+ }
+}
+
int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
{
libxl_ctx *ctx = libxl__gc_owner(gc);
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 9903fb7..72483ce 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -675,6 +675,12 @@ static void spawn_stubdom_pvqemu_cb(libxl__egc *egc,
static void spawn_stub_disk_connected(libxl__egc *egc, libxl__ao_device *aodev);
+static void stubdom_nics_connected(libxl__egc *egc, libxl__ao_device *aodev);
+
+static void stubdom_pvqemu_cb(libxl__egc *egc,
+ libxl__dm_spawn_state *stubdom_dmss,
+ int rc);
+
static void spaw_stubdom_pvqemu_destroy_cb(libxl__egc *egc,
libxl__destroy_domid_state *dis,
int rc);
@@ -840,9 +846,11 @@ static void spawn_stub_disk_connected(libxl__egc *egc, libxl__ao_device *aodev)
}
for (i = 0; i < dm_config->num_vifs; i++) {
- ret = libxl_device_nic_add(ctx, dm_domid, &dm_config->vifs[i]);
- if (ret)
- goto out;
+ /* We have to init the nic here, because we still haven't
+ * called libxl_device_nic_add at this point, but qemu needs
+ * the nic information to be complete.
+ */
+ libxl__device_nic_setdefault(gc, &dm_config->vifs[i]);
}
ret = libxl_device_vfb_add(ctx, dm_domid, &dm_config->vfbs[0]);
if (ret)
@@ -919,9 +927,49 @@ static void spawn_stubdom_pvqemu_cb(libxl__egc *egc,
CONTAINER_OF(stubdom_dmss, *sdss, pvqemu);
STATE_AO_GC(sdss->dm.spawn.ao);
uint32_t dm_domid = sdss->pvqemu.guest_domid;
+ libxl_domain_config *d_config = stubdom_dmss->guest_config;
if (rc) goto out;
+ if (!rc && d_config->num_vifs > 0) {
+ sdss->num_devices = d_config->num_vifs;
+ libxl__add_nics(egc, ao, dm_domid, d_config, &sdss->devices,
+ stubdom_nics_connected);
+ return;
+ }
+
+out:
+ stubdom_pvqemu_cb(egc, stubdom_dmss, rc);
+}
+
+static void stubdom_nics_connected(libxl__egc *egc, libxl__ao_device *aodev)
+{
+ STATE_AO_GC(aodev->ao);
+ libxl__stub_dm_spawn_state *sdss =
+ CONTAINER_OF(aodev->base, *sdss, devices);
+ int rc = 0;
+
+ rc = libxl__ao_device_check_last(gc, aodev, sdss->devices,
+ sdss->num_devices);
+
+ if (rc > 0) return;
+ if (rc)
+ LOGE(ERROR, "error connecting nics devices");
+
+ stubdom_pvqemu_cb(egc, &sdss->pvqemu, rc);
+ return;
+}
+
+static void stubdom_pvqemu_cb(libxl__egc *egc,
+ libxl__dm_spawn_state *stubdom_dmss,
+ int rc)
+{
+ libxl__stub_dm_spawn_state *sdss =
+ CONTAINER_OF(stubdom_dmss, *sdss, pvqemu);
+ STATE_AO_GC(sdss->dm.spawn.ao);
+ uint32_t dm_domid = sdss->pvqemu.guest_domid;
+
+
rc = libxl_domain_unpause(CTX, dm_domid);
if (rc) goto out;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 963e734..44c7c66 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1818,6 +1818,11 @@ _hidden void libxl__device_disk_add(libxl__egc *egc, uint32_t domid,
libxl_device_disk *disk,
libxl__ao_device *aodev);
+/* Internal AO operation to connect a nic device */
+_hidden void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
+ libxl_device_nic *nic,
+ libxl__ao_device *aodev);
+
/* Arranges that dev will be added to the guest, and the
* hotplug scripts will be executed (if necessary). When
* this is done (or an error happens), the callback in
@@ -1921,6 +1926,12 @@ void libxl__add_disks(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
libxl__ao_device **devices,
libxl__device_callback callback);
+/* Helper function to add a bunch of disks */
+void libxl__add_nics(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
+ libxl_domain_config *d_config,
+ libxl__ao_device **devices,
+ libxl__device_callback callback);
+
/*----- device model creation -----*/
/* First layer; wraps libxl__spawn_spawn. */
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 5f15b4c..a1883b5 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5223,7 +5223,7 @@ int main_networkattach(int argc, char **argv)
return 0;
}
- if (libxl_device_nic_add(ctx, domid, &nic)) {
+ if (libxl_device_nic_add(ctx, domid, &nic, 0)) {
fprintf(stderr, "libxl_device_nic_add failed.\n");
return 1;
}
--
1.7.7.5 (Apple Git-26)
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v4 05/10] libxl: convert libxl_device_nic_add to an async operation
2012-05-24 10:24 ` [PATCH v4 05/10] libxl: convert libxl_device_nic_add to an async operation Roger Pau Monne
@ 2012-05-29 14:36 ` Ian Jackson
2012-05-30 9:54 ` Roger Pau Monne
0 siblings, 1 reply; 26+ messages in thread
From: Ian Jackson @ 2012-05-29 14:36 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel@lists.xen.org
Roger Pau Monne writes ("[PATCH v4 05/10] libxl: convert libxl_device_nic_add to an async operation"):
> This patch converts libxl_device_nic_add to an ao operation that
> waits for device backend to reach state XenbusStateInitWait and then
> marks the operation as completed. This is not really useful now, but
> will be used by latter patches that will launch hotplug scripts after
> we reached the desired xenbus state.
Thanks. I'm afraid this is still tripping my repetition detector.
See below.
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index 6c9bbc2..9933cc2 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -444,6 +444,24 @@ void libxl__add_disks(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
> }
> }
>
> +void libxl__add_nics(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
> + libxl_domain_config *d_config,
> + libxl__ao_device **devices,
> + libxl__device_callback callback)
> +{
> + AO_GC;
> + GCNEW_ARRAY(*devices, d_config->num_vifs);
> + libxl__ao_device *aodev = *devices;
> + int i;
> +
> + for (i = 0; i < d_config->num_vifs; i++) {
> + libxl__prepare_ao_device(&aodev[i], ao, devices);
> + aodev[i].callback = callback;
> + libxl__device_nic_add(egc, domid, &d_config->vifs[i],
> + &aodev[i]);
> + }
> +}
The same comment applies as before about libxl__add_disks and the two
loops.
But, this is another copy of libxl__add_disks. Perhaps you want a
macro to generate these functions ?
In general this patch seems to contain an awful lot of very similar
code to the previous one. Eg domcreate_nic_connected is practically
identical to domcreate_disk_connected. And domcreate_attach_nick is
practically identical to domcreate_attach_disk.
Another option would be to make the types of libxl__device_FOO_add
compatible (by replacing the actual config parameter with const void*)
and then pass them as function pointers.
Or you could even have a table
{ offsetof(disks, libxl_domain_config), sizeof(libxl_device_disk),
libxl__device_disk_add },
{ offsetof(vifs, libxl_domain_config), sizeof(libxl_device_vif),
libxl__device_nic_add },
...
which your domain creation logic could iterate over. That way you
would do away with domcreate_FOO_connected and you'd end up with
static void domcreate_device_connected(libxl__egc *egc,
libxl__ao_device *aodev)
{
tableentry = devicekindstable[dcs->currentdevicekind];
rc = libxl__ao_device_check_last(gc, aodev, dcs->devices,
dcs->num_devices);
if (rc > 0) return;
if (rc) {
LOGE(ERROR, "error connecting devices");
goto error_out;
}
dcs->currentdevicekind++;
if (!devicekindstable[dcs->currentdevicekind].add_function) {
domcreate_complete(egc, dcs, rc);
} else {
domcreate_attach_devices(egc, dcs);
}
or something like it.
Or something. Anyway, can you try to find a way to avoid me having to
review lots of nearly-identical code ?
Thanks,
Ian.
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v4 05/10] libxl: convert libxl_device_nic_add to an async operation
2012-05-29 14:36 ` Ian Jackson
@ 2012-05-30 9:54 ` Roger Pau Monne
2012-05-30 10:06 ` Ian Jackson
0 siblings, 1 reply; 26+ messages in thread
From: Roger Pau Monne @ 2012-05-30 9:54 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel@lists.xen.org
Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH v4 05/10] libxl: convert libxl_device_nic_add to an async operation"):
>> This patch converts libxl_device_nic_add to an ao operation that
>> waits for device backend to reach state XenbusStateInitWait and then
>> marks the operation as completed. This is not really useful now, but
>> will be used by latter patches that will launch hotplug scripts after
>> we reached the desired xenbus state.
>
> Thanks. I'm afraid this is still tripping my repetition detector.
> See below.
>
>> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
>> index 6c9bbc2..9933cc2 100644
>> --- a/tools/libxl/libxl_device.c
>> +++ b/tools/libxl/libxl_device.c
>> @@ -444,6 +444,24 @@ void libxl__add_disks(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
>> }
>> }
>>
>> +void libxl__add_nics(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
>> + libxl_domain_config *d_config,
>> + libxl__ao_device **devices,
>> + libxl__device_callback callback)
>> +{
>> + AO_GC;
>> + GCNEW_ARRAY(*devices, d_config->num_vifs);
>> + libxl__ao_device *aodev = *devices;
>> + int i;
>> +
>> + for (i = 0; i< d_config->num_vifs; i++) {
>> + libxl__prepare_ao_device(&aodev[i], ao, devices);
>> + aodev[i].callback = callback;
>> + libxl__device_nic_add(egc, domid,&d_config->vifs[i],
>> +&aodev[i]);
>> + }
>> +}
>
> The same comment applies as before about libxl__add_disks and the two
> loops.
>
>
> But, this is another copy of libxl__add_disks. Perhaps you want a
> macro to generate these functions ?
Yes, a macro would be ok I think.
> In general this patch seems to contain an awful lot of very similar
> code to the previous one. Eg domcreate_nic_connected is practically
> identical to domcreate_disk_connected. And domcreate_attach_nick is
> practically identical to domcreate_attach_disk.
>
>
> Another option would be to make the types of libxl__device_FOO_add
> compatible (by replacing the actual config parameter with const void*)
> and then pass them as function pointers.
>
> Or you could even have a table
> { offsetof(disks, libxl_domain_config), sizeof(libxl_device_disk),
> libxl__device_disk_add },
> { offsetof(vifs, libxl_domain_config), sizeof(libxl_device_vif),
> libxl__device_nic_add },
> ...
> which your domain creation logic could iterate over. That way you
> would do away with domcreate_FOO_connected and you'd end up with
>
> static void domcreate_device_connected(libxl__egc *egc,
> libxl__ao_device *aodev)
> {
>
> tableentry = devicekindstable[dcs->currentdevicekind];
>
> rc = libxl__ao_device_check_last(gc, aodev, dcs->devices,
> dcs->num_devices);
>
> if (rc> 0) return;
> if (rc) {
> LOGE(ERROR, "error connecting devices");
> goto error_out;
> }
>
> dcs->currentdevicekind++;
>
> if (!devicekindstable[dcs->currentdevicekind].add_function) {
> domcreate_complete(egc, dcs, rc);
> } else {
> domcreate_attach_devices(egc, dcs);
> }
>
> or something like it.
We can really iterate over this to add the devices, since we might need
to launch the device model between connecting the disks and connecting
the nics, I will try to find a nicer way to do this, I will try to come
up with some macros to define this in a nicer way.
> Or something. Anyway, can you try to find a way to avoid me having to
> review lots of nearly-identical code ?
>
>
> Thanks,
> Ian.
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v4 05/10] libxl: convert libxl_device_nic_add to an async operation
2012-05-30 9:54 ` Roger Pau Monne
@ 2012-05-30 10:06 ` Ian Jackson
0 siblings, 0 replies; 26+ messages in thread
From: Ian Jackson @ 2012-05-30 10:06 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel@lists.xen.org
Roger Pau Monne writes ("Re: [PATCH v4 05/10] libxl: convert libxl_device_nic_add to an async operation"):
> Ian Jackson wrote:
> > Or you could even have a table
...
>
> We can really iterate over this to add the devices, since we might need
> to launch the device model between connecting the disks and connecting
> the nics, I will try to find a nicer way to do this, I will try to come
> up with some macros to define this in a nicer way.
I guess you mean `we can't really'. OK, thanks. Let me know if you
need any help/suggestions.
Ian.
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v4 06/10] libxl: add option to choose who executes hotplug scripts
2012-05-24 10:23 [PATCH v4 0/10] execute hotplug scripts from libxl Roger Pau Monne
` (4 preceding siblings ...)
2012-05-24 10:24 ` [PATCH v4 05/10] libxl: convert libxl_device_nic_add to an async operation Roger Pau Monne
@ 2012-05-24 10:24 ` Roger Pau Monne
2012-05-29 14:38 ` Ian Jackson
2012-05-24 10:24 ` [PATCH v4 07/10] libxl: set nic type to VIF by default Roger Pau Monne
` (3 subsequent siblings)
9 siblings, 1 reply; 26+ messages in thread
From: Roger Pau Monne @ 2012-05-24 10:24 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Roger Pau Monne
Add and option to xl.conf file to decide if hotplug scripts are
executed from the toolstack (xl) or from udev as it used to be in the
past.
This option is only introduced in this patch, but it has no effect
since the code to call hotplug scripts from libxl is introduced in a
latter patch.
This choice will be saved in "libxl/disable_udev", as specified in the
DISABLE_UDEV_PATH constant.
Changes since v2:
* Change atoi(...) to !!atoi(...) to prevent returning negative
values from xenstore (which will be handled as errors).
* Check for errors on the return value of libxl__hotplug_settings.
Changes since v1:
* Used an auxiliary function to check for the current setting.
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
---
docs/man/xl.conf.pod.5 | 8 ++++++++
tools/examples/xl.conf | 5 +++++
tools/libxl/libxl_create.c | 36 +++++++++++++++++++++++++++++++++++-
tools/libxl/libxl_internal.c | 19 +++++++++++++++++++
tools/libxl/libxl_internal.h | 3 +++
tools/libxl/libxl_types.idl | 1 +
tools/libxl/xl.c | 4 ++++
tools/libxl/xl.h | 1 +
tools/libxl/xl_cmdimpl.c | 1 +
9 files changed, 77 insertions(+), 1 deletions(-)
diff --git a/docs/man/xl.conf.pod.5 b/docs/man/xl.conf.pod.5
index 8bd45ea..72825a0 100644
--- a/docs/man/xl.conf.pod.5
+++ b/docs/man/xl.conf.pod.5
@@ -55,6 +55,14 @@ default.
Default: C<1>
+=item B<run_hotplug_scripts=BOOLEAN>
+
+If disabled hotplug scripts will be called from udev, as it used to
+be in the previous releases. With the default option, hotplug scripts
+will be launched by xl directly.
+
+Default: C<1>
+
=item B<lockfile="PATH">
Sets the path to the lock file used by xl to serialise certain
diff --git a/tools/examples/xl.conf b/tools/examples/xl.conf
index 56d3b3b..75b00e0 100644
--- a/tools/examples/xl.conf
+++ b/tools/examples/xl.conf
@@ -12,3 +12,8 @@
# default output format used by "xl list -l"
#output_format="json"
+
+# default option to run hotplug scripts from xl
+# if disabled the old behaviour will be used, and hotplug scripts will be
+# launched by udev.
+#run_hotplug_scripts=1
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 7466498..2f92252 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -398,7 +398,7 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_create_info *info,
uint32_t *domid)
{
libxl_ctx *ctx = libxl__gc_owner(gc);
- int flags, ret, rc;
+ int flags, ret, rc, nb_vm;
char *uuid_string;
char *dom_path, *vm_path, *libxl_path;
struct xs_permissions roperm[2];
@@ -519,6 +519,40 @@ retry_transaction:
libxl__sprintf(gc, "%s/hvmloader/generation-id-address", dom_path),
rwperm, ARRAY_SIZE(rwperm));
+ if (libxl_list_vm(ctx, &nb_vm) < 0) {
+ LOG(ERROR, "cannot get number of running guests");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ int hotplug_setting = libxl__hotplug_settings(gc, t);
+ if (hotplug_setting < 0) {
+ LOG(ERROR, "unable to get current hotplug scripts execution setting");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ if (libxl_defbool_val(info->run_hotplug_scripts) != hotplug_setting &&
+ (nb_vm - 1)) {
+ LOG(ERROR, "cannot change hotplug execution option once set, "
+ "please shutdown all guests before changing it");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ if (libxl_defbool_val(info->run_hotplug_scripts)) {
+ rc = libxl__xs_write(gc, t, DISABLE_UDEV_PATH, "1");
+ if (rc) {
+ LOGE(ERROR, "unable to write %s = 1", DISABLE_UDEV_PATH);
+ goto out;
+ }
+ } else {
+ rc = xs_rm(ctx->xsh, t, DISABLE_UDEV_PATH);
+ if (rc) {
+ LOGE(ERROR, "unable to delete %s", DISABLE_UDEV_PATH);
+ goto out;
+ }
+ }
+
xs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/uuid", vm_path), uuid_string, strlen(uuid_string));
xs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/name", vm_path), info->name, strlen(info->name));
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index b89aef7..5525575 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -346,6 +346,25 @@ libxl_device_model_version libxl__device_model_version_running(libxl__gc *gc,
return value;
}
+int libxl__hotplug_settings(libxl__gc *gc, xs_transaction_t t)
+{
+ int rc = 0;
+ char *val;
+
+ val = libxl__xs_read(gc, t, DISABLE_UDEV_PATH);
+ if (!val && errno != ENOENT) {
+ LOGE(ERROR, "cannot read %s from xenstore", DISABLE_UDEV_PATH);
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ if (!val) val = "0";
+
+ rc = !!atoi(val);
+
+out:
+ return rc;
+}
+
/*
* Local variables:
* mode: C
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 44c7c66..45b776c 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -86,6 +86,7 @@
#define STUBDOM_CONSOLE_SERIAL 3
#define STUBDOM_SPECIAL_CONSOLES 3
#define TAP_DEVICE_SUFFIX "-emu"
+#define DISABLE_UDEV_PATH "libxl/disable_udev"
#define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
@@ -1393,6 +1394,8 @@ _hidden libxl__json_object *libxl__json_parse(libxl__gc *gc, const char *s);
_hidden libxl_device_model_version
libxl__device_model_version_running(libxl__gc *gc, uint32_t domid);
+/* Check how executes hotplug script currently */
+int libxl__hotplug_settings(libxl__gc *gc, xs_transaction_t t);
/*
* Calling context and GC for event-generating functions:
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index a21bd85..a82bdb9 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -220,6 +220,7 @@ libxl_domain_create_info = Struct("domain_create_info",[
("xsdata", libxl_key_value_list),
("platformdata", libxl_key_value_list),
("poolid", uint32),
+ ("run_hotplug_scripts",libxl_defbool),
], dir=DIR_IN)
MemKB = UInt(64, init_val = "LIBXL_MEMKB_DEFAULT")
diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
index 750a61c..28b3218 100644
--- a/tools/libxl/xl.c
+++ b/tools/libxl/xl.c
@@ -38,6 +38,7 @@ xentoollog_logger_stdiostream *logger;
int dryrun_only;
int force_execution;
int autoballoon = 1;
+libxl_defbool run_hotplug_scripts;
char *lockfile;
char *default_vifscript = NULL;
char *default_bridge = NULL;
@@ -69,6 +70,9 @@ static void parse_global_config(const char *configfile,
if (!xlu_cfg_get_long (config, "autoballoon", &l, 0))
autoballoon = l;
+ libxl_defbool_setdefault(&run_hotplug_scripts, true);
+ xlu_cfg_get_defbool(config, "run_hotplug_scripts", &run_hotplug_scripts, 0);
+
if (!xlu_cfg_get_string (config, "lockfile", &buf, 0))
lockfile = strdup(buf);
else {
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index b7eacaa..c597d24 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -113,6 +113,7 @@ void postfork(void);
/* global options */
extern int autoballoon;
+extern libxl_defbool run_hotplug_scripts;
extern int dryrun_only;
extern char *lockfile;
extern char *default_vifscript;
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index a1883b5..ea9a267 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -593,6 +593,7 @@ static void parse_config_data(const char *config_source,
}
}
+ c_info->run_hotplug_scripts = run_hotplug_scripts;
c_info->type = LIBXL_DOMAIN_TYPE_PV;
if (!xlu_cfg_get_string (config, "builder", &buf, 0) &&
!strncmp(buf, "hvm", strlen(buf)))
--
1.7.7.5 (Apple Git-26)
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v4 07/10] libxl: set nic type to VIF by default
2012-05-24 10:23 [PATCH v4 0/10] execute hotplug scripts from libxl Roger Pau Monne
` (5 preceding siblings ...)
2012-05-24 10:24 ` [PATCH v4 06/10] libxl: add option to choose who executes hotplug scripts Roger Pau Monne
@ 2012-05-24 10:24 ` Roger Pau Monne
2012-05-24 10:24 ` [PATCH v4 08/10] libxl: call hotplug scripts for disk devices from libxl Roger Pau Monne
` (2 subsequent siblings)
9 siblings, 0 replies; 26+ messages in thread
From: Roger Pau Monne @ 2012-05-24 10:24 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Roger Pau Monne
Set the default value for nic interfaces to VIF, since it used to be
IOEMU, even for PV guests.
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
---
tools/libxl/libxl.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index a5fc42d..2f27fd5 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2008,7 +2008,7 @@ int libxl__device_nic_setdefault(libxl__gc *gc, libxl_device_nic *nic)
libxl__xen_script_dir_path()) < 0 )
return ERROR_FAIL;
if (!nic->nictype)
- nic->nictype = LIBXL_NIC_TYPE_IOEMU;
+ nic->nictype = LIBXL_NIC_TYPE_VIF;
return 0;
}
--
1.7.7.5 (Apple Git-26)
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v4 08/10] libxl: call hotplug scripts for disk devices from libxl
2012-05-24 10:23 [PATCH v4 0/10] execute hotplug scripts from libxl Roger Pau Monne
` (6 preceding siblings ...)
2012-05-24 10:24 ` [PATCH v4 07/10] libxl: set nic type to VIF by default Roger Pau Monne
@ 2012-05-24 10:24 ` Roger Pau Monne
2012-05-25 15:13 ` Roger Pau Monne
2012-05-29 14:50 ` Ian Jackson
2012-05-24 10:24 ` [PATCH v4 09/10] libxl: call hotplug scripts for nic " Roger Pau Monne
2012-05-24 10:24 ` [PATCH v4 10/10] libxl: use libxl__xs_path_cleanup on device_destroy Roger Pau Monne
9 siblings, 2 replies; 26+ messages in thread
From: Roger Pau Monne @ 2012-05-24 10:24 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Roger Pau Monne
Since most of the needed work is already done in previous patches,
this patch only contains the necessary code to call hotplug scripts
for disk devices, that should be called when the device is added or
removed from a guest.
We will chain the launch of the disk hotplug scripts after the
device_backend_callback callback, or directly from
libxl__initiate_device_{add,remove} if the device is already in the
desired state.
Changes since v2:
* Added array size check with assert.
* Added NetBSD code (so compilation is not broken).
* Removed a check for null in device_hotplug_timeout_cb.
Changes since v1:
* Moved all the event related code that was inside libxl_linux.c into
libxl_device.c, so the flow of the device addition/removal event is
all in the same file.
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
---
tools/hotplug/Linux/xen-backend.rules | 6 +-
tools/hotplug/Linux/xen-hotplug-common.sh | 6 ++
tools/libxl/libxl.c | 10 +++
tools/libxl/libxl_device.c | 121 ++++++++++++++++++++++++++++-
tools/libxl/libxl_internal.h | 20 +++++
tools/libxl/libxl_linux.c | 98 +++++++++++++++++++++++
tools/libxl/libxl_netbsd.c | 9 ++
7 files changed, 266 insertions(+), 4 deletions(-)
diff --git a/tools/hotplug/Linux/xen-backend.rules b/tools/hotplug/Linux/xen-backend.rules
index 405387f..d55ff11 100644
--- a/tools/hotplug/Linux/xen-backend.rules
+++ b/tools/hotplug/Linux/xen-backend.rules
@@ -1,11 +1,11 @@
-SUBSYSTEM=="xen-backend", KERNEL=="tap*", RUN+="/etc/xen/scripts/blktap $env{ACTION}"
-SUBSYSTEM=="xen-backend", KERNEL=="vbd*", RUN+="/etc/xen/scripts/block $env{ACTION}"
+SUBSYSTEM=="xen-backend", KERNEL=="tap*", ENV{UDEV_CALL}="1", RUN+="/etc/xen/scripts/blktap $env{ACTION}"
+SUBSYSTEM=="xen-backend", KERNEL=="vbd*", ENV{UDEV_CALL}="1", RUN+="/etc/xen/scripts/block $env{ACTION}"
SUBSYSTEM=="xen-backend", KERNEL=="vtpm*", RUN+="/etc/xen/scripts/vtpm $env{ACTION}"
SUBSYSTEM=="xen-backend", KERNEL=="vif2-*", RUN+="/etc/xen/scripts/vif2 $env{ACTION}"
SUBSYSTEM=="xen-backend", KERNEL=="vif-*", ACTION=="online", RUN+="/etc/xen/scripts/vif-setup online type_if=vif"
SUBSYSTEM=="xen-backend", KERNEL=="vif-*", ACTION=="offline", RUN+="/etc/xen/scripts/vif-setup offline type_if=vif"
SUBSYSTEM=="xen-backend", KERNEL=="vscsi*", RUN+="/etc/xen/scripts/vscsi $env{ACTION}"
-SUBSYSTEM=="xen-backend", ACTION=="remove", RUN+="/etc/xen/scripts/xen-hotplug-cleanup"
+SUBSYSTEM=="xen-backend", ACTION=="remove", ENV{UDEV_CALL}="1", RUN+="/etc/xen/scripts/xen-hotplug-cleanup"
KERNEL=="evtchn", NAME="xen/%k"
SUBSYSTEM=="xen", KERNEL=="blktap[0-9]*", NAME="xen/%k", MODE="0600"
SUBSYSTEM=="blktap2", KERNEL=="blktap[0-9]*", NAME="xen/blktap-2/%k", MODE="0600"
diff --git a/tools/hotplug/Linux/xen-hotplug-common.sh b/tools/hotplug/Linux/xen-hotplug-common.sh
index 8f6557d..4a7bc73 100644
--- a/tools/hotplug/Linux/xen-hotplug-common.sh
+++ b/tools/hotplug/Linux/xen-hotplug-common.sh
@@ -15,6 +15,12 @@
# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
#
+# Hack to prevent the execution of hotplug scripts from udev if the domain
+# has been launched from libxl
+if [ -n "${UDEV_CALL}" ] && \
+ xenstore-read "libxl/disable_udev" >/dev/null 2>&1; then
+ exit 0
+fi
dir=$(dirname "$0")
. "$dir/hotplugpath.sh"
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2f27fd5..ccb5bdc 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1607,6 +1607,11 @@ void libxl__device_disk_add(libxl__egc *egc, uint32_t domid,
flexarray_append(back, "params");
flexarray_append(back, dev);
+ flexarray_append(back, "script");
+ flexarray_append(back, GCSPRINTF("%s/%s",
+ libxl__xen_script_dir_path(),
+ "block"));
+
assert(device->backend_kind == LIBXL__DEVICE_KIND_VBD);
break;
case LIBXL_DISK_BACKEND_TAP:
@@ -1622,6 +1627,11 @@ void libxl__device_disk_add(libxl__egc *egc, uint32_t domid,
libxl__device_disk_string_of_format(disk->format),
disk->pdev_path));
+ flexarray_append(back, "script");
+ flexarray_append(back, GCSPRINTF("%s/%s",
+ libxl__xen_script_dir_path(),
+ "blktap"));
+
/* now create a phy device to export the device to the guest */
goto do_backend_phy;
case LIBXL_DISK_BACKEND_QDISK:
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 9933cc2..8e1ec0f 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -569,6 +569,14 @@ static void device_backend_callback(libxl__egc *egc, libxl__ev_devstate *ds,
static void device_backend_cleanup(libxl__gc *gc,
libxl__ao_device *aodev);
+static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev);
+
+static void device_hotplug_fork_cb(libxl__egc *egc, libxl__ev_child *child,
+ pid_t pid, int status);
+
+static void device_hotplug_timeout_cb(libxl__egc *egc, libxl__ev_time *ev,
+ const struct timeval *requested_abs);
+
void libxl__initiate_device_add(libxl__egc *egc, libxl__ao_device *aodev)
{
STATE_AO_GC(aodev->ao);
@@ -657,7 +665,7 @@ retry_transaction:
out_ok:
if (t) xs_transaction_end(ctx->xsh, t, 0);
- aodev->callback(egc, aodev);
+ device_hotplug(egc, aodev);
return;
}
@@ -689,6 +697,9 @@ static void device_backend_callback(libxl__egc *egc, libxl__ev_devstate *ds,
goto out;
}
+ device_hotplug(egc, aodev);
+ return;
+
out:
aodev->rc = rc;
aodev->callback(egc, aodev);
@@ -701,6 +712,114 @@ static void device_backend_cleanup(libxl__gc *gc, libxl__ao_device *aodev)
libxl__ev_devstate_cancel(gc, &aodev->ds);
}
+static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev)
+{
+ STATE_AO_GC(aodev->ao);
+ char *be_path = libxl__device_backend_path(gc, aodev->dev);
+ char **args, **env;
+ int rc = 0;
+ int hotplug;
+
+ /* Check if we have to execute hotplug scripts for this device
+ * and return the necessary args/env vars for execution */
+ hotplug = libxl__get_hotplug_script_info(gc, aodev->dev, &args, &env,
+ aodev->action);
+ switch (hotplug) {
+ case 0:
+ /* no hotplug script to execute */
+ goto out;
+ case 1:
+ /* execute hotplug script */
+ break;
+ default:
+ /* everything else is an error */
+ LOG(ERROR, "unable to get args/env to execute hotplug script for "
+ "device %s", libxl__device_backend_path(gc, aodev->dev));
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ /* Set hotplug timeout */
+ libxl__ev_time_init(&aodev->ev);
+ rc = libxl__ev_time_register_rel(gc, &aodev->ev, device_hotplug_timeout_cb,
+ LIBXL_HOTPLUG_TIMEOUT * 1000);
+ if (rc) {
+ LOG(ERROR, "unable to register timeout for hotplug device %s", be_path);
+ goto out;
+ }
+
+ aodev->what = GCSPRINTF("%s %s", args[0], args[1]);
+ LOG(DEBUG, "calling hotplug script: %s %s", args[0], args[1]);
+ libxl__ev_child_init(&aodev->child);
+
+ /* fork and execute hotplug script */
+ aodev->pid = libxl__ev_child_fork(gc, &aodev->child,
+ device_hotplug_fork_cb);
+ if (aodev->pid == -1) {
+ LOG(ERROR, "unable to fork");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ if (!aodev->pid) {
+ /* child */
+ libxl__exec(gc, -1, -1, -1, args[0], args, env);
+ /* notreached */
+ abort();
+ }
+
+ if (!libxl__ev_child_inuse(&aodev->child)) {
+ /* hotplug launch failed */
+ LOG(ERROR, "unable to launch hotplug script for device %s", be_path);
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ return;
+
+out:
+ libxl__ev_time_deregister(gc, &aodev->ev);
+ aodev->rc = rc;
+ aodev->callback(egc, aodev);
+ return;
+}
+
+static void device_hotplug_fork_cb(libxl__egc *egc, libxl__ev_child *child,
+ pid_t pid, int status)
+{
+ libxl__ao_device *aodev = CONTAINER_OF(child, *aodev, child);
+ STATE_AO_GC(aodev->ao);
+
+ libxl__ev_time_deregister(gc, &aodev->ev);
+
+ if (status) {
+ libxl_report_child_exitstatus(CTX, aodev->rc ? LIBXL__LOG_ERROR
+ : LIBXL__LOG_WARNING,
+ aodev->what, pid, status);
+ aodev->rc = ERROR_FAIL;
+ }
+ aodev->callback(egc, aodev);
+}
+
+static void device_hotplug_timeout_cb(libxl__egc *egc, libxl__ev_time *ev,
+ const struct timeval *requested_abs)
+{
+ libxl__ao_device *aodev = CONTAINER_OF(ev, *aodev, ev);
+ STATE_AO_GC(aodev->ao);
+
+ if (libxl__ev_child_inuse(&aodev->child)) {
+ if (kill(aodev->pid, SIGKILL)) {
+ LOGEV(ERROR, errno, "unable to kill hotplug script %s [%ld]",
+ aodev->what, (unsigned long)aodev->pid);
+ goto out;
+ }
+ }
+
+out:
+ libxl__ev_time_deregister(gc, &aodev->ev);
+ return;
+}
+
static void device_remove_callback(libxl__egc *egc, libxl__ao_device *aodev)
{
STATE_AO_GC(aodev->ao);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 45b776c..da5b02b 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -72,6 +72,7 @@
#define LIBXL_INIT_TIMEOUT 10
#define LIBXL_DESTROY_TIMEOUT 10
+#define LIBXL_HOTPLUG_TIMEOUT 10
#define LIBXL_DEVICE_MODEL_START_TIMEOUT 10
#define LIBXL_XENCONSOLE_LIMIT 1048576
#define LIBXL_XENCONSOLE_PROTOCOL "vt100"
@@ -1814,6 +1815,11 @@ struct libxl__ao_device {
int rc;
libxl__ev_devstate ds;
void *base;
+ /* device hotplug execution */
+ pid_t pid;
+ char *what;
+ libxl__ev_time ev;
+ libxl__ev_child child;
};
/* Internal AO operation to connect a disk device */
@@ -1842,6 +1848,20 @@ _hidden void libxl__initiate_device_add(libxl__egc*, libxl__ao_device *aodev);
_hidden void libxl__initiate_device_remove(libxl__egc *egc,
libxl__ao_device *aodev);
+/*
+ * libxl__get_hotplug_script_info returns the args and env that should
+ * be passed to the hotplug script for the requested device.
+ *
+ * Since a device might not need to execute any hotplug script, this function
+ * can return the following values:
+ * < 0: Error
+ * 0: No need to execute hotplug script
+ * 1: Execute hotplug script
+ */
+_hidden int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
+ char ***args, char ***env,
+ libxl__device_action action);
+
/*----- Domain destruction -----*/
/* Domain destruction has been split into two functions:
diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index 925248b..98cd25f 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -25,3 +25,101 @@ int libxl__try_phy_backend(mode_t st_mode)
return 1;
}
+
+/* Hotplug scripts helpers */
+
+static char **get_hotplug_env(libxl__gc *gc, libxl__device *dev)
+{
+ char *be_path = libxl__device_backend_path(gc, dev);
+ char *script;
+ const char *type = libxl__device_kind_to_string(dev->backend_kind);
+ char **env;
+ int nr = 0, arraysize = 9;
+
+ script = libxl__xs_read(gc, XBT_NULL,
+ GCSPRINTF("%s/%s", be_path, "script"));
+ if (!script) {
+ LOGEV(ERROR, errno, "unable to read script from %s", be_path);
+ return NULL;
+ }
+
+ GCNEW_ARRAY(env, arraysize);
+ env[nr++] = "script";
+ env[nr++] = script;
+ env[nr++] = "XENBUS_TYPE";
+ env[nr++] = libxl__strdup(gc, type);
+ env[nr++] = "XENBUS_PATH";
+ env[nr++] = GCSPRINTF("backend/%s/%u/%d", type, dev->domid, dev->devid);
+ env[nr++] = "XENBUS_BASE_PATH";
+ env[nr++] = "backend";
+ env[nr++] = NULL;
+ assert(nr == arraysize);
+
+ return env;
+}
+
+/* Hotplug scripts caller functions */
+
+static int libxl__hotplug_disk(libxl__gc *gc, libxl__device *dev,
+ char ***args, char ***env,
+ libxl__device_action action)
+{
+ char *be_path = libxl__device_backend_path(gc, dev);
+ char *script;
+ int nr = 0, rc = 0, arraysize = 3;
+
+ script = libxl__xs_read(gc, XBT_NULL,
+ GCSPRINTF("%s/%s", be_path, "script"));
+ if (!script) {
+ LOGEV(ERROR, errno, "unable to read script from %s", be_path);
+ rc = ERROR_FAIL;
+ goto error;
+ }
+
+ *env = get_hotplug_env(gc, dev);
+ if (!*env) {
+ rc = ERROR_FAIL;
+ goto error;
+ }
+
+ GCNEW_ARRAY(*args, arraysize);
+ (*args)[nr++] = script;
+ (*args)[nr++] = action == DEVICE_CONNECT ? "add" : "remove";
+ (*args)[nr++] = NULL;
+ assert(nr == arraysize);
+
+ rc = 0;
+
+error:
+ return rc;
+}
+
+int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
+ char ***args, char ***env,
+ libxl__device_action action)
+{
+ char *disable_udev = libxl__xs_read(gc, XBT_NULL, DISABLE_UDEV_PATH);
+ int rc;
+
+ /* Check if we have to run hotplug scripts */
+ if (!disable_udev) {
+ rc = 0;
+ goto out;
+ }
+
+ switch (dev->backend_kind) {
+ case LIBXL__DEVICE_KIND_VBD:
+ rc = libxl__hotplug_disk(gc, dev, args, env, action);
+ if (!rc) rc = 1;
+ break;
+ default:
+ /* If no need to execute any hotplug scripts,
+ * call the callback manually
+ */
+ rc = 0;
+ break;
+ }
+
+out:
+ return rc;
+}
diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
index 9e0ed6d..0f2cdaa 100644
--- a/tools/libxl/libxl_netbsd.c
+++ b/tools/libxl/libxl_netbsd.c
@@ -24,3 +24,12 @@ int libxl__try_phy_backend(mode_t st_mode)
return 0;
}
+
+/* Hotplug scripts caller functions */
+
+int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
+ char ***args, char ***env,
+ libxl__device_action action)
+{
+ return 0;
+}
\ No newline at end of file
--
1.7.7.5 (Apple Git-26)
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v4 08/10] libxl: call hotplug scripts for disk devices from libxl
2012-05-24 10:24 ` [PATCH v4 08/10] libxl: call hotplug scripts for disk devices from libxl Roger Pau Monne
@ 2012-05-25 15:13 ` Roger Pau Monne
2012-05-29 14:50 ` Ian Jackson
1 sibling, 0 replies; 26+ messages in thread
From: Roger Pau Monne @ 2012-05-25 15:13 UTC (permalink / raw)
To: xen-devel@lists.xen.org; +Cc: Ian Jackson
Roger Pau Monne wrote:
> Since most of the needed work is already done in previous patches,
> this patch only contains the necessary code to call hotplug scripts
> for disk devices, that should be called when the device is added or
> removed from a guest.
>
> We will chain the launch of the disk hotplug scripts after the
> device_backend_callback callback, or directly from
> libxl__initiate_device_{add,remove} if the device is already in the
> desired state.
>
> Changes since v2:
>
> * Added array size check with assert.
>
> * Added NetBSD code (so compilation is not broken).
>
> * Removed a check for null in device_hotplug_timeout_cb.
>
> Changes since v1:
>
> * Moved all the event related code that was inside libxl_linux.c into
> libxl_device.c, so the flow of the device addition/removal event is
> all in the same file.
>
> Cc: Ian Jackson<ian.jackson@eu.citrix.com>
> Signed-off-by: Roger Pau Monne<roger.pau@citrix.com>
> ---
> tools/hotplug/Linux/xen-backend.rules | 6 +-
> tools/hotplug/Linux/xen-hotplug-common.sh | 6 ++
> tools/libxl/libxl.c | 10 +++
> tools/libxl/libxl_device.c | 121 ++++++++++++++++++++++++++++-
> tools/libxl/libxl_internal.h | 20 +++++
> tools/libxl/libxl_linux.c | 98 +++++++++++++++++++++++
> tools/libxl/libxl_netbsd.c | 9 ++
> 7 files changed, 266 insertions(+), 4 deletions(-)
>
> diff --git a/tools/hotplug/Linux/xen-backend.rules b/tools/hotplug/Linux/xen-backend.rules
> index 405387f..d55ff11 100644
> --- a/tools/hotplug/Linux/xen-backend.rules
> +++ b/tools/hotplug/Linux/xen-backend.rules
> @@ -1,11 +1,11 @@
> -SUBSYSTEM=="xen-backend", KERNEL=="tap*", RUN+="/etc/xen/scripts/blktap $env{ACTION}"
> -SUBSYSTEM=="xen-backend", KERNEL=="vbd*", RUN+="/etc/xen/scripts/block $env{ACTION}"
> +SUBSYSTEM=="xen-backend", KERNEL=="tap*", ENV{UDEV_CALL}="1", RUN+="/etc/xen/scripts/blktap $env{ACTION}"
> +SUBSYSTEM=="xen-backend", KERNEL=="vbd*", ENV{UDEV_CALL}="1", RUN+="/etc/xen/scripts/block $env{ACTION}"
> SUBSYSTEM=="xen-backend", KERNEL=="vtpm*", RUN+="/etc/xen/scripts/vtpm $env{ACTION}"
> SUBSYSTEM=="xen-backend", KERNEL=="vif2-*", RUN+="/etc/xen/scripts/vif2 $env{ACTION}"
> SUBSYSTEM=="xen-backend", KERNEL=="vif-*", ACTION=="online", RUN+="/etc/xen/scripts/vif-setup online type_if=vif"
> SUBSYSTEM=="xen-backend", KERNEL=="vif-*", ACTION=="offline", RUN+="/etc/xen/scripts/vif-setup offline type_if=vif"
> SUBSYSTEM=="xen-backend", KERNEL=="vscsi*", RUN+="/etc/xen/scripts/vscsi $env{ACTION}"
> -SUBSYSTEM=="xen-backend", ACTION=="remove", RUN+="/etc/xen/scripts/xen-hotplug-cleanup"
> +SUBSYSTEM=="xen-backend", ACTION=="remove", ENV{UDEV_CALL}="1", RUN+="/etc/xen/scripts/xen-hotplug-cleanup"
> KERNEL=="evtchn", NAME="xen/%k"
> SUBSYSTEM=="xen", KERNEL=="blktap[0-9]*", NAME="xen/%k", MODE="0600"
> SUBSYSTEM=="blktap2", KERNEL=="blktap[0-9]*", NAME="xen/blktap-2/%k", MODE="0600"
> diff --git a/tools/hotplug/Linux/xen-hotplug-common.sh b/tools/hotplug/Linux/xen-hotplug-common.sh
> index 8f6557d..4a7bc73 100644
> --- a/tools/hotplug/Linux/xen-hotplug-common.sh
> +++ b/tools/hotplug/Linux/xen-hotplug-common.sh
> @@ -15,6 +15,12 @@
> # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> #
>
> +# Hack to prevent the execution of hotplug scripts from udev if the domain
> +# has been launched from libxl
> +if [ -n "${UDEV_CALL}" ]&& \
> + xenstore-read "libxl/disable_udev">/dev/null 2>&1; then
> + exit 0
> +fi
>
> dir=$(dirname "$0")
> . "$dir/hotplugpath.sh"
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 2f27fd5..ccb5bdc 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1607,6 +1607,11 @@ void libxl__device_disk_add(libxl__egc *egc, uint32_t domid,
> flexarray_append(back, "params");
> flexarray_append(back, dev);
>
> + flexarray_append(back, "script");
> + flexarray_append(back, GCSPRINTF("%s/%s",
> + libxl__xen_script_dir_path(),
> + "block"));
> +
> assert(device->backend_kind == LIBXL__DEVICE_KIND_VBD);
> break;
> case LIBXL_DISK_BACKEND_TAP:
> @@ -1622,6 +1627,11 @@ void libxl__device_disk_add(libxl__egc *egc, uint32_t domid,
> libxl__device_disk_string_of_format(disk->format),
> disk->pdev_path));
>
> + flexarray_append(back, "script");
> + flexarray_append(back, GCSPRINTF("%s/%s",
> + libxl__xen_script_dir_path(),
> + "blktap"));
> +
> /* now create a phy device to export the device to the guest */
> goto do_backend_phy;
> case LIBXL_DISK_BACKEND_QDISK:
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index 9933cc2..8e1ec0f 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -569,6 +569,14 @@ static void device_backend_callback(libxl__egc *egc, libxl__ev_devstate *ds,
> static void device_backend_cleanup(libxl__gc *gc,
> libxl__ao_device *aodev);
>
> +static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev);
> +
> +static void device_hotplug_fork_cb(libxl__egc *egc, libxl__ev_child *child,
> + pid_t pid, int status);
> +
> +static void device_hotplug_timeout_cb(libxl__egc *egc, libxl__ev_time *ev,
> + const struct timeval *requested_abs);
> +
> void libxl__initiate_device_add(libxl__egc *egc, libxl__ao_device *aodev)
> {
> STATE_AO_GC(aodev->ao);
> @@ -657,7 +665,7 @@ retry_transaction:
>
> out_ok:
> if (t) xs_transaction_end(ctx->xsh, t, 0);
> - aodev->callback(egc, aodev);
> + device_hotplug(egc, aodev);
> return;
> }
>
> @@ -689,6 +697,9 @@ static void device_backend_callback(libxl__egc *egc, libxl__ev_devstate *ds,
> goto out;
> }
>
> + device_hotplug(egc, aodev);
> + return;
> +
> out:
> aodev->rc = rc;
> aodev->callback(egc, aodev);
> @@ -701,6 +712,114 @@ static void device_backend_cleanup(libxl__gc *gc, libxl__ao_device *aodev)
> libxl__ev_devstate_cancel(gc,&aodev->ds);
> }
>
> +static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev)
> +{
> + STATE_AO_GC(aodev->ao);
> + char *be_path = libxl__device_backend_path(gc, aodev->dev);
> + char **args, **env;
These should we initialised to NULL.
char **args = NULL, **env = NULL;
> + int rc = 0;
> + int hotplug;
> +
> + /* Check if we have to execute hotplug scripts for this device
> + * and return the necessary args/env vars for execution */
> + hotplug = libxl__get_hotplug_script_info(gc, aodev->dev,&args,&env,
> + aodev->action);
> + switch (hotplug) {
> + case 0:
> + /* no hotplug script to execute */
> + goto out;
> + case 1:
> + /* execute hotplug script */
> + break;
> + default:
> + /* everything else is an error */
> + LOG(ERROR, "unable to get args/env to execute hotplug script for "
> + "device %s", libxl__device_backend_path(gc, aodev->dev));
> + rc = ERROR_FAIL;
> + goto out;
> + }
> +
> + /* Set hotplug timeout */
> + libxl__ev_time_init(&aodev->ev);
> + rc = libxl__ev_time_register_rel(gc,&aodev->ev, device_hotplug_timeout_cb,
> + LIBXL_HOTPLUG_TIMEOUT * 1000);
> + if (rc) {
> + LOG(ERROR, "unable to register timeout for hotplug device %s", be_path);
> + goto out;
> + }
> +
> + aodev->what = GCSPRINTF("%s %s", args[0], args[1]);
> + LOG(DEBUG, "calling hotplug script: %s %s", args[0], args[1]);
> + libxl__ev_child_init(&aodev->child);
> +
> + /* fork and execute hotplug script */
> + aodev->pid = libxl__ev_child_fork(gc,&aodev->child,
> + device_hotplug_fork_cb);
> + if (aodev->pid == -1) {
> + LOG(ERROR, "unable to fork");
> + rc = ERROR_FAIL;
> + goto out;
> + }
> +
> + if (!aodev->pid) {
> + /* child */
> + libxl__exec(gc, -1, -1, -1, args[0], args, env);
> + /* notreached */
> + abort();
> + }
> +
> + if (!libxl__ev_child_inuse(&aodev->child)) {
> + /* hotplug launch failed */
> + LOG(ERROR, "unable to launch hotplug script for device %s", be_path);
> + rc = ERROR_FAIL;
> + goto out;
> + }
> +
> + return;
> +
> +out:
> + libxl__ev_time_deregister(gc,&aodev->ev);
> + aodev->rc = rc;
> + aodev->callback(egc, aodev);
> + return;
> +}
> +
> +static void device_hotplug_fork_cb(libxl__egc *egc, libxl__ev_child *child,
> + pid_t pid, int status)
> +{
> + libxl__ao_device *aodev = CONTAINER_OF(child, *aodev, child);
> + STATE_AO_GC(aodev->ao);
> +
> + libxl__ev_time_deregister(gc,&aodev->ev);
> +
> + if (status) {
> + libxl_report_child_exitstatus(CTX, aodev->rc ? LIBXL__LOG_ERROR
> + : LIBXL__LOG_WARNING,
> + aodev->what, pid, status);
> + aodev->rc = ERROR_FAIL;
> + }
> + aodev->callback(egc, aodev);
> +}
> +
> +static void device_hotplug_timeout_cb(libxl__egc *egc, libxl__ev_time *ev,
> + const struct timeval *requested_abs)
> +{
> + libxl__ao_device *aodev = CONTAINER_OF(ev, *aodev, ev);
> + STATE_AO_GC(aodev->ao);
> +
> + if (libxl__ev_child_inuse(&aodev->child)) {
> + if (kill(aodev->pid, SIGKILL)) {
> + LOGEV(ERROR, errno, "unable to kill hotplug script %s [%ld]",
> + aodev->what, (unsigned long)aodev->pid);
> + goto out;
> + }
> + }
> +
> +out:
> + libxl__ev_time_deregister(gc,&aodev->ev);
> + return;
> +}
> +
> static void device_remove_callback(libxl__egc *egc, libxl__ao_device *aodev)
> {
> STATE_AO_GC(aodev->ao);
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 45b776c..da5b02b 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -72,6 +72,7 @@
>
> #define LIBXL_INIT_TIMEOUT 10
> #define LIBXL_DESTROY_TIMEOUT 10
> +#define LIBXL_HOTPLUG_TIMEOUT 10
> #define LIBXL_DEVICE_MODEL_START_TIMEOUT 10
> #define LIBXL_XENCONSOLE_LIMIT 1048576
> #define LIBXL_XENCONSOLE_PROTOCOL "vt100"
> @@ -1814,6 +1815,11 @@ struct libxl__ao_device {
> int rc;
> libxl__ev_devstate ds;
> void *base;
> + /* device hotplug execution */
> + pid_t pid;
> + char *what;
> + libxl__ev_time ev;
> + libxl__ev_child child;
> };
>
> /* Internal AO operation to connect a disk device */
> @@ -1842,6 +1848,20 @@ _hidden void libxl__initiate_device_add(libxl__egc*, libxl__ao_device *aodev);
> _hidden void libxl__initiate_device_remove(libxl__egc *egc,
> libxl__ao_device *aodev);
>
> +/*
> + * libxl__get_hotplug_script_info returns the args and env that should
> + * be passed to the hotplug script for the requested device.
> + *
> + * Since a device might not need to execute any hotplug script, this function
> + * can return the following values:
> + *< 0: Error
> + * 0: No need to execute hotplug script
> + * 1: Execute hotplug script
> + */
> +_hidden int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
> + char ***args, char ***env,
> + libxl__device_action action);
> +
> /*----- Domain destruction -----*/
>
> /* Domain destruction has been split into two functions:
> diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
> index 925248b..98cd25f 100644
> --- a/tools/libxl/libxl_linux.c
> +++ b/tools/libxl/libxl_linux.c
> @@ -25,3 +25,101 @@ int libxl__try_phy_backend(mode_t st_mode)
>
> return 1;
> }
> +
> +/* Hotplug scripts helpers */
> +
> +static char **get_hotplug_env(libxl__gc *gc, libxl__device *dev)
> +{
> + char *be_path = libxl__device_backend_path(gc, dev);
> + char *script;
> + const char *type = libxl__device_kind_to_string(dev->backend_kind);
> + char **env;
> + int nr = 0, arraysize = 9;
> +
> + script = libxl__xs_read(gc, XBT_NULL,
> + GCSPRINTF("%s/%s", be_path, "script"));
> + if (!script) {
> + LOGEV(ERROR, errno, "unable to read script from %s", be_path);
> + return NULL;
> + }
> +
> + GCNEW_ARRAY(env, arraysize);
> + env[nr++] = "script";
> + env[nr++] = script;
> + env[nr++] = "XENBUS_TYPE";
> + env[nr++] = libxl__strdup(gc, type);
> + env[nr++] = "XENBUS_PATH";
> + env[nr++] = GCSPRINTF("backend/%s/%u/%d", type, dev->domid, dev->devid);
> + env[nr++] = "XENBUS_BASE_PATH";
> + env[nr++] = "backend";
> + env[nr++] = NULL;
> + assert(nr == arraysize);
> +
> + return env;
> +}
> +
> +/* Hotplug scripts caller functions */
> +
> +static int libxl__hotplug_disk(libxl__gc *gc, libxl__device *dev,
> + char ***args, char ***env,
> + libxl__device_action action)
> +{
> + char *be_path = libxl__device_backend_path(gc, dev);
> + char *script;
> + int nr = 0, rc = 0, arraysize = 3;
> +
> + script = libxl__xs_read(gc, XBT_NULL,
> + GCSPRINTF("%s/%s", be_path, "script"));
> + if (!script) {
> + LOGEV(ERROR, errno, "unable to read script from %s", be_path);
> + rc = ERROR_FAIL;
> + goto error;
> + }
> +
> + *env = get_hotplug_env(gc, dev);
> + if (!*env) {
> + rc = ERROR_FAIL;
> + goto error;
> + }
> +
> + GCNEW_ARRAY(*args, arraysize);
> + (*args)[nr++] = script;
> + (*args)[nr++] = action == DEVICE_CONNECT ? "add" : "remove";
> + (*args)[nr++] = NULL;
> + assert(nr == arraysize);
> +
> + rc = 0;
> +
> +error:
> + return rc;
> +}
> +
> +int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
> + char ***args, char ***env,
> + libxl__device_action action)
> +{
> + char *disable_udev = libxl__xs_read(gc, XBT_NULL, DISABLE_UDEV_PATH);
> + int rc;
> +
> + /* Check if we have to run hotplug scripts */
> + if (!disable_udev) {
> + rc = 0;
> + goto out;
> + }
> +
> + switch (dev->backend_kind) {
> + case LIBXL__DEVICE_KIND_VBD:
> + rc = libxl__hotplug_disk(gc, dev, args, env, action);
> + if (!rc) rc = 1;
> + break;
> + default:
> + /* If no need to execute any hotplug scripts,
> + * call the callback manually
> + */
> + rc = 0;
> + break;
> + }
> +
> +out:
> + return rc;
> +}
> diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
> index 9e0ed6d..0f2cdaa 100644
> --- a/tools/libxl/libxl_netbsd.c
> +++ b/tools/libxl/libxl_netbsd.c
> @@ -24,3 +24,12 @@ int libxl__try_phy_backend(mode_t st_mode)
>
> return 0;
> }
> +
> +/* Hotplug scripts caller functions */
> +
> +int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
> + char ***args, char ***env,
> + libxl__device_action action)
> +{
> + return 0;
> +}
> \ No newline at end of file
> --
> 1.7.7.5 (Apple Git-26)
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v4 08/10] libxl: call hotplug scripts for disk devices from libxl
2012-05-24 10:24 ` [PATCH v4 08/10] libxl: call hotplug scripts for disk devices from libxl Roger Pau Monne
2012-05-25 15:13 ` Roger Pau Monne
@ 2012-05-29 14:50 ` Ian Jackson
2012-05-30 11:33 ` Roger Pau Monne
1 sibling, 1 reply; 26+ messages in thread
From: Ian Jackson @ 2012-05-29 14:50 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel@lists.xen.org
Roger Pau Monne writes ("[PATCH v4 08/10] libxl: call hotplug scripts for disk devices from libxl"):
> Since most of the needed work is already done in previous patches,
> this patch only contains the necessary code to call hotplug scripts
> for disk devices, that should be called when the device is added or
> removed from a guest.
Thanks.
> +static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev)
> +{
...
> + /* Check if we have to execute hotplug scripts for this device
> + * and return the necessary args/env vars for execution */
> + hotplug = libxl__get_hotplug_script_info(gc, aodev->dev, &args, &env,
> + aodev->action);
> + switch (hotplug) {
...
> + default:
> + /* everything else is an error */
> + LOG(ERROR, "unable to get args/env to execute hotplug script for "
> + "device %s", libxl__device_backend_path(gc, aodev->dev));
> + rc = ERROR_FAIL;
Again, `rc = hotplug' perhaps ?
> + /* fork and execute hotplug script */
> + aodev->pid = libxl__ev_child_fork(gc, &aodev->child,
> + device_hotplug_fork_cb);
> + if (aodev->pid == -1) {
> + LOG(ERROR, "unable to fork");
> + rc = ERROR_FAIL;
> + goto out;
> + }
> +
> + if (!aodev->pid) {
> + /* child */
> + libxl__exec(gc, -1, -1, -1, args[0], args, env);
> + /* notreached */
> + abort();
> + }
> +
> + if (!libxl__ev_child_inuse(&aodev->child)) {
> + /* hotplug launch failed */
> + LOG(ERROR, "unable to launch hotplug script for device %s", be_path);
> + rc = ERROR_FAIL;
> + goto out;
> + }
This isn't right. After a successful libxl__ev_child_fork, the child
structure is definitely in use. So an assert would be appropriate but
really this last stanza can just be removed.
> + return;
> +
> +out:
> + libxl__ev_time_deregister(gc, &aodev->ev);
> + aodev->rc = rc;
> + aodev->callback(egc, aodev);
> + return;
> +}
Shouldn't something set aodev->state to FINISHED in this error path ?
It might be better to have a helper function to call when declaring
the aodev finished. That helper function would also do the
_ev_time_deregister, and assert !libxl__ev_child_inuse.
> +static void device_hotplug_fork_cb(libxl__egc *egc, libxl__ev_child *child,
> + pid_t pid, int status)
This shouldn't be called `fork_cb'. It's a callback that happens when
the child dies, not when it forks. I wouldn't call it `death_cb' or
`child_cb' or something.
> +static void device_hotplug_timeout_cb(libxl__egc *egc, libxl__ev_time *ev,
> + const struct timeval *requested_abs)
> +{
> + libxl__ao_device *aodev = CONTAINER_OF(ev, *aodev, ev);
> + STATE_AO_GC(aodev->ao);
> +
> + if (libxl__ev_child_inuse(&aodev->child)) {
> + if (kill(aodev->pid, SIGKILL)) {
> + LOGEV(ERROR, errno, "unable to kill hotplug script %s [%ld]",
> + aodev->what, (unsigned long)aodev->pid);
> + goto out;
> + }
> + }
> +
> +out:
> + libxl__ev_time_deregister(gc, &aodev->ev);
> + return;
You could profitably move that deregistration to the top of the
function. That would save the goto.
Also libxl__ev_child_inuse should always be true here, surely ? After
all we shouldn't have a timeout running unless we also have the
child. So yo should assert libxl__ev_child_inuse.
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 45b776c..da5b02b 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
...
> + /* device hotplug execution */
> + pid_t pid;
> + char *what;
> + libxl__ev_time ev;
> + libxl__ev_child child;
Is the pid inside child not sufficient ?
> diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
> index 925248b..98cd25f 100644
> --- a/tools/libxl/libxl_linux.c
> +++ b/tools/libxl/libxl_linux.c
> @@ -25,3 +25,101 @@ int libxl__try_phy_backend(mode_t st_mode)
>
> return 1;
> }
> +
> +/* Hotplug scripts helpers */
> +
> +static char **get_hotplug_env(libxl__gc *gc, libxl__device *dev)
> +{
...
> + int nr = 0, arraysize = 9;
A nit, but here we see memetic leakage from the tyranny of
-Wdeclaration-after-statement. Surely the definition of arraysize
would be better placed:
> + script = libxl__xs_read(gc, XBT_NULL,
> + GCSPRINTF("%s/%s", be_path, "script"));
> + if (!script) {
> + LOGEV(ERROR, errno, "unable to read script from %s", be_path);
> + return NULL;
> + }
> +
Here:
const int arraysize = 9;
> + GCNEW_ARRAY(env, arraysize);
> + env[nr++] = "script";
> + env[nr++] = script;
> + env[nr++] = "XENBUS_TYPE";
> + env[nr++] = libxl__strdup(gc, type);
> + env[nr++] = "XENBUS_PATH";
> + env[nr++] = GCSPRINTF("backend/%s/%u/%d", type, dev->domid, dev->devid);
> + env[nr++] = "XENBUS_BASE_PATH";
> + env[nr++] = "backend";
> + env[nr++] = NULL;
> + assert(nr == arraysize);
> diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
> index 9e0ed6d..0f2cdaa 100644
> --- a/tools/libxl/libxl_netbsd.c
> +++ b/tools/libxl/libxl_netbsd.c
> @@ -24,3 +24,12 @@ int libxl__try_phy_backend(mode_t st_mode)
>
> return 0;
> }
> +
> +/* Hotplug scripts caller functions */
> +
> +int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
> + char ***args, char ***env,
> + libxl__device_action action)
> +{
> + return 0;
> +}
> \ No newline at end of file
That last complaint from diff should be fixed :-).
Ian.
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v4 08/10] libxl: call hotplug scripts for disk devices from libxl
2012-05-29 14:50 ` Ian Jackson
@ 2012-05-30 11:33 ` Roger Pau Monne
0 siblings, 0 replies; 26+ messages in thread
From: Roger Pau Monne @ 2012-05-30 11:33 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel@lists.xen.org
Ian Jackson wrote:
>> + return;
>> +
>> +out:
>> + libxl__ev_time_deregister(gc,&aodev->ev);
>> + aodev->rc = rc;
>> + aodev->callback(egc, aodev);
>> + return;
>> +}
>
> Shouldn't something set aodev->state to FINISHED in this error path ?
>
> It might be better to have a helper function to call when declaring
> the aodev finished. That helper function would also do the
> _ev_time_deregister, and assert !libxl__ev_child_inuse.
Setting the aodev to finished is always done in
libxl__ao_device_check_last, which should be called by the user.
>
>> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
>> index 45b776c..da5b02b 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
> ...
>> + /* device hotplug execution */
>> + pid_t pid;
>> + char *what;
>> + libxl__ev_time ev;
>> + libxl__ev_child child;
>
> Is the pid inside child not sufficient ?
Probably, I just wasn't sure if the user of libxl__ev_child was allowed
to pick values inside the struct, now I saw the comment :)
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v4 09/10] libxl: call hotplug scripts for nic devices from libxl
2012-05-24 10:23 [PATCH v4 0/10] execute hotplug scripts from libxl Roger Pau Monne
` (7 preceding siblings ...)
2012-05-24 10:24 ` [PATCH v4 08/10] libxl: call hotplug scripts for disk devices from libxl Roger Pau Monne
@ 2012-05-24 10:24 ` Roger Pau Monne
2012-05-29 14:57 ` Ian Jackson
2012-05-24 10:24 ` [PATCH v4 10/10] libxl: use libxl__xs_path_cleanup on device_destroy Roger Pau Monne
9 siblings, 1 reply; 26+ messages in thread
From: Roger Pau Monne @ 2012-05-24 10:24 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Roger Pau Monne
Since most of the needed work is already done in previous patches,
this patch only contains the necessary code to call hotplug scripts
for nic devices, that should be called when the device is added or
removed from a guest.
Changes since v2:
* Change libxl__nic_type to return the value in a parameter passed by
the caller.
* Rename vif_execute to num_exec, to represent the number of times
hotplug scripts have been called for that device.
Changes since v1:
* Move event code to libxl_device.c (as in previous patch).
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
---
tools/hotplug/Linux/xen-backend.rules | 6 +-
tools/libxl/libxl_device.c | 46 ++++++++++++++++-
tools/libxl/libxl_internal.h | 6 ++-
tools/libxl/libxl_linux.c | 92 +++++++++++++++++++++++++++++++-
4 files changed, 142 insertions(+), 8 deletions(-)
diff --git a/tools/hotplug/Linux/xen-backend.rules b/tools/hotplug/Linux/xen-backend.rules
index d55ff11..c591a3f 100644
--- a/tools/hotplug/Linux/xen-backend.rules
+++ b/tools/hotplug/Linux/xen-backend.rules
@@ -2,8 +2,8 @@ SUBSYSTEM=="xen-backend", KERNEL=="tap*", ENV{UDEV_CALL}="1", RUN+="/etc/xen/scr
SUBSYSTEM=="xen-backend", KERNEL=="vbd*", ENV{UDEV_CALL}="1", RUN+="/etc/xen/scripts/block $env{ACTION}"
SUBSYSTEM=="xen-backend", KERNEL=="vtpm*", RUN+="/etc/xen/scripts/vtpm $env{ACTION}"
SUBSYSTEM=="xen-backend", KERNEL=="vif2-*", RUN+="/etc/xen/scripts/vif2 $env{ACTION}"
-SUBSYSTEM=="xen-backend", KERNEL=="vif-*", ACTION=="online", RUN+="/etc/xen/scripts/vif-setup online type_if=vif"
-SUBSYSTEM=="xen-backend", KERNEL=="vif-*", ACTION=="offline", RUN+="/etc/xen/scripts/vif-setup offline type_if=vif"
+SUBSYSTEM=="xen-backend", KERNEL=="vif-*", ENV{UDEV_CALL}="1", ACTION=="online", RUN+="/etc/xen/scripts/vif-setup online type_if=vif"
+SUBSYSTEM=="xen-backend", KERNEL=="vif-*", ENV{UDEV_CALL}="1", ACTION=="offline", RUN+="/etc/xen/scripts/vif-setup offline type_if=vif"
SUBSYSTEM=="xen-backend", KERNEL=="vscsi*", RUN+="/etc/xen/scripts/vscsi $env{ACTION}"
SUBSYSTEM=="xen-backend", ACTION=="remove", ENV{UDEV_CALL}="1", RUN+="/etc/xen/scripts/xen-hotplug-cleanup"
KERNEL=="evtchn", NAME="xen/%k"
@@ -13,4 +13,4 @@ KERNEL=="blktap-control", NAME="xen/blktap-2/control", MODE="0600"
KERNEL=="gntdev", NAME="xen/%k", MODE="0600"
KERNEL=="pci_iomul", NAME="xen/%k", MODE="0600"
KERNEL=="tapdev[a-z]*", NAME="xen/blktap-2/tapdev%m", MODE="0600"
-SUBSYSTEM=="net", KERNEL=="vif*-emu", ACTION=="add", RUN+="/etc/xen/scripts/vif-setup $env{ACTION} type_if=tap"
+SUBSYSTEM=="net", KERNEL=="vif*-emu", ACTION=="add", ENV{UDEV_CALL}="1", RUN+="/etc/xen/scripts/vif-setup $env{ACTION} type_if=tap"
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 8e1ec0f..5c840f8 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -100,6 +100,29 @@ out:
return numdevs;
}
+int libxl__nic_type(libxl__gc *gc, libxl__device *dev, libxl_nic_type *nictype)
+{
+ char *snictype, *be_path;
+ int rc = 0;
+
+ be_path = libxl__device_backend_path(gc, dev);
+ snictype = libxl__xs_read(gc, XBT_NULL,
+ GCSPRINTF("%s/%s", be_path, "type"));
+ if (!snictype) {
+ LOGE(ERROR, "unable to read nictype from %s", be_path);
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ rc = libxl_nic_type_from_string(snictype, nictype);
+ if (rc) {
+ LOGE(ERROR, "unable to parse nictype from %s", be_path);
+ goto out;
+ }
+
+out:
+ return rc;
+}
+
int libxl__device_generic_add(libxl__gc *gc, libxl__device *device,
char **bents, char **fents)
{
@@ -723,7 +746,8 @@ static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev)
/* Check if we have to execute hotplug scripts for this device
* and return the necessary args/env vars for execution */
hotplug = libxl__get_hotplug_script_info(gc, aodev->dev, &args, &env,
- aodev->action);
+ aodev->action,
+ aodev->num_exec);
switch (hotplug) {
case 0:
/* no hotplug script to execute */
@@ -789,6 +813,8 @@ static void device_hotplug_fork_cb(libxl__egc *egc, libxl__ev_child *child,
{
libxl__ao_device *aodev = CONTAINER_OF(child, *aodev, child);
STATE_AO_GC(aodev->ao);
+ libxl_nic_type nictype;
+ int rc;
libxl__ev_time_deregister(gc, &aodev->ev);
@@ -797,7 +823,25 @@ static void device_hotplug_fork_cb(libxl__egc *egc, libxl__ev_child *child,
: LIBXL__LOG_WARNING,
aodev->what, pid, status);
aodev->rc = ERROR_FAIL;
+ goto out;
}
+
+ if (aodev->dev->backend_kind == LIBXL__DEVICE_KIND_VIF &&
+ aodev->num_exec == 0) {
+ rc = libxl__nic_type(gc, aodev->dev, &nictype);
+ if (rc) {
+ LOG(ERROR, "unable to get type of nic device");
+ aodev->rc = rc;
+ goto out;
+ }
+ if (nictype == LIBXL_NIC_TYPE_IOEMU) {
+ aodev->num_exec++;
+ device_hotplug(egc, aodev);
+ return;
+ }
+ }
+
+out:
aodev->callback(egc, aodev);
}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index da5b02b..766f1f3 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -803,6 +803,8 @@ _hidden int libxl__parse_backend_path(libxl__gc *gc, const char *path,
libxl__device *dev);
_hidden int libxl__device_destroy(libxl__gc *gc, libxl__device *dev);
_hidden int libxl__wait_for_backend(libxl__gc *gc, char *be_path, char *state);
+_hidden int libxl__nic_type(libxl__gc *gc, libxl__device *dev,
+ libxl_nic_type *nictype);
/*
* For each aggregate type which can be used as an input we provide:
@@ -1818,6 +1820,7 @@ struct libxl__ao_device {
/* device hotplug execution */
pid_t pid;
char *what;
+ int num_exec;
libxl__ev_time ev;
libxl__ev_child child;
};
@@ -1860,7 +1863,8 @@ _hidden void libxl__initiate_device_remove(libxl__egc *egc,
*/
_hidden int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
char ***args, char ***env,
- libxl__device_action action);
+ libxl__device_action action,
+ int num_exec);
/*----- Domain destruction -----*/
diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index 98cd25f..e1e2abe 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -34,7 +34,8 @@ static char **get_hotplug_env(libxl__gc *gc, libxl__device *dev)
char *script;
const char *type = libxl__device_kind_to_string(dev->backend_kind);
char **env;
- int nr = 0, arraysize = 9;
+ int nr = 0, arraysize = 13;
+ libxl_nic_type nictype;
script = libxl__xs_read(gc, XBT_NULL,
GCSPRINTF("%s/%s", be_path, "script"));
@@ -52,14 +53,94 @@ static char **get_hotplug_env(libxl__gc *gc, libxl__device *dev)
env[nr++] = GCSPRINTF("backend/%s/%u/%d", type, dev->domid, dev->devid);
env[nr++] = "XENBUS_BASE_PATH";
env[nr++] = "backend";
+ if (dev->backend_kind == LIBXL__DEVICE_KIND_VIF) {
+ if (libxl__nic_type(gc, dev, &nictype)) {
+ LOG(ERROR, "unable to get nictype");
+ return NULL;
+ }
+ switch (nictype) {
+ case LIBXL_NIC_TYPE_IOEMU:
+ env[nr++] = "INTERFACE";
+ env[nr++] = libxl__strdup(gc, libxl__device_nic_devname(gc,
+ dev->domid, dev->devid,
+ LIBXL_NIC_TYPE_IOEMU));
+ case LIBXL_NIC_TYPE_VIF:
+ env[nr++] = "vif";
+ env[nr++] = libxl__strdup(gc, libxl__device_nic_devname(gc,
+ dev->domid, dev->devid,
+ LIBXL_NIC_TYPE_VIF));
+ break;
+ default:
+ return NULL;
+ }
+ }
+
env[nr++] = NULL;
- assert(nr == arraysize);
+ assert(nr <= arraysize);
return env;
}
/* Hotplug scripts caller functions */
+static int libxl__hotplug_nic(libxl__gc *gc, libxl__device *dev,
+ char ***args, char ***env,
+ libxl__device_action action, int num_exec)
+{
+ char *be_path = libxl__device_backend_path(gc, dev);
+ char *script;
+ int nr = 0, rc = 0, arraysize = 4;
+ libxl_nic_type nictype;
+
+ script = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/%s", be_path,
+ "script"));
+ if (!script) {
+ LOGE(ERROR, "unable to read script from %s", be_path);
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ rc = libxl__nic_type(gc, dev, &nictype);
+ if (rc) {
+ LOG(ERROR, "error when fetching nic type");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ *env = get_hotplug_env(gc, dev);
+ if (!env) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ GCNEW_ARRAY(*args, arraysize);
+ (*args)[nr++] = script;
+
+ switch (nictype) {
+ case LIBXL_NIC_TYPE_IOEMU:
+ if (num_exec == 0) goto execute_vif;
+ (*args)[nr++] = action == DEVICE_CONNECT ? "add" : "remove";
+ (*args)[nr++] = libxl__strdup(gc, "type_if=tap");
+ (*args)[nr++] = NULL;
+ break;
+ case LIBXL_NIC_TYPE_VIF:
+execute_vif:
+ (*args)[nr++] = action == DEVICE_CONNECT ? "online" : "offline";
+ (*args)[nr++] = libxl__strdup(gc, "type_if=vif");
+ (*args)[nr++] = NULL;
+ break;
+ default:
+ /* Unknown network type */
+ LOG(ERROR, "unknown network card type %s", be_path);
+ return 0;
+ }
+ assert(nr == arraysize);
+ rc = 0;
+
+out:
+ return rc;
+}
+
static int libxl__hotplug_disk(libxl__gc *gc, libxl__device *dev,
char ***args, char ***env,
libxl__device_action action)
@@ -96,7 +177,8 @@ error:
int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
char ***args, char ***env,
- libxl__device_action action)
+ libxl__device_action action,
+ int num_exec)
{
char *disable_udev = libxl__xs_read(gc, XBT_NULL, DISABLE_UDEV_PATH);
int rc;
@@ -112,6 +194,10 @@ int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
rc = libxl__hotplug_disk(gc, dev, args, env, action);
if (!rc) rc = 1;
break;
+ case LIBXL__DEVICE_KIND_VIF:
+ rc = libxl__hotplug_nic(gc, dev, args, env, action, num_exec);
+ if (!rc) rc = 1;
+ break;
default:
/* If no need to execute any hotplug scripts,
* call the callback manually
--
1.7.7.5 (Apple Git-26)
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v4 09/10] libxl: call hotplug scripts for nic devices from libxl
2012-05-24 10:24 ` [PATCH v4 09/10] libxl: call hotplug scripts for nic " Roger Pau Monne
@ 2012-05-29 14:57 ` Ian Jackson
0 siblings, 0 replies; 26+ messages in thread
From: Ian Jackson @ 2012-05-29 14:57 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel@lists.xen.org
Roger Pau Monne writes ("[PATCH v4 09/10] libxl: call hotplug scripts for nic devices from libxl"):
> Since most of the needed work is already done in previous patches,
> this patch only contains the necessary code to call hotplug scripts
> for nic devices, that should be called when the device is added or
> removed from a guest.
>
> Changes since v2:
>
> * Change libxl__nic_type to return the value in a parameter passed by
> the caller.
>
> * Rename vif_execute to num_exec, to represent the number of times
> hotplug scripts have been called for that device.
This observation needs to be reworked into a suitable form and placed
as a comment here in the code:
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index da5b02b..766f1f3 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
...
> @@ -1860,7 +1863,8 @@ _hidden void libxl__initiate_device_remove(libxl__egc *egc,
> */
> _hidden int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
> char ***args, char ***env,
> - libxl__device_action action);
> + libxl__device_action action,
> + int num_exec);
And the semantics need to be clearly explained. In particular, this
is an interface that is supposed to be implemented by multiple
providers for different platforms.
I _think_ what is happening is that `num_exec' is always 0 for
everything but vifs. For vifs libxl__get_hotplug_script_info is
always called with num_exec==0 for the actual PV vif (Xen PV
interface) but may also be called with num_exec==1 for the ioemu
device, but I may be wrong.
> diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
> index 98cd25f..e1e2abe 100644
> --- a/tools/libxl/libxl_linux.c
> +++ b/tools/libxl/libxl_linux.c
> @@ -34,7 +34,8 @@ static char **get_hotplug_env(libxl__gc *gc, libxl__device *dev)
...
> +static int libxl__hotplug_nic(libxl__gc *gc, libxl__device *dev,
> + char ***args, char ***env,
> + libxl__device_action action, int num_exec)
...
> + switch (nictype) {
> + case LIBXL_NIC_TYPE_IOEMU:
> + if (num_exec == 0) goto execute_vif;
Urgh! This is a pretty nasty use of goto.
Thanks,
Ian.
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v4 10/10] libxl: use libxl__xs_path_cleanup on device_destroy
2012-05-24 10:23 [PATCH v4 0/10] execute hotplug scripts from libxl Roger Pau Monne
` (8 preceding siblings ...)
2012-05-24 10:24 ` [PATCH v4 09/10] libxl: call hotplug scripts for nic " Roger Pau Monne
@ 2012-05-24 10:24 ` Roger Pau Monne
2012-05-29 14:58 ` Ian Jackson
9 siblings, 1 reply; 26+ messages in thread
From: Roger Pau Monne @ 2012-05-24 10:24 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Roger Pau Monne
Since the hotplug script that was in charge of cleaning the backend is
no longer launched, we need to clean the backend by ourselves, so use
libxl__xs_path_cleanup instead of xs_rm.
Changes sinve v2:
* Changed the goto construction to a do-while loop.
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
---
tools/libxl/libxl_device.c | 18 ++++++++++++++----
1 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 5c840f8..0f0465a 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -487,16 +487,26 @@ void libxl__add_nics(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
{
- libxl_ctx *ctx = libxl__gc_owner(gc);
char *be_path = libxl__device_backend_path(gc, dev);
char *fe_path = libxl__device_frontend_path(gc, dev);
+ xs_transaction_t t = 0;
+ int rc = 0;
- xs_rm(ctx->xsh, XBT_NULL, be_path);
- xs_rm(ctx->xsh, XBT_NULL, fe_path);
+ do {
+ t = xs_transaction_start(CTX->xsh);
+ libxl__xs_path_cleanup(gc, t, fe_path);
+ libxl__xs_path_cleanup(gc, t, be_path);
+ rc = !xs_transaction_end(CTX->xsh, t, 0);
+ } while (rc && errno == EAGAIN);
+ if (rc) {
+ LOGE(ERROR, "unable to finish transaction");
+ goto out;
+ }
libxl__device_destroy_tapdisk(gc, be_path);
- return 0;
+out:
+ return rc;
}
/* Callback for device destruction */
--
1.7.7.5 (Apple Git-26)
^ permalink raw reply related [flat|nested] 26+ messages in thread