* [PATCH v2 01/15] libxl: pass env vars to libxl__exec
2012-05-22 14:02 [PATCH v2 0/15] execute hotplug scripts from libxl Roger Pau Monne
@ 2012-05-22 14:02 ` Roger Pau Monne
2012-05-22 14:02 ` [PATCH v2 02/15] libxl: fix libxl__xs_directory usage of transaction Roger Pau Monne
` (14 subsequent siblings)
15 siblings, 0 replies; 43+ messages in thread
From: Roger Pau Monne @ 2012-05-22 14:02 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Roger Pau Monne
Add another parameter to libxl__exec call that contains the
environment variables to use when performing the execvp call.
Changes since v4:
* Unify error checking.
Changes since v3:
* Use CTX instead of defining libxl_ctx *ctx.
* Error checking on setenv.
* Better comment on header for libxl__exec function.
* Added some const-correctness.
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.c | 2 +-
tools/libxl/libxl_bootloader.c | 4 ++--
tools/libxl/libxl_dm.c | 2 +-
tools/libxl/libxl_exec.c | 14 ++++++++++++--
tools/libxl/libxl_internal.h | 20 ++++++++++++++++++--
5 files changed, 34 insertions(+), 8 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 4d01cf8..2a09192 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1261,7 +1261,7 @@ int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass)
args[2] = "-autopass";
}
- libxl__exec(autopass_fd, -1, -1, args[0], args);
+ libxl__exec(gc, autopass_fd, -1, -1, args[0], args, NULL);
abort();
x_fail:
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index fb1302b..0021a7b 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -359,6 +359,7 @@ static void bootloader_gotptys(libxl__egc *egc, libxl__openpty_state *op)
libxl__bootloader_state *bl = CONTAINER_OF(op, *bl, openpty);
STATE_AO_GC(bl->ao);
int rc, r;
+ char *const env[] = { "TERM", "vt100", NULL };
if (bl->openpty.rc) {
rc = bl->openpty.rc;
@@ -452,8 +453,7 @@ static void bootloader_gotptys(libxl__egc *egc, libxl__openpty_state *op)
/* child */
r = login_tty(libxl__carefd_fd(bl->ptys[0].slave));
if (r) { LOGE(ERROR, "login_tty failed"); exit(-1); }
- setenv("TERM", "vt100", 1);
- libxl__exec(-1, -1, -1, bl->args[0], (char**)bl->args);
+ libxl__exec(gc, -1, -1, -1, bl->args[0], (char **) bl->args, env);
exit(-1);
}
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 4ad7d02..95fd0ac 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1015,7 +1015,7 @@ retry_transaction:
goto out_close;
if (!rc) { /* inner child */
setsid();
- libxl__exec(null, logfile_w, logfile_w, dm, args);
+ libxl__exec(gc, null, logfile_w, logfile_w, dm, args, NULL);
}
rc = 0;
diff --git a/tools/libxl/libxl_exec.c b/tools/libxl/libxl_exec.c
index b46b893..082bf44 100644
--- a/tools/libxl/libxl_exec.c
+++ b/tools/libxl/libxl_exec.c
@@ -66,8 +66,8 @@ static void check_open_fds(const char *what)
if (badness) abort();
}
-void libxl__exec(int stdinfd, int stdoutfd, int stderrfd, const char *arg0,
- char **args)
+void libxl__exec(libxl__gc *gc, int stdinfd, int stdoutfd, int stderrfd,
+ const char *arg0, char *const args[], char *const env[])
/* call this in the child */
{
if (stdinfd != -1)
@@ -90,8 +90,18 @@ void libxl__exec(int stdinfd, int stdoutfd, int stderrfd, const char *arg0,
/* in case our caller set it to IGN. subprocesses are entitled
* to assume they got DFL. */
+ if (env != NULL) {
+ for (int i = 0; env[i] != NULL && env[i+1] != NULL; i += 2) {
+ if (setenv(env[i], env[i+1], 1) < 0) {
+ LOGEV(ERROR, errno, "setting env vars (%s = %s)",
+ env[i], env[i+1]);
+ goto out;
+ }
+ }
+ }
execvp(arg0, args);
+out:
fprintf(stderr, "libxl: cannot execute %s: %s\n", arg0, strerror(errno));
_exit(-1);
}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index f71e76a..aaaf644 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1138,8 +1138,24 @@ _hidden int libxl__wait_for_offspring(libxl__gc *gc,
/* low-level stuff, for synchronous subprocesses etc. */
-_hidden void libxl__exec(int stdinfd, int stdoutfd, int stderrfd,
- const char *arg0, char **args); // logs errors, never returns
+/*
+ * env should be passed using the following format,
+ *
+ * env[0]: name of env variable
+ * env[1]: value of env variable
+ * env[n]: ...
+ *
+ * So it efectively becomes something like:
+ * export env[n]=env[n+1]
+ * (where n%2 = 0)
+ *
+ * The last entry of the array always has to be NULL.
+ *
+ * Logs errors, never returns.
+ */
+_hidden void libxl__exec(libxl__gc *gc, int stdinfd, int stdoutfd,
+ int stderrfd, const char *arg0, char *const args[],
+ char *const env[]);
/* from xl_create */
--
1.7.7.5 (Apple Git-26)
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCH v2 02/15] libxl: fix libxl__xs_directory usage of transaction
2012-05-22 14:02 [PATCH v2 0/15] execute hotplug scripts from libxl Roger Pau Monne
2012-05-22 14:02 ` [PATCH v2 01/15] libxl: pass env vars to libxl__exec Roger Pau Monne
@ 2012-05-22 14:02 ` Roger Pau Monne
2012-05-22 14:02 ` [PATCH v2 03/15] libxl: add libxl__xs_path_cleanup Roger Pau Monne
` (13 subsequent siblings)
15 siblings, 0 replies; 43+ messages in thread
From: Roger Pau Monne @ 2012-05-22 14:02 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Roger Pau Monne
libxl__xs_directory takes a transaction parameter, but completely
ignores it, passing XBT_NULL unconditionally to xs_directory.
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_xshelp.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/tools/libxl/libxl_xshelp.c b/tools/libxl/libxl_xshelp.c
index 3ea8d08..6ca1afe 100644
--- a/tools/libxl/libxl_xshelp.c
+++ b/tools/libxl/libxl_xshelp.c
@@ -111,7 +111,7 @@ char **libxl__xs_directory(libxl__gc *gc, xs_transaction_t t,
{
libxl_ctx *ctx = libxl__gc_owner(gc);
char **ret = NULL;
- ret = xs_directory(ctx->xsh, XBT_NULL, path, nb);
+ ret = xs_directory(ctx->xsh, t, path, nb);
libxl__ptr_add(gc, ret);
return ret;
}
--
1.7.7.5 (Apple Git-26)
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCH v2 03/15] libxl: add libxl__xs_path_cleanup
2012-05-22 14:02 [PATCH v2 0/15] execute hotplug scripts from libxl Roger Pau Monne
2012-05-22 14:02 ` [PATCH v2 01/15] libxl: pass env vars to libxl__exec Roger Pau Monne
2012-05-22 14:02 ` [PATCH v2 02/15] libxl: fix libxl__xs_directory usage of transaction Roger Pau Monne
@ 2012-05-22 14:02 ` Roger Pau Monne
2012-05-22 14:19 ` Ian Jackson
2012-05-22 14:02 ` [PATCH v2 04/15] libxl: reoder libxl_device unplug functions Roger Pau Monne
` (12 subsequent siblings)
15 siblings, 1 reply; 43+ messages in thread
From: Roger Pau Monne @ 2012-05-22 14:02 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Roger Pau Monne
Add a function which behaves like "xenstore-rm -t", and which will be
used to clean xenstore after unplug since we will be no longer
executing xen-hotplug-cleanup script, that used to do that for us.
Changes since v5:
* Abort if no transaction or user_path supplied.
* Log all errors.
Changes since v4:
* Caller must supply a transaction.
Changes since v3:
* Better error checking and function description.
Changes since v2:
* Moved the function to libxl_xshelp.c and added the prototype to
libxl_internal.h.
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
---
tools/libxl/libxl_internal.h | 8 ++++++++
tools/libxl/libxl_xshelp.c | 38 ++++++++++++++++++++++++++++++++++++++
2 files changed, 46 insertions(+), 0 deletions(-)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index aaaf644..ae5527b 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -494,6 +494,14 @@ _hidden bool libxl__xs_mkdir(libxl__gc *gc, xs_transaction_t t,
_hidden char *libxl__xs_libxl_path(libxl__gc *gc, uint32_t domid);
+/*
+ * This is a recursive delete, from top to bottom. What this function does
+ * is remove empty folders that contained the deleted entry.
+ *
+ * It mimics xenstore-rm -t behaviour.
+ */
+_hidden int libxl__xs_path_cleanup(libxl__gc *gc, xs_transaction_t t,
+ char *user_path);
/*
* Event generation functions provided by the libxl event core to the
diff --git a/tools/libxl/libxl_xshelp.c b/tools/libxl/libxl_xshelp.c
index 6ca1afe..c5b5364 100644
--- a/tools/libxl/libxl_xshelp.c
+++ b/tools/libxl/libxl_xshelp.c
@@ -135,6 +135,44 @@ char *libxl__xs_libxl_path(libxl__gc *gc, uint32_t domid)
return s;
}
+int libxl__xs_path_cleanup(libxl__gc *gc, xs_transaction_t t, char *user_path)
+{
+ unsigned int nb = 0;
+ char *path, *last, *val;
+ int rc;
+
+ /* A path and transaction must be provided by the caller */
+ assert(user_path && t);
+
+ path = libxl__strdup(gc, user_path);
+ if (!xs_rm(CTX->xsh, t, path)) {
+ LOGE(DEBUG, "unable to remove path %s", path);
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ for (last = strrchr(path, '/'); last != NULL; last = strrchr(path, '/')) {
+ *last = '\0';
+
+ if (!strlen(path)) break;
+
+ val = libxl__xs_read(gc, t, path);
+ if (!val || strlen(val) != 0) break;
+
+ if (!libxl__xs_directory(gc, t, path, &nb) || nb != 0) break;
+
+ if (!xs_rm(CTX->xsh, t, path)) {
+ LOGE(DEBUG, "unable to remove path %s", path);
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ }
+ rc = 0;
+
+out:
+ return rc;
+}
+
/*
* Local variables:
* mode: C
--
1.7.7.5 (Apple Git-26)
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 03/15] libxl: add libxl__xs_path_cleanup
2012-05-22 14:02 ` [PATCH v2 03/15] libxl: add libxl__xs_path_cleanup Roger Pau Monne
@ 2012-05-22 14:19 ` Ian Jackson
0 siblings, 0 replies; 43+ messages in thread
From: Ian Jackson @ 2012-05-22 14:19 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel@lists.xen.org
Roger Pau Monne writes ("[PATCH v2 03/15] libxl: add libxl__xs_path_cleanup"):
> Add a function which behaves like "xenstore-rm -t", and which will be
> used to clean xenstore after unplug since we will be no longer
> executing xen-hotplug-cleanup script, that used to do that for us.
>
> Changes since v5:
>
> * Abort if no transaction or user_path supplied.
Thanks.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> diff --git a/tools/libxl/libxl_xshelp.c b/tools/libxl/libxl_xshelp.c
> index 6ca1afe..c5b5364 100644
> --- a/tools/libxl/libxl_xshelp.c
> +++ b/tools/libxl/libxl_xshelp.c
> @@ -135,6 +135,44 @@ char *libxl__xs_libxl_path(libxl__gc *gc, uint32_t domid)
> return s;
> }
>
> +int libxl__xs_path_cleanup(libxl__gc *gc, xs_transaction_t t, char *user_path)
...
> + /* A path and transaction must be provided by the caller */
> + assert(user_path && t);
Personally I wouldn't bother asserting user_path here.
> + path = libxl__strdup(gc, user_path);
After all this will tidily dereference null anyway. In general we
don't bother asserting that function arguments we are going to
dereference are non-0, since the crash is debuggable either way.
But I don't particularly object, especially as you do need to assert
that t is non-0 as this function is not safe outside a transaction.
Ian.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 04/15] libxl: reoder libxl_device unplug functions
2012-05-22 14:02 [PATCH v2 0/15] execute hotplug scripts from libxl Roger Pau Monne
` (2 preceding siblings ...)
2012-05-22 14:02 ` [PATCH v2 03/15] libxl: add libxl__xs_path_cleanup Roger Pau Monne
@ 2012-05-22 14:02 ` Roger Pau Monne
2012-05-22 14:21 ` Ian Jackson
2012-05-22 14:02 ` [PATCH v2 05/15] libxl: change libxl__ao_device_remove to libxl__ao_device Roger Pau Monne
` (11 subsequent siblings)
15 siblings, 1 reply; 43+ messages in thread
From: Roger Pau Monne @ 2012-05-22 14:02 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Roger Pau Monne
This is a reorder of functions, no functional change. This is needed
because in future patches much code is added to libxl_device and it
needs to follow the usual ao operation scheme (prototypes, functions
and callbacks in order they should be called)
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
---
tools/libxl/libxl_device.c | 148 +++++++++++++++++++++++---------------------
1 files changed, 78 insertions(+), 70 deletions(-)
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index c7e057d..2006406 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -362,76 +362,6 @@ typedef struct {
libxl__ev_devstate ds;
} libxl__ao_device_remove;
-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_remove_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);
-}
-
-int libxl__initiate_device_remove(libxl__egc *egc, libxl__ao *ao,
- libxl__device *dev)
-{
- AO_GC;
- libxl_ctx *ctx = libxl__gc_owner(gc);
- xs_transaction_t t;
- char *be_path = libxl__device_backend_path(gc, dev);
- char *state_path = libxl__sprintf(gc, "%s/state", be_path);
- char *state = libxl__xs_read(gc, XBT_NULL, state_path);
- int rc = 0;
- libxl__ao_device_remove *aorm = 0;
-
- if (!state)
- goto out_ok;
- if (atoi(state) != 4) {
- 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)) {
- if (errno == EAGAIN)
- goto retry_transaction;
- else {
- rc = ERROR_FAIL;
- goto out_fail;
- }
- }
-
- libxl__device_destroy_tapdisk(gc, be_path);
-
- aorm = libxl__zalloc(gc, sizeof(*aorm));
- aorm->ao = ao;
- libxl__ev_devstate_init(&aorm->ds);
-
- rc = libxl__ev_devstate_wait(gc, &aorm->ds, device_remove_callback,
- state_path, XenbusStateClosed,
- LIBXL_DESTROY_TIMEOUT * 1000);
- if (rc) goto out_fail;
-
- return 0;
-
- out_fail:
- assert(rc);
- device_remove_cleanup(gc, aorm);
- return rc;
-
- out_ok:
- libxl__ao_complete(egc, ao, 0);
- return 0;
-}
-
int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
{
libxl_ctx *ctx = libxl__gc_owner(gc);
@@ -504,6 +434,84 @@ out:
return 0;
}
+/* Callbacks for device related operations */
+
+static void device_remove_callback(libxl__egc *egc, libxl__ev_devstate *ds,
+ int rc);
+
+static void device_remove_cleanup(libxl__gc *gc,
+ libxl__ao_device_remove *aorm);
+
+int libxl__initiate_device_remove(libxl__egc *egc, libxl__ao *ao,
+ libxl__device *dev)
+{
+ AO_GC;
+ libxl_ctx *ctx = libxl__gc_owner(gc);
+ xs_transaction_t t;
+ char *be_path = libxl__device_backend_path(gc, dev);
+ char *state_path = libxl__sprintf(gc, "%s/state", be_path);
+ char *state = libxl__xs_read(gc, XBT_NULL, state_path);
+ int rc = 0;
+ libxl__ao_device_remove *aorm = 0;
+
+ if (!state)
+ goto out_ok;
+ if (atoi(state) != 4) {
+ 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)) {
+ if (errno == EAGAIN)
+ goto retry_transaction;
+ else {
+ rc = ERROR_FAIL;
+ goto out_fail;
+ }
+ }
+
+ libxl__device_destroy_tapdisk(gc, be_path);
+
+ aorm = libxl__zalloc(gc, sizeof(*aorm));
+ aorm->ao = ao;
+ libxl__ev_devstate_init(&aorm->ds);
+
+ rc = libxl__ev_devstate_wait(gc, &aorm->ds, device_remove_callback,
+ state_path, XenbusStateClosed,
+ LIBXL_DESTROY_TIMEOUT * 1000);
+ if (rc) goto out_fail;
+
+ return 0;
+
+ out_fail:
+ assert(rc);
+ device_remove_cleanup(gc, aorm);
+ return rc;
+
+ out_ok:
+ libxl__ao_complete(egc, ao, 0);
+ return 0;
+}
+
+static void device_remove_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);
+}
+
+static void device_remove_cleanup(libxl__gc *gc,
+ libxl__ao_device_remove *aorm) {
+ if (!aorm) return;
+ libxl__ev_devstate_cancel(gc, &aorm->ds);
+}
+
int libxl__wait_for_device_model(libxl__gc *gc,
uint32_t domid, char *state,
libxl__spawn_starting *spawning,
--
1.7.7.5 (Apple Git-26)
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 04/15] libxl: reoder libxl_device unplug functions
2012-05-22 14:02 ` [PATCH v2 04/15] libxl: reoder libxl_device unplug functions Roger Pau Monne
@ 2012-05-22 14:21 ` Ian Jackson
0 siblings, 0 replies; 43+ messages in thread
From: Ian Jackson @ 2012-05-22 14:21 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel@lists.xen.org
Roger Pau Monne writes ("[PATCH v2 04/15] libxl: reoder libxl_device unplug functions"):
> This is a reorder of functions, no functional change. This is needed
> because in future patches much code is added to libxl_device and it
> needs to follow the usual ao operation scheme (prototypes, functions
> and callbacks in order they should be called)
>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
Thanks.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
(I'm assuming that your commit message isn't a lie and that this is
indeed pure code motion...)
Ian.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 05/15] libxl: change libxl__ao_device_remove to libxl__ao_device
2012-05-22 14:02 [PATCH v2 0/15] execute hotplug scripts from libxl Roger Pau Monne
` (3 preceding siblings ...)
2012-05-22 14:02 ` [PATCH v2 04/15] libxl: reoder libxl_device unplug functions Roger Pau Monne
@ 2012-05-22 14:02 ` Roger Pau Monne
2012-05-22 15:10 ` Ian Jackson
2012-05-22 14:02 ` [PATCH v2 06/15] libxl: move device model creation prototypes Roger Pau Monne
` (10 subsequent siblings)
15 siblings, 1 reply; 43+ messages in thread
From: Roger Pau Monne @ 2012-05-22 14:02 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.
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
---
tools/libxl/libxl.c | 192 +++++++++++++++++++++++++++++------------
tools/libxl/libxl.h | 15 +++-
tools/libxl/libxl_device.c | 107 ++++++++++++++++--------
tools/libxl/libxl_internal.h | 45 ++++++++--
4 files changed, 257 insertions(+), 102 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2a09192..b13de4f 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1271,6 +1271,26 @@ int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass)
/******************************************************************************/
+/* generic callback for devices that only need to set ao_complete */
+static void libxl__device_cb(libxl__egc *egc, libxl__ao_device *aorm)
+{
+ STATE_AO_GC(aorm->ao);
+
+ if (aorm->rc) {
+ LOGE(ERROR, "unable to %s %s with id %u",
+ aorm->action == DEVICE_CONNECT ? "add" : "remove",
+ libxl__device_kind_to_string(aorm->dev->kind),
+ aorm->dev->devid);
+ goto out;
+ }
+
+out:
+ libxl__ao_complete(egc, ao, aorm->rc);
+ return;
+}
+
+/******************************************************************************/
+
int libxl__device_disk_setdefault(libxl__gc *gc, libxl_device_disk *disk)
{
int rc;
@@ -1445,35 +1465,50 @@ int libxl_device_disk_remove(libxl_ctx *ctx, uint32_t domid,
const libxl_asyncop_how *ao_how)
{
AO_CREATE(ctx, domid, ao_how);
- libxl__device device;
+ libxl__device *device;
+ libxl__ao_device *aorm;
int rc;
- rc = libxl__device_from_disk(gc, domid, disk, &device);
+ GCNEW(device);
+ rc = libxl__device_from_disk(gc, domid, disk, device);
if (rc != 0) goto out;
- rc = libxl__initiate_device_remove(egc, ao, &device);
- if (rc) goto out;
-
- return AO_INPROGRESS;
+ GCNEW(aorm);
+ libxl__init_ao_device(aorm, ao, NULL);
+ aorm->action = DEVICE_DISCONNECT;
+ aorm->dev = device;
+ aorm->callback = libxl__device_cb;
+ libxl__initiate_device_remove(egc, aorm);
out:
- return AO_ABORT(rc);
+ if (rc) return AO_ABORT(rc);
+ return AO_INPROGRESS;
}
int libxl_device_disk_destroy(libxl_ctx *ctx, uint32_t domid,
- libxl_device_disk *disk)
+ libxl_device_disk *disk,
+ const libxl_asyncop_how *ao_how)
{
- GC_INIT(ctx);
- libxl__device device;
+ AO_CREATE(ctx, domid, ao_how);
+ libxl__device *device;
+ libxl__ao_device *aorm;
int rc;
- rc = libxl__device_from_disk(gc, domid, disk, &device);
+ GCNEW(device);
+ rc = libxl__device_from_disk(gc, domid, disk, device);
if (rc != 0) goto out;
- rc = libxl__device_destroy(gc, &device);
+ GCNEW(aorm);
+ libxl__init_ao_device(aorm, ao, NULL);
+ aorm->action = DEVICE_DISCONNECT;
+ aorm->force = 1;
+ aorm->dev = device;
+ aorm->callback = libxl__device_cb;
+ libxl__initiate_device_remove(egc, aorm);
+
out:
- GC_FREE;
- return rc;
+ if (rc) return AO_ABORT(rc);
+ return AO_INPROGRESS;
}
static void libxl__device_disk_from_xs_be(libxl__gc *gc,
@@ -1923,35 +1958,50 @@ int libxl_device_nic_remove(libxl_ctx *ctx, uint32_t domid,
const libxl_asyncop_how *ao_how)
{
AO_CREATE(ctx, domid, ao_how);
- libxl__device device;
+ libxl__device *device;
+ libxl__ao_device *aorm;
int rc;
- rc = libxl__device_from_nic(gc, domid, nic, &device);
+ GCNEW(device);
+ rc = libxl__device_from_nic(gc, domid, nic, device);
if (rc != 0) goto out;
- rc = libxl__initiate_device_remove(egc, ao, &device);
- if (rc) goto out;
-
- return AO_INPROGRESS;
+ GCNEW(aorm);
+ libxl__init_ao_device(aorm, ao, NULL);
+ aorm->action = DEVICE_DISCONNECT;
+ aorm->dev = device;
+ aorm->callback = libxl__device_cb;
+ libxl__initiate_device_remove(egc, aorm);
out:
- return AO_ABORT(rc);
+ if (rc) return AO_ABORT(rc);
+ return AO_INPROGRESS;
}
int libxl_device_nic_destroy(libxl_ctx *ctx, uint32_t domid,
- libxl_device_nic *nic)
+ libxl_device_nic *nic,
+ const libxl_asyncop_how *ao_how)
{
- GC_INIT(ctx);
- libxl__device device;
+ AO_CREATE(ctx, domid, ao_how);
+ libxl__device *device;
+ libxl__ao_device *aorm;
int rc;
- rc = libxl__device_from_nic(gc, domid, nic, &device);
+ GCNEW(device);
+ rc = libxl__device_from_nic(gc, domid, nic, device);
if (rc != 0) goto out;
- rc = libxl__device_destroy(gc, &device);
+ GCNEW(aorm);
+ libxl__init_ao_device(aorm, ao, NULL);
+ aorm->action = DEVICE_DISCONNECT;
+ aorm->force = 1;
+ aorm->dev = device;
+ aorm->callback = libxl__device_cb;
+ libxl__initiate_device_remove(egc, aorm);
+
out:
- GC_FREE;
- return rc;
+ if (rc) return AO_ABORT(rc);
+ return AO_INPROGRESS;
}
static void libxl__device_nic_from_xs_be(libxl__gc *gc,
@@ -2285,35 +2335,50 @@ int libxl_device_vkb_remove(libxl_ctx *ctx, uint32_t domid,
const libxl_asyncop_how *ao_how)
{
AO_CREATE(ctx, domid, ao_how);
- libxl__device device;
+ libxl__device *device;
+ libxl__ao_device *aorm;
int rc;
- rc = libxl__device_from_vkb(gc, domid, vkb, &device);
+ GCNEW(device);
+ rc = libxl__device_from_vkb(gc, domid, vkb, device);
if (rc != 0) goto out;
- rc = libxl__initiate_device_remove(egc, ao, &device);
- if (rc) goto out;
-
- return AO_INPROGRESS;
+ GCNEW(aorm);
+ libxl__init_ao_device(aorm, ao, NULL);
+ aorm->action = DEVICE_DISCONNECT;
+ aorm->dev = device;
+ aorm->callback = libxl__device_cb;
+ libxl__initiate_device_remove(egc, aorm);
out:
- return AO_ABORT(rc);
+ if (rc) return AO_ABORT(rc);
+ return AO_INPROGRESS;
}
int libxl_device_vkb_destroy(libxl_ctx *ctx, uint32_t domid,
- libxl_device_vkb *vkb)
+ libxl_device_vkb *vkb,
+ const libxl_asyncop_how *ao_how)
{
- GC_INIT(ctx);
- libxl__device device;
+ AO_CREATE(ctx, domid, ao_how);
+ libxl__device *device;
+ libxl__ao_device *aorm;
int rc;
- rc = libxl__device_from_vkb(gc, domid, vkb, &device);
+ GCNEW(device);
+ rc = libxl__device_from_vkb(gc, domid, vkb, device);
if (rc != 0) goto out;
- rc = libxl__device_destroy(gc, &device);
+ GCNEW(aorm);
+ libxl__init_ao_device(aorm, ao, NULL);
+ aorm->action = DEVICE_DISCONNECT;
+ aorm->force = 1;
+ aorm->dev = device;
+ aorm->callback = libxl__device_cb;
+ libxl__initiate_device_remove(egc, aorm);
+
out:
- GC_FREE;
- return rc;
+ if (rc) return AO_ABORT(rc);
+ return AO_INPROGRESS;
}
/******************************************************************************/
@@ -2418,35 +2483,50 @@ int libxl_device_vfb_remove(libxl_ctx *ctx, uint32_t domid,
const libxl_asyncop_how *ao_how)
{
AO_CREATE(ctx, domid, ao_how);
- libxl__device device;
+ libxl__device *device;
+ libxl__ao_device *aorm;
int rc;
- rc = libxl__device_from_vfb(gc, domid, vfb, &device);
+ GCNEW(device);
+ rc = libxl__device_from_vfb(gc, domid, vfb, device);
if (rc != 0) goto out;
- rc = libxl__initiate_device_remove(egc, ao, &device);
- if (rc) goto out;
-
- return AO_INPROGRESS;
+ GCNEW(aorm);
+ libxl__init_ao_device(aorm, ao, NULL);
+ aorm->action = DEVICE_DISCONNECT;
+ aorm->dev = device;
+ aorm->callback = libxl__device_cb;
+ libxl__initiate_device_remove(egc, aorm);
out:
- return AO_ABORT(rc);
+ if (rc) return AO_ABORT(rc);
+ return AO_INPROGRESS;
}
int libxl_device_vfb_destroy(libxl_ctx *ctx, uint32_t domid,
- libxl_device_vfb *vfb)
+ libxl_device_vfb *vfb,
+ const libxl_asyncop_how *ao_how)
{
- GC_INIT(ctx);
- libxl__device device;
+ AO_CREATE(ctx, domid, ao_how);
+ libxl__device *device;
+ libxl__ao_device *aorm;
int rc;
- rc = libxl__device_from_vfb(gc, domid, vfb, &device);
+ GCNEW(device);
+ rc = libxl__device_from_vfb(gc, domid, vfb, device);
if (rc != 0) goto out;
- rc = libxl__device_destroy(gc, &device);
+ GCNEW(aorm);
+ libxl__init_ao_device(aorm, ao, NULL);
+ aorm->action = DEVICE_DISCONNECT;
+ aorm->force = 1;
+ aorm->dev = device;
+ aorm->callback = libxl__device_cb;
+ libxl__initiate_device_remove(egc, aorm);
+
out:
- GC_FREE;
- return rc;
+ if (rc) return AO_ABORT(rc);
+ return AO_INPROGRESS;
}
/******************************************************************************/
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 979940a..c3115a9 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -667,7 +667,8 @@ int libxl_device_disk_remove(libxl_ctx *ctx, uint32_t domid,
libxl_device_disk *disk,
const libxl_asyncop_how *ao_how);
int libxl_device_disk_destroy(libxl_ctx *ctx, uint32_t domid,
- libxl_device_disk *disk);
+ libxl_device_disk *disk,
+ const libxl_asyncop_how *ao_how);
libxl_device_disk *libxl_device_disk_list(libxl_ctx *ctx, uint32_t domid, int *num);
int libxl_device_disk_getinfo(libxl_ctx *ctx, uint32_t domid,
@@ -691,7 +692,9 @@ int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic);
int libxl_device_nic_remove(libxl_ctx *ctx, uint32_t domid,
libxl_device_nic *nic,
const libxl_asyncop_how *ao_how);
-int libxl_device_nic_destroy(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic);
+int libxl_device_nic_destroy(libxl_ctx *ctx, uint32_t domid,
+ libxl_device_nic *nic,
+ const libxl_asyncop_how *ao_how);
libxl_device_nic *libxl_device_nic_list(libxl_ctx *ctx, uint32_t domid, int *num);
int libxl_device_nic_getinfo(libxl_ctx *ctx, uint32_t domid,
@@ -702,14 +705,18 @@ int libxl_device_vkb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vkb *vkb);
int libxl_device_vkb_remove(libxl_ctx *ctx, uint32_t domid,
libxl_device_vkb *vkb,
const libxl_asyncop_how *ao_how);
-int libxl_device_vkb_destroy(libxl_ctx *ctx, uint32_t domid, libxl_device_vkb *vkb);
+int libxl_device_vkb_destroy(libxl_ctx *ctx, uint32_t domid,
+ libxl_device_vkb *vkb,
+ const libxl_asyncop_how *ao_how);
/* Framebuffer */
int libxl_device_vfb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vfb *vfb);
int libxl_device_vfb_remove(libxl_ctx *ctx, uint32_t domid,
libxl_device_vfb *vfb,
const libxl_asyncop_how *ao_how);
-int libxl_device_vfb_destroy(libxl_ctx *ctx, uint32_t domid, libxl_device_vfb *vfb);
+int libxl_device_vfb_destroy(libxl_ctx *ctx, uint32_t domid,
+ libxl_device_vfb *vfb,
+ const libxl_asyncop_how *ao_how);
/* PCI Passthrough */
int libxl_device_pci_add(libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev);
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 2006406..e572d4d 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -356,11 +356,16 @@ int libxl__device_disk_dev_number(const char *virtpath, int *pdisk,
return -1;
}
+/* Device AO operations */
-typedef struct {
- libxl__ao *ao;
- libxl__ev_devstate ds;
-} libxl__ao_device_remove;
+void libxl__init_ao_device(libxl__ao_device *aorm, libxl__ao *ao,
+ libxl__ao_device **base)
+{
+ aorm->ao = ao;
+ aorm->active = 1;
+ aorm->rc = 0;
+ aorm->base = base;
+}
int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
{
@@ -436,34 +441,35 @@ out:
/* Callbacks for device related operations */
-static void device_remove_callback(libxl__egc *egc, libxl__ev_devstate *ds,
+static void device_backend_callback(libxl__egc *egc, libxl__ev_devstate *ds,
int rc);
-static void device_remove_cleanup(libxl__gc *gc,
- libxl__ao_device_remove *aorm);
+static void device_backend_cleanup(libxl__gc *gc,
+ libxl__ao_device *aorm);
-int libxl__initiate_device_remove(libxl__egc *egc, libxl__ao *ao,
- libxl__device *dev)
+void libxl__initiate_device_remove(libxl__egc *egc,
+ libxl__ao_device *aorm)
{
- AO_GC;
+ STATE_AO_GC(aorm->ao);
libxl_ctx *ctx = libxl__gc_owner(gc);
- xs_transaction_t t;
- char *be_path = libxl__device_backend_path(gc, dev);
+ xs_transaction_t t = 0;
+ char *be_path = libxl__device_backend_path(gc, aorm->dev);
char *state_path = libxl__sprintf(gc, "%s/state", be_path);
- char *state = libxl__xs_read(gc, XBT_NULL, state_path);
+ char *state;
int rc = 0;
- libxl__ao_device_remove *aorm = 0;
+retry_transaction:
+ t = xs_transaction_start(ctx->xsh);
+ if (aorm->force)
+ libxl__xs_path_cleanup(gc, t,
+ libxl__device_frontend_path(gc, aorm->dev));
+ state = libxl__xs_read(gc, t, state_path);
if (!state)
goto out_ok;
- if (atoi(state) != 4) {
+ if (atoi(state) == XenbusStateClosed) {
libxl__device_destroy_tapdisk(gc, be_path);
- xs_rm(ctx->xsh, XBT_NULL, be_path);
goto out_ok;
}
-
-retry_transaction:
- t = xs_transaction_start(ctx->xsh);
xs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/online", be_path), "0", strlen("0"));
xs_write(ctx->xsh, t, state_path, "5", strlen("5"));
if (!xs_transaction_end(ctx->xsh, t, 0)) {
@@ -474,40 +480,71 @@ retry_transaction:
goto out_fail;
}
}
+ /* mark transaction as ended, to prevent double closing it on out_ok */
+ t = 0;
libxl__device_destroy_tapdisk(gc, be_path);
- aorm = libxl__zalloc(gc, sizeof(*aorm));
- aorm->ao = ao;
libxl__ev_devstate_init(&aorm->ds);
- rc = libxl__ev_devstate_wait(gc, &aorm->ds, device_remove_callback,
+ rc = libxl__ev_devstate_wait(gc, &aorm->ds, device_backend_callback,
state_path, XenbusStateClosed,
LIBXL_DESTROY_TIMEOUT * 1000);
- if (rc) goto out_fail;
+ if (rc) {
+ LOG(ERROR, "unable to remove device %s", be_path);
+ goto out_fail;
+ }
- return 0;
+ return;
out_fail:
assert(rc);
- device_remove_cleanup(gc, aorm);
- return rc;
+ aorm->rc = rc;
+ aorm->callback(egc, aorm);
+ return;
out_ok:
- libxl__ao_complete(egc, ao, 0);
- return 0;
+ if (t) xs_transaction_end(ctx->xsh, t, 0);
+ aorm->callback(egc, aorm);
+ return;
}
-static void device_remove_callback(libxl__egc *egc, libxl__ev_devstate *ds,
+static void device_backend_callback(libxl__egc *egc, libxl__ev_devstate *ds,
int rc) {
- libxl__ao_device_remove *aorm = CONTAINER_OF(ds, *aorm, ds);
- libxl__gc *gc = &aorm->ao->gc;
- libxl__ao_complete(egc, aorm->ao, rc);
- device_remove_cleanup(gc, aorm);
+ libxl__ao_device *aorm = CONTAINER_OF(ds, *aorm, ds);
+ STATE_AO_GC(aorm->ao);
+
+ device_backend_cleanup(gc, aorm);
+
+ if (rc == ERROR_TIMEDOUT && aorm->action == DEVICE_DISCONNECT &&
+ !aorm->force) {
+ aorm->force = 1;
+ libxl__initiate_device_remove(egc, aorm);
+ return;
+ }
+
+ /* Some devices (vkbd) fail to disconnect properly,
+ * but we shouldn't alarm the user if it's during
+ * domain destruction.
+ */
+ if (rc && aorm->action == DEVICE_CONNECT) {
+ LOG(ERROR, "unable to connect device with path %s",
+ libxl__device_backend_path(gc, aorm->dev));
+ goto out;
+ } else if(rc) {
+ LOG(DEBUG, "unable to disconnect device with path %s",
+ libxl__device_backend_path(gc, aorm->dev));
+ goto out;
+ }
+
+out:
+ aorm->rc = rc;
+ aorm->callback(egc, aorm);
+ return;
}
-static void device_remove_cleanup(libxl__gc *gc,
- libxl__ao_device_remove *aorm) {
+static void device_backend_cleanup(libxl__gc *gc, libxl__ao_device *aorm)
+{
if (!aorm) return;
libxl__ev_devstate_cancel(gc, &aorm->ds);
}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index ae5527b..b62110a 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -827,13 +827,6 @@ _hidden const char *libxl__device_nic_devname(libxl__gc *gc,
uint32_t devid,
libxl_nic_type type);
-/* Arranges that dev will be removed from its guest. When
- * this is done, the ao will be completed. An error
- * return from libxl__initiate_device_remove means that the ao
- * will _not_ be completed and the caller must do so. */
-_hidden int libxl__initiate_device_remove(libxl__egc*, libxl__ao*,
- libxl__device *dev);
-
/*
* libxl__ev_devstate - waits a given time for a device to
* reach a given state. Follows the libxl_ev_* conventions.
@@ -1802,6 +1795,44 @@ _hidden void libxl__bootloader_init(libxl__bootloader_state *bl);
* If callback is passed rc==0, will have updated st->info appropriately */
_hidden void libxl__bootloader_run(libxl__egc*, libxl__bootloader_state *st);
+/*----- device addition/removal -----*/
+
+/* Action to perform (either connect or disconnect) */
+typedef enum {
+ DEVICE_CONNECT,
+ DEVICE_DISCONNECT
+} libxl__device_action;
+
+typedef struct libxl__ao_device libxl__ao_device;
+typedef void libxl__device_callback(libxl__egc*, libxl__ao_device*);
+
+_hidden void libxl__init_ao_device(libxl__ao_device *aorm, libxl__ao *ao,
+ libxl__ao_device **base);
+
+struct libxl__ao_device {
+ /* filled in by user */
+ libxl__ao *ao;
+ /* action being performed */
+ libxl__device_action action;
+ libxl__device *dev;
+ int force;
+ libxl__device_callback *callback;
+ /* private for implementation */
+ /* State in which the device is */
+ int active;
+ int rc;
+ libxl__ev_devstate ds;
+ void *base;
+};
+
+/* Arranges that dev will be removed to the guest, and the
+ * hotplug scripts will be executed (if necessary). When
+ * this is done (or an error happens), the callback in
+ * aorm->callback will be called.
+ */
+_hidden void libxl__initiate_device_remove(libxl__egc *egc,
+ libxl__ao_device *aorm);
+
/*----- Domain creation -----*/
typedef struct libxl__domain_create_state libxl__domain_create_state;
--
1.7.7.5 (Apple Git-26)
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 05/15] libxl: change libxl__ao_device_remove to libxl__ao_device
2012-05-22 14:02 ` [PATCH v2 05/15] libxl: change libxl__ao_device_remove to libxl__ao_device Roger Pau Monne
@ 2012-05-22 15:10 ` Ian Jackson
2012-05-22 15:27 ` Ian Campbell
` (2 more replies)
0 siblings, 3 replies; 43+ messages in thread
From: Ian Jackson @ 2012-05-22 15:10 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel@lists.xen.org
Roger Pau Monne writes ("[PATCH v2 05/15] 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_internal.h b/tools/libxl/libxl_internal.h
> index ae5527b..b62110a 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1802,6 +1795,44 @@ _hidden void libxl__bootloader_init(libxl__bootloader_state *bl);
...
> +struct libxl__ao_device {
> + /* filled in by user */
> + libxl__ao *ao;
> + /* action being performed */
> + libxl__device_action action;
> + libxl__device *dev;
> + int force;
> + libxl__device_callback *callback;
> + /* private for implementation */
> + /* State in which the device is */
> + int active;
> + int rc;
> + libxl__ev_devstate ds;
> + void *base;
> +};
Re your comment, don't you mean "state of the operation" rather than
"state of the device" ?
I would prefer some reformatting to make the scope of comments like
"filled in by user" clearer. Perhaps a blank line before "private for
implementation". Or perhaps moving all the small comments to after
the variables.
TBH I'm not sure what the comment "action being performed" is for.
After all the variable is called "action" so it's obvious.
> +/* Arranges that dev will be removed to the guest, and the
> + * hotplug scripts will be executed (if necessary). When
> + * this is done (or an error happens), the callback in
> + * aorm->callback will be called.
> + */
> +_hidden void libxl__initiate_device_remove(libxl__egc *egc,
> + libxl__ao_device *aorm);
> +
Does this really only do removals ? If so where is the addition
function and what is the "action" parameter for ?
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 2a09192..b13de4f 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1271,6 +1271,26 @@ int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass)
>
> /******************************************************************************/
>
> +/* generic callback for devices that only need to set ao_complete */
> +static void libxl__device_cb(libxl__egc *egc, libxl__ao_device *aorm)
Maybe this should have a more descriptive name ?
device_addrm_aocomplete
depending on whether it's for removal only or for addition too.
You are still using the parameter name "aorm" for something which is
not only a removal operation. "aodev" or something.
> int libxl_device_nic_destroy(libxl_ctx *ctx, uint32_t domid,
> - libxl_device_nic *nic)
> + libxl_device_nic *nic,
> + const libxl_asyncop_how *ao_how)
> {
> - GC_INIT(ctx);
> - libxl__device device;
> + AO_CREATE(ctx, domid, ao_how);
> + libxl__device *device;
> + libxl__ao_device *aorm;
> int rc;
>
> - rc = libxl__device_from_nic(gc, domid, nic, &device);
> + GCNEW(device);
> + rc = libxl__device_from_nic(gc, domid, nic, device);
> if (rc != 0) goto out;
>
> - rc = libxl__device_destroy(gc, &device);
> + GCNEW(aorm);
> + libxl__init_ao_device(aorm, ao, NULL);
> + aorm->action = DEVICE_DISCONNECT;
> + aorm->force = 1;
> + aorm->dev = device;
> + aorm->callback = libxl__device_cb;
> + libxl__initiate_device_remove(egc, aorm);
> +
Is there some way to make these functions less repetitive ?
We have {nic,disk,vkb,vfb}_{remove,destroy} all of which have
essentially identical innards.
I have some suggestions, in descending order of my own preference.
How about writing the common code once in a macro:
1 #define DEFINE_DEVICE_REMOVE(type, removedestroy, force) \
1 int libxl_device_##type##_##removedestroy(libxl_ctx *ctx, \
1 uint32_t domid, libxl_device_##type *nic, \
1 const libxl_asyncop_how *ao_how) \
1 { \
1 AO_CREATE etc. etc. \
1 rc = libxl__device_from_##type( etc. etc. ) \
1 etc. etc. \
1 }
8 DEFINE_DEVICE_REMOVE(nic, remove, 0)
. DEFINE_DEVICE_REMOVE(nic, destroy, 1)
. DEFINE_DEVICE_REMOVE(disk, remove, 0)
. DEFINE_DEVICE_REMOVE(disk, destroy, 1)
. DEFINE_DEVICE_REMOVE(vkb, remove, 0)
. DEFINE_DEVICE_REMOVE(vkb, destroy, 1)
. DEFINE_DEVICE_REMOVE(vfb, remove, 0)
. DEFINE_DEVICE_REMOVE(vfb, destroy, 1)
(Numbers on the left show how many instances of formulaic code there
are.)
Or writing the common code once in a function which takes an
appropriate function pointer type for the conversion:
1 int device_remove(libxl_ao *ao, uint32_t domid, int force,
1 void *libxl_foo_device,
1 int (*libxl_device_from_foo_device)
1 (libxl__gc *gc, uint32_t domid,
1 void *libxl_foo_device, libxl__device **device))
1 {
1 AO_CREATE etc. etc.
4 static int device_from_nic_void(libxl__gc *gc, uint32_t domid,
4 void *libxl_foo_device, libxl__device **device)
4 { return libxl__device_from_nic(gc, domid, libxl_foo_device, device); }
8 int libxl_device_nic_remove(libxl_ctx *ctx, uint32_t domid,
8 libxl_device_nic *nic,
8 const libxl_asyncop_how *ao_how)
8 { return device_remove(ctx, domid, 0, nic, device_from_nic_void); }
. int libxl_device_nic_destroy(libxl_ctx *ctx, uint32_t domid,
. libxl_device_nic *nic,
. const libxl_asyncop_how *ao_how)
. { return device_remove(ctx, domid, 1, nic, device_from_nic_void); }
Or combining the above:
1 int device_remove(libxl_ao *ao, uint32_t domid, int force,
1 void *libxl_foo_device,
1 int (*libxl_device_from_foo_device)
1 (libxl__gc *gc, uint32_t domid,
1 void *libxl_foo_device, libxl__device **device))
1 {
1 AO_CREATE etc. etc.
1 #define DEFINE_DEVICE_REMOVE(type) \
4 static int device_from_##type##_void(libxl__gc *gc, uint32_t domid, \
4 void *libxl_foo_device, libxl__device **device) \
4 { return libxl__device_from_##type(gc, domid, \
4 libxl_foo_device, device); } \
\
2 int libxl_device_##type##_remove(libxl_ctx *ctx, uint32_t domid, \
2 libxl_device_##type *thing, \
2 const libxl_asyncop_how *ao_how) \
2 { return device_remove(ctx, domid, 0, thing, \
2 device_from_##type##_void); } \
\
. int libxl_device_##type##_destroy(libxl_ctx *ctx, uint32_t domid, \
. libxl_device_##type *thing, \
. const libxl_asyncop_how *ao_how) \
. { return device_remove(ctx, domid, 1, thing, \
. device_from_##type##_void); }
4 DEVICE_DEVICE_REMOVE(nic)
. DEVICE_DEVICE_REMOVE(disk)
. DEVICE_DEVICE_REMOVE(vfb)
. DEVICE_DEVICE_REMOVE(vkb)
Or at least reducing the number of copies from 8 to 4 like this:
4 int nic_remove(libxl_ctx *ctx, uint32_t domid,
4 libxl__device_nic *nic, int force,
4 const libxl_asyncop_how *ao_how);
8 int libxl_device_nic_remove(libxl_ctx *ctx, uint32_t domid,
8 libxl_device_nic *nic,
8 const libxl_asyncop_how *ao_how)
8 { return nic_remove(ctx, domid, nic, 0, ao_how); }
. int libxl_device_nic_destroy(libxl_ctx *ctx, uint32_t domid,
. libxl_device_nic *nic,
. const libxl_asyncop_how *ao_how)
. { return nic_remove(ctx, domid, nic, 1, ao_how); }
Or at least reducing the size of the octuplicated code like this:
1 int device_remove(libxl_ao *ao, uint32_t domid,
1 libxl__device *device, int force)
1 {
1 ...
1 GCNEW(aodev);
1 libxl__init_ao_device(aodev, ao, NULL);
1 aodev->action = DEVICE_DISCONNECT;
1 aodev->force = 1;
1 aodev->dev = device;
1 aodev->callback = libxl__device_cb;
1 libxl__initiate_device_addremove(egc, aodev);
1 etc. etc.
8 int libxl_device_nic_remove(libxl_ctx *ctx, uint32_t domid,
8 libxl_device_nic *nic,
8 const libxl_asyncop_how *ao_how)
8 {
8 AO_CREATE(ctx, domid, ao_how);
8 int rc;
8 libxl__device *device;
8
8 rc = libxl__device_from_nic(gc, domid, nic, &device);
8 if (rc) return AO_ABORT(rc);
8
8 device_remove(ao, domid, device, 0);
8 return AO_INPROGRESS;
8 }
?
> +/* Device AO operations */
>
> -typedef struct {
> - libxl__ao *ao;
> - libxl__ev_devstate ds;
> -} libxl__ao_device_remove;
> +void libxl__init_ao_device(libxl__ao_device *aorm, libxl__ao *ao,
> + libxl__ao_device **base)
> +{
> + aorm->ao = ao;
> + aorm->active = 1;
> + aorm->rc = 0;
> + aorm->base = base;
> +}
I think a function "init" should set "active" to 0, surely ?
Since the operation has not in fact been started. So perhaps this
should just be a memset.
> +retry_transaction:
> + t = xs_transaction_start(ctx->xsh);
> + if (aorm->force)
> + libxl__xs_path_cleanup(gc, t,
> + libxl__device_frontend_path(gc, aorm->dev));
> + state = libxl__xs_read(gc, t, state_path);
> if (!state)
> goto out_ok;
If you're reworking this, you should check for errors other than
ENOENT here.
But actually, 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.
Also you seem to only do the _path_cleanup thing (removing newly empty
parent directories) in the force case, if I'm not mistaken. I think
it would be better to make the force case simply skip the device wait
and then reuse the rest of the code path if possible.
> + if (rc == ERROR_TIMEDOUT && aorm->action == DEVICE_DISCONNECT &&
> + !aorm->force) {
> + aorm->force = 1;
> + libxl__initiate_device_remove(egc, aorm);
> + return;
> + }
I like this approach. Economical.
> + /* Some devices (vkbd) fail to disconnect properly,
> + * but we shouldn't alarm the user if it's during
> + * domain destruction.
> + */
Perhaps we should have a separate flag which controls error reporting
so that a user-requested graceful device disconnect gets full error
logging ?
Thanks,
Ian.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 05/15] libxl: change libxl__ao_device_remove to libxl__ao_device
2012-05-22 15:10 ` Ian Jackson
@ 2012-05-22 15:27 ` Ian Campbell
2012-05-22 16:23 ` Ian Jackson
2012-05-22 15:34 ` Roger Pau Monne
2012-05-22 15:55 ` Roger Pau Monne
2 siblings, 1 reply; 43+ messages in thread
From: Ian Campbell @ 2012-05-22 15:27 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel@lists.xen.org, Roger Pau Monne
On Tue, 2012-05-22 at 16:10 +0100, Ian Jackson wrote:
> > int libxl_device_nic_destroy(libxl_ctx *ctx, uint32_t domid,
> > - libxl_device_nic *nic)
> > + libxl_device_nic *nic,
> > + const libxl_asyncop_how *ao_how)
> > {
> > - GC_INIT(ctx);
> > - libxl__device device;
> > + AO_CREATE(ctx, domid, ao_how);
> > + libxl__device *device;
> > + libxl__ao_device *aorm;
> > int rc;
> >
> > - rc = libxl__device_from_nic(gc, domid, nic, &device);
> > + GCNEW(device);
> > + rc = libxl__device_from_nic(gc, domid, nic, device);
> > if (rc != 0) goto out;
> >
> > - rc = libxl__device_destroy(gc, &device);
> > + GCNEW(aorm);
> > + libxl__init_ao_device(aorm, ao, NULL);
> > + aorm->action = DEVICE_DISCONNECT;
> > + aorm->force = 1;
> > + aorm->dev = device;
> > + aorm->callback = libxl__device_cb;
> > + libxl__initiate_device_remove(egc, aorm);
> > +
>
> Is there some way to make these functions less repetitive ?
> We have {nic,disk,vkb,vfb}_{remove,destroy} all of which have
> essentially identical innards.
>
> I have some suggestions, in descending order of my own preference.
>
> How about writing the common code once in a macro:
>
> 1 #define DEFINE_DEVICE_REMOVE(type, removedestroy, force) \
> 1 int libxl_device_##type##_##removedestroy(libxl_ctx *ctx, \
> 1 uint32_t domid, libxl_device_##type *nic, \
> 1 const libxl_asyncop_how *ao_how) \
> 1 { \
> 1 AO_CREATE etc. etc. \
> 1 rc = libxl__device_from_##type( etc. etc. ) \
> 1 etc. etc. \
> 1 }
>
> 8 DEFINE_DEVICE_REMOVE(nic, remove, 0)
> . DEFINE_DEVICE_REMOVE(nic, destroy, 1)
> . DEFINE_DEVICE_REMOVE(disk, remove, 0)
> . DEFINE_DEVICE_REMOVE(disk, destroy, 1)
> . DEFINE_DEVICE_REMOVE(vkb, remove, 0)
> . DEFINE_DEVICE_REMOVE(vkb, destroy, 1)
> . DEFINE_DEVICE_REMOVE(vfb, remove, 0)
> . DEFINE_DEVICE_REMOVE(vfb, destroy, 1)
[...]
> Or at least reducing the size of the octuplicated code like this:
>
> 1 int device_remove(libxl_ao *ao, uint32_t domid,
> 1 libxl__device *device, int force)
> 1 {
> 1 ...
> 1 GCNEW(aodev);
> 1 libxl__init_ao_device(aodev, ao, NULL);
> 1 aodev->action = DEVICE_DISCONNECT;
> 1 aodev->force = 1;
> 1 aodev->dev = device;
> 1 aodev->callback = libxl__device_cb;
> 1 libxl__initiate_device_addremove(egc, aodev);
> 1 etc. etc.
>
> 8 int libxl_device_nic_remove(libxl_ctx *ctx, uint32_t domid,
> 8 libxl_device_nic *nic,
> 8 const libxl_asyncop_how *ao_how)
> 8 {
> 8 AO_CREATE(ctx, domid, ao_how);
> 8 int rc;
> 8 libxl__device *device;
> 8
> 8 rc = libxl__device_from_nic(gc, domid, nic, &device);
> 8 if (rc) return AO_ABORT(rc);
> 8
> 8 device_remove(ao, domid, device, 0);
> 8 return AO_INPROGRESS;
> 8 }
Weirdly (given they were ordered by your preference) I have a preference
for either the first or last form (which I have left quoted above).
I probably slightly prefer the second option, but only slightly
(probably due to a general disquiet about macros which define
functions).
Ian.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 05/15] libxl: change libxl__ao_device_remove to libxl__ao_device
2012-05-22 15:27 ` Ian Campbell
@ 2012-05-22 16:23 ` Ian Jackson
0 siblings, 0 replies; 43+ messages in thread
From: Ian Jackson @ 2012-05-22 16:23 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel@lists.xen.org, Roger Pau Monne
Ian Campbell writes ("Re: [Xen-devel] [PATCH v2 05/15] libxl: change libxl__ao_device_remove to libxl__ao_device"):
> Weirdly (given they were ordered by your preference) I have a preference
> for either the first or last form (which I have left quoted above).
My ordering is based almost entirely on the number of lines of code in
the result :-).
> I probably slightly prefer the second option, but only slightly
> (probably due to a general disquiet about macros which define
> functions).
Right. If you are happy with any of the options then Roger can
decide.
Ian.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 05/15] libxl: change libxl__ao_device_remove to libxl__ao_device
2012-05-22 15:10 ` Ian Jackson
2012-05-22 15:27 ` Ian Campbell
@ 2012-05-22 15:34 ` Roger Pau Monne
2012-05-22 15:55 ` Roger Pau Monne
2 siblings, 0 replies; 43+ messages in thread
From: Roger Pau Monne @ 2012-05-22 15:34 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel@lists.xen.org
Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH v2 05/15] 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_internal.h b/tools/libxl/libxl_internal.h
>> index ae5527b..b62110a 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
>> @@ -1802,6 +1795,44 @@ _hidden void libxl__bootloader_init(libxl__bootloader_state *bl);
> ...
>> +struct libxl__ao_device {
>> + /* filled in by user */
>> + libxl__ao *ao;
>> + /* action being performed */
>> + libxl__device_action action;
>> + libxl__device *dev;
>> + int force;
>> + libxl__device_callback *callback;
>> + /* private for implementation */
>> + /* State in which the device is */
>> + int active;
>> + int rc;
>> + libxl__ev_devstate ds;
>> + void *base;
>> +};
>
> Re your comment, don't you mean "state of the operation" rather than
> "state of the device" ?
>
> I would prefer some reformatting to make the scope of comments like
> "filled in by user" clearer. Perhaps a blank line before "private for
> implementation". Or perhaps moving all the small comments to after
> the variables.
>
> TBH I'm not sure what the comment "action being performed" is for.
> After all the variable is called "action" so it's obvious.
No, probably no need to write any comment, since it's obvious.
>> +/* Arranges that dev will be removed to the guest, and the
>> + * hotplug scripts will be executed (if necessary). When
>> + * this is done (or an error happens), the callback in
>> + * aorm->callback will be called.
>> + */
>> +_hidden void libxl__initiate_device_remove(libxl__egc *egc,
>> + libxl__ao_device *aorm);
>> +
>
> Does this really only do removals ? If so where is the addition
> function and what is the "action" parameter for ?
The addition code comes in a next patch, but I've already added the
"action" libxl__ao_device var to reduce the noise, since it's a harmless
addition.
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index 2a09192..b13de4f 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -1271,6 +1271,26 @@ int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass)
>>
>> /******************************************************************************/
>>
>> +/* generic callback for devices that only need to set ao_complete */
>> +static void libxl__device_cb(libxl__egc *egc, libxl__ao_device *aorm)
>
> Maybe this should have a more descriptive name ?
> device_addrm_aocomplete
> depending on whether it's for removal only or for addition too.
It's for both addition/removal.
> You are still using the parameter name "aorm" for something which is
> not only a removal operation. "aodev" or something.
>
>> int libxl_device_nic_destroy(libxl_ctx *ctx, uint32_t domid,
>> - libxl_device_nic *nic)
>> + libxl_device_nic *nic,
>> + const libxl_asyncop_how *ao_how)
>> {
>> - GC_INIT(ctx);
>> - libxl__device device;
>> + AO_CREATE(ctx, domid, ao_how);
>> + libxl__device *device;
>> + libxl__ao_device *aorm;
>> int rc;
>>
>> - rc = libxl__device_from_nic(gc, domid, nic,&device);
>> + GCNEW(device);
>> + rc = libxl__device_from_nic(gc, domid, nic, device);
>> if (rc != 0) goto out;
>>
>> - rc = libxl__device_destroy(gc,&device);
>> + GCNEW(aorm);
>> + libxl__init_ao_device(aorm, ao, NULL);
>> + aorm->action = DEVICE_DISCONNECT;
>> + aorm->force = 1;
>> + aorm->dev = device;
>> + aorm->callback = libxl__device_cb;
>> + libxl__initiate_device_remove(egc, aorm);
>> +
>
> Is there some way to make these functions less repetitive ?
> We have {nic,disk,vkb,vfb}_{remove,destroy} all of which have
> essentially identical innards.
>
> I have some suggestions, in descending order of my own preference.
Well, I have to say I prefer the first option, since it reduces the
length of code, but I don't know if it will be too complicated. Is it ok
if I include this in this patch, or should I put it on a separate patch?
> How about writing the common code once in a macro:
>
> 1 #define DEFINE_DEVICE_REMOVE(type, removedestroy, force) \
> 1 int libxl_device_##type##_##removedestroy(libxl_ctx *ctx, \
> 1 uint32_t domid, libxl_device_##type *nic, \
> 1 const libxl_asyncop_how *ao_how) \
> 1 { \
> 1 AO_CREATE etc. etc. \
> 1 rc = libxl__device_from_##type( etc. etc. ) \
> 1 etc. etc. \
> 1 }
>
> 8 DEFINE_DEVICE_REMOVE(nic, remove, 0)
> . DEFINE_DEVICE_REMOVE(nic, destroy, 1)
> . DEFINE_DEVICE_REMOVE(disk, remove, 0)
> . DEFINE_DEVICE_REMOVE(disk, destroy, 1)
> . DEFINE_DEVICE_REMOVE(vkb, remove, 0)
> . DEFINE_DEVICE_REMOVE(vkb, destroy, 1)
> . DEFINE_DEVICE_REMOVE(vfb, remove, 0)
> . DEFINE_DEVICE_REMOVE(vfb, destroy, 1)
>
> (Numbers on the left show how many instances of formulaic code there
> are.)
>
> Or writing the common code once in a function which takes an
> appropriate function pointer type for the conversion:
>
> 1 int device_remove(libxl_ao *ao, uint32_t domid, int force,
> 1 void *libxl_foo_device,
> 1 int (*libxl_device_from_foo_device)
> 1 (libxl__gc *gc, uint32_t domid,
> 1 void *libxl_foo_device, libxl__device **device))
> 1 {
> 1 AO_CREATE etc. etc.
>
> 4 static int device_from_nic_void(libxl__gc *gc, uint32_t domid,
> 4 void *libxl_foo_device, libxl__device **device)
> 4 { return libxl__device_from_nic(gc, domid, libxl_foo_device, device); }
>
> 8 int libxl_device_nic_remove(libxl_ctx *ctx, uint32_t domid,
> 8 libxl_device_nic *nic,
> 8 const libxl_asyncop_how *ao_how)
> 8 { return device_remove(ctx, domid, 0, nic, device_from_nic_void); }
>
> . int libxl_device_nic_destroy(libxl_ctx *ctx, uint32_t domid,
> . libxl_device_nic *nic,
> . const libxl_asyncop_how *ao_how)
> . { return device_remove(ctx, domid, 1, nic, device_from_nic_void); }
>
> Or combining the above:
>
> 1 int device_remove(libxl_ao *ao, uint32_t domid, int force,
> 1 void *libxl_foo_device,
> 1 int (*libxl_device_from_foo_device)
> 1 (libxl__gc *gc, uint32_t domid,
> 1 void *libxl_foo_device, libxl__device **device))
> 1 {
> 1 AO_CREATE etc. etc.
>
> 1 #define DEFINE_DEVICE_REMOVE(type) \
> 4 static int device_from_##type##_void(libxl__gc *gc, uint32_t domid, \
> 4 void *libxl_foo_device, libxl__device **device) \
> 4 { return libxl__device_from_##type(gc, domid, \
> 4 libxl_foo_device, device); } \
> \
> 2 int libxl_device_##type##_remove(libxl_ctx *ctx, uint32_t domid, \
> 2 libxl_device_##type *thing, \
> 2 const libxl_asyncop_how *ao_how) \
> 2 { return device_remove(ctx, domid, 0, thing, \
> 2 device_from_##type##_void); } \
> \
> . int libxl_device_##type##_destroy(libxl_ctx *ctx, uint32_t domid, \
> . libxl_device_##type *thing, \
> . const libxl_asyncop_how *ao_how) \
> . { return device_remove(ctx, domid, 1, thing, \
> . device_from_##type##_void); }
>
> 4 DEVICE_DEVICE_REMOVE(nic)
> . DEVICE_DEVICE_REMOVE(disk)
> . DEVICE_DEVICE_REMOVE(vfb)
> . DEVICE_DEVICE_REMOVE(vkb)
>
> Or at least reducing the number of copies from 8 to 4 like this:
>
> 4 int nic_remove(libxl_ctx *ctx, uint32_t domid,
> 4 libxl__device_nic *nic, int force,
> 4 const libxl_asyncop_how *ao_how);
>
> 8 int libxl_device_nic_remove(libxl_ctx *ctx, uint32_t domid,
> 8 libxl_device_nic *nic,
> 8 const libxl_asyncop_how *ao_how)
> 8 { return nic_remove(ctx, domid, nic, 0, ao_how); }
>
> . int libxl_device_nic_destroy(libxl_ctx *ctx, uint32_t domid,
> . libxl_device_nic *nic,
> . const libxl_asyncop_how *ao_how)
> . { return nic_remove(ctx, domid, nic, 1, ao_how); }
>
> Or at least reducing the size of the octuplicated code like this:
>
> 1 int device_remove(libxl_ao *ao, uint32_t domid,
> 1 libxl__device *device, int force)
> 1 {
> 1 ...
> 1 GCNEW(aodev);
> 1 libxl__init_ao_device(aodev, ao, NULL);
> 1 aodev->action = DEVICE_DISCONNECT;
> 1 aodev->force = 1;
> 1 aodev->dev = device;
> 1 aodev->callback = libxl__device_cb;
> 1 libxl__initiate_device_addremove(egc, aodev);
> 1 etc. etc.
>
> 8 int libxl_device_nic_remove(libxl_ctx *ctx, uint32_t domid,
> 8 libxl_device_nic *nic,
> 8 const libxl_asyncop_how *ao_how)
> 8 {
> 8 AO_CREATE(ctx, domid, ao_how);
> 8 int rc;
> 8 libxl__device *device;
> 8
> 8 rc = libxl__device_from_nic(gc, domid, nic,&device);
> 8 if (rc) return AO_ABORT(rc);
> 8
> 8 device_remove(ao, domid, device, 0);
> 8 return AO_INPROGRESS;
> 8 }
>
> ?
>
>> +/* Device AO operations */
>>
>> -typedef struct {
>> - libxl__ao *ao;
>> - libxl__ev_devstate ds;
>> -} libxl__ao_device_remove;
>> +void libxl__init_ao_device(libxl__ao_device *aorm, libxl__ao *ao,
>> + libxl__ao_device **base)
>> +{
>> + aorm->ao = ao;
>> + aorm->active = 1;
>> + aorm->rc = 0;
>> + aorm->base = base;
>> +}
>
> I think a function "init" should set "active" to 0, surely ?
> Since the operation has not in fact been started. So perhaps this
> should just be a memset.
>
>> +retry_transaction:
>> + t = xs_transaction_start(ctx->xsh);
>> + if (aorm->force)
>> + libxl__xs_path_cleanup(gc, t,
>> + libxl__device_frontend_path(gc, aorm->dev));
>> + state = libxl__xs_read(gc, t, state_path);
>> if (!state)
>> goto out_ok;
>
> If you're reworking this, you should check for errors other than
> ENOENT here.
>
> But actually, 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.
>
> Also you seem to only do the _path_cleanup thing (removing newly empty
> parent directories) in the force case, if I'm not mistaken. I think
> it would be better to make the force case simply skip the device wait
> and then reuse the rest of the code path if possible.
Right now it is true that the path_cleanup thing is only used when using
device_{...}_remove/destroy functions, but later patches change that. It
is not modified here because I wanted to keep the changes that touch
libxl__devices_destroy together.
Not really, at least for vif we actually need to wait for the device to
switch to state 6 or we will get error messages on xenstore that come
from the fact that the kernel is also monitoring xenstore and it looses
track of the vif interface before it's properly shut down.
In the past we never saw this errors because xen-hotplug-cleanup script
deletes them from xenstore (nice).
>> + if (rc == ERROR_TIMEDOUT&& aorm->action == DEVICE_DISCONNECT&&
>> + !aorm->force) {
>> + aorm->force = 1;
>> + libxl__initiate_device_remove(egc, aorm);
>> + return;
>> + }
>
> I like this approach. Economical.
>
>> + /* Some devices (vkbd) fail to disconnect properly,
>> + * but we shouldn't alarm the user if it's during
>> + * domain destruction.
>> + */
>
> Perhaps we should have a separate flag which controls error reporting
> so that a user-requested graceful device disconnect gets full error
> logging ?
Can we left that for a next patch?
I would like this series to not keep growing indefinitely.
> Thanks,
Thanks for the review!
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 05/15] libxl: change libxl__ao_device_remove to libxl__ao_device
2012-05-22 15:10 ` Ian Jackson
2012-05-22 15:27 ` Ian Campbell
2012-05-22 15:34 ` Roger Pau Monne
@ 2012-05-22 15:55 ` Roger Pau Monne
2012-05-22 16:32 ` Ian Jackson
2 siblings, 1 reply; 43+ messages in thread
From: Roger Pau Monne @ 2012-05-22 15:55 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel@lists.xen.org
Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH v2 05/15] 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_internal.h b/tools/libxl/libxl_internal.h
>> index ae5527b..b62110a 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
>> @@ -1802,6 +1795,44 @@ _hidden void libxl__bootloader_init(libxl__bootloader_state *bl);
> ...
>> +/* Device AO operations */
>>
>> -typedef struct {
>> - libxl__ao *ao;
>> - libxl__ev_devstate ds;
>> -} libxl__ao_device_remove;
>> +void libxl__init_ao_device(libxl__ao_device *aorm, libxl__ao *ao,
>> + libxl__ao_device **base)
>> +{
>> + aorm->ao = ao;
>> + aorm->active = 1;
>> + aorm->rc = 0;
>> + aorm->base = base;
>> +}
>
> I think a function "init" should set "active" to 0, surely ?
> Since the operation has not in fact been started. So perhaps this
> should just be a memset.
Forget to answer this, we cannot use active = 0 at init because in the
hypothetical case that a device add/remove finished before queuing all
the others (so the others would be active == 0) it would mark the ao as
complete, since it would see all other operations as finished.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 05/15] libxl: change libxl__ao_device_remove to libxl__ao_device
2012-05-22 15:55 ` Roger Pau Monne
@ 2012-05-22 16:32 ` Ian Jackson
0 siblings, 0 replies; 43+ messages in thread
From: Ian Jackson @ 2012-05-22 16:32 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel@lists.xen.org
Roger Pau Monne writes ("Re: [PATCH v2 05/15] libxl: change libxl__ao_device_remove to libxl__ao_device"):
> Ian Jackson wrote:
> > Roger Pau Monne writes ("[PATCH v2 05/15] libxl: change libxl__ao_device_remove to libxl__ao_device"):
> >> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> >> index ae5527b..b62110a 100644
> >> --- a/tools/libxl/libxl_internal.h
> >> +++ b/tools/libxl/libxl_internal.h
> >> @@ -1802,6 +1795,44 @@ _hidden void libxl__bootloader_init(libxl__bootloader_state *bl);
> > ...
> >> +{
> >> + aorm->ao = ao;
> >> + aorm->active = 1;
> >> + aorm->rc = 0;
> >> + aorm->base = base;
> >> +}
> >
> > I think a function "init" should set "active" to 0, surely ?
> > Since the operation has not in fact been started. So perhaps this
> > should just be a memset.
>
> Forget to answer this, we cannot use active = 0 at init because in the
> hypothetical case that a device add/remove finished before queuing all
> the others (so the others would be active == 0) it would mark the ao as
> complete, since it would see all other operations as finished.
Hmmm. I see. Yes.
I think you need to rename _init to _prepare or something then. And
it will then need a comment explaining the semantics. Are you allowed
to call the operation start function without calling _prepare, for
example? Do you have to take any destruction steps after calling just
prepare but changing your mind ? (answer seems to be "no") etc.
Ian.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 06/15] libxl: move device model creation prototypes
2012-05-22 14:02 [PATCH v2 0/15] execute hotplug scripts from libxl Roger Pau Monne
` (4 preceding siblings ...)
2012-05-22 14:02 ` [PATCH v2 05/15] libxl: change libxl__ao_device_remove to libxl__ao_device Roger Pau Monne
@ 2012-05-22 14:02 ` Roger Pau Monne
2012-05-22 15:10 ` Ian Jackson
2012-05-22 14:02 ` [PATCH v2 07/15] libxl: convert libxl_domain_destroy to an async op Roger Pau Monne
` (9 subsequent siblings)
15 siblings, 1 reply; 43+ messages in thread
From: Roger Pau Monne @ 2012-05-22 14:02 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>
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 b62110a..d2c013c 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1067,44 +1067,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
@@ -1833,6 +1795,43 @@ struct libxl__ao_device {
_hidden void libxl__initiate_device_remove(libxl__egc *egc,
libxl__ao_device *aorm);
+/*----- 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] 43+ messages in thread* [PATCH v2 07/15] libxl: convert libxl_domain_destroy to an async op
2012-05-22 14:02 [PATCH v2 0/15] execute hotplug scripts from libxl Roger Pau Monne
` (5 preceding siblings ...)
2012-05-22 14:02 ` [PATCH v2 06/15] libxl: move device model creation prototypes Roger Pau Monne
@ 2012-05-22 14:02 ` Roger Pau Monne
2012-05-22 14:11 ` Roger Pau Monne
2012-05-22 17:01 ` Ian Jackson
2012-05-22 14:02 ` [PATCH v2 08/15] libxl: convert libxl_device_disk_add to an asyn op Roger Pau Monne
` (8 subsequent siblings)
15 siblings, 2 replies; 43+ messages in thread
From: Roger Pau Monne @ 2012-05-22 14:02 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.
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 | 192 ++++++++++++++++++++++++++++++++++--------
tools/libxl/libxl_dm.c | 85 ++++++++++---------
tools/libxl/libxl_internal.h | 90 +++++++++++++++++++-
tools/libxl/xl_cmdimpl.c | 12 ++--
7 files changed, 473 insertions(+), 105 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index b13de4f..b44ae75 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1070,11 +1070,119 @@ 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);
+
+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->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);
+ }
+
+ if (dds->domain_finished)
+ dds->callback(egc, dds, rc);
+}
+
+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->domain_finished = 1;
+ if (dds->stubdom_finished)
+ dds->callback(egc, 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;
@@ -1085,7 +1193,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)) {
@@ -1118,7 +1226,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);
@@ -1141,9 +1279,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)
@@ -1272,20 +1411,20 @@ int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass)
/******************************************************************************/
/* generic callback for devices that only need to set ao_complete */
-static void libxl__device_cb(libxl__egc *egc, libxl__ao_device *aorm)
+static void libxl__device_cb(libxl__egc *egc, libxl__ao_device *aodev)
{
- STATE_AO_GC(aorm->ao);
+ STATE_AO_GC(aodev->ao);
- if (aorm->rc) {
+ if (aodev->rc) {
LOGE(ERROR, "unable to %s %s with id %u",
- aorm->action == DEVICE_CONNECT ? "add" : "remove",
- libxl__device_kind_to_string(aorm->dev->kind),
- aorm->dev->devid);
+ 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, aorm->rc);
+ libxl__ao_complete(egc, ao, aodev->rc);
return;
}
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index c3115a9..39a8c58 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -530,7 +530,8 @@ int libxl_domain_suspend(libxl_ctx *ctx, libxl_domain_suspend_info *info,
int libxl_domain_resume(libxl_ctx *ctx, uint32_t domid);
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..63f34fa 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, rc, 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 e572d4d..020e934 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)
{
@@ -358,13 +400,30 @@ int libxl__device_disk_dev_number(const char *virtpath, int *pdisk,
/* Device AO operations */
-void libxl__init_ao_device(libxl__ao_device *aorm, libxl__ao *ao,
+void libxl__init_ao_device(libxl__ao_device *aodev, libxl__ao *ao,
libxl__ao_device **base)
{
- aorm->ao = ao;
- aorm->active = 1;
- aorm->rc = 0;
- aorm->base = base;
+ aodev->ao = ao;
+ aodev->active = 1;
+ aodev->rc = 0;
+ aodev->base = base;
+}
+
+int libxl__ao_device_check_last(libxl__gc *gc, libxl__ao_device *device,
+ libxl__ao_device *list, int num, int *last)
+{
+ int i, ret = 0;
+
+ device->active = 0;
+ *last = 1;
+ for (i = 0; i < num; i++) {
+ if (list[i].active && *last) {
+ *last = 0;
+ }
+ if (list[i].rc)
+ ret = list[i].rc;
+ }
+ return ret;
}
int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
@@ -381,16 +440,34 @@ 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 *aorm);
+
+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__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->aorm, drs->num_devices);
+ for (i = 0; i < drs->num_devices; i++) {
+ libxl__init_ao_device(&drs->aorm[i], drs->ao, &drs->aorm);
+ }
+
path = libxl__sprintf(gc, "/local/domain/%d/device", domid);
kinds = libxl__xs_directory(gc, XBT_NULL, path, &num_kinds);
if (!kinds) {
@@ -413,30 +490,40 @@ 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);
+ printf("device: %s\n", path);
+ GCNEW(dev);
+ if (path && libxl__parse_backend_path(gc, path, dev) == 0) {
+ dev->domid = domid;
+ dev->kind = kind;
+ dev->devid = atoi(devs[j]);
+ drs->aorm[numdev].action = DEVICE_DISCONNECT;
+ drs->aorm[numdev].dev = dev;
+ drs->aorm[numdev].callback = device_remove_callback;
+ drs->aorm[numdev].force = drs->force;
+ libxl__initiate_device_remove(egc, &drs->aorm[numdev]);
+ numdev++;
}
}
}
/* console 0 frontend directory is not under /local/domain/<domid>/device */
path = libxl__sprintf(gc, "/local/domain/%d/console/backend", domid);
+ printf("frontend: %s\n", path);
path = libxl__xs_read(gc, XBT_NULL, path);
+ printf("backend: %s\n", 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 */
@@ -444,8 +531,7 @@ out:
static void device_backend_callback(libxl__egc *egc, libxl__ev_devstate *ds,
int rc);
-static void device_backend_cleanup(libxl__gc *gc,
- libxl__ao_device *aorm);
+static void device_backend_cleanup(libxl__gc *gc, libxl__ao_device *aodev);
void libxl__initiate_device_remove(libxl__egc *egc,
libxl__ao_device *aorm)
@@ -511,15 +597,15 @@ retry_transaction:
static void device_backend_callback(libxl__egc *egc, libxl__ev_devstate *ds,
int rc) {
- libxl__ao_device *aorm = CONTAINER_OF(ds, *aorm, ds);
- STATE_AO_GC(aorm->ao);
+ libxl__ao_device *aodev = CONTAINER_OF(ds, *aodev, ds);
+ STATE_AO_GC(aodev->ao);
- device_backend_cleanup(gc, aorm);
+ device_backend_cleanup(gc, aodev);
- if (rc == ERROR_TIMEDOUT && aorm->action == DEVICE_DISCONNECT &&
- !aorm->force) {
- aorm->force = 1;
- libxl__initiate_device_remove(egc, aorm);
+ if (rc == ERROR_TIMEDOUT && aodev->action == DEVICE_DISCONNECT &&
+ !aodev->force) {
+ aodev->force = 1;
+ libxl__initiate_device_remove(egc, aodev);
return;
}
@@ -527,26 +613,58 @@ static void device_backend_callback(libxl__egc *egc, libxl__ev_devstate *ds,
* but we shouldn't alarm the user if it's during
* domain destruction.
*/
- if (rc && aorm->action == DEVICE_CONNECT) {
+ if (rc && aodev->action == DEVICE_CONNECT) {
LOG(ERROR, "unable to connect device with path %s",
- libxl__device_backend_path(gc, aorm->dev));
+ 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, aorm->dev));
+ libxl__device_backend_path(gc, aodev->dev));
goto out;
}
out:
- aorm->rc = rc;
- aorm->callback(egc, aorm);
+ aodev->rc = rc;
+ aodev->callback(egc, aodev);
return;
}
-static void device_backend_cleanup(libxl__gc *gc, libxl__ao_device *aorm)
+static void device_backend_cleanup(libxl__gc *gc, libxl__ao_device *aodev)
{
- if (!aorm) return;
- libxl__ev_devstate_cancel(gc, &aorm->ds);
+ if (!aodev) return;
+ libxl__ev_devstate_cancel(gc, &aodev->ds);
+}
+
+static void device_remove_callback(libxl__egc *egc, libxl__ao_device *aorm)
+{
+ STATE_AO_GC(aorm->ao);
+ libxl__devices_remove_state *drs = CONTAINER_OF(aorm->base, *drs, aorm);
+ char *be_path = libxl__device_backend_path(gc, aorm->dev);
+ char *fe_path = libxl__device_frontend_path(gc, aorm->dev);
+ xs_transaction_t t = 0;
+ int rc = 0, last = 1;
+
+retry_transaction:
+ t = xs_transaction_start(CTX->xsh);
+ if (aorm->action == DEVICE_DISCONNECT) {
+ libxl__xs_path_cleanup(gc, t, fe_path);
+ libxl__xs_path_cleanup(gc, t, be_path);
+ }
+ if (!xs_transaction_end(CTX->xsh, t, 0)) {
+ if (errno == EAGAIN)
+ goto retry_transaction;
+ else {
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ }
+
+out:
+ rc = libxl__ao_device_check_last(gc, aorm, drs->aorm,
+ drs->num_devices, &last);
+ if (last)
+ drs->callback(egc, drs, rc);
+ return;
}
int libxl__wait_for_device_model(libxl__gc *gc,
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 95fd0ac..6604549 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);
@@ -891,12 +895,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);
@@ -1089,48 +1113,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 d2c013c..68d076c 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -796,7 +796,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);
/*
@@ -1771,6 +1770,10 @@ typedef void libxl__device_callback(libxl__egc*, libxl__ao_device*);
_hidden void libxl__init_ao_device(libxl__ao_device *aorm, libxl__ao *ao,
libxl__ao_device **base);
+_hidden int libxl__ao_device_check_last(libxl__gc *gc, libxl__ao_device *device,
+ libxl__ao_device *list, int num,
+ int *last);
+
struct libxl__ao_device {
/* filled in by user */
libxl__ao *ao;
@@ -1795,6 +1798,88 @@ struct libxl__ao_device {
_hidden void libxl__initiate_device_remove(libxl__egc *egc,
libxl__ao_device *aorm);
+/*----- Domain destruction -----*/
+
+/* Domain destruction has been splitted in two functions:
+ *
+ * libxl__domain_destroy is the main destroy function, it detects
+ * stubdoms and calls libxl__destroy_domid on the domain and it's
+ * 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;
+ /* force device destruction */
+ int force;
+ /* private */
+ libxl__ao_device *aorm;
+ int num_devices;
+};
+
+struct libxl__destroy_domid_state {
+ /* filled in by user */
+ libxl__ao *ao;
+ uint32_t domid;
+ libxl__domid_destroy_cb *callback;
+ /* force device destruction */
+ int force;
+ /* 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 */
+ 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. */
@@ -1828,6 +1913,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*);
@@ -1854,6 +1940,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 efaf3de..3c02b69 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1333,7 +1333,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:
@@ -1899,7 +1899,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)
@@ -2390,7 +2390,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); }
}
@@ -2664,7 +2664,7 @@ static int save_domain(const char *p, const char *filename, int checkpoint,
if (checkpoint)
libxl_domain_unpause(ctx, domid);
else
- libxl_domain_destroy(ctx, domid);
+ libxl_domain_destroy(ctx, domid, 0);
exit(0);
}
@@ -2896,7 +2896,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);
@@ -3014,7 +3014,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);
--
1.7.7.5 (Apple Git-26)
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 07/15] libxl: convert libxl_domain_destroy to an async op
2012-05-22 14:02 ` [PATCH v2 07/15] libxl: convert libxl_domain_destroy to an async op Roger Pau Monne
@ 2012-05-22 14:11 ` Roger Pau Monne
2012-05-22 17:01 ` Ian Jackson
1 sibling, 0 replies; 43+ messages in thread
From: Roger Pau Monne @ 2012-05-22 14:11 UTC (permalink / raw)
To: xen-devel@lists.xen.org; +Cc: Ian Jackson
Roger Pau Monne wrote:
> 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.
>
> 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 | 192 ++++++++++++++++++++++++++++++++++--------
> tools/libxl/libxl_dm.c | 85 ++++++++++---------
> tools/libxl/libxl_internal.h | 90 +++++++++++++++++++-
> tools/libxl/xl_cmdimpl.c | 12 ++--
> 7 files changed, 473 insertions(+), 105 deletions(-)
[ ...]
> @@ -413,30 +490,40 @@ 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);
> + printf("device: %s\n", path);
This should not be here.
> + GCNEW(dev);
> + if (path&& libxl__parse_backend_path(gc, path, dev) == 0) {
> + dev->domid = domid;
> + dev->kind = kind;
> + dev->devid = atoi(devs[j]);
> + drs->aorm[numdev].action = DEVICE_DISCONNECT;
> + drs->aorm[numdev].dev = dev;
> + drs->aorm[numdev].callback = device_remove_callback;
> + drs->aorm[numdev].force = drs->force;
> + libxl__initiate_device_remove(egc,&drs->aorm[numdev]);
> + numdev++;
> }
> }
> }
>
> /* console 0 frontend directory is not under /local/domain/<domid>/device */
> path = libxl__sprintf(gc, "/local/domain/%d/console/backend", domid);
> + printf("frontend: %s\n", path);
> path = libxl__xs_read(gc, XBT_NULL, path);
> + printf("backend: %s\n", path);
Neither should this. I will resend without this printfs, but please
ignore them for review.
[...]
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 07/15] libxl: convert libxl_domain_destroy to an async op
2012-05-22 14:02 ` [PATCH v2 07/15] libxl: convert libxl_domain_destroy to an async op Roger Pau Monne
2012-05-22 14:11 ` Roger Pau Monne
@ 2012-05-22 17:01 ` Ian Jackson
2012-05-22 17:30 ` Roger Pau Monne
2012-05-23 9:47 ` Roger Pau Monne
1 sibling, 2 replies; 43+ messages in thread
From: Ian Jackson @ 2012-05-22 17:01 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel@lists.xen.org
Roger Pau Monne writes ("[PATCH v2 07/15] 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 for what is in general an admirably clear patch for such a
substantial change.
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index b13de4f..b44ae75 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
...
> +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 (dds->domain_finished)
> + dds->callback(egc, dds, rc);
A suggestion, which you should feel free to reject. You might like to
concentrate this "have we done all this" logic in one place. So
instead of the above,
destroy_finish_check(egc, dds);
}
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);
}
Also thinking about it like this exposes the fact that you aren't
consistent about how to aggregate the rc values from the two destroy
operations. You give the caller the rc from whichever one finished
last, which doesn't seem like it's correct.
> /* generic callback for devices that only need to set ao_complete */
> -static void libxl__device_cb(libxl__egc *egc, libxl__ao_device *aorm)
> +static void libxl__device_cb(libxl__egc *egc, libxl__ao_device *aodev)
> {
> - STATE_AO_GC(aorm->ao);
> + STATE_AO_GC(aodev->ao);
I think it would be clearer if this use of "aodev" rather than "aorm"
etc. was already in the previous patch, rather than appearing here.
Likewise later in libxl__init_ao_device, etc.
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index d2c013c..68d076c 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
...
> +_hidden int libxl__ao_device_check_last(libxl__gc *gc, libxl__ao_device *device,
> + libxl__ao_device *list, int num,
> + int *last);
> +
This is a good helper but it could do with a doc comment explaining
what it does.
Just a suggestion: rather than this business with *last it might be
easier to assign a magic error code, or use +1, for "not all done
yet". (+1 would work since all actual libxl error codes are -ve).
By my reading of the current interface the return value is not
meaningful if *last==0 on output, which is the opposite of the usual
way round.
> @@ -1795,6 +1798,88 @@ struct libxl__ao_device {
> _hidden void libxl__initiate_device_remove(libxl__egc *egc,
> libxl__ao_device *aorm);
>
> +/*----- Domain destruction -----*/
> +
> +/* Domain destruction has been splitted in two functions:
"... has been split into two functions:"
> + * libxl__domain_destroy is the main destroy function, it detects
"... function, which detects ..." or "function. It detects ..."
> + * stubdoms and calls libxl__destroy_domid on the domain and it's
"its stubdom", not "it's"
> + * stubdom if present, creating a different libxl__destroy_domid_state
> + * for each one of them.
...
> +struct libxl__devices_remove_state {
> + /* filled in by user */
> + libxl__ao *ao;
> + uint32_t domid;
> + libxl__devices_remove_callback *callback;
> + /* force device destruction */
> + int force;
"force device destruction" is falling somewhat into the vacuous
comment antipattern. We know that "force" forces it we knew the
operation was removal.
It would be better to cross-reference this to the public API. For
example
int force; /* libxl_device_TYPE_destroy rather than _remove */
and then rely on the public API documentation to describe the
semantics.
> + printf("device: %s\n", path);
> + GCNEW(dev);
> + if (path && libxl__parse_backend_path(gc, path, dev) == 0) {
> + dev->domid = domid;
> + dev->kind = kind;
> + dev->devid = atoi(devs[j]);
> + drs->aorm[numdev].action = DEVICE_DISCONNECT;
> + drs->aorm[numdev].dev = dev;
> + drs->aorm[numdev].callback = device_remove_callback;
> + drs->aorm[numdev].force = drs->force;
A suggestion: do you want a helper variable
libxl__ao_device *aodev = &drs->aorm[numdev];
so that you can write
aodev->action = DEVICE_DISCONNECT;
etc. ?
> +static void device_remove_callback(libxl__egc *egc, libxl__ao_device *aorm)
> +{
> + STATE_AO_GC(aorm->ao);
> + libxl__devices_remove_state *drs = CONTAINER_OF(aorm->base, *drs, aorm);
> + char *be_path = libxl__device_backend_path(gc, aorm->dev);
> + char *fe_path = libxl__device_frontend_path(gc, aorm->dev);
> + xs_transaction_t t = 0;
> + int rc = 0, last = 1;
> +
> +retry_transaction:
> + t = xs_transaction_start(CTX->xsh);
> + if (aorm->action == DEVICE_DISCONNECT) {
> + libxl__xs_path_cleanup(gc, t, fe_path);
> + libxl__xs_path_cleanup(gc, t, be_path);
> + }
> + if (!xs_transaction_end(CTX->xsh, t, 0)) {
> + if (errno == EAGAIN)
> + goto retry_transaction;
> + else {
> + rc = ERROR_FAIL;
> + goto out;
> + }
> + }
> +
> +out:
> + rc = libxl__ao_device_check_last(gc, aorm, drs->aorm,
> + drs->num_devices, &last);
> + if (last)
> + drs->callback(egc, drs, rc);
> + return;
I don't understand why this is here in this patch. Surely it belongs
in the previous one ?
Thanks,
Ian.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 07/15] libxl: convert libxl_domain_destroy to an async op
2012-05-22 17:01 ` Ian Jackson
@ 2012-05-22 17:30 ` Roger Pau Monne
2012-05-22 17:48 ` Ian Jackson
2012-05-23 9:47 ` Roger Pau Monne
1 sibling, 1 reply; 43+ messages in thread
From: Roger Pau Monne @ 2012-05-22 17:30 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel@lists.xen.org
Ian Jackson wrote:
[...]
I will apply the block of changes that I've removed from this reply.
>> +static void device_remove_callback(libxl__egc *egc, libxl__ao_device *aorm)
>> +{
>> + STATE_AO_GC(aorm->ao);
>> + libxl__devices_remove_state *drs = CONTAINER_OF(aorm->base, *drs, aorm);
>> + char *be_path = libxl__device_backend_path(gc, aorm->dev);
>> + char *fe_path = libxl__device_frontend_path(gc, aorm->dev);
>> + xs_transaction_t t = 0;
>> + int rc = 0, last = 1;
>> +
>> +retry_transaction:
>> + t = xs_transaction_start(CTX->xsh);
>> + if (aorm->action == DEVICE_DISCONNECT) {
>> + libxl__xs_path_cleanup(gc, t, fe_path);
>> + libxl__xs_path_cleanup(gc, t, be_path);
>> + }
>> + if (!xs_transaction_end(CTX->xsh, t, 0)) {
>> + if (errno == EAGAIN)
>> + goto retry_transaction;
>> + else {
>> + rc = ERROR_FAIL;
>> + goto out;
>> + }
>> + }
>> +
>> +out:
>> + rc = libxl__ao_device_check_last(gc, aorm, drs->aorm,
>> + drs->num_devices,&last);
>> + if (last)
>> + drs->callback(egc, drs, rc);
>> + return;
>
> I don't understand why this is here in this patch. Surely it belongs
> in the previous one ?
Not really, because libxl__destroy_devices is called from
libxl_domain_destroy, which does not ao until this patch, so this
callback is the libxl__devices_destroy one (which was not async in
previous patches), since it called libxl__device_destroy instead of
libxl__initiate_device_remove.
Thanks for the review.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 07/15] libxl: convert libxl_domain_destroy to an async op
2012-05-22 17:30 ` Roger Pau Monne
@ 2012-05-22 17:48 ` Ian Jackson
0 siblings, 0 replies; 43+ messages in thread
From: Ian Jackson @ 2012-05-22 17:48 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel@lists.xen.org
Roger Pau Monne writes ("Re: [PATCH v2 07/15] libxl: convert libxl_domain_destroy to an async op"):
> Ian Jackson wrote:
>
> [...]
>
> I will apply the block of changes that I've removed from this reply.
>
> >> +static void device_remove_callback(libxl__egc *egc, libxl__ao_device *aorm)
...
> > I don't understand why this is here in this patch. Surely it belongs
> > in the previous one ?
>
> Not really, because libxl__destroy_devices is called from
> libxl_domain_destroy, which does not ao until this patch, so this
> callback is the libxl__devices_destroy one (which was not async in
> previous patches), since it called libxl__device_destroy instead of
> libxl__initiate_device_remove.
Oh, yes, of course. Thanks.
Ian.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 07/15] libxl: convert libxl_domain_destroy to an async op
2012-05-22 17:01 ` Ian Jackson
2012-05-22 17:30 ` Roger Pau Monne
@ 2012-05-23 9:47 ` Roger Pau Monne
2012-05-23 10:45 ` Ian Jackson
1 sibling, 1 reply; 43+ messages in thread
From: Roger Pau Monne @ 2012-05-23 9:47 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel@lists.xen.org
Ian Jackson wrote:
>> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
>> index d2c013c..68d076c 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
> ...
>> +_hidden int libxl__ao_device_check_last(libxl__gc *gc, libxl__ao_device *device,
>> + libxl__ao_device *list, int num,
>> + int *last);
>> +
>
> This is a good helper but it could do with a doc comment explaining
> what it does.
>
> Just a suggestion: rather than this business with *last it might be
> easier to assign a magic error code, or use +1, for "not all done
> yet". (+1 would work since all actual libxl error codes are -ve).
> By my reading of the current interface the return value is not
> meaningful if *last==0 on output, which is the opposite of the usual
> way round.
This function currently returns two different things, the return value
returns the rc code of any operation that has failed, and the parameter
last marks if all device events are finished. Without the last
parameter, we won't be able to return the failed error code if all
devices had finished, since we had to return 0, so the error code would
be lost.
Since I think this is not really clear, I will split this function into
two different ones, one that checks if all devices are finished, and
another one that checks if some devices have reported errors.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 07/15] libxl: convert libxl_domain_destroy to an async op
2012-05-23 9:47 ` Roger Pau Monne
@ 2012-05-23 10:45 ` Ian Jackson
0 siblings, 0 replies; 43+ messages in thread
From: Ian Jackson @ 2012-05-23 10:45 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel@lists.xen.org
Roger Pau Monne writes ("Re: [PATCH v2 07/15] libxl: convert libxl_domain_destroy to an async op"):
> Ian Jackson wrote:
> > Just a suggestion: rather than this business with *last it might be
> > easier to assign a magic error code, or use +1, for "not all done
> > yet". (+1 would work since all actual libxl error codes are -ve).
> > By my reading of the current interface the return value is not
> > meaningful if *last==0 on output, which is the opposite of the usual
> > way round.
>
> This function currently returns two different things, the return value
> returns the rc code of any operation that has failed, and the parameter
> last marks if all device events are finished. Without the last
> parameter, we won't be able to return the failed error code if all
> devices had finished, since we had to return 0, so the error code would
> be lost.
I was suggesting that it should return:
rc==0 if all devices completed successfully
rc==ERROR_SOMETHING if all devices completed with at least one error
rc==+1 if something is still going
Or if you prefer
rc==ERROR_OPERATION_NOT_YET_COMPLETE if something is still going
> Since I think this is not really clear, I will split this function into
> two different ones, one that checks if all devices are finished, and
> another one that checks if some devices have reported errors.
Please don't do that - you'll have to turn it into two loops and it'll
be a lot more fiddly.
Ian.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 08/15] libxl: convert libxl_device_disk_add to an asyn op
2012-05-22 14:02 [PATCH v2 0/15] execute hotplug scripts from libxl Roger Pau Monne
` (6 preceding siblings ...)
2012-05-22 14:02 ` [PATCH v2 07/15] libxl: convert libxl_domain_destroy to an async op Roger Pau Monne
@ 2012-05-22 14:02 ` Roger Pau Monne
2012-05-22 17:04 ` Ian Jackson
2012-05-22 14:02 ` [PATCH v2 09/15] libxl: convert libxl_device_nic_add to an async operation Roger Pau Monne
` (7 subsequent siblings)
15 siblings, 1 reply; 43+ messages in thread
From: Roger Pau Monne @ 2012-05-22 14:02 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.
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 | 57 +++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_dm.c | 61 +++++++++++++++++++++++++++++++----------
tools/libxl/libxl_internal.h | 26 ++++++++++++++++++
tools/libxl/xl_cmdimpl.c | 2 +-
8 files changed, 204 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 b44ae75..46d13a7 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1480,13 +1480,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__init_ao_device(device, ao, NULL);
+ device->callback = libxl__device_cb;
+ 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 *aoadd)
+{
+ STATE_AO_GC(aoadd->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);
@@ -1510,7 +1528,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);
@@ -1528,7 +1547,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);
@@ -1549,7 +1568,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);
@@ -1581,22 +1600,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));
+ aoadd->dev = device;
+ aoadd->action = DEVICE_CONNECT;
+ libxl__initiate_device_add(egc, aoadd);
+
rc = 0;
out_free:
flexarray_free(back);
flexarray_free(front);
out:
- GC_FREE;
- return rc;
+ aoadd->rc = rc;
+ if (rc) aoadd->callback(egc, aoadd);
+ return;
}
int libxl_device_disk_remove(libxl_ctx *ctx, uint32_t domid,
@@ -1850,11 +1874,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 39a8c58..c14e832 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -663,7 +663,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 63f34fa..4378f48 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 *aorm);
+
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 *aorm)
+{
+ STATE_AO_GC(aorm->ao);
+ libxl__domain_create_state *dcs = CONTAINER_OF(aorm->base, *dcs, devices);
+ int i, last, 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, aorm, dcs->devices,
+ dcs->num_devices, &last);
+ if (!last) 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 020e934..8058afa 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 ret;
}
+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__init_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,45 @@ 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 *aoadd)
+{
+ STATE_AO_GC(aoadd->ao);
+ char *be_path = libxl__device_backend_path(gc, aoadd->dev);
+ char *state_path = libxl__sprintf(gc, "%s/state", be_path);
+ char *state = libxl__xs_read(gc, XBT_NULL, state_path);
+ int rc = 0;
+
+ if (aoadd->dev->backend_kind == LIBXL__DEVICE_KIND_QDISK) {
+ aoadd->callback(egc, aoadd);
+ return;
+ }
+
+ if (atoi(state) == XenbusStateInitWait)
+ goto out_ok;
+
+ libxl__ev_devstate_init(&aoadd->ds);
+ rc = libxl__ev_devstate_wait(gc, &aoadd->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);
+ aoadd->rc = rc;
+ aoadd->callback(egc, aoadd);
+ return;
+
+out_ok:
+ assert(!rc);
+ aoadd->callback(egc, aoadd);
+ return;
+}
+
void libxl__initiate_device_remove(libxl__egc *egc,
libxl__ao_device *aorm)
{
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 6604549..670d1dd 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 *aorm);
+
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 *aorm)
+{
+ STATE_AO_GC(aorm->ao);
+ libxl_ctx *ctx = libxl__gc_owner(gc);
+ libxl__stub_dm_spawn_state *sdss = CONTAINER_OF(aorm->base, *sdss, devices);
+ int i, num_console = STUBDOM_SPECIAL_CONSOLES, ret = 0, last;
+ 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, aorm, sdss->devices,
+ sdss->num_devices, &last);
+ if (!last) 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.guest_domid = dm_domid;
@@ -869,11 +903,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 68d076c..a5fc092 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
@@ -1790,6 +1791,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 *aorm);
+
+/* 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
+ * aorm->callback will be called.
+ */
+_hidden void libxl__initiate_device_add(libxl__egc*, libxl__ao_device *aorm);
+
+
/* 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
@@ -1880,6 +1894,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. */
@@ -1914,6 +1934,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*);
@@ -1942,6 +1965,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 3c02b69..69ce45a 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5081,7 +5081,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] 43+ messages in thread* Re: [PATCH v2 08/15] libxl: convert libxl_device_disk_add to an asyn op
2012-05-22 14:02 ` [PATCH v2 08/15] libxl: convert libxl_device_disk_add to an asyn op Roger Pau Monne
@ 2012-05-22 17:04 ` Ian Jackson
0 siblings, 0 replies; 43+ messages in thread
From: Ian Jackson @ 2012-05-22 17:04 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel@lists.xen.org
Roger Pau Monne writes ("[PATCH v2 08/15] libxl: convert libxl_device_disk_add to an asyn op"):
> 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.
...
> +void libxl__initiate_device_add(libxl__egc *egc, libxl__ao_device *aoadd)
> +{
> + STATE_AO_GC(aoadd->ao);
> + char *be_path = libxl__device_backend_path(gc, aoadd->dev);
> + char *state_path = libxl__sprintf(gc, "%s/state", be_path);
> + char *state = libxl__xs_read(gc, XBT_NULL, state_path);
> + int rc = 0;
Do you really need to do the xenstore state read here ? Surely
libxl__ev_devstate_wait will do it for you.
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 68d076c..a5fc092 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
...
> +/* 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 *aorm);
> +
> +/* 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
> + * aorm->callback will be called.
> + */
You really can't call this an aorm if it's being used for device
addition :-). Can we call this "aodev" everywhere, right from the
beginning ?
Thanks,
Ian.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 09/15] libxl: convert libxl_device_nic_add to an async operation
2012-05-22 14:02 [PATCH v2 0/15] execute hotplug scripts from libxl Roger Pau Monne
` (7 preceding siblings ...)
2012-05-22 14:02 ` [PATCH v2 08/15] libxl: convert libxl_device_disk_add to an asyn op Roger Pau Monne
@ 2012-05-22 14:02 ` Roger Pau Monne
2012-05-22 17:06 ` Ian Jackson
2012-05-22 14:02 ` [PATCH v2 10/15] libxl: add option to choose who executes hotplug scripts Roger Pau Monne
` (6 subsequent siblings)
15 siblings, 1 reply; 43+ messages in thread
From: Roger Pau Monne @ 2012-05-22 14:02 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 46d13a7..1a04f7a 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2017,12 +2017,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__init_ao_device(device, ao, NULL);
+ device->callback = libxl__device_cb;
+ 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 *aoadd)
+{
+ STATE_AO_GC(aoadd->ao);
flexarray_t *front;
flexarray_t *back;
- libxl__device device;
+ libxl__device *device;
char *dompath, **l;
unsigned int nb, rc;
@@ -2053,7 +2068,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");
@@ -2094,6 +2110,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));
@@ -2104,18 +2123,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 */
+ aoadd->dev = device;
+ aoadd->action = DEVICE_CONNECT;
+ libxl__initiate_device_add(egc, aoadd);
+
rc = 0;
out_free:
flexarray_free(back);
flexarray_free(front);
out:
- GC_FREE;
- return rc;
+ aoadd->rc = rc;
+ if (rc) aoadd->callback(egc, aoadd);
+ return;
}
int libxl_device_nic_remove(libxl_ctx *ctx, uint32_t domid,
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index c14e832..0b15586 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -691,7 +691,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 4378f48..076f755 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 *aorm);
+static void domcreate_nic_connected(libxl__egc *egc, libxl__ao_device *aorm);
+
+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 *aorm)
}
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 *aorm)
+{
+ STATE_AO_GC(aorm->ao);
+ libxl__domain_create_state *dcs = CONTAINER_OF(aorm->base, *dcs, devices);
+ int last, ret = 0;
+
+ ret = libxl__ao_device_check_last(gc, aorm, dcs->devices,
+ dcs->num_devices, &last);
+
+ if (!last) return;
+ if (ret) {
+ LOGE(ERROR, "error connecting nics devices");
+ goto error_out;
+ }
+
+ domcreate_attach_pci(egc, dcs);
+ return;
+
+error_out:
+ assert(ret);
+ domcreate_complete(egc, dcs, ret);
+}
+
+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 8058afa..91ec69f 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__init_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 670d1dd..034ca8e 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 *aorm);
+static void stubdom_nics_connected(libxl__egc *egc, libxl__ao_device *aorm);
+
+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 *aorm)
}
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)
@@ -918,9 +926,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 *aoadd)
+{
+ STATE_AO_GC(aoadd->ao);
+ libxl__stub_dm_spawn_state *sdss =
+ CONTAINER_OF(aoadd->base, *sdss, devices);
+ int last, ret = 0;
+
+ ret = libxl__ao_device_check_last(gc, aoadd, sdss->devices,
+ sdss->num_devices, &last);
+
+ if (!last) return;
+ if (ret)
+ LOGE(ERROR, "error connecting nics devices");
+
+ stubdom_pvqemu_cb(egc, &sdss->pvqemu, ret);
+ 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 a5fc092..d303123 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1796,6 +1796,11 @@ _hidden void libxl__device_disk_add(libxl__egc *egc, uint32_t domid,
libxl_device_disk *disk,
libxl__ao_device *aorm);
+/* 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 *aorm);
+
/* 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
@@ -1900,6 +1905,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 69ce45a..46af9fb 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4970,7 +4970,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] 43+ messages in thread* Re: [PATCH v2 09/15] libxl: convert libxl_device_nic_add to an async operation
2012-05-22 14:02 ` [PATCH v2 09/15] libxl: convert libxl_device_nic_add to an async operation Roger Pau Monne
@ 2012-05-22 17:06 ` Ian Jackson
2012-05-23 10:24 ` Roger Pau Monne
0 siblings, 1 reply; 43+ messages in thread
From: Ian Jackson @ 2012-05-22 17:06 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel@lists.xen.org
Roger Pau Monne writes ("[PATCH v2 09/15] 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.
Isn't this patch very very similar to the previous one, just with
"nic" instead of "disk" in a lot of places ?
I'm allergic to repetition and these parts of the series seem to be
adding a lot of near-copies of the same code.
Ian.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 09/15] libxl: convert libxl_device_nic_add to an async operation
2012-05-22 17:06 ` Ian Jackson
@ 2012-05-23 10:24 ` Roger Pau Monne
0 siblings, 0 replies; 43+ messages in thread
From: Roger Pau Monne @ 2012-05-23 10:24 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel@lists.xen.org
Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH v2 09/15] 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.
>
> Isn't this patch very very similar to the previous one, just with
> "nic" instead of "disk" in a lot of places ?
>
> I'm allergic to repetition and these parts of the series seem to be
> adding a lot of near-copies of the same code.
Yes, it's mostly the same, but on different points of domain/stubdomain
creation.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 10/15] libxl: add option to choose who executes hotplug scripts
2012-05-22 14:02 [PATCH v2 0/15] execute hotplug scripts from libxl Roger Pau Monne
` (8 preceding siblings ...)
2012-05-22 14:02 ` [PATCH v2 09/15] libxl: convert libxl_device_nic_add to an async operation Roger Pau Monne
@ 2012-05-22 14:02 ` Roger Pau Monne
2012-05-22 17:13 ` Ian Jackson
2012-05-22 14:02 ` [PATCH v2 11/15] libxl: set nic type to VIF by default Roger Pau Monne
` (5 subsequent siblings)
15 siblings, 1 reply; 43+ messages in thread
From: Roger Pau Monne @ 2012-05-22 14:02 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 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 | 31 ++++++++++++++++++++++++++++++-
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, 72 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 076f755..15f94bd 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,35 @@ 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 (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..bdf14d5 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 d303123..dfbeb75 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]))
@@ -1386,6 +1387,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 551e367..b1351de 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 d4db1f8..d992c32 100644
--- a/tools/libxl/xl.c
+++ b/tools/libxl/xl.c
@@ -35,6 +35,7 @@
xentoollog_logger_stdiostream *logger;
int dryrun_only;
int autoballoon = 1;
+libxl_defbool run_hotplug_scripts;
char *lockfile;
char *default_vifscript = NULL;
char *default_bridge = NULL;
@@ -66,6 +67,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 2b6714a..43d727a 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -109,6 +109,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 46af9fb..c13c48b 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -562,6 +562,7 @@ static void parse_config_data(const char *configfile_filename_report,
}
}
+ 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] 43+ messages in thread* Re: [PATCH v2 10/15] libxl: add option to choose who executes hotplug scripts
2012-05-22 14:02 ` [PATCH v2 10/15] libxl: add option to choose who executes hotplug scripts Roger Pau Monne
@ 2012-05-22 17:13 ` Ian Jackson
0 siblings, 0 replies; 43+ messages in thread
From: Ian Jackson @ 2012-05-22 17:13 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel@lists.xen.org
Roger Pau Monne writes ("[PATCH v2 10/15] libxl: add option to choose who executes hotplug scripts"):
> 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.
>
> + int hotplug_setting = libxl__hotplug_settings(gc, t);
This function sometimes returns a non-negative boolean, and sometimes
a (negative) libxl error code. That doesn't seem to be handled
correctly here at the call site. (And it should be mentioned in a doc
comment.)
> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index b89aef7..bdf14d5 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);
This is a bit hazardous. What if the value in xenstore is "42" or
"-3" ? At the very least I think you should write:
rc = !!atoi(val);
Thanks,
Ian.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 11/15] libxl: set nic type to VIF by default
2012-05-22 14:02 [PATCH v2 0/15] execute hotplug scripts from libxl Roger Pau Monne
` (9 preceding siblings ...)
2012-05-22 14:02 ` [PATCH v2 10/15] libxl: add option to choose who executes hotplug scripts Roger Pau Monne
@ 2012-05-22 14:02 ` Roger Pau Monne
2012-05-22 14:02 ` [PATCH v2 12/15] libxl: call hotplug scripts for disk devices from libxl Roger Pau Monne
` (4 subsequent siblings)
15 siblings, 0 replies; 43+ messages in thread
From: Roger Pau Monne @ 2012-05-22 14:02 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 1a04f7a..8faf79a 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1999,7 +1999,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] 43+ messages in thread* [PATCH v2 12/15] libxl: call hotplug scripts for disk devices from libxl
2012-05-22 14:02 [PATCH v2 0/15] execute hotplug scripts from libxl Roger Pau Monne
` (10 preceding siblings ...)
2012-05-22 14:02 ` [PATCH v2 11/15] libxl: set nic type to VIF by default Roger Pau Monne
@ 2012-05-22 14:02 ` Roger Pau Monne
2012-05-22 17:40 ` Ian Jackson
2012-05-22 14:02 ` [PATCH v2 13/15] libxl: call hotplug scripts for nic " Roger Pau Monne
` (3 subsequent siblings)
15 siblings, 1 reply; 43+ messages in thread
From: Roger Pau Monne @ 2012-05-22 14:02 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 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 | 124 ++++++++++++++++++++++++++++-
tools/libxl/libxl_internal.h | 20 +++++
tools/libxl/libxl_linux.c | 97 ++++++++++++++++++++++
6 files changed, 258 insertions(+), 5 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 8faf79a..8fe0d17 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1547,6 +1547,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:
@@ -1562,6 +1567,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 91ec69f..d7dd7a3 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 *aoadd)
{
STATE_AO_GC(aoadd->ao);
@@ -604,7 +612,7 @@ out_fail:
out_ok:
assert(!rc);
- aoadd->callback(egc, aoadd);
+ device_hotplug(egc, aoadd);
return;
}
@@ -666,7 +674,7 @@ retry_transaction:
out_ok:
if (t) xs_transaction_end(ctx->xsh, t, 0);
- aorm->callback(egc, aorm);
+ device_hotplug(egc, aorm);
return;
}
@@ -698,6 +706,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);
@@ -710,6 +721,115 @@ 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 (!aodev) return;
+ 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 *aorm)
{
STATE_AO_GC(aorm->ao);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index dfbeb75..4af35cb 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"
@@ -1792,6 +1793,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 */
@@ -1820,6 +1826,20 @@ _hidden void libxl__initiate_device_add(libxl__egc*, libxl__ao_device *aorm);
_hidden void libxl__initiate_device_remove(libxl__egc *egc,
libxl__ao_device *aorm);
+/*
+ * 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 splitted in two functions:
diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index 925248b..83b85b3 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -25,3 +25,100 @@ 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;
+
+ 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, 9);
+ 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;
+
+ 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;
+
+ 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, 3);
+
+ (*args)[nr++] = script;
+ (*args)[nr++] = action == DEVICE_CONNECT ? "add" : "remove";
+ (*args)[nr++] = NULL;
+
+ 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;
+}
--
1.7.7.5 (Apple Git-26)
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 12/15] libxl: call hotplug scripts for disk devices from libxl
2012-05-22 14:02 ` [PATCH v2 12/15] libxl: call hotplug scripts for disk devices from libxl Roger Pau Monne
@ 2012-05-22 17:40 ` Ian Jackson
0 siblings, 0 replies; 43+ messages in thread
From: Ian Jackson @ 2012-05-22 17:40 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel@lists.xen.org
Roger Pau Monne writes ("[PATCH v2 12/15] 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.
>
> 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.
...
> +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 (!aodev) return;
Uh, what is this for ? How can aodev be null ?
> +
> +/* Hotplug scripts helpers */
> +
> +static char **get_hotplug_env(libxl__gc *gc, libxl__device *dev)
> +{
...
How about, to avoid mistakes with the array size:
int arraysize = 9;
> +
> + GCNEW_ARRAY(env, 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);
We should really have something better than this as there is a lot of
this kind of thing in libxl but now is not the time.
Ian.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 13/15] libxl: call hotplug scripts for nic devices from libxl
2012-05-22 14:02 [PATCH v2 0/15] execute hotplug scripts from libxl Roger Pau Monne
` (11 preceding siblings ...)
2012-05-22 14:02 ` [PATCH v2 12/15] libxl: call hotplug scripts for disk devices from libxl Roger Pau Monne
@ 2012-05-22 14:02 ` Roger Pau Monne
2012-05-22 17:45 ` Ian Jackson
2012-05-22 14:02 ` [PATCH v2 14/15] libxl: use libxl__xs_path_cleanup on device_destroy Roger Pau Monne
` (2 subsequent siblings)
15 siblings, 1 reply; 43+ messages in thread
From: Roger Pau Monne @ 2012-05-22 14:02 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 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 | 34 +++++++++++++-
tools/libxl/libxl_internal.h | 5 ++-
tools/libxl/libxl_linux.c | 84 ++++++++++++++++++++++++++++++++-
4 files changed, 122 insertions(+), 7 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 d7dd7a3..b7bcffa 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -100,6 +100,26 @@ out:
return numdevs;
}
+libxl_nic_type libxl__nic_type(libxl__gc *gc, libxl__device *dev)
+{
+ char *snictype, *be_path;
+ libxl_nic_type nictype;
+
+ 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);
+ return -1;
+ }
+ if (libxl_nic_type_from_string(snictype, &nictype) < 0) {
+ LOGE(ERROR, "unable to parse nictype from %s", be_path);
+ return -1;
+ }
+
+ return nictype;
+}
+
int libxl__device_generic_add(libxl__gc *gc, libxl__device *device,
char **bents, char **fents)
{
@@ -732,7 +752,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->vif_executed);
switch (hotplug) {
case 0:
/* no hotplug script to execute */
@@ -806,7 +827,18 @@ 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 &&
+ libxl__nic_type(gc, aodev->dev) == LIBXL_NIC_TYPE_IOEMU &&
+ !aodev->vif_executed) {
+ aodev->vif_executed = 1;
+ 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 4af35cb..68a6122 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -800,6 +800,7 @@ _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 libxl_nic_type libxl__nic_type(libxl__gc *gc, libxl__device *dev);
/*
* For each aggregate type which can be used as an input we provide:
@@ -1796,6 +1797,7 @@ struct libxl__ao_device {
/* device hotplug execution */
pid_t pid;
char *what;
+ int vif_executed;
libxl__ev_time ev;
libxl__ev_child child;
};
@@ -1838,7 +1840,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 vif_executed);
/*----- Domain destruction -----*/
diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index 83b85b3..bd37f7f 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -43,7 +43,7 @@ static char **get_hotplug_env(libxl__gc *gc, libxl__device *dev)
return NULL;
}
- GCNEW_ARRAY(env, 9);
+ GCNEW_ARRAY(env, 13);
env[nr++] = "script";
env[nr++] = script;
env[nr++] = "XENBUS_TYPE";
@@ -52,6 +52,24 @@ 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) {
+ switch (libxl__nic_type(gc, dev)) {
+ 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;
return env;
@@ -59,6 +77,63 @@ static char **get_hotplug_env(libxl__gc *gc, libxl__device *dev)
/* Hotplug scripts caller functions */
+static int libxl__hotplug_nic(libxl__gc *gc, libxl__device *dev,
+ char ***args, char ***env,
+ libxl__device_action action, int vif_executed)
+{
+ char *be_path = libxl__device_backend_path(gc, dev);
+ char *script;
+ int nr = 0, rc = 0;
+ 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);
+ return -1;
+ }
+
+ nictype = libxl__nic_type(gc, dev);
+ if (nictype < 0) {
+ 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, 4);
+ (*args)[nr++] = script;
+
+ switch (nictype) {
+ case LIBXL_NIC_TYPE_IOEMU:
+ if (!vif_executed) 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;
+ }
+
+ rc = 0;
+
+out:
+ return rc;
+}
+
static int libxl__hotplug_disk(libxl__gc *gc, libxl__device *dev,
char ***args, char ***env,
libxl__device_action action)
@@ -95,7 +170,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 vif_executed)
{
char *disable_udev = libxl__xs_read(gc, XBT_NULL, DISABLE_UDEV_PATH);
int rc;
@@ -111,6 +187,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, vif_executed);
+ 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] 43+ messages in thread* Re: [PATCH v2 13/15] libxl: call hotplug scripts for nic devices from libxl
2012-05-22 14:02 ` [PATCH v2 13/15] libxl: call hotplug scripts for nic " Roger Pau Monne
@ 2012-05-22 17:45 ` Ian Jackson
2012-05-23 15:14 ` Roger Pau Monne
0 siblings, 1 reply; 43+ messages in thread
From: Ian Jackson @ 2012-05-22 17:45 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel@lists.xen.org
Roger Pau Monne writes ("[PATCH v2 13/15] 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.
> +libxl_nic_type libxl__nic_type(libxl__gc *gc, libxl__device *dev)
> +{
> + char *snictype, *be_path;
> + libxl_nic_type nictype;
> +
> + 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);
> + return -1;
Please let us not have any more functions that return -1. Do you mean
ERROR_FAIL ?
> @@ -806,7 +827,18 @@ 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 &&
> + libxl__nic_type(gc, aodev->dev) == LIBXL_NIC_TYPE_IOEMU &&
And you need to handle the error here.
> + !aodev->vif_executed) {
> + aodev->vif_executed = 1;
> + device_hotplug(egc, aodev);
> + return;
> + }
This logic surrounding vif_executed is rather opaque. Are you trying
to run this whole lot twice so that you can run two lots of scripts ?
If so then perhaps the hotplug helper should simply take a counter, so
that we don't expose this vif abstraction thing ? And it would be
applicable to everything, not just vifs.
Ian.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 13/15] libxl: call hotplug scripts for nic devices from libxl
2012-05-22 17:45 ` Ian Jackson
@ 2012-05-23 15:14 ` Roger Pau Monne
0 siblings, 0 replies; 43+ messages in thread
From: Roger Pau Monne @ 2012-05-23 15:14 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel@lists.xen.org
Ian Jackson wrote:
>> + !aodev->vif_executed) {
>> + aodev->vif_executed = 1;
>> + device_hotplug(egc, aodev);
>> + return;
>> + }
>
> This logic surrounding vif_executed is rather opaque. Are you trying
> to run this whole lot twice so that you can run two lots of scripts ?
>
> If so then perhaps the hotplug helper should simply take a counter, so
> that we don't expose this vif abstraction thing ? And it would be
> applicable to everything, not just vifs.
Yes, it's kind of a counter, but first and second calls have different
arguments/env. The fact is that from my point of view they should be two
different devices (vif and tap), but that will require major changes in
libxl.
I could change vif_executed to something like exec_num, but it won't
make much sense anyway I think, and we will have to pass it to the
hotplug helper the same way we are passing vif_executed.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 14/15] libxl: use libxl__xs_path_cleanup on device_destroy
2012-05-22 14:02 [PATCH v2 0/15] execute hotplug scripts from libxl Roger Pau Monne
` (12 preceding siblings ...)
2012-05-22 14:02 ` [PATCH v2 13/15] libxl: call hotplug scripts for nic " Roger Pau Monne
@ 2012-05-22 14:02 ` Roger Pau Monne
2012-05-22 17:46 ` Ian Jackson
2012-05-22 14:02 ` [PATCH v2 15/15] libxl: add dummy netbsd functions Roger Pau Monne
2012-05-23 10:07 ` [PATCH v2 0/15] execute hotplug scripts from libxl Ian Campbell
15 siblings, 1 reply; 43+ messages in thread
From: Roger Pau Monne @ 2012-05-22 14:02 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.
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
---
tools/libxl/libxl_device.c | 20 ++++++++++++++++----
1 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index b7bcffa..b8ef9b4 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -484,16 +484,28 @@ 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);
+retry_transaction:
+ t = xs_transaction_start(CTX->xsh);
+ libxl__xs_path_cleanup(gc, t, fe_path);
+ libxl__xs_path_cleanup(gc, t, be_path);
+ if (!xs_transaction_end(CTX->xsh, t, 0)) {
+ if (errno == EAGAIN)
+ goto retry_transaction;
+ else {
+ rc = ERROR_FAIL;
+ 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] 43+ messages in thread* [PATCH v2 15/15] libxl: add dummy netbsd functions
2012-05-22 14:02 [PATCH v2 0/15] execute hotplug scripts from libxl Roger Pau Monne
` (13 preceding siblings ...)
2012-05-22 14:02 ` [PATCH v2 14/15] libxl: use libxl__xs_path_cleanup on device_destroy Roger Pau Monne
@ 2012-05-22 14:02 ` Roger Pau Monne
2012-05-22 17:47 ` Ian Jackson
2012-05-22 17:47 ` Ian Jackson
2012-05-23 10:07 ` [PATCH v2 0/15] execute hotplug scripts from libxl Ian Campbell
15 siblings, 2 replies; 43+ messages in thread
From: Roger Pau Monne @ 2012-05-22 14:02 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Roger Pau Monne
Add dummy NetBSD functions related to hotplug scripts, so compilation
doesn't fail. This functions will be filled in a following series.
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
---
tools/libxl/libxl_netbsd.c | 10 ++++++++++
1 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
index 9e0ed6d..75fbce1 100644
--- a/tools/libxl/libxl_netbsd.c
+++ b/tools/libxl/libxl_netbsd.c
@@ -24,3 +24,13 @@ 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,
+ int vif_executed)
+{
+ return 0;
+}
--
1.7.7.5 (Apple Git-26)
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 15/15] libxl: add dummy netbsd functions
2012-05-22 14:02 ` [PATCH v2 15/15] libxl: add dummy netbsd functions Roger Pau Monne
@ 2012-05-22 17:47 ` Ian Jackson
2012-05-22 17:47 ` Ian Jackson
1 sibling, 0 replies; 43+ messages in thread
From: Ian Jackson @ 2012-05-22 17:47 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel@lists.xen.org
Roger Pau Monne writes ("[PATCH v2 15/15] libxl: add dummy netbsd functions"):
> Add dummy NetBSD functions related to hotplug scripts, so compilation
> doesn't fail. This functions will be filled in a following series.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Ian.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 15/15] libxl: add dummy netbsd functions
2012-05-22 14:02 ` [PATCH v2 15/15] libxl: add dummy netbsd functions Roger Pau Monne
2012-05-22 17:47 ` Ian Jackson
@ 2012-05-22 17:47 ` Ian Jackson
2012-05-23 15:05 ` Roger Pau Monne
1 sibling, 1 reply; 43+ messages in thread
From: Ian Jackson @ 2012-05-22 17:47 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel@lists.xen.org
Roger Pau Monne writes ("[PATCH v2 15/15] libxl: add dummy netbsd functions"):
> Add dummy NetBSD functions related to hotplug scripts, so compilation
> doesn't fail. This functions will be filled in a following series.
Actually I just acked this but it should actually be done in the
previous patch, where the Linux version is added, so that the build
works at every changeset.
Ian.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 15/15] libxl: add dummy netbsd functions
2012-05-22 17:47 ` Ian Jackson
@ 2012-05-23 15:05 ` Roger Pau Monne
0 siblings, 0 replies; 43+ messages in thread
From: Roger Pau Monne @ 2012-05-23 15:05 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel@lists.xen.org
Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH v2 15/15] libxl: add dummy netbsd functions"):
>> Add dummy NetBSD functions related to hotplug scripts, so compilation
>> doesn't fail. This functions will be filled in a following series.
>
> Actually I just acked this but it should actually be done in the
> previous patch, where the Linux version is added, so that the build
> works at every changeset.
Yes, I've merged this one with "libxl: call hotplug scripts for disk
devices from libxl".
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 0/15] execute hotplug scripts from libxl
2012-05-22 14:02 [PATCH v2 0/15] execute hotplug scripts from libxl Roger Pau Monne
` (14 preceding siblings ...)
2012-05-22 14:02 ` [PATCH v2 15/15] libxl: add dummy netbsd functions Roger Pau Monne
@ 2012-05-23 10:07 ` Ian Campbell
15 siblings, 0 replies; 43+ messages in thread
From: Ian Campbell @ 2012-05-23 10:07 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel@lists.xen.org
On Tue, 2012-05-22 at 15:02 +0100, Roger Pau Monne wrote:
> This series have been splitted in several patches, to make them easier
> to review. Also the amount of changes introduced is quite important,
> since apart from all the hotplug necessary functions and
> modifications, libxl_domain_destroy has been converted to an async op.
> This was necessary in order to have async operations during device
> removal.
>
> Also, as an important change, disk and nics are added at different
> points for HVM and device model based guests, since we need the disk
> in order to start Qemu, but the nic hotplug scripts should be called
> at a later point, when Qemu has created the corresponding tap device.
>
> Acked patches:
>
> [PATCH v2 01/15] libxl: pass env vars to libxl__exec
> [PATCH v2 02/15] libxl: fix libxl__xs_directory usage of transaction
>
> Pending:
>
> [PATCH v2 03/15] libxl: add libxl__xs_path_cleanup
> [PATCH v2 04/15] libxl: reoder libxl_device unplug functions
Ian subsequently acked these two so applied the first four. I didn't go
looking for patches which could be independently applied -- please let
me know if I should...
> [PATCH v2 05/15] libxl: change libxl__ao_device_remove to
> [PATCH v2 06/15] libxl: move device model creation prototypes
> [PATCH v2 07/15] libxl: convert libxl_domain_destroy to an async op
> [PATCH v2 08/15] libxl: convert libxl_device_disk_add to an asyn op
> [PATCH v2 09/15] libxl: convert libxl_device_nic_add to an async
> [PATCH v2 10/15] libxl: add option to choose who executes hotplug
> [PATCH v2 11/15] libxl: set nic type to VIF by default
> [PATCH v2 12/15] libxl: call hotplug scripts for disk devices from
> [PATCH v2 13/15] libxl: call hotplug scripts for nic devices from
> [PATCH v2 14/15] libxl: use libxl__xs_path_cleanup on device_destroy
> [PATCH v2 15/15] libxl: add dummy netbsd functions
>
>
> Change since v1:
>
> * Removed all the unecessary code motion and code cleanup
>
> * Split "convert libxl_domain_destroy to an async op" into two
> separate patches.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread