* [PATCH v3 0/5] libxl: call hotplug scripts from libxl
@ 2012-04-20 13:23 Roger Pau Monne
2012-04-20 13:23 ` [PATCH v3 1/5] libxl: allow libxl__exec to take a parameter containing the env variables Roger Pau Monne
` (5 more replies)
0 siblings, 6 replies; 41+ messages in thread
From: Roger Pau Monne @ 2012-04-20 13:23 UTC (permalink / raw)
To: xen-devel
This series removes the use of udev rules to call hotplug scripts when using
libxl. Scripts are directly called from the toolstack at the necessary points,
making use of the new event library and it's fork support.
Fixed Ian Campbell reviews and added support for named vifs using a custom
constant as prefix (to be merged/mixed with Ian Campbell named interfaces
patch). This series should be applied after Ian Jackson event fork and Ian
Campbell support for named vifs.
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v3 1/5] libxl: allow libxl__exec to take a parameter containing the env variables
2012-04-20 13:23 [PATCH v3 0/5] libxl: call hotplug scripts from libxl Roger Pau Monne
@ 2012-04-20 13:23 ` Roger Pau Monne
2012-04-23 15:37 ` Ian Jackson
2012-04-20 13:23 ` [PATCH v3 2/5] libxl: add libxl__xs_path_cleanup Roger Pau Monne
` (4 subsequent siblings)
5 siblings, 1 reply; 41+ messages in thread
From: Roger Pau Monne @ 2012-04-20 13:23 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne
Add another parameter to libxl__exec call that contains the
environment variables to use when performing the execvp call.
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 | 7 ++++++-
tools/libxl/libxl_internal.h | 13 ++++++++++++-
5 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 42ac89f..f0372d0 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1253,7 +1253,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(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 2774062..7120dad 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -113,12 +113,12 @@ static int open_xenconsoled_pty(int *master, int *slave, char *slave_path, size_
static pid_t fork_exec_bootloader(int *master, const char *arg0, char **args)
{
struct termios termattr;
+ char *env[] = {"TERM", "vt100", NULL};
pid_t pid = forkpty(master, NULL, NULL, NULL);
if (pid == -1)
return -1;
else if (pid == 0) {
- setenv("TERM", "vt100", 1);
- libxl__exec(-1, -1, -1, arg0, args);
+ libxl__exec(-1, -1, -1, arg0, args, env);
return -1;
}
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index e25a767..15e24b9 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -994,7 +994,7 @@ retry_transaction:
goto out_close;
if (!rc) { /* inner child */
setsid();
- libxl__exec(null, logfile_w, logfile_w, dm, args);
+ libxl__exec(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 b10e79f..be9e407 100644
--- a/tools/libxl/libxl_exec.c
+++ b/tools/libxl/libxl_exec.c
@@ -72,7 +72,7 @@ static void check_open_fds(const char *what)
}
void libxl__exec(int stdinfd, int stdoutfd, int stderrfd, const char *arg0,
- char **args)
+ char **args, char **env)
/* call this in the child */
{
if (stdinfd != -1)
@@ -95,6 +95,11 @@ 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) {
+ setenv(env[i], env[i+1], 1);
+ }
+ }
execvp(arg0, args);
fprintf(stderr, "libxl: cannot execute %s: %s\n", arg0, strerror(errno));
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index d524945..9e15f95 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -952,8 +952,19 @@ _hidden int libxl__spawn_check(libxl__gc *gc,
/* low-level stuff, for synchronous subprocesses etc. */
+/*
+ * env should be passed using the following format,
+ *
+ * env[0]: name of env variable
+ * env[1]: value of env variable
+ *
+ * So it efectively becomes something like:
+ * export env[0]=env[1]
+ *
+ * The last entry of the array always has to be NULL.
+ */
_hidden void libxl__exec(int stdinfd, int stdoutfd, int stderrfd,
- const char *arg0, char **args); // logs errors, never returns
+ const char *arg0, char **args, char **env); // logs errors, never returns
/* from xl_create */
_hidden int libxl__domain_make(libxl__gc *gc,
--
1.7.7.5 (Apple Git-26)
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v3 2/5] libxl: add libxl__xs_path_cleanup
2012-04-20 13:23 [PATCH v3 0/5] libxl: call hotplug scripts from libxl Roger Pau Monne
2012-04-20 13:23 ` [PATCH v3 1/5] libxl: allow libxl__exec to take a parameter containing the env variables Roger Pau Monne
@ 2012-04-20 13:23 ` Roger Pau Monne
2012-04-23 15:33 ` Ian Jackson
2012-04-23 16:52 ` Ian Jackson
2012-04-20 13:23 ` [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd Roger Pau Monne
` (3 subsequent siblings)
5 siblings, 2 replies; 41+ messages in thread
From: Roger Pau Monne @ 2012-04-20 13:23 UTC (permalink / raw)
To: xen-devel; +Cc: 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 v2:
* Moved the function to libxl_xshelp.c and added the prototype to
libxl_internal.h.
Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
---
tools/libxl/libxl_device.c | 1 -
tools/libxl/libxl_internal.h | 6 ++++++
tools/libxl/libxl_xshelp.c | 22 ++++++++++++++++++++++
3 files changed, 28 insertions(+), 1 deletions(-)
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index c7e057d..36d58cd 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -356,7 +356,6 @@ int libxl__device_disk_dev_number(const char *virtpath, int *pdisk,
return -1;
}
-
typedef struct {
libxl__ao *ao;
libxl__ev_devstate ds;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 9e15f95..020810a 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -458,6 +458,12 @@ _hidden bool libxl__xs_mkdir(libxl__gc *gc, xs_transaction_t t,
_hidden char *libxl__xs_libxl_path(libxl__gc *gc, uint32_t domid);
+/*
+ * Perfrom recursive cleanup of xenstore path, from top to bottom
+ * just like xenstore-rm -t
+ */
+_hidden int libxl__xs_path_cleanup(libxl__gc *gc, char *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 3ea8d08..0b1b844 100644
--- a/tools/libxl/libxl_xshelp.c
+++ b/tools/libxl/libxl_xshelp.c
@@ -135,6 +135,28 @@ char *libxl__xs_libxl_path(libxl__gc *gc, uint32_t domid)
return s;
}
+int libxl__xs_path_cleanup(libxl__gc *gc, char *path)
+{
+ libxl_ctx *ctx = libxl__gc_owner(gc);
+ unsigned int nb = 0;
+ char *last;
+
+ if (!path)
+ return 0;
+
+ xs_rm(ctx->xsh, XBT_NULL, path);
+
+ for (last = strrchr(path, '/'); last != NULL; last = strrchr(path, '/')) {
+ *last = '\0';
+ if (!libxl__xs_directory(gc, XBT_NULL, path, &nb))
+ continue;
+ if (nb == 0) {
+ xs_rm(ctx->xsh, XBT_NULL, path);
+ }
+ }
+ return 0;
+}
+
/*
* Local variables:
* mode: C
--
1.7.7.5 (Apple Git-26)
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd
2012-04-20 13:23 [PATCH v3 0/5] libxl: call hotplug scripts from libxl Roger Pau Monne
2012-04-20 13:23 ` [PATCH v3 1/5] libxl: allow libxl__exec to take a parameter containing the env variables Roger Pau Monne
2012-04-20 13:23 ` [PATCH v3 2/5] libxl: add libxl__xs_path_cleanup Roger Pau Monne
@ 2012-04-20 13:23 ` Roger Pau Monne
2012-04-23 16:48 ` Ian Jackson
2012-04-20 13:23 ` [PATCH v3 4/5] libxl: call hotplug scripts from libxl for vif Roger Pau Monne
` (2 subsequent siblings)
5 siblings, 1 reply; 41+ messages in thread
From: Roger Pau Monne @ 2012-04-20 13:23 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne
This is a rather big change, that adds the necessary machinery to perform
hotplug script calling from libxl for Linux. This patch launches the necessary
scripts to attach and detach PHY and TAP backend types (Qemu doesn't use hotplug
scripts).
Here's a list of the major changes introduced:
* libxl_device_disk_add makes use of the new event library and takes the
optional parameter libxl_asyncop_how, to choose how the operation has to be
issued (sync or async).
* libxl_device_disk_add waits for backend to switch to state 2 (XenbusInitWait)
and then launches the hotplug script.
* libxl__devices_destroy no longer calls libxl__device_destroy, and instead
calls libxl__initiate_device_remove, so we can disconnect the device and
execute the necessary hotplug scripts instead of just deleting the backend
entries. So libxl__devices_destroy now uses the event library, and so does
libxl_domain_destroy.
* Since libxl__devices_destroy calls multiple times
libxl__initiate_device_remove, this function now returns a different value
regarding the actions taken (if an event was added or not). The user has to
call libxl__ao_complete after using this function if necessary.
* The internal API for hotplug scripts has been added, which consists of one
function; libxl__device_hotplug that takes the device and the action to
execute.
* Linux hotplug scripts are called by setting the necessary env variables to
emulate udev rules, so there's no need to change them (although a rework to
pass this as parameters insted of env variables would be suitable, so both
NetBSD and Linux hotplug scripts take the same parameters).
* Added a check in xen-hotplug-common.sh, so scripts are only executed from
udev when using xend. xl adds a disable_udev=y to xenstore private directory
and with the modification of the udev rules every call from udev gets
HOTPLUG_GATE passed, that points to disable_udev. If HOTPLUG_GATE is passed
and points to a value, the hotplug script is not executed.
I've also modified the python binding for pyxl_domain_destroy, that now take the
ao_how parameter, but I'm not really sure if it's done correctly, let's just say
that it compiles.
Changes since v3:
* disable_udev is now stored in /libxl/backend/vbd/<domid>/<devid>/.
* Passed NULL as ao_how in Python bindings.
Changes since v1:
* Replaced the hotplug snippet that prevents execution from udev when
necessary, and now we set the env var from udev rules instead of libxl.
* Replaced the libxl_device_disk_add "if" with a "switch".
* Removed libxl__xs_path_cleanup (replaced with xs_rm).
* Fixed several spelling mistakes.
Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
tools/hotplug/Linux/xen-backend.rules | 6 +-
tools/hotplug/Linux/xen-hotplug-common.sh | 6 +
tools/libxl/Makefile | 3 +-
tools/libxl/libxl.c | 144 ++++++++++++++++++++++-----
tools/libxl/libxl.h | 7 +-
tools/libxl/libxl_create.c | 4 +-
tools/libxl/libxl_device.c | 152 ++++++++++++++++++++++++++---
tools/libxl/libxl_dm.c | 4 +-
tools/libxl/libxl_hotplug.c | 67 +++++++++++++
tools/libxl/libxl_internal.h | 46 ++++++++-
tools/libxl/libxl_linux.c | 117 ++++++++++++++++++++++
tools/libxl/libxl_pci.c | 5 +-
tools/libxl/xl_cmdimpl.c | 14 ++--
tools/python/xen/lowlevel/xl/xl.c | 2 +-
14 files changed, 516 insertions(+), 61 deletions(-)
create mode 100644 tools/libxl/libxl_hotplug.c
diff --git a/tools/hotplug/Linux/xen-backend.rules b/tools/hotplug/Linux/xen-backend.rules
index 19bd0fa..19f3d27 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{HOTPLUG_GATE}="/libxl/$env{XENBUS_PATH}/disable_udev", RUN+="/etc/xen/scripts/blktap $env{ACTION}"
+SUBSYSTEM=="xen-backend", KERNEL=="vbd*", ENV{HOTPLUG_GATE}="/libxl/$env{XENBUS_PATH}/disable_udev", 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{HOTPLUG_GATE}="/libxl/$env{XENBUS_PATH}/disable_udev", 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..d2aac3a 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 "${HOTPLUG_GATE}" ] && \
+ `xenstore-read "$HOTPLUG_GATE" >/dev/null 2>&1`; then
+ exit 0
+fi
dir=$(dirname "$0")
. "$dir/hotplugpath.sh"
diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index e5ea867..0ac43bd 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -53,7 +53,8 @@ LIBXL_LIBS += -lyajl
LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \
libxl_dom.o libxl_exec.o libxl_xshelp.o libxl_device.o \
libxl_internal.o libxl_utils.o libxl_uuid.o libxl_json.o \
- libxl_qmp.o libxl_event.o libxl_fork.o $(LIBXL_OBJS-y)
+ libxl_qmp.o libxl_event.o libxl_fork.o libxl_hotplug.o \
+ $(LIBXL_OBJS-y)
LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o
$(LIBXL_OBJS): CFLAGS += $(CFLAGS_libxenctrl) $(CFLAGS_libxenguest) $(CFLAGS_libxenstore) $(CFLAGS_libblktapctl) -include $(XEN_ROOT)/tools/config.h
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index f0372d0..eaa3095 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1062,13 +1062,15 @@ 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)
+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);
char *dom_path;
char *vm_path;
char *pid;
int rc, dm_present;
+ int rc_ao = 0;
rc = libxl_domain_info(ctx, NULL, domid);
switch(rc) {
@@ -1110,7 +1112,8 @@ int libxl_domain_destroy(libxl_ctx *ctx, uint32_t domid)
libxl__qmp_cleanup(gc, domid);
}
- if (libxl__devices_destroy(gc, domid) < 0)
+ rc_ao = libxl__devices_destroy(egc, ao, domid);
+ if (rc_ao < 0)
LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
"libxl__devices_destroy failed for %d", domid);
@@ -1133,9 +1136,10 @@ int libxl_domain_destroy(libxl_ctx *ctx, uint32_t domid)
goto out;
}
rc = 0;
+
out:
- GC_FREE;
- return rc;
+ if (rc_ao) return AO_ABORT(rc_ao);
+ return AO_INPROGRESS;
}
int libxl_console_exec(libxl_ctx *ctx, uint32_t domid, int cons_num, libxl_console_type type)
@@ -1313,11 +1317,14 @@ 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);
flexarray_t *front;
flexarray_t *back;
+ flexarray_t *private;
char *dev;
libxl__device device;
int major, minor, rc;
@@ -1330,10 +1337,15 @@ int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *dis
rc = ERROR_NOMEM;
goto out;
}
- back = flexarray_make(16, 1);
+ back = flexarray_make(20, 1);
if (!back) {
rc = ERROR_NOMEM;
- goto out_free;
+ goto out_free_f;
+ }
+ private = flexarray_make(2, 1);
+ if (!private) {
+ rc = ERROR_NOMEM;
+ goto out_free_b;
}
if (disk->script) {
@@ -1361,6 +1373,11 @@ int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *dis
flexarray_append(back, "params");
flexarray_append(back, dev);
+ flexarray_append(back, "script");
+ flexarray_append(back, libxl__sprintf(gc, "%s/%s",
+ libxl_xen_script_dir_path(),
+ "block"));
+
assert(device.backend_kind == LIBXL__DEVICE_KIND_VBD);
break;
case LIBXL_DISK_BACKEND_TAP:
@@ -1374,6 +1391,11 @@ int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *dis
libxl__device_disk_string_of_format(disk->format),
disk->pdev_path));
+ flexarray_append(back, "script");
+ flexarray_append(back, libxl__sprintf(gc, "%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:
@@ -1416,18 +1438,38 @@ int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *dis
flexarray_append(front, "device-type");
flexarray_append(front, disk->is_cdrom ? "cdrom" : "disk");
+ /* compatibility addon to keep udev rules */
+ flexarray_append(private, "disable_udev");
+ flexarray_append(private, "y");
+
libxl__device_generic_add(gc, &device,
- libxl__xs_kvs_of_flexarray(gc, back, back->count),
- libxl__xs_kvs_of_flexarray(gc, front, front->count));
+ libxl__xs_kvs_of_flexarray(gc, back, back->count),
+ libxl__xs_kvs_of_flexarray(gc, front, front->count),
+ libxl__xs_kvs_of_flexarray(gc, private, private->count));
+
+ switch(disk->backend) {
+ case LIBXL_DISK_BACKEND_PHY:
+ case LIBXL_DISK_BACKEND_TAP:
+ rc = libxl__initiate_device_add(egc, ao, &device);
+ if (rc) goto out_free;
+ break;
+ case LIBXL_DISK_BACKEND_QDISK:
+ default:
+ libxl__ao_complete(egc, ao, 0);
+ break;
+ }
rc = 0;
out_free:
+ flexarray_free(private);
+out_free_b:
flexarray_free(back);
+out_free_f:
flexarray_free(front);
out:
- GC_FREE;
- return rc;
+ if (rc) return AO_ABORT(rc);
+ return AO_INPROGRESS;
}
int libxl_device_disk_remove(libxl_ctx *ctx, uint32_t domid,
@@ -1442,7 +1484,18 @@ int libxl_device_disk_remove(libxl_ctx *ctx, uint32_t domid,
if (rc != 0) goto out;
rc = libxl__initiate_device_remove(egc, ao, &device);
- if (rc) goto out;
+ switch(rc) {
+ case 1:
+ /* event added */
+ break;
+ case 0:
+ /* no event added */
+ libxl__ao_complete(egc, ao, 0);
+ break;
+ default:
+ /* error */
+ goto out;
+ }
return AO_INPROGRESS;
@@ -1666,11 +1719,11 @@ 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);
+ 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);
+ libxl_device_disk_add(ctx, stubdomid, disk, 0);
}
out:
for (i = 0; i < num; i++)
@@ -1884,8 +1937,9 @@ int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic)
flexarray_append(front, libxl__sprintf(gc,
LIBXL_MAC_FMT, LIBXL_MAC_BYTES(nic->mac)));
libxl__device_generic_add(gc, &device,
- libxl__xs_kvs_of_flexarray(gc, back, back->count),
- libxl__xs_kvs_of_flexarray(gc, front, front->count));
+ libxl__xs_kvs_of_flexarray(gc, back, back->count),
+ libxl__xs_kvs_of_flexarray(gc, front, front->count),
+ NULL);
/* FIXME: wait for plug */
rc = 0;
@@ -1909,7 +1963,18 @@ int libxl_device_nic_remove(libxl_ctx *ctx, uint32_t domid,
if (rc != 0) goto out;
rc = libxl__initiate_device_remove(egc, ao, &device);
- if (rc) goto out;
+ switch(rc) {
+ case 1:
+ /* event added */
+ break;
+ case 0:
+ /* no event added */
+ libxl__ao_complete(egc, ao, 0);
+ break;
+ default:
+ /* error */
+ goto out;
+ }
return AO_INPROGRESS;
@@ -2162,8 +2227,9 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
}
libxl__device_generic_add(gc, &device,
- libxl__xs_kvs_of_flexarray(gc, back, back->count),
- libxl__xs_kvs_of_flexarray(gc, front, front->count));
+ libxl__xs_kvs_of_flexarray(gc, back, back->count),
+ libxl__xs_kvs_of_flexarray(gc, front, front->count),
+ NULL);
rc = 0;
out_free:
flexarray_free(back);
@@ -2233,8 +2299,9 @@ int libxl_device_vkb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vkb *vkb)
flexarray_append(front, libxl__sprintf(gc, "%d", 1));
libxl__device_generic_add(gc, &device,
- libxl__xs_kvs_of_flexarray(gc, back, back->count),
- libxl__xs_kvs_of_flexarray(gc, front, front->count));
+ libxl__xs_kvs_of_flexarray(gc, back, back->count),
+ libxl__xs_kvs_of_flexarray(gc, front, front->count),
+ NULL);
rc = 0;
out_free:
flexarray_free(back);
@@ -2256,7 +2323,18 @@ int libxl_device_vkb_remove(libxl_ctx *ctx, uint32_t domid,
if (rc != 0) goto out;
rc = libxl__initiate_device_remove(egc, ao, &device);
- if (rc) goto out;
+ switch(rc) {
+ case 1:
+ /* event added */
+ break;
+ case 0:
+ /* no event added */
+ libxl__ao_complete(egc, ao, 0);
+ break;
+ default:
+ /* error */
+ goto out;
+ }
return AO_INPROGRESS;
@@ -2366,8 +2444,9 @@ int libxl_device_vfb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vfb *vfb)
flexarray_append_pair(front, "state", libxl__sprintf(gc, "%d", 1));
libxl__device_generic_add(gc, &device,
- libxl__xs_kvs_of_flexarray(gc, back, back->count),
- libxl__xs_kvs_of_flexarray(gc, front, front->count));
+ libxl__xs_kvs_of_flexarray(gc, back, back->count),
+ libxl__xs_kvs_of_flexarray(gc, front, front->count),
+ NULL);
rc = 0;
out_free:
flexarray_free(front);
@@ -2389,7 +2468,18 @@ int libxl_device_vfb_remove(libxl_ctx *ctx, uint32_t domid,
if (rc != 0) goto out;
rc = libxl__initiate_device_remove(egc, ao, &device);
- if (rc) goto out;
+ switch(rc) {
+ case 1:
+ /* event added */
+ break;
+ case 0:
+ /* no event added */
+ libxl__ao_complete(egc, ao, 0);
+ break;
+ default:
+ /* error */
+ goto out;
+ }
return AO_INPROGRESS;
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index edbca53..6687e2e 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -472,7 +472,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 */
@@ -601,7 +602,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 3675afe..de598ad 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -598,7 +598,7 @@ static int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
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]);
+ ret = libxl_device_disk_add(ctx, domid, &d_config->disks[i], 0);
if (ret) {
LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
"cannot add disk %d to domain: %d", i, ret);
@@ -721,7 +721,7 @@ static int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
error_out:
if (domid)
- libxl_domain_destroy(ctx, domid);
+ libxl_domain_destroy(ctx, domid, 0);
return ret;
}
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 36d58cd..aa19fab 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -40,6 +40,13 @@ char *libxl__device_backend_path(libxl__gc *gc, libxl__device *device)
device->domid, device->devid);
}
+char *libxl__device_private_path(libxl__gc *gc, libxl__device *device)
+{
+ return libxl__sprintf(gc, "/libxl/backend/%s/%u/%d",
+ libxl__device_kind_to_string(device->backend_kind),
+ device->domid, device->devid);
+}
+
int libxl__parse_backend_path(libxl__gc *gc,
const char *path,
libxl__device *dev)
@@ -59,16 +66,18 @@ int libxl__parse_backend_path(libxl__gc *gc,
}
int libxl__device_generic_add(libxl__gc *gc, libxl__device *device,
- char **bents, char **fents)
+ char **bents, char **fents, char **private)
{
libxl_ctx *ctx = libxl__gc_owner(gc);
- char *frontend_path, *backend_path;
+ char *frontend_path, *backend_path, *private_path;
xs_transaction_t t;
struct xs_permissions frontend_perms[2];
struct xs_permissions backend_perms[2];
+ struct xs_permissions private_perms[1];
frontend_path = libxl__device_frontend_path(gc, device);
backend_path = libxl__device_backend_path(gc, device);
+ private_path = libxl__device_private_path(gc, device);
frontend_perms[0].id = device->domid;
frontend_perms[0].perms = XS_PERM_NONE;
@@ -80,6 +89,9 @@ int libxl__device_generic_add(libxl__gc *gc, libxl__device *device,
backend_perms[1].id = device->domid;
backend_perms[1].perms = XS_PERM_READ;
+ private_perms[0].id = device->backend_domid;
+ private_perms[0].perms = XS_PERM_NONE;
+
retry_transaction:
t = xs_transaction_start(ctx->xsh);
/* FIXME: read frontend_path and check state before removing stuff */
@@ -100,6 +112,14 @@ retry_transaction:
libxl__xs_writev(gc, t, backend_path, bents);
}
+ if (private) {
+ xs_rm(ctx->xsh, t, private_path);
+ xs_mkdir(ctx->xsh, t, private_path);
+ xs_set_permissions(ctx->xsh, t, private_path, private_perms,
+ ARRAY_SIZE(private_perms));
+ libxl__xs_writev(gc, t, private_path, private);
+ }
+
if (!xs_transaction_end(ctx->xsh, t, 0)) {
if (errno == EAGAIN)
goto retry_transaction;
@@ -359,22 +379,71 @@ int libxl__device_disk_dev_number(const char *virtpath, int *pdisk,
typedef struct {
libxl__ao *ao;
libxl__ev_devstate ds;
+ libxl__device *dev;
} libxl__ao_device_remove;
+typedef struct {
+ libxl__ao *ao;
+ libxl__ev_devstate ds;
+ libxl__device *dev;
+} libxl__ao_device_add;
+
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_add_cleanup(libxl__gc *gc,
+ libxl__ao_device_add *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_ctx *ctx = libxl__gc_owner(gc);
+
+ rc = libxl__device_hotplug(gc, aorm->dev, DISCONNECT);
+ if (rc)
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to execute hotplug script for"
+ " device %"PRIu32, aorm->dev->devid);
+ libxl__xs_path_cleanup(gc, libxl__device_frontend_path(gc, aorm->dev));
+ libxl__xs_path_cleanup(gc, libxl__device_backend_path(gc, aorm->dev));
+ libxl__xs_path_cleanup(gc, libxl__device_private_path(gc, aorm->dev));
libxl__ao_complete(egc, aorm->ao, rc);
device_remove_cleanup(gc, aorm);
}
+static void device_add_callback(libxl__egc *egc, libxl__ev_devstate *ds,
+ int rc)
+{
+ libxl__ao_device_add *aorm = CONTAINER_OF(ds, *aorm, ds);
+ libxl__gc *gc = &aorm->ao->gc;
+ libxl_ctx *ctx = libxl__gc_owner(gc);
+
+ rc = libxl__device_hotplug(gc, aorm->dev, CONNECT);
+ if (rc)
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to execute hotplug script for"
+ " device %"PRIu32, aorm->dev->devid);
+ libxl__ao_complete(egc, aorm->ao, rc);
+ if (aorm)
+ libxl__ev_devstate_cancel(gc, &aorm->ds);
+ return;
+}
+
+/*
+ * Return codes:
+ * 1 Success and event(s) HAVE BEEN added
+ * 0 Success and NO events were added
+ * < 0 Error
+ *
+ * Since this function can be called several times, and several events
+ * can be added to the same context, the user has to call
+ * libxl__ao_complete when necessary (if no events are added).
+ */
int libxl__initiate_device_remove(libxl__egc *egc, libxl__ao *ao,
libxl__device *dev)
{
@@ -391,7 +460,6 @@ int libxl__initiate_device_remove(libxl__egc *egc, libxl__ao *ao,
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;
}
@@ -412,6 +480,7 @@ retry_transaction:
aorm = libxl__zalloc(gc, sizeof(*aorm));
aorm->ao = ao;
+ aorm->dev = dev;
libxl__ev_devstate_init(&aorm->ds);
rc = libxl__ev_devstate_wait(gc, &aorm->ds, device_remove_callback,
@@ -419,7 +488,7 @@ retry_transaction:
LIBXL_DESTROY_TIMEOUT * 1000);
if (rc) goto out_fail;
- return 0;
+ return 1;
out_fail:
assert(rc);
@@ -427,31 +496,77 @@ retry_transaction:
return rc;
out_ok:
- libxl__ao_complete(egc, ao, 0);
+ rc = libxl__device_hotplug(gc, dev, DISCONNECT);
+ libxl__xs_path_cleanup(gc, libxl__device_frontend_path(gc, dev));
+ libxl__xs_path_cleanup(gc, be_path);
+ libxl__xs_path_cleanup(gc, libxl__device_private_path(gc, dev));
return 0;
}
-int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
+int libxl__initiate_device_add(libxl__egc *egc, libxl__ao *ao,
+ libxl__device *dev)
{
+ AO_GC;
libxl_ctx *ctx = libxl__gc_owner(gc);
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_add *aorm = 0;
+
+ if (atoi(state) == XenbusStateInitWait)
+ goto out_ok;
+
+ aorm = libxl__zalloc(gc, sizeof(*aorm));
+ aorm->ao = ao;
+ aorm->dev = dev;
+ libxl__ev_devstate_init(&aorm->ds);
+
+ rc = libxl__ev_devstate_wait(gc, &aorm->ds, device_add_callback,
+ state_path, XenbusStateInitWait,
+ LIBXL_INIT_TIMEOUT * 1000);
+ if (rc) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to initialize device %"
+ PRIu32, dev->devid);
+ libxl__ev_devstate_cancel(gc, &aorm->ds);
+ goto out_fail;
+ }
+
+ return 0;
+
+out_fail:
+ assert(rc);
+ device_add_cleanup(gc, aorm);
+ return rc;
+out_ok:
+ rc = libxl__device_hotplug(gc, dev, CONNECT);
+ libxl__ao_complete(egc, ao, 0);
+ return rc;
+}
+
+int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
+{
+ char *be_path = libxl__device_backend_path(gc, dev);
char *fe_path = libxl__device_frontend_path(gc, dev);
+ char *pr_path = libxl__device_private_path(gc, dev);
- xs_rm(ctx->xsh, XBT_NULL, be_path);
- xs_rm(ctx->xsh, XBT_NULL, fe_path);
+ libxl__xs_path_cleanup(gc, be_path);
+ libxl__xs_path_cleanup(gc, fe_path);
+ libxl__xs_path_cleanup(gc, pr_path);
libxl__device_destroy_tapdisk(gc, be_path);
return 0;
}
-int libxl__devices_destroy(libxl__gc *gc, uint32_t domid)
+int libxl__devices_destroy(libxl__egc *egc, libxl__ao *ao, uint32_t domid)
{
+ AO_GC;
libxl_ctx *ctx = libxl__gc_owner(gc);
char *path;
unsigned int num_kinds, num_devs;
char **kinds = NULL, **devs = NULL;
- int i, j;
+ int i, j, events = 0, rc;
libxl__device dev;
libxl__device_kind kind;
@@ -481,11 +596,24 @@ int libxl__devices_destroy(libxl__gc *gc, uint32_t domid)
dev.domid = domid;
dev.kind = kind;
dev.devid = atoi(devs[j]);
-
- libxl__device_destroy(gc, &dev);
+ rc = libxl__initiate_device_remove(egc, ao, &dev);
+ switch(rc) {
+ case 1:
+ events++;
+ break;
+ case 0:
+ /* no events added */
+ break;
+ default:
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Could not remove device "
+ "%s", path);
+ }
}
}
}
+ /* Check if any events were added */
+ if (!events)
+ libxl__ao_complete(egc, ao, 0);
/* console 0 frontend directory is not under /local/domain/<domid>/device */
path = libxl__sprintf(gc, "/local/domain/%d/console/backend", domid);
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 15e24b9..4ce58f6 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -789,7 +789,7 @@ retry_transaction:
goto retry_transaction;
for (i = 0; i < dm_config.num_disks; i++) {
- ret = libxl_device_disk_add(ctx, dm_domid, &dm_config.disks[i]);
+ ret = libxl_device_disk_add(ctx, dm_domid, &dm_config.disks[i], 0);
if (ret)
goto out_free;
}
@@ -1047,7 +1047,7 @@ int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
goto out;
}
LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Device model is a stubdom, domid=%d", stubdomid);
- ret = libxl_domain_destroy(ctx, stubdomid);
+ ret = libxl_domain_destroy(ctx, stubdomid, 0);
if (ret)
goto out;
diff --git a/tools/libxl/libxl_hotplug.c b/tools/libxl/libxl_hotplug.c
new file mode 100644
index 0000000..2eace0c
--- /dev/null
+++ b/tools/libxl/libxl_hotplug.c
@@ -0,0 +1,67 @@
+/*
+ * Copyright (C) 2012
+ * Author Roger Pau Monne <roger.pau@citrix.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Lesser General Public License for more details.
+ */
+
+#include "libxl_osdeps.h" /* must come before any other headers */
+
+#include "libxl_internal.h"
+
+/*
+ * Generic hotplug helpers
+ *
+ * This helpers are both used by Linux and NetBSD hotplug script
+ * dispatcher functions.
+ */
+
+static void hotplug_exited(libxl__egc *egc, libxl__ev_child *child,
+ pid_t pid, int status)
+{
+ libxl__hotplug_state *hs = CONTAINER_OF(child, *hs, child);
+ EGC_GC;
+
+ if (status) {
+ libxl_report_child_exitstatus(CTX, hs->rc ? LIBXL__LOG_ERROR
+ : LIBXL__LOG_WARNING,
+ "hotplug child", pid, status);
+ }
+}
+
+int libxl__hotplug_exec(libxl__gc *gc, char *arg0, char **args, char **env)
+{
+ int rc = 0;
+ pid_t pid = -1;
+ libxl__hotplug_state *hs;
+
+ hs = libxl__zalloc(gc, sizeof(*hs));
+
+ pid = libxl__ev_child_fork(gc, &hs->child, hotplug_exited);
+ if (pid == -1) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ if (!pid) {
+ /* child */
+ signal(SIGCHLD, SIG_DFL);
+ libxl__exec(-1, -1, -1, arg0, args, env);
+ }
+out:
+ if (libxl__ev_child_inuse(&hs->child)) {
+ hs->rc = rc;
+ /* we will get a callback when the child dies */
+ return 0;
+ }
+
+ assert(rc);
+ return rc;
+}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 020810a..95d5c40 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -70,6 +70,7 @@
#include "_libxl_types_internal_json.h"
#define LIBXL_DESTROY_TIMEOUT 10
+#define LIBXL_INIT_TIMEOUT 10
#define LIBXL_DEVICE_MODEL_START_TIMEOUT 10
#define LIBXL_XENCONSOLE_LIMIT 1048576
#define LIBXL_XENCONSOLE_PROTOCOL "vt100"
@@ -464,6 +465,37 @@ _hidden char *libxl__xs_libxl_path(libxl__gc *gc, uint32_t domid);
*/
_hidden int libxl__xs_path_cleanup(libxl__gc *gc, char *path);
+/*
+ * Hotplug handling
+ *
+ * Hotplug scripts are launched automatically by libxl at guest creation &
+ * shutdown/destroy.
+ */
+
+/* Action to pass to hotplug caller functions */
+typedef enum {
+ CONNECT = 1,
+ DISCONNECT = 2
+} libxl__hotplug_action;
+
+typedef struct libxl__hotplug_state libxl__hotplug_state;
+
+struct libxl__hotplug_state {
+ /* public, result, caller may only read in callback */
+ int rc;
+ /* private for implementation */
+ libxl__ev_child child;
+};
+
+/*
+ * libxl__hotplug_exec is an internal function that should not be used
+ * directly, all hotplug script calls have to implement and use
+ * libxl__device_hotplug.
+ */
+_hidden int libxl__device_hotplug(libxl__gc *gc, libxl__device *dev,
+ libxl__hotplug_action action);
+_hidden int libxl__hotplug_exec(libxl__gc *gc, char *arg0, char **args,
+ char **env);
/*
* Event generation functions provided by the libxl event core to the
@@ -743,13 +775,15 @@ _hidden int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
libxl__domain_build_state *state);
_hidden int libxl__device_generic_add(libxl__gc *gc, libxl__device *device,
- char **bents, char **fents);
+ char **bents, char **fents, char **private);
_hidden char *libxl__device_backend_path(libxl__gc *gc, libxl__device *device);
+_hidden char *libxl__device_private_path(libxl__gc *gc, libxl__device *device);
_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__devices_destroy(libxl__egc *egc, libxl__ao *ao,
+ uint32_t domid);
_hidden int libxl__wait_for_backend(libxl__gc *gc, char *be_path, char *state);
/*
@@ -772,6 +806,14 @@ _hidden int libxl__wait_for_backend(libxl__gc *gc, char *be_path, char *state);
_hidden int libxl__initiate_device_remove(libxl__egc*, libxl__ao*,
libxl__device *dev);
+/* Arranges that dev will be added to the guest, and the
+ * hotplug scripts will be executed (if necessary). When
+ * this is done, the ao will be completed. An error
+ * return from libxl__initiate_device_add means that the ao
+ * will _not_ be completed and the caller must do so. */
+_hidden int libxl__initiate_device_add(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.
diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index 925248b..7c107dc 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -25,3 +25,120 @@ 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)
+{
+ libxl_ctx *ctx = libxl__gc_owner(gc);
+ char *be_path = libxl__device_backend_path(gc, dev);
+ char *script;
+ flexarray_t *f_env;
+ char **env;
+ int nr = 0;
+
+ script = libxl__xs_read(gc, XBT_NULL,
+ libxl__sprintf(gc, "%s/%s", be_path, "script"));
+ if (!script) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to read script from %s",
+ be_path);
+ return NULL;
+ }
+
+ f_env = flexarray_make(9, 1);
+ if (!f_env) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+ "unable to create environment array");
+ return NULL;
+ }
+
+ flexarray_set(f_env, nr++, "script");
+ flexarray_set(f_env, nr++, script);
+ flexarray_set(f_env, nr++, "XENBUS_TYPE");
+ flexarray_set(f_env, nr++, (char *)
+ libxl__device_kind_to_string(dev->backend_kind));
+ flexarray_set(f_env, nr++, "XENBUS_PATH");
+ flexarray_set(f_env, nr++,
+ libxl__sprintf(gc, "backend/%s/%u/%d",
+ libxl__device_kind_to_string(dev->backend_kind),
+ dev->domid, dev->devid));
+ flexarray_set(f_env, nr++, "XENBUS_BASE_PATH");
+ flexarray_set(f_env, nr++, "backend");
+
+ flexarray_set(f_env, nr++, NULL);
+
+ env = (char **) flexarray_contents(f_env);
+
+ return env;
+}
+
+/* Hotplug scripts caller functions */
+
+static int libxl__hotplug_disk(libxl__gc *gc, libxl__device *dev,
+ libxl__hotplug_action action)
+{
+ libxl_ctx *ctx = libxl__gc_owner(gc);
+ char *be_path = libxl__device_backend_path(gc, dev);
+ char *what, *script;
+ char **args, **env;
+ int nr = 0, rc = 0;
+ flexarray_t *f_args;
+
+ script = libxl__xs_read(gc, XBT_NULL,
+ libxl__sprintf(gc, "%s/%s", be_path, "script"));
+ if (!script) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to read script from %s",
+ be_path);
+ return -1;
+ }
+
+ env = get_hotplug_env(gc, dev);
+ if (!env)
+ return -1;
+
+ f_args = flexarray_make(3, 1);
+ if (!f_args) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to create arguments array");
+ return -1;
+ }
+
+ flexarray_set(f_args, nr++, script);
+ flexarray_set(f_args, nr++, action == CONNECT ? "add" : "remove");
+ flexarray_set(f_args, nr++, NULL);
+
+ args = (char **) flexarray_contents(f_args);
+ what = libxl__sprintf(gc, "%s %s", args[0],
+ action == CONNECT ? "connect" : "disconnect");
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG,
+ "Calling hotplug script: %s %s",
+ args[0], args[1]);
+ rc = libxl__hotplug_exec(gc, args[0], args, env);
+ if (rc) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable execute hotplug scripts for "
+ "device %"PRIu32, dev->devid);
+ goto out_free;
+ }
+
+ rc = 0;
+
+out_free:
+ free(env);
+ free(args);
+ return rc;
+}
+
+int libxl__device_hotplug(libxl__gc *gc, libxl__device *dev,
+ libxl__hotplug_action action)
+{
+ int rc = 0;
+
+ switch (dev->backend_kind) {
+ case LIBXL__DEVICE_KIND_VBD:
+ rc = libxl__hotplug_disk(gc, dev, action);
+ break;
+ default:
+ rc = 0;
+ break;
+ }
+
+ return rc;
+}
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index e8b8839..fd79d1d 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -103,8 +103,9 @@ int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid,
flexarray_append_pair(front, "state", libxl__sprintf(gc, "%d", 1));
libxl__device_generic_add(gc, &device,
- libxl__xs_kvs_of_flexarray(gc, back, back->count),
- libxl__xs_kvs_of_flexarray(gc, front, front->count));
+ libxl__xs_kvs_of_flexarray(gc, back, back->count),
+ libxl__xs_kvs_of_flexarray(gc, front, front->count),
+ NULL);
out:
if (back)
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index c9e9943..0bc3ac2 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1306,7 +1306,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:
@@ -1867,7 +1867,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)
@@ -2354,7 +2354,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); }
}
@@ -2628,7 +2628,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);
}
@@ -2861,7 +2861,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);
@@ -2979,7 +2979,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);
@@ -5026,7 +5026,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;
diff --git a/tools/python/xen/lowlevel/xl/xl.c b/tools/python/xen/lowlevel/xl/xl.c
index c4f7c52..ce7a2b2 100644
--- a/tools/python/xen/lowlevel/xl/xl.c
+++ b/tools/python/xen/lowlevel/xl/xl.c
@@ -447,7 +447,7 @@ static PyObject *pyxl_domain_destroy(XlObject *self, PyObject *args)
int domid;
if ( !PyArg_ParseTuple(args, "i", &domid) )
return NULL;
- if ( libxl_domain_destroy(self->ctx, domid) ) {
+ if ( libxl_domain_destroy(self->ctx, domid, NULL) ) {
PyErr_SetString(xl_error_obj, "cannot destroy domain");
return NULL;
}
--
1.7.7.5 (Apple Git-26)
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v3 4/5] libxl: call hotplug scripts from libxl for vif
2012-04-20 13:23 [PATCH v3 0/5] libxl: call hotplug scripts from libxl Roger Pau Monne
` (2 preceding siblings ...)
2012-04-20 13:23 ` [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd Roger Pau Monne
@ 2012-04-20 13:23 ` Roger Pau Monne
2012-04-23 16:59 ` Ian Jackson
2012-04-25 11:19 ` Ian Campbell
2012-04-20 13:23 ` [PATCH v3 5/5] libxl: add "downscript=no" to Qemu call Roger Pau Monne
2012-04-23 12:12 ` [PATCH v3 0/5] libxl: call hotplug scripts from libxl Marek Marczykowski
5 siblings, 2 replies; 41+ messages in thread
From: Roger Pau Monne @ 2012-04-20 13:23 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne
As the previous change already introduces most of needed machinery to call
hotplug scripts from libxl, this only adds the necessary bits to call this
scripts for vif interfaces also.
libxl_device_nic_add has been changed to make use of the new event
functionality, and the necessary vif hotplug code has been added. No changes
where needed in the teardown part, since it uses exactly the same code
introduced for vbd.
An exception has been introduced for tap devices, since hotplug scripts
should be called after the device model has been created, so the hotplug
call for those is done in do_domain_create after confirmation of the device
model startup. On the other hand, tap devices don't use teardown scripts,
so add another exception for that.
libxl__device_from_nic was moved to libxl_device.c, so it can be used
in libxl_create.c, and libxl__device_from_disk was also moved to maintain
the symmetry.
libxl__initiate_device_remove has been changed a little, to nuke the frontend
xenstore path before trying to perform device teardown, this allows for
uninitialized devices to be closed gracefully, specially vif interfaces added to
HVM machines and not used.
PV nic devices are set to LIBXL_NIC_TYPE_VIF, since the default value is
LIBXL_NIC_TYPE_IOEMU regardless of the guest type.
A new global option, called disable_vif_scripts has been added to allow the user
decide if he wants to execute the hotplug scripts from xl or from udev. This has
been documented in the xl.conf man page.
Changes since v2:
* Added fancy functions to fetch tap and vif names, now the prefix of the tap
device has been saved in a constant, called TAP_DEVICE_PREFIX.
* Wait for timeout before nuking frontend xenstore entries.
* Changed disable_vif_scripts to disable_xl_vif_scripts, it's easier to
understand.
* Default nic type set by libxl__device_nic_setdefault is VIF now (was IOEMU).
* Save nic type and udev script exection inside
/libxl/devices/<domid>/nic/<devid>/ instead of saving it in the backend path.
Changes since v1:
* Propagated changes from previous patch (disable_udev and libxl_device_nic_add
switch).
* Modified udev rules to add the new env variable.
* Added support for named interfaces.
Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
---
docs/man/xl.conf.pod.5 | 7 ++
tools/examples/xl.conf | 3 +
tools/hotplug/Linux/xen-backend.rules | 6 +-
tools/libxl/libxl.c | 125 +++++++++++++-----------------
tools/libxl/libxl.h | 3 +-
tools/libxl/libxl_create.c | 22 +++++-
tools/libxl/libxl_device.c | 122 +++++++++++++++++++++++++++--
tools/libxl/libxl_dm.c | 2 +-
tools/libxl/libxl_internal.h | 18 ++++-
tools/libxl/libxl_linux.c | 137 ++++++++++++++++++++++++++++++++-
tools/libxl/libxl_types.idl | 1 +
tools/libxl/xl.c | 4 +
tools/libxl/xl.h | 1 +
tools/libxl/xl_cmdimpl.c | 8 ++-
14 files changed, 369 insertions(+), 90 deletions(-)
diff --git a/docs/man/xl.conf.pod.5 b/docs/man/xl.conf.pod.5
index 8bd45ea..da8f16f 100644
--- a/docs/man/xl.conf.pod.5
+++ b/docs/man/xl.conf.pod.5
@@ -55,6 +55,13 @@ default.
Default: C<1>
+=item B<disable_xl_vif_scripts=BOOLEAN>
+
+If enabled xl will not execute nic hotplug scripts itself, and instead
+relegate this task to udev.
+
+Default: C<0>
+
=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..172ff80 100644
--- a/tools/examples/xl.conf
+++ b/tools/examples/xl.conf
@@ -12,3 +12,6 @@
# default output format used by "xl list -l"
#output_format="json"
+
+# default caller of hotplug scripts for vif interfaces
+#disable_xl_vif_scripts=0
diff --git a/tools/hotplug/Linux/xen-backend.rules b/tools/hotplug/Linux/xen-backend.rules
index 19f3d27..5dca574 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{HOTPLUG_GATE}="/libxl/$env{XENBUS_
SUBSYSTEM=="xen-backend", KERNEL=="vbd*", ENV{HOTPLUG_GATE}="/libxl/$env{XENBUS_PATH}/disable_udev", 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{HOTPLUG_GATE}="/libxl/$env{XENBUS_PATH}/disable_udev", ACTION=="online", RUN+="/etc/xen/scripts/vif-setup online type_if=vif"
+SUBSYSTEM=="xen-backend", KERNEL=="vif-*", ENV{HOTPLUG_GATE}="/libxl/$env{XENBUS_PATH}/disable_udev", 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{HOTPLUG_GATE}="/libxl/$env{XENBUS_PATH}/disable_udev", 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=="xentap*", ACTION=="add", RUN+="/etc/xen/scripts/vif-setup $env{ACTION} type_if=tap"
+SUBSYSTEM=="net", KERNEL=="emu*", ACTION=="add", ENV{HOTPLUG_GATE}="/libxl/$env{XENBUS_PATH}/disable_udev", RUN+="/etc/xen/scripts/vif-setup $env{ACTION} type_if=tap"
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index eaa3095..11d3b3b 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1277,46 +1277,6 @@ int libxl__device_disk_setdefault(libxl__gc *gc, libxl_device_disk *disk)
return rc;
}
-static int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
- libxl_device_disk *disk,
- libxl__device *device)
-{
- libxl_ctx *ctx = libxl__gc_owner(gc);
- int devid;
-
- devid = libxl__device_disk_dev_number(disk->vdev, NULL, NULL);
- if (devid==-1) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Invalid or unsupported"
- " virtual disk identifier %s", disk->vdev);
- return ERROR_INVAL;
- }
-
- device->backend_domid = disk->backend_domid;
- device->backend_devid = devid;
-
- switch (disk->backend) {
- case LIBXL_DISK_BACKEND_PHY:
- device->backend_kind = LIBXL__DEVICE_KIND_VBD;
- break;
- case LIBXL_DISK_BACKEND_TAP:
- device->backend_kind = LIBXL__DEVICE_KIND_VBD;
- break;
- case LIBXL_DISK_BACKEND_QDISK:
- device->backend_kind = LIBXL__DEVICE_KIND_QDISK;
- break;
- default:
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend type: %d\n",
- disk->backend);
- return ERROR_INVAL;
- }
-
- device->domid = domid;
- device->devid = devid;
- device->kind = LIBXL__DEVICE_KIND_VBD;
-
- return 0;
-}
-
int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid,
libxl_device_disk *disk,
const libxl_asyncop_how *ao_how)
@@ -1483,7 +1443,7 @@ int libxl_device_disk_remove(libxl_ctx *ctx, uint32_t domid,
rc = libxl__device_from_disk(gc, domid, disk, &device);
if (rc != 0) goto out;
- rc = libxl__initiate_device_remove(egc, ao, &device);
+ rc = libxl__initiate_device_remove(egc, ao, &device, 0);
switch(rc) {
case 1:
/* event added */
@@ -1838,29 +1798,17 @@ 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;
- return 0;
-}
-
-static int libxl__device_from_nic(libxl__gc *gc, uint32_t domid,
- libxl_device_nic *nic,
- libxl__device *device)
-{
- device->backend_devid = nic->devid;
- device->backend_domid = nic->backend_domid;
- device->backend_kind = LIBXL__DEVICE_KIND_VIF;
- device->devid = nic->devid;
- device->domid = domid;
- device->kind = LIBXL__DEVICE_KIND_VIF;
-
+ nic->nictype = LIBXL_NIC_TYPE_VIF;
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);
flexarray_t *front;
flexarray_t *back;
+ flexarray_t *private;
libxl__device device;
char *dompath, **l;
unsigned int nb, rc;
@@ -1873,10 +1821,15 @@ int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic)
rc = ERROR_NOMEM;
goto out;
}
- back = flexarray_make(16, 1);
+ back = flexarray_make(18, 1);
if (!back) {
rc = ERROR_NOMEM;
- goto out_free;
+ goto out_free_f;
+ }
+ private = flexarray_make(4, 1);
+ if (!private) {
+ rc = ERROR_NOMEM;
+ goto out_free_b;
}
if (nic->devid == -1) {
@@ -1936,19 +1889,53 @@ 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)));
+
+ flexarray_append(private, "type");
+ flexarray_append(private, libxl__sprintf(gc, "%s", libxl_nic_type_to_string(
+ nic->nictype)));
+ /* compatibility addon to keep udev rules */
+ if (!nic->disable_xl_vif_script) {
+ flexarray_append(private, "disable_udev");
+ flexarray_append(private, "y");
+ }
+
libxl__device_generic_add(gc, &device,
- libxl__xs_kvs_of_flexarray(gc, back, back->count),
- libxl__xs_kvs_of_flexarray(gc, front, front->count),
- NULL);
+ libxl__xs_kvs_of_flexarray(gc, back, back->count),
+ libxl__xs_kvs_of_flexarray(gc, front, front->count),
+ libxl__xs_kvs_of_flexarray(gc, private, private->count));
+
+ switch(nic->nictype) {
+ case LIBXL_NIC_TYPE_VIF:
+ if (!nic->disable_xl_vif_script) {
+ rc = libxl__initiate_device_add(egc, ao, &device);
+ if (rc) goto out_free;
+ } else {
+ libxl__ao_complete(egc, ao, 0);
+ }
+ break;
+ case LIBXL_NIC_TYPE_IOEMU:
+ /*
+ * Don't execute hotplug scripts for IOEMU interfaces yet,
+ * we need to launch the device model first.
+ *
+ * This are actually launched from do_domain_create after the
+ * domain model has been started.
+ */
+ default:
+ libxl__ao_complete(egc, ao, 0);
+ break;
+ }
- /* FIXME: wait for plug */
rc = 0;
out_free:
+ flexarray_free(private);
+out_free_b:
flexarray_free(back);
+out_free_f:
flexarray_free(front);
out:
- GC_FREE;
- return rc;
+ if (rc) return AO_ABORT(rc);
+ return AO_INPROGRESS;
}
int libxl_device_nic_remove(libxl_ctx *ctx, uint32_t domid,
@@ -1962,7 +1949,7 @@ int libxl_device_nic_remove(libxl_ctx *ctx, uint32_t domid,
rc = libxl__device_from_nic(gc, domid, nic, &device);
if (rc != 0) goto out;
- rc = libxl__initiate_device_remove(egc, ao, &device);
+ rc = libxl__initiate_device_remove(egc, ao, &device, 0);
switch(rc) {
case 1:
/* event added */
@@ -2322,7 +2309,7 @@ int libxl_device_vkb_remove(libxl_ctx *ctx, uint32_t domid,
rc = libxl__device_from_vkb(gc, domid, vkb, &device);
if (rc != 0) goto out;
- rc = libxl__initiate_device_remove(egc, ao, &device);
+ rc = libxl__initiate_device_remove(egc, ao, &device, 0);
switch(rc) {
case 1:
/* event added */
@@ -2467,7 +2454,7 @@ int libxl_device_vfb_remove(libxl_ctx *ctx, uint32_t domid,
rc = libxl__device_from_vfb(gc, domid, vfb, &device);
if (rc != 0) goto out;
- rc = libxl__initiate_device_remove(egc, ao, &device);
+ rc = libxl__initiate_device_remove(egc, ao, &device, 0);
switch(rc) {
case 1:
/* event added */
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 6687e2e..723c3aa 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -629,7 +629,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 de598ad..023159b 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -607,7 +607,7 @@ static int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
}
}
for (i = 0; i < d_config->num_vifs; i++) {
- ret = libxl_device_nic_add(ctx, domid, &d_config->vifs[i]);
+ ret = libxl_device_nic_add(ctx, domid, &d_config->vifs[i], 0);
if (ret) {
LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
"cannot add nic %d to domain: %d", i, ret);
@@ -685,6 +685,26 @@ static int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
"device model did not start: %d", ret);
goto error_out;
}
+ /*
+ * Execute hotplug scripts for tap devices, this has to be done
+ * after the domain model has been started.
+ */
+ for (i = 0; i < d_config->num_vifs; i++) {
+ if (d_config->vifs[i].nictype == LIBXL_NIC_TYPE_IOEMU &&
+ !d_config->vifs[i].disable_xl_vif_script) {
+ libxl__device device;
+ ret = libxl__device_from_nic(gc, domid, &d_config->vifs[i],
+ &device);
+ if (ret < 0) goto error_out;
+ ret = libxl__device_hotplug(gc, &device, CONNECT);
+ if (ret < 0) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+ "unable to launch hotplug script for device: "
+ "%"PRIu32, device.devid);
+ goto error_out;
+ }
+ }
+ }
}
for (i = 0; i < d_config->num_pcidevs; i++)
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index aa19fab..7514290 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -47,6 +47,34 @@ char *libxl__device_private_path(libxl__gc *gc, libxl__device *device)
device->domid, device->devid);
}
+char *libxl__device_vif_name(libxl__gc *gc, libxl__device *dev)
+{
+ char *be_path = libxl__device_private_path(gc, dev);
+ char *ifname = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s",
+ be_path,
+ "vifname"));
+ if (!ifname) {
+ ifname = libxl__sprintf(gc, "vif%u.%d", dev->domid, dev->devid);
+ }
+
+ return ifname;
+}
+
+char *libxl__device_tap_name(libxl__gc *gc, libxl__device *dev)
+{
+ char *be_path = libxl__device_private_path(gc, dev);
+ char *ifname = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s",
+ be_path,
+ "vifname"));
+ if (!ifname) {
+ ifname = libxl__sprintf(gc, TAP_DEVICE_PREFIX"%u.%d", dev->domid, dev->devid);
+ } else {
+ ifname = libxl__sprintf(gc, TAP_DEVICE_PREFIX"-%s", ifname);
+ }
+
+ return ifname;
+}
+
int libxl__parse_backend_path(libxl__gc *gc,
const char *path,
libxl__device *dev)
@@ -269,6 +297,60 @@ char *libxl__device_disk_string_of_backend(libxl_disk_backend backend)
}
}
+int libxl__device_from_nic(libxl__gc *gc, uint32_t domid,
+ libxl_device_nic *nic,
+ libxl__device *device)
+{
+ device->backend_devid = nic->devid;
+ device->backend_domid = nic->backend_domid;
+ device->backend_kind = LIBXL__DEVICE_KIND_VIF;
+ device->devid = nic->devid;
+ device->domid = domid;
+ device->kind = LIBXL__DEVICE_KIND_VIF;
+
+ return 0;
+}
+
+int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
+ libxl_device_disk *disk,
+ libxl__device *device)
+{
+ libxl_ctx *ctx = libxl__gc_owner(gc);
+ int devid;
+
+ devid = libxl__device_disk_dev_number(disk->vdev, NULL, NULL);
+ if (devid==-1) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Invalid or unsupported"
+ " virtual disk identifier %s", disk->vdev);
+ return ERROR_INVAL;
+ }
+
+ device->backend_domid = disk->backend_domid;
+ device->backend_devid = devid;
+
+ switch (disk->backend) {
+ case LIBXL_DISK_BACKEND_PHY:
+ device->backend_kind = LIBXL__DEVICE_KIND_VBD;
+ break;
+ case LIBXL_DISK_BACKEND_TAP:
+ device->backend_kind = LIBXL__DEVICE_KIND_VBD;
+ break;
+ case LIBXL_DISK_BACKEND_QDISK:
+ device->backend_kind = LIBXL__DEVICE_KIND_QDISK;
+ break;
+ default:
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend type: %d\n",
+ disk->backend);
+ return ERROR_INVAL;
+ }
+
+ device->domid = domid;
+ device->devid = devid;
+ device->kind = LIBXL__DEVICE_KIND_VBD;
+
+ return 0;
+}
+
int libxl__device_physdisk_major_minor(const char *physpath, int *major, int *minor)
{
struct stat buf;
@@ -380,6 +462,7 @@ typedef struct {
libxl__ao *ao;
libxl__ev_devstate ds;
libxl__device *dev;
+ int force;
} libxl__ao_device_remove;
typedef struct {
@@ -406,6 +489,16 @@ static void device_remove_callback(libxl__egc *egc, libxl__ev_devstate *ds,
libxl__gc *gc = &aorm->ao->gc;
libxl_ctx *ctx = libxl__gc_owner(gc);
+ if (rc == ERROR_TIMEDOUT && !aorm->force) {
+ LIBXL__LOG(ctx, LIBXL__LOG_WARNING, "unable to remove device %"PRIu32
+ ", destroying frontend to force "
+ "backend disconnection",
+ aorm->dev->devid);
+ libxl__ev_devstate_cancel(gc, &aorm->ds);
+ libxl__initiate_device_remove(egc, aorm->ao, aorm->dev, 1);
+ return;
+ }
+
rc = libxl__device_hotplug(gc, aorm->dev, DISCONNECT);
if (rc)
LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to execute hotplug script for"
@@ -445,26 +538,34 @@ static void device_add_callback(libxl__egc *egc, libxl__ev_devstate *ds,
* libxl__ao_complete when necessary (if no events are added).
*/
int libxl__initiate_device_remove(libxl__egc *egc, libxl__ao *ao,
- libxl__device *dev)
+ libxl__device *dev, int force)
{
AO_GC;
libxl_ctx *ctx = libxl__gc_owner(gc);
- xs_transaction_t t;
+ xs_transaction_t t = 0;
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);
+ char *state;
int rc = 0;
libxl__ao_device_remove *aorm = 0;
+ if (force)
+ libxl__xs_path_cleanup(gc, libxl__device_frontend_path(gc, dev));
+ /*
+ * Nuke frontend to force backend teardown
+ * It's not pretty, but it's the only way that seems to work for all
+ * types of backends
+ */
+
+retry_transaction:
+ t = xs_transaction_start(ctx->xsh);
+ 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);
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)) {
@@ -475,12 +576,15 @@ 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;
aorm->dev = dev;
+ aorm->force = force;
libxl__ev_devstate_init(&aorm->ds);
rc = libxl__ev_devstate_wait(gc, &aorm->ds, device_remove_callback,
@@ -496,8 +600,8 @@ retry_transaction:
return rc;
out_ok:
+ if (t) xs_transaction_end(ctx->xsh, t, 0);
rc = libxl__device_hotplug(gc, dev, DISCONNECT);
- libxl__xs_path_cleanup(gc, libxl__device_frontend_path(gc, dev));
libxl__xs_path_cleanup(gc, be_path);
libxl__xs_path_cleanup(gc, libxl__device_private_path(gc, dev));
return 0;
@@ -596,7 +700,7 @@ int libxl__devices_destroy(libxl__egc *egc, libxl__ao *ao, uint32_t domid)
dev.domid = domid;
dev.kind = kind;
dev.devid = atoi(devs[j]);
- rc = libxl__initiate_device_remove(egc, ao, &dev);
+ rc = libxl__initiate_device_remove(egc, ao, &dev, 0);
switch(rc) {
case 1:
events++;
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 4ce58f6..8cf9d9d 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -794,7 +794,7 @@ retry_transaction:
goto out_free;
}
for (i = 0; i < dm_config.num_vifs; i++) {
- ret = libxl_device_nic_add(ctx, dm_domid, &dm_config.vifs[i]);
+ ret = libxl_device_nic_add(ctx, dm_domid, &dm_config.vifs[i], 0);
if (ret)
goto out_free;
}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 95d5c40..68c7514 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -84,6 +84,7 @@
#define STUBDOM_CONSOLE_RESTORE 2
#define STUBDOM_CONSOLE_SERIAL 3
#define STUBDOM_SPECIAL_CONSOLES 3
+#define TAP_DEVICE_PREFIX "emu"
#define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
@@ -765,6 +766,12 @@ _hidden int libxl__domain_pvcontrol_write(libxl__gc *gc, xs_transaction_t t,
_hidden char *libxl__device_disk_string_of_backend(libxl_disk_backend backend);
_hidden char *libxl__device_disk_string_of_format(libxl_disk_format format);
_hidden int libxl__device_disk_set_backend(libxl__gc*, libxl_device_disk*);
+_hidden int libxl__device_from_nic(libxl__gc *gc, uint32_t domid,
+ libxl_device_nic *nic,
+ libxl__device *device);
+_hidden int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
+ libxl_device_disk *disk,
+ libxl__device *device);
_hidden int libxl__device_physdisk_major_minor(const char *physpath, int *major, int *minor);
_hidden int libxl__device_disk_dev_number(const char *virtpath,
@@ -775,10 +782,13 @@ _hidden int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
libxl__domain_build_state *state);
_hidden int libxl__device_generic_add(libxl__gc *gc, libxl__device *device,
- char **bents, char **fents, char **private);
+ char **bents, char **fents,
+ char **private);
_hidden char *libxl__device_backend_path(libxl__gc *gc, libxl__device *device);
-_hidden char *libxl__device_private_path(libxl__gc *gc, libxl__device *device);
_hidden char *libxl__device_frontend_path(libxl__gc *gc, libxl__device *device);
+_hidden char *libxl__device_private_path(libxl__gc *gc, libxl__device *device);
+_hidden char *libxl__device_vif_name(libxl__gc *gc, libxl__device *dev);
+_hidden char *libxl__device_tap_name(libxl__gc *gc, libxl__device *dev);
_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);
@@ -803,8 +813,8 @@ _hidden int libxl__wait_for_backend(libxl__gc *gc, char *be_path, char *state);
* 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);
+_hidden int libxl__initiate_device_remove(libxl__egc *egc, libxl__ao *ao,
+ libxl__device *dev, int force);
/* Arranges that dev will be added to the guest, and the
* hotplug scripts will be executed (if necessary). When
diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index 7c107dc..745a11a 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -27,6 +27,30 @@ int libxl__try_phy_backend(mode_t st_mode)
}
/* Hotplug scripts helpers */
+
+static libxl_nic_type get_nic_type(libxl__gc *gc, libxl__device *dev)
+{
+ libxl_ctx *ctx = libxl__gc_owner(gc);
+ char *snictype, *pr_path;
+ libxl_nic_type nictype;
+
+ pr_path = libxl__device_private_path(gc, dev);
+ snictype = libxl__xs_read(gc, XBT_NULL,
+ libxl__sprintf(gc, "%s/%s", pr_path, "type"));
+ if (!snictype) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to read nictype from %s",
+ pr_path);
+ return -1;
+ }
+ if (libxl_nic_type_from_string(snictype, &nictype) < 0) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to parse nictype from %s",
+ pr_path);
+ return -1;
+ }
+
+ return nictype;
+}
+
static char **get_hotplug_env(libxl__gc *gc, libxl__device *dev)
{
libxl_ctx *ctx = libxl__gc_owner(gc);
@@ -44,7 +68,7 @@ static char **get_hotplug_env(libxl__gc *gc, libxl__device *dev)
return NULL;
}
- f_env = flexarray_make(9, 1);
+ f_env = flexarray_make(13, 1);
if (!f_env) {
LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
"unable to create environment array");
@@ -63,6 +87,19 @@ static char **get_hotplug_env(libxl__gc *gc, libxl__device *dev)
dev->domid, dev->devid));
flexarray_set(f_env, nr++, "XENBUS_BASE_PATH");
flexarray_set(f_env, nr++, "backend");
+ if (dev->backend_kind == LIBXL__DEVICE_KIND_VIF) {
+ switch (get_nic_type(gc, dev)) {
+ case LIBXL_NIC_TYPE_IOEMU:
+ flexarray_set(f_env, nr++, "INTERFACE");
+ flexarray_set(f_env, nr++, libxl__device_tap_name(gc, dev));
+ case LIBXL_NIC_TYPE_VIF:
+ flexarray_set(f_env, nr++, "vif");
+ flexarray_set(f_env, nr++, libxl__device_vif_name(gc, dev));
+ break;
+ default:
+ return NULL;
+ }
+ }
flexarray_set(f_env, nr++, NULL);
@@ -126,6 +163,101 @@ out_free:
return rc;
}
+static int libxl__hotplug_nic(libxl__gc *gc, libxl__device *dev,
+ libxl__hotplug_action action)
+{
+ libxl_ctx *ctx = libxl__gc_owner(gc);
+ char *be_path = libxl__device_backend_path(gc, dev);
+ char *what, *script;
+ char **args, **env;
+ int nr = 0, rc = 0;
+ libxl_nic_type nictype;
+ flexarray_t *vif_args, *tap_args;
+
+ script = libxl__xs_read(gc, XBT_NULL,
+ libxl__sprintf(gc, "%s/%s", be_path, "script"));
+ if (!script) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to read script from %s",
+ be_path);
+ return -1;
+ }
+
+ nictype = get_nic_type(gc, dev);
+ if (nictype < 0) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "error when fetching nic type");
+ return -1;
+ }
+
+ env = get_hotplug_env(gc, dev);
+ if (!env)
+ return -1;
+
+ vif_args = flexarray_make(4, 1);
+ if (!vif_args) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to create arguments array");
+ return -1;
+ }
+ tap_args = flexarray_make(4, 1);
+ if (!tap_args) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to create arguments array");
+ return -1;
+ }
+
+ switch(nictype) {
+ case LIBXL_NIC_TYPE_IOEMU:
+ flexarray_set(tap_args, nr++, script);
+ flexarray_set(tap_args, nr++, action == CONNECT ? "add" : "remove");
+ flexarray_set(tap_args, nr++, libxl__sprintf(gc, "type_if=tap"));
+ flexarray_set(tap_args, nr++, NULL);
+ args = (char **) flexarray_contents(tap_args);
+ what = libxl__sprintf(gc, "%s %s", args[0],
+ action == CONNECT ? "connect" : "disconnect");
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG,
+ "Calling hotplug script: %s %s %s",
+ args[0], args[1], args[2]);
+ rc = libxl__hotplug_exec(gc, args[0], args, env);
+ if (rc) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable execute hotplug scripts "
+ "for vif device %"PRIu32,
+ dev->devid);
+ goto out;
+ }
+ free(args);
+ nr = 0;
+ case LIBXL_NIC_TYPE_VIF:
+ flexarray_set(vif_args, nr++, script);
+ flexarray_set(vif_args, nr++, action == CONNECT ? "online" : "offline");
+ flexarray_set(vif_args, nr++, libxl__sprintf(gc, "type_if=vif"));
+ flexarray_set(vif_args, nr++, NULL);
+ args = (char **) flexarray_contents(vif_args);
+ what = libxl__sprintf(gc, "%s %s", args[0],
+ action == CONNECT ? "connect" : "disconnect");
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG,
+ "Calling hotplug script: %s %s %s",
+ args[0], args[1], args[2]);
+ rc = libxl__hotplug_exec(gc, args[0], args, env);
+ if (rc) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable execute hotplug scripts "
+ "for vif device %"PRIu32,
+ dev->devid);
+ goto out;
+ }
+ break;
+ default:
+ /* Unknown network type */
+ LIBXL__LOG(ctx, LIBXL__LOG_WARNING, "unknown network card type with "
+ "id %"PRIu32, dev->devid);
+ return 0;
+ }
+
+ rc = 0;
+
+out:
+ free(env);
+ free(args);
+ return rc;
+}
+
int libxl__device_hotplug(libxl__gc *gc, libxl__device *dev,
libxl__hotplug_action action)
{
@@ -135,6 +267,9 @@ int libxl__device_hotplug(libxl__gc *gc, libxl__device *dev,
case LIBXL__DEVICE_KIND_VBD:
rc = libxl__hotplug_disk(gc, dev, action);
break;
+ case LIBXL__DEVICE_KIND_VIF:
+ rc = libxl__hotplug_nic(gc, dev, action);
+ break;
default:
rc = 0;
break;
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 5cf9708..0b2c832 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -343,6 +343,7 @@ libxl_device_nic = Struct("device_nic", [
("ifname", string),
("script", string),
("nictype", libxl_nic_type),
+ ("disable_xl_vif_script", integer),
])
libxl_device_pci = Struct("device_pci", [
diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
index a6ffd25..2f96468 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;
+int disable_xl_vif_scripts = 0;
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;
+ if (!xlu_cfg_get_long (config, "disable_xl_vif_scripts", &l, 0))
+ disable_xl_vif_scripts = l;
+
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 7e258d5..b60cd12 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -109,6 +109,7 @@ void postfork(void);
/* global options */
extern int autoballoon;
+extern int disable_xl_vif_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 0bc3ac2..704d938 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -847,6 +847,12 @@ static void parse_config_data(const char *configfile_filename_report,
nic->script = strdup(default_vifscript);
}
+ /*
+ * Disable nic network script calling, to allow the user
+ * to attach the nic backend from a different domain.
+ */
+ nic->disable_xl_vif_script = disable_xl_vif_scripts;
+
if (default_bridge) {
free(nic->bridge);
nic->bridge = strdup(default_bridge);
@@ -4916,7 +4922,7 @@ int main_networkattach(int argc, char **argv)
return 1;
}
}
- 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] 41+ messages in thread
* [PATCH v3 5/5] libxl: add "downscript=no" to Qemu call
2012-04-20 13:23 [PATCH v3 0/5] libxl: call hotplug scripts from libxl Roger Pau Monne
` (3 preceding siblings ...)
2012-04-20 13:23 ` [PATCH v3 4/5] libxl: call hotplug scripts from libxl for vif Roger Pau Monne
@ 2012-04-20 13:23 ` Roger Pau Monne
2012-04-23 15:38 ` Ian Jackson
2012-04-23 12:12 ` [PATCH v3 0/5] libxl: call hotplug scripts from libxl Marek Marczykowski
5 siblings, 1 reply; 41+ messages in thread
From: Roger Pau Monne @ 2012-04-20 13:23 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne
Currently we only pass script=no to Qemu, to avoid calling any scripts when
attaching a tap interface, but we should also pass downscript=no to avoid Qemu
trying to execute a script when disconnecting the interface. This prevents the
following harmless error message:
/etc/qemu-ifdown: could not launch network script
Changes since v1:
* Indentation fixes.
Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
---
tools/libxl/libxl_dm.c | 27 ++++++++++++++++++---------
1 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8cf9d9d..0f6c4cf 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -216,11 +216,18 @@ static char ** libxl__build_device_model_args_old(libxl__gc *gc,
else
ifname = libxl__sprintf(gc, "xentap-%s", vifs[i].ifname);
flexarray_vappend(dm_args,
- "-net", libxl__sprintf(gc, "nic,vlan=%d,macaddr=%s,model=%s",
- vifs[i].devid, smac, vifs[i].model),
- "-net", libxl__sprintf(gc, "tap,vlan=%d,ifname=%s,bridge=%s,script=%s",
- vifs[i].devid, ifname, vifs[i].bridge, libxl_tapif_script(gc)),
- NULL);
+ "-net",
+ libxl__sprintf(gc,
+ "nic,vlan=%d,macaddr=%s,model=%s",
+ vifs[i].devid, smac, vifs[i].model),
+ "-net",
+ libxl__sprintf(gc,
+ "tap,vlan=%d,ifname=%s,bridge=%s,"
+ "script=%s,downscript=%s",
+ vifs[i].devid, ifname, vifs[i].bridge,
+ libxl_tapif_script(gc),
+ libxl_tapif_script(gc)),
+ NULL);
ioemu_vifs++;
}
}
@@ -462,10 +469,12 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
vifs[i].model, vifs[i].devid,
vifs[i].devid, smac));
flexarray_append(dm_args, "-netdev");
- flexarray_append(dm_args,
- libxl__sprintf(gc, "type=tap,id=net%d,ifname=%s,script=%s",
- vifs[i].devid, ifname,
- libxl_tapif_script(gc)));
+ flexarray_append(dm_args, libxl__sprintf(gc,
+ "type=tap,id=net%d,ifname=%s,"
+ "script=%s,downscript=%s",
+ vifs[i].devid, ifname,
+ libxl_tapif_script(gc),
+ libxl_tapif_script(gc)));
ioemu_vifs++;
}
}
--
1.7.7.5 (Apple Git-26)
^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v3 0/5] libxl: call hotplug scripts from libxl
2012-04-20 13:23 [PATCH v3 0/5] libxl: call hotplug scripts from libxl Roger Pau Monne
` (4 preceding siblings ...)
2012-04-20 13:23 ` [PATCH v3 5/5] libxl: add "downscript=no" to Qemu call Roger Pau Monne
@ 2012-04-23 12:12 ` Marek Marczykowski
2012-04-23 13:31 ` Roger Pau Monne
5 siblings, 1 reply; 41+ messages in thread
From: Marek Marczykowski @ 2012-04-23 12:12 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Joanna Rutkowska, xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 747 bytes --]
On 20.04.2012 15:23, Roger Pau Monne wrote:
> This series removes the use of udev rules to call hotplug scripts when using
> libxl. Scripts are directly called from the toolstack at the necessary points,
> making use of the new event library and it's fork support.
What about non-dom0 backends? There will be no simple way to execute script
there by libxl without help from udev...
In Qubes-OS we heavily use network backend in domU: dom0 have no network at
all, all NICs are attached (as PCI device) to some domU - called NetVM, where
network backend resides.
Also vbd backend in domU is used - eg to boot HVM from iso, which is stored in
some domU.
--
Best Regards / Pozdrawiam,
Marek Marczykowski
Invisible Things Lab
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 554 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 0/5] libxl: call hotplug scripts from libxl
2012-04-23 12:12 ` [PATCH v3 0/5] libxl: call hotplug scripts from libxl Marek Marczykowski
@ 2012-04-23 13:31 ` Roger Pau Monne
2012-04-23 13:47 ` Marek Marczykowski
2012-04-23 14:02 ` [PATCH v3 0/5] libxl: call hotplug scripts from libxl Ian Campbell
0 siblings, 2 replies; 41+ messages in thread
From: Roger Pau Monne @ 2012-04-23 13:31 UTC (permalink / raw)
To: Marek Marczykowski; +Cc: Joanna Rutkowska, xen-devel
Marek Marczykowski escribió:
> On 20.04.2012 15:23, Roger Pau Monne wrote:
>> This series removes the use of udev rules to call hotplug scripts when using
>> libxl. Scripts are directly called from the toolstack at the necessary points,
>> making use of the new event library and it's fork support.
>
> What about non-dom0 backends? There will be no simple way to execute script
> there by libxl without help from udev...
A new config option has been added on this series
(disable_xl_vif_scripts) that allows the user to keep executing vif
scripts from udev, so this functionality is not lost.
> In Qubes-OS we heavily use network backend in domU: dom0 have no network at
> all, all NICs are attached (as PCI device) to some domU - called NetVM, where
> network backend resides.
There should be no problem with that, you will just need to use the new
option.
> Also vbd backend in domU is used - eg to boot HVM from iso, which is stored in
> some domU.
I didn't know you where able to use vbd from driver domains with xl, if
so I will have to add a similar option for vbd devices
(disable_xl_vbd_scripts).
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 0/5] libxl: call hotplug scripts from libxl
2012-04-23 13:31 ` Roger Pau Monne
@ 2012-04-23 13:47 ` Marek Marczykowski
2012-04-23 13:59 ` Non-dom0 block backends (was: Re: [PATCH v3 0/5] libxl: call hotplug scripts from libxl) Joanna Rutkowska
2012-04-23 14:02 ` [PATCH v3 0/5] libxl: call hotplug scripts from libxl Ian Campbell
1 sibling, 1 reply; 41+ messages in thread
From: Marek Marczykowski @ 2012-04-23 13:47 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Joanna Rutkowska, xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 1635 bytes --]
On 23.04.2012 15:31, Roger Pau Monne wrote:
> Marek Marczykowski escribió:
>> On 20.04.2012 15:23, Roger Pau Monne wrote:
>>> This series removes the use of udev rules to call hotplug scripts when using
>>> libxl. Scripts are directly called from the toolstack at the necessary points,
>>> making use of the new event library and it's fork support.
>>
>> What about non-dom0 backends? There will be no simple way to execute script
>> there by libxl without help from udev...
>
> A new config option has been added on this series (disable_xl_vif_scripts)
> that allows the user to keep executing vif scripts from udev, so this
> functionality is not lost.
>
>> In Qubes-OS we heavily use network backend in domU: dom0 have no network at
>> all, all NICs are attached (as PCI device) to some domU - called NetVM, where
>> network backend resides.
>
> There should be no problem with that, you will just need to use the new option.
>
>> Also vbd backend in domU is used - eg to boot HVM from iso, which is stored in
>> some domU.
>
> I didn't know you where able to use vbd from driver domains with xl, if so I
> will have to add a similar option for vbd devices (disable_xl_vbd_scripts).
When starting domU using xl create, I needed to slightly modify disk config
syntax in xl_cmdimpl.c to add backend field (still using xen 4.1, backend
added as the end of disk spec). But everything else worked fine. Especially xl
block-attach, which allow to specify backend domain.
So disable_xl_vbd_scripts option will be helpful.
--
Best Regards / Pozdrawiam,
Marek Marczykowski
Invisible Things Lab
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 554 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Non-dom0 block backends (was: Re: [PATCH v3 0/5] libxl: call hotplug scripts from libxl)
2012-04-23 13:47 ` Marek Marczykowski
@ 2012-04-23 13:59 ` Joanna Rutkowska
0 siblings, 0 replies; 41+ messages in thread
From: Joanna Rutkowska @ 2012-04-23 13:59 UTC (permalink / raw)
To: Marek Marczykowski; +Cc: xen-devel, Roger Pau Monne
[-- Attachment #1.1: Type: text/plain, Size: 1479 bytes --]
On 04/23/12 15:47, Marek Marczykowski wrote:
/.../
>>> >> Also vbd backend in domU is used - eg to boot HVM from iso, which is stored in
>>> >> some domU.
>> >
>> > I didn't know you where able to use vbd from driver domains with xl, if so I
>> > will have to add a similar option for vbd devices (disable_xl_vbd_scripts).
> When starting domU using xl create, I needed to slightly modify disk config
> syntax in xl_cmdimpl.c to add backend field (still using xen 4.1, backend
> added as the end of disk spec). But everything else worked fine. Especially xl
> block-attach, which allow to specify backend domain.
> So disable_xl_vbd_scripts option will be helpful.
On a side note: some cool applications of this:
1) We can have a UsbVM, which has assigned all the USB controllers (pci
attach), which greatly minimizes threats from various USB attacks [1] on
the overall system. Now, if one plugs a USB disk, those disks can be
made available to other domains, without the need for Dom0 to plug them
(so no need to parse their, untrusted, partition tables, or other fs
metadata).
2) We can store various installation ISOs, e.g. that cool new "hacker"
Linux distro ISO, and pass it to an HVM domain (for installation)
directly from the VM where we downloaded it (e.g.
"untrusted-internet-browsing-vm") without the need to store it first on
the Dom0 fs.
joanna.
[1]
http://theinvisiblethings.blogspot.com/2011/06/usb-security-challenges.html
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 0/5] libxl: call hotplug scripts from libxl
2012-04-23 13:31 ` Roger Pau Monne
2012-04-23 13:47 ` Marek Marczykowski
@ 2012-04-23 14:02 ` Ian Campbell
2012-04-23 16:25 ` Roger Pau Monne
1 sibling, 1 reply; 41+ messages in thread
From: Ian Campbell @ 2012-04-23 14:02 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Marek Marczykowski, Joanna Rutkowska, xen-devel
On Mon, 2012-04-23 at 14:31 +0100, Roger Pau Monne wrote:
> Marek Marczykowski escribió:
> > On 20.04.2012 15:23, Roger Pau Monne wrote:
> >> This series removes the use of udev rules to call hotplug scripts when using
> >> libxl. Scripts are directly called from the toolstack at the necessary points,
> >> making use of the new event library and it's fork support.
> >
> > What about non-dom0 backends? There will be no simple way to execute script
> > there by libxl without help from udev...
>
> A new config option has been added on this series
> (disable_xl_vif_scripts) that allows the user to keep executing vif
> scripts from udev, so this functionality is not lost.
In the long term (e.g. for 4.3) we intend to overhaul this in a way
which makes driver domains work without udev at all, see "Driver domains
communication protocol proposal" posted by Ian Jackson several weeks ago
-- it would be good to confirm that the scheme proposed there works well
for Qubes-OS too.
In the short term (i.e. 4.2) we felt it was too late to be making these
sorts of changes (e.g. implementing that complete protocol) and
therefore the compromise is that xl will execute the scripts only in the
case that dom0 is also the backend domain while for driver domains we
retain the pre-4.2 behaviour of executing the hotplug scripts via udev
inside the driver domain. This was necessary in order to fix things such
as teardown of disks on NetBSD and teardown of NICs on openvswitch
(currently both are broken even with backend = dom0 due to short comings
in the previous approach) while not regressing the driver domain use
case.
By default do we write the xenstore key to suppress udev running the
scripts regardless of which domain the backend is in or only for
backend=0? Or is it necessary to use the override config option for
driver domain?
Ian.
> > In Qubes-OS we heavily use network backend in domU: dom0 have no network at
> > all, all NICs are attached (as PCI device) to some domU - called NetVM, where
> > network backend resides.
>
> There should be no problem with that, you will just need to use the new
> option.
>
> > Also vbd backend in domU is used - eg to boot HVM from iso, which is stored in
> > some domU.
>
> I didn't know you where able to use vbd from driver domains with xl, if
> so I will have to add a similar option for vbd devices
> (disable_xl_vbd_scripts).
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 2/5] libxl: add libxl__xs_path_cleanup
2012-04-20 13:23 ` [PATCH v3 2/5] libxl: add libxl__xs_path_cleanup Roger Pau Monne
@ 2012-04-23 15:33 ` Ian Jackson
2012-04-23 15:42 ` Roger Pau Monne
2012-04-23 16:52 ` Ian Jackson
1 sibling, 1 reply; 41+ messages in thread
From: Ian Jackson @ 2012-04-23 15:33 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel
Roger Pau Monne writes ("[Xen-devel] [PATCH v3 2/5] 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.
This is all rather odd. I hadn't previously noticed the existence of
xenstore-rm -t.
With the C xenstored, the RM command will delete a whole directory
tree, regardless of its contents. This is documented in
docs/misc/xenstore.txt.
The comment in xs.c near xs_rm, which says that directories must be
empty, seems to be wrong.
What does oxenstored do ? I'm tempted to say that it should follow
the C xenstored behaviour.
Ian.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 1/5] libxl: allow libxl__exec to take a parameter containing the env variables
2012-04-20 13:23 ` [PATCH v3 1/5] libxl: allow libxl__exec to take a parameter containing the env variables Roger Pau Monne
@ 2012-04-23 15:37 ` Ian Jackson
0 siblings, 0 replies; 41+ messages in thread
From: Ian Jackson @ 2012-04-23 15:37 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel
Roger Pau Monne writes ("[Xen-devel] [PATCH v3 1/5] libxl: allow libxl__exec to take a parameter containing the env variables"):
> Add another parameter to libxl__exec call that contains the
> environment variables to use when performing the execvp call.
This looks OK to me. However, shouldn't this be const char** ?
> struct termios termattr;
> + char *env[] = {"TERM", "vt100", NULL};
We don't compile with -Wwrite-strings but if we did this would
trigger. Also it needs spaces inside { } I think.
> + if (env != NULL) {
> + for (int i = 0; env[i] != NULL && env[i+1] != NULL; i += 2) {
> + setenv(env[i], env[i+1], 1);
setenv can fail. If it does you should probably check errno and
probably call libxl__alloc_failed.
> +/*
> + * env should be passed using the following format,
> + *
> + * env[0]: name of env variable
> + * env[1]: value of env variable
You need to mention that it may have more than one setting!
Ian.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 5/5] libxl: add "downscript=no" to Qemu call
2012-04-20 13:23 ` [PATCH v3 5/5] libxl: add "downscript=no" to Qemu call Roger Pau Monne
@ 2012-04-23 15:38 ` Ian Jackson
0 siblings, 0 replies; 41+ messages in thread
From: Ian Jackson @ 2012-04-23 15:38 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel
Roger Pau Monne writes ("[Xen-devel] [PATCH v3 5/5] libxl: add "downscript=no" to Qemu call"):
> Currently we only pass script=no to Qemu, to avoid calling any scripts when
> attaching a tap interface, but we should also pass downscript=no to avoid Qemu
> trying to execute a script when disconnecting the interface. This prevents the
> following harmless error message:
>
> /etc/qemu-ifdown: could not launch network script
>
> Changes since v1:
>
> * Indentation fixes.
This looks plausible but can you please split out the indentation
change from the functional change ?
Thanks,
Ian.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 2/5] libxl: add libxl__xs_path_cleanup
2012-04-23 15:33 ` Ian Jackson
@ 2012-04-23 15:42 ` Roger Pau Monne
2012-04-23 16:49 ` Ian Jackson
0 siblings, 1 reply; 41+ messages in thread
From: Roger Pau Monne @ 2012-04-23 15:42 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel
Ian Jackson escribió:
> Roger Pau Monne writes ("[Xen-devel] [PATCH v3 2/5] 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.
>
> This is all rather odd. I hadn't previously noticed the existence of
> xenstore-rm -t.
>
> With the C xenstored, the RM command will delete a whole directory
> tree, regardless of its contents. This is documented in
> docs/misc/xenstore.txt.
This is a recursive delete, from top to bottom, let me put an example
which will make this clear, since probably the title is wrong. Imagine
you have the following xenstore entry:
/foo/bar/baz = 123
If you do a:
xenstore-rm /foo/bar/baz
the following will remain in xenstore:
/foo/bar
What this function does is clean empty folders that contained the
deleted entry, so using this function on /foo/bar/baz would have cleaned
the whole directory.
> The comment in xs.c near xs_rm, which says that directories must be
> empty, seems to be wrong.
>
> What does oxenstored do ? I'm tempted to say that it should follow
> the C xenstored behaviour.
>
> Ian.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 0/5] libxl: call hotplug scripts from libxl
2012-04-23 14:02 ` [PATCH v3 0/5] libxl: call hotplug scripts from libxl Ian Campbell
@ 2012-04-23 16:25 ` Roger Pau Monne
2012-04-24 9:18 ` Ian Campbell
0 siblings, 1 reply; 41+ messages in thread
From: Roger Pau Monne @ 2012-04-23 16:25 UTC (permalink / raw)
To: Ian Campbell; +Cc: Marek Marczykowski, Joanna Rutkowska, xen-devel
Ian Campbell escribió:
> On Mon, 2012-04-23 at 14:31 +0100, Roger Pau Monne wrote:
>> Marek Marczykowski escribió:
>>> On 20.04.2012 15:23, Roger Pau Monne wrote:
>>>> This series removes the use of udev rules to call hotplug scripts when using
>>>> libxl. Scripts are directly called from the toolstack at the necessary points,
>>>> making use of the new event library and it's fork support.
>>> What about non-dom0 backends? There will be no simple way to execute script
>>> there by libxl without help from udev...
>> A new config option has been added on this series
>> (disable_xl_vif_scripts) that allows the user to keep executing vif
>> scripts from udev, so this functionality is not lost.
>
> In the long term (e.g. for 4.3) we intend to overhaul this in a way
> which makes driver domains work without udev at all, see "Driver domains
> communication protocol proposal" posted by Ian Jackson several weeks ago
> -- it would be good to confirm that the scheme proposed there works well
> for Qubes-OS too.
>
> In the short term (i.e. 4.2) we felt it was too late to be making these
> sorts of changes (e.g. implementing that complete protocol) and
> therefore the compromise is that xl will execute the scripts only in the
> case that dom0 is also the backend domain while for driver domains we
> retain the pre-4.2 behaviour of executing the hotplug scripts via udev
> inside the driver domain. This was necessary in order to fix things such
> as teardown of disks on NetBSD and teardown of NICs on openvswitch
> (currently both are broken even with backend = dom0 due to short comings
> in the previous approach) while not regressing the driver domain use
> case.
>
> By default do we write the xenstore key to suppress udev running the
> scripts regardless of which domain the backend is in or only for
> backend=0?
For vbd devices we write it everytime, for vifs devices we write it if
disable_xl_vif_scripts is not set.
> Or is it necessary to use the override config option for
> driver domain?
So the new proposal is to add a disable_xl_vbd_scripts and to detect if
the device backend is different than dom0, if it is in fact different
than dom0 don't execute hotplug scripts from xl (this will only work for
vifs, because there's no support for setting the device domain for vbd
devices yet).
> Ian.
>
>>> In Qubes-OS we heavily use network backend in domU: dom0 have no network at
>>> all, all NICs are attached (as PCI device) to some domU - called NetVM, where
>>> network backend resides.
>> There should be no problem with that, you will just need to use the new
>> option.
>>
>>> Also vbd backend in domU is used - eg to boot HVM from iso, which is stored in
>>> some domU.
>> I didn't know you where able to use vbd from driver domains with xl, if
>> so I will have to add a similar option for vbd devices
>> (disable_xl_vbd_scripts).
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd
2012-04-20 13:23 ` [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd Roger Pau Monne
@ 2012-04-23 16:48 ` Ian Jackson
2012-04-24 9:16 ` Ian Campbell
[not found] ` <4F982F18.2080902@citrix.com>
0 siblings, 2 replies; 41+ messages in thread
From: Ian Jackson @ 2012-04-23 16:48 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel
Thanks for this mammoth patch. My comments are below.
Roger Pau Monne writes ("[Xen-devel] [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd"):
> This is a rather big change, that adds the necessary machinery to perform
> hotplug script calling from libxl for Linux. This patch launches the necessary
> scripts to attach and detach PHY and TAP backend types (Qemu doesn't
> use hotplug scripts).
>
> Here's a list of the major changes introduced:
Can you please wrap your commit messages to no more than 75
characters ? Some vcs's indent them. And of course they get indented
when we reply leading to wrap damage.
> * libxl__devices_destroy no longer calls libxl__device_destroy, and instead
> calls libxl__initiate_device_remove, so we can disconnect the device and
> execute the necessary hotplug scripts instead of just deleting the backend
> entries. So libxl__devices_destroy now uses the event library, and so does
> libxl_domain_destroy.
So we no longer have any function which will synchronously and
immediately destroy a domain and throw away everything to do with it.
Is it actually the case that you think such a function cannot be
provided ?
> * Added a check in xen-hotplug-common.sh, so scripts are only executed from
> udev when using xend. xl adds a disable_udev=y to xenstore private directory
> and with the modification of the udev rules every call from udev gets
> HOTPLUG_GATE passed, that points to disable_udev. If HOTPLUG_GATE is passed
> and points to a value, the hotplug script is not executed.
WDYM "points to a value"? Did you just mean "is set to a nonempty
value" ? I'm not convinced by the name of this variable. Shouldn't
it have Xen in the name, and specify its own sense ? Eg,
XEN_HOTPLUG_DISABLE or something ?
> +SUBSYSTEM=="xen-backend", KERNEL=="tap*", ENV{HOTPLUG_GATE}="/libxl/$env{XENBUS_PATH}/disable_udev", RUN+="/etc/xen/scripts/blktap $env{ACTION}"
...
> +# Hack to prevent the execution of hotplug scripts from udev if the domain
> +# has been launched from libxl
> +if [ -n "${HOTPLUG_GATE}" ] && \
> + `xenstore-read "$HOTPLUG_GATE" >/dev/null 2>&1`; then
> + exit 0
> +fi
Oh I see. Hmm. Why should this string not be fixed ? I'm not sure I
like the idea of being able to pass some random other xenstore path.
Also: this xenstore path should be a relative path, ie one relative to
the xenstore home of domain running this part of the tools. That way
the scripts can be easily and automatically disabled for dom0 and
enabled in driver domains.
> + /* compatibility addon to keep udev rules */
> + flexarray_append(private, "disable_udev");
> + flexarray_append(private, "y");
We would normally use "1" for true in xenstore.
> libxl__device_generic_add(gc, &device,
> - libxl__xs_kvs_of_flexarray(gc, back, back->count),
> - libxl__xs_kvs_of_flexarray(gc, front, front->count));
> + libxl__xs_kvs_of_flexarray(gc, back, back->count),
> + libxl__xs_kvs_of_flexarray(gc, front, front->count),
> + libxl__xs_kvs_of_flexarray(gc, private, private->count));
> +
> + switch(disk->backend) {
> + case LIBXL_DISK_BACKEND_PHY:
> + case LIBXL_DISK_BACKEND_TAP:
> + rc = libxl__initiate_device_add(egc, ao, &device);
> + if (rc) goto out_free;
> + break;
> + case LIBXL_DISK_BACKEND_QDISK:
> + default:
> + libxl__ao_complete(egc, ao, 0);
> + break;
> + }
Does this really need no confirmation from qemu ?
> out_free:
> + flexarray_free(private);
> +out_free_b:
> flexarray_free(back);
> +out_free_f:
> flexarray_free(front);
It would be much better to allocate these from the gc somehow.
Failing that, perhaps we could initialise them to NULL and free
them iff necessary on the exit path:
out:
...
if (back) flexarray_free(back);
if (front) flexarray_free(front);
etc.
> int libxl_device_disk_remove(libxl_ctx *ctx, uint32_t domid,
> @@ -1442,7 +1484,18 @@ int libxl_device_disk_remove(libxl_ctx *ctx, uint32_t domid,
> if (rc != 0) goto out;
>
> rc = libxl__initiate_device_remove(egc, ao, &device);
> - if (rc) goto out;
> + switch(rc) {
> + case 1:
> + /* event added */
I think this terminology "event added" is confusingly wrong. What you
mean is "event queued", I think. You should change this everywhere.
But before you do that, please see what I write below about your
approach to libxl__initiate_device_remove.
> @@ -1666,11 +1719,11 @@ 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);
> + libxl_device_disk_add(ctx, domid, disk, 0);
This needs a /* fixme-ao */ because inside-libxl callers are not
allowed to invent an ao_how*, even NULL. You need to mark every
occurrence of these that way.
> @@ -1884,8 +1937,9 @@ int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic)
> flexarray_append(front, libxl__sprintf(gc,
> LIBXL_MAC_FMT, LIBXL_MAC_BYTES(nic->mac)));
> libxl__device_generic_add(gc, &device,
> - libxl__xs_kvs_of_flexarray(gc, back, back->count),
> - libxl__xs_kvs_of_flexarray(gc, front, front->count));
> + libxl__xs_kvs_of_flexarray(gc, back, back->count),
> + libxl__xs_kvs_of_flexarray(gc, front, front->count),
> + NULL);
Likewise. Also unintentional indentation change ? (In several more
places, too.)
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 3675afe..de598ad 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -598,7 +598,7 @@ static int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
> 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]);
> + ret = libxl_device_disk_add(ctx, domid, &d_config->disks[i], 0);
Here in xl simply passing 0 is correct.
> +char *libxl__device_private_path(libxl__gc *gc, libxl__device *device)
> +{
> + return libxl__sprintf(gc, "/libxl/backend/%s/%u/%d",
> + libxl__device_kind_to_string(device->backend_kind),
> + device->domid, device->devid);
> +}
Did you want to use GCSPRINTF ?
> + rc = libxl__device_hotplug(gc, aorm->dev, DISCONNECT);
This is not correct. You need to do something more with the exit
status of the hotplug script than simply throwing it away as you do in
hotplug_exited. libxl__device_hotplug needs to take a callback from
the caller so that it can notify the caller when the hotplug script
has exited.
You need to not allow the ao to complete until the hotplug script has
exited. Otherwise you will enter hotplug_exited after the
libxl__hotplug_state has been destroyed with the ao.
If it's too slow and you need to time out, you need to send the
hotplug script a signal, and then wait for it to exit. If necessary
that signal could be SIGKILL; SIGKILL can be relied on to cause the
hotplug script to actually terminate and thus the hotplug_exited
callback to occur.
> +/*
> + * Return codes:
> + * 1 Success and event(s) HAVE BEEN added
> + * 0 Success and NO events were added
> + * < 0 Error
> + *
> + * Since this function can be called several times, and several events
> + * can be added to the same context, the user has to call
> + * libxl__ao_complete when necessary (if no events are added).
> + */
> int libxl__initiate_device_remove(libxl__egc *egc, libxl__ao *ao,
> libxl__device *dev)
This comment should be in the header file near the declaration.
And indeed if you had put it there you would have discovered another
comment already there describing the old behaviour, which you have now
left behind as an erroneous comment.
And I think the new interface is entirely wrong. How does the caller
know when to complete the ao if libxl__initiate_device_remove never
calls back ?
You need to split this into two functions: one with the current
interface which is a simple wrapper, used for all the existing call
sites, and one which never completes any ao but which always makes a
callback when the operation is complete. That callback should be used
by the caller of libxl__initiate_device_remove to do whatever it needs
to, which might include completing the ao.
At the moment, AFAICT from reading your code, the caller's ao will be
completed by the first nontrivial device remove to complete, ie a bug.
> -int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
> +int libxl__initiate_device_add(libxl__egc *egc, libxl__ao *ao,
> + libxl__device *dev)
...
> + rc = libxl__ev_devstate_wait(gc, &aorm->ds, device_add_callback,
> + state_path, XenbusStateInitWait,
> + LIBXL_INIT_TIMEOUT * 1000);
> + if (rc) {
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to initialize device %"
> + PRIu32, dev->devid);
> + libxl__ev_devstate_cancel(gc, &aorm->ds);
This cancel is not necessary, because device_add_cleanup will do
this. (Or at least it should do; I haven't checked.)
> + goto out_fail;
> + }
> +
> + return 0;
> +
> +out_fail:
> + assert(rc);
> + device_add_cleanup(gc, aorm);
> + return rc;
> +out_ok:
^ blank line needed after the return.
> + rc = libxl__device_hotplug(gc, dev, CONNECT);
> + libxl__ao_complete(egc, ao, 0);
> + return rc;
> +}
Some of my comments about initiate_device_remove's callback probably
apply here too. In particular, I have found it is often better to
make a callback-queueing function always call the callback right away,
recursively, on the error path. The result is that the function can
return void, which simplifies callers considerably.
Whichever way you do it you do need to document your choices about
reentrancy.
> +static void hotplug_exited(libxl__egc *egc, libxl__ev_child *child,
> + pid_t pid, int status)
> +{
> + libxl__hotplug_state *hs = CONTAINER_OF(child, *hs, child);
> + EGC_GC;
> +
> + if (status) {
> + libxl_report_child_exitstatus(CTX, hs->rc ? LIBXL__LOG_ERROR
> + : LIBXL__LOG_WARNING,
> + "hotplug child", pid, status);
> + }
As I say, this needs to make a callback.
> +int libxl__hotplug_exec(libxl__gc *gc, char *arg0, char **args, char **env)
> +{
It would be better IMO to call this function "launch" or something,
since it doesn't actually call exec in the parent (which is what I
think a function called "exec" should do).
> + libxl__hotplug_state *hs;
> +
> + hs = libxl__zalloc(gc, sizeof(*hs));
How about
GCNEW(hs)
?
> + if (!pid) {
> + /* child */
> + signal(SIGCHLD, SIG_DFL);
What is this for ? Is the problem that children of libxl otherwise
inherit a weird SIGCHLD disposition ? If so you need to fix this in
libxl__exec, probably in a separate patch - and you probably need to
sort out sigprocmask too in case the libxl caller has set up something
weird. This is something that ought to go in a new patch.
> + if (libxl__ev_child_inuse(&hs->child)) {
> + hs->rc = rc;
Under what circumstances will rc!=0 here ? Surely that is the success
path? It might be easier simply to have "out:" be only the error
path. The success path always has _inuse and the failure path never
does.
> +/* Action to pass to hotplug caller functions */
> +typedef enum {
> + CONNECT = 1,
> + DISCONNECT = 2
> +} libxl__hotplug_action;
These names are rather too generic, I think.
> +typedef struct libxl__hotplug_state libxl__hotplug_state;
> +
> +struct libxl__hotplug_state {
> + /* public, result, caller may only read in callback */
> + int rc;
> + /* private for implementation */
> + libxl__ev_child child;
> +};
And this is where the callback member ought to go, in the public
part, with a note saying it should be set by the caller.
Is there any provision for timeout here ? Would here be a good place
to do the timeout, rather than higher up the stack ?
Also you might find it better to move the args and env and so forth
into the hotplug_state. That way, for example, you can actually
produce a useful message when the script fails.
> +/*
> + * libxl__hotplug_exec is an internal function that should not be used
> + * directly, all hotplug script calls have to implement and use
> + * libxl__device_hotplug.
> + */
> +_hidden int libxl__device_hotplug(libxl__gc *gc, libxl__device *dev,
> + libxl__hotplug_action action);
> +_hidden int libxl__hotplug_exec(libxl__gc *gc, char *arg0, char **args,
> + char **env);
Why does libxl__hotplug_exec need to be declared here at all then ?
Could it be static in libxl_hotplug.c ?
> +/* Hotplug scripts helpers */
> +static char **get_hotplug_env(libxl__gc *gc, libxl__device *dev)
> +{
> + libxl_ctx *ctx = libxl__gc_owner(gc);
Why not use CTX ? For that matter, if you use my new LOG macros you
don't need to refer to CTX at all.
> + script = libxl__xs_read(gc, XBT_NULL,
> + libxl__sprintf(gc, "%s/%s", be_path, "script"));
> + if (!script) {
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to read script from %s",
> + be_path);
You need to log the errno.
> + f_env = flexarray_make(9, 1);
> + if (!f_env) {
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> + "unable to create environment array");
> + return NULL;
> + }
Isn't this an allocation failure which should be dealt with by
libxl__alloc_failed ?
> + flexarray_set(f_env, nr++, "XENBUS_TYPE");
> + flexarray_set(f_env, nr++, (char *)
> + libxl__device_kind_to_string(dev->backend_kind));
Why is the cast needed ?
> + flexarray_set(f_env, nr++, "XENBUS_PATH");
> + flexarray_set(f_env, nr++,
> + libxl__sprintf(gc, "backend/%s/%u/%d",
> + libxl__device_kind_to_string(dev->backend_kind),
> + dev->domid, dev->devid));
GCSPRINTF might help shorten this ?
For that matter, you've called libxl__device_kind_to_string twice.
Calling it only once would make this easier to read.
I won't comment any more on places where I think GCSPRINTF,
LOG/LOGE/LOGEV and CTX might usefully replace what you have written.
> + env = get_hotplug_env(gc, dev);
> + if (!env)
> + return -1;
Surely this should never fail as it just results in allocation failure ?
This function seems to be supposed to return an rc value, so return -1
is wrong.
> + args = (char **) flexarray_contents(f_args);
Why is the cast needed ?
> + what = libxl__sprintf(gc, "%s %s", args[0],
> + action == CONNECT ? "connect" : "disconnect");
Surely
action == CONNECT ? "connect" :
action == DISCONNECT ? "disconnect" : abort()
?
> + rc = libxl__hotplug_exec(gc, args[0], args, env);
> + if (rc) {
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable execute hotplug scripts for "
> + "device %"PRIu32, dev->devid);
libxl__hotplug_exec ought to do all the logging. That means that all
the information needed to do so should be passed to it, including the
domid (which you don't currently log) and the device id. That should
probably be filled in by its caller in the libxl__hotplug_state
struct.
> diff --git a/tools/python/xen/lowlevel/xl/xl.c b/tools/python/xen/lowlevel/xl/xl.c
> index c4f7c52..ce7a2b2 100644
> --- a/tools/python/xen/lowlevel/xl/xl.c
> +++ b/tools/python/xen/lowlevel/xl/xl.c
> @@ -447,7 +447,7 @@ static PyObject *pyxl_domain_destroy(XlObject *self, PyObject *args)
> int domid;
> if ( !PyArg_ParseTuple(args, "i", &domid) )
> return NULL;
> - if ( libxl_domain_destroy(self->ctx, domid) ) {
> + if ( libxl_domain_destroy(self->ctx, domid, NULL) ) {
I think this is correct. Or, at least, we don't currently provide any
asynchronous capability in the Python code and I don't mind not doing
so.
Ian.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 2/5] libxl: add libxl__xs_path_cleanup
2012-04-23 15:42 ` Roger Pau Monne
@ 2012-04-23 16:49 ` Ian Jackson
0 siblings, 0 replies; 41+ messages in thread
From: Ian Jackson @ 2012-04-23 16:49 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel
Roger Pau Monne writes ("Re: [Xen-devel] [PATCH v3 2/5] libxl: add libxl__xs_path_cleanup"):
> Ian Jackson escribió:
> > With the C xenstored, the RM command will delete a whole directory
> > tree, regardless of its contents. This is documented in
> > docs/misc/xenstore.txt.
>
> This is a recursive delete, from top to bottom, let me put an example
> which will make this clear, since probably the title is wrong. Imagine
> you have the following xenstore entry:
>
> /foo/bar/baz = 123
>
> If you do a:
>
> xenstore-rm /foo/bar/baz
>
> the following will remain in xenstore:
>
> /foo/bar
>
> What this function does is clean empty folders that contained the
> deleted entry, so using this function on /foo/bar/baz would have cleaned
> the whole directory.
Oh! This was quite unclear to me and I didn't read your code closely
enough to spot this. I'll go back and read your patch again.
Ian.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 2/5] libxl: add libxl__xs_path_cleanup
2012-04-20 13:23 ` [PATCH v3 2/5] libxl: add libxl__xs_path_cleanup Roger Pau Monne
2012-04-23 15:33 ` Ian Jackson
@ 2012-04-23 16:52 ` Ian Jackson
2012-04-25 13:19 ` Roger Pau Monne
2012-05-02 9:44 ` Roger Pau Monne
1 sibling, 2 replies; 41+ messages in thread
From: Ian Jackson @ 2012-04-23 16:52 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel
Roger Pau Monne writes ("[Xen-devel] [PATCH v3 2/5] libxl: add libxl__xs_path_cleanup"):
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index c7e057d..36d58cd 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -356,7 +356,6 @@ int libxl__device_disk_dev_number(const char *virtpath, int *pdisk,
> return -1;
> }
>
> -
Unrelated whitespace change.
> +/*
> + * Perfrom recursive cleanup of xenstore path, from top to bottom
> + * just like xenstore-rm -t
> + */
> +_hidden int libxl__xs_path_cleanup(libxl__gc *gc, char *path);
I think, following my confusion, that this needs some better
documentation comment.
> diff --git a/tools/libxl/libxl_xshelp.c b/tools/libxl/libxl_xshelp.c
> index 3ea8d08..0b1b844 100644
> --- a/tools/libxl/libxl_xshelp.c
> +++ b/tools/libxl/libxl_xshelp.c
> @@ -135,6 +135,28 @@ char *libxl__xs_libxl_path(libxl__gc *gc, uint32_t domid)
> return s;
> }
>
> +int libxl__xs_path_cleanup(libxl__gc *gc, char *path)
> +{
> + libxl_ctx *ctx = libxl__gc_owner(gc);
> + unsigned int nb = 0;
> + char *last;
> +
> + if (!path)
> + return 0;
> +
> + xs_rm(ctx->xsh, XBT_NULL, path);
> +
> + for (last = strrchr(path, '/'); last != NULL; last = strrchr(path, '/')) {
If the path is relative, this won't work correctly.
Also this whole thing needs to take place in a transaction, or it is
racy. Probably a transaction supplied by the caller, in which case
you should assert it.
> + *last = '\0';
> + if (!libxl__xs_directory(gc, XBT_NULL, path, &nb))
> + continue;
If this fails, it should be a fatal error; we should not just blunder
on.
Ian.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 4/5] libxl: call hotplug scripts from libxl for vif
2012-04-20 13:23 ` [PATCH v3 4/5] libxl: call hotplug scripts from libxl for vif Roger Pau Monne
@ 2012-04-23 16:59 ` Ian Jackson
2012-04-24 9:18 ` Ian Campbell
2012-04-25 11:19 ` Ian Campbell
1 sibling, 1 reply; 41+ messages in thread
From: Ian Jackson @ 2012-04-23 16:59 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel
Roger Pau Monne writes ("[Xen-devel] [PATCH v3 4/5] libxl: call hotplug scripts from libxl for vif"):
> As the previous change already introduces most of needed machinery to call
> hotplug scripts from libxl, this only adds the necessary bits to call this
> scripts for vif interfaces also.
I have read this only cursorily, given my comments on the previous
patch, but:
> + char *ifname = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s",
> + be_path,
> + "vifname"));
Probably all of these functions need to take a xenstore transaction to
deal with concurrency properly.
You need to avoid assuming that NULL from libxl__xs_* means what you
think it means. It might be better to invent a new wrapper which you
could use like this
rc = libxl__xs_read_that_actually_works(gc, trans, path, &result);
if (rc) goto out;
if (!result) {
/* code for if thing doesn't exist */
}
GCSPRINTF might avoid the weird wrapping.
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 5cf9708..0b2c832 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -343,6 +343,7 @@ libxl_device_nic = Struct("device_nic", [
> ("ifname", string),
> ("script", string),
> ("nictype", libxl_nic_type),
> + ("disable_xl_vif_script", integer),
> ])
We shouldn't have config options of the form "disable_...". They
should all be phrased as "enable" or in this case "run". And I'm not
convinced by "xl", but since there isn't any patch to the xl domain
configuration doc for this new option I'm not sure. Of course
documentation is needed...
Ian.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd
2012-04-23 16:48 ` Ian Jackson
@ 2012-04-24 9:16 ` Ian Campbell
2012-04-24 10:09 ` Ian Jackson
[not found] ` <4F982F18.2080902@citrix.com>
1 sibling, 1 reply; 41+ messages in thread
From: Ian Campbell @ 2012-04-24 9:16 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel, Roger Pau Monne
On Mon, 2012-04-23 at 17:48 +0100, Ian Jackson wrote:
> > * libxl__devices_destroy no longer calls libxl__device_destroy, and instead
> > calls libxl__initiate_device_remove, so we can disconnect the device and
> > execute the necessary hotplug scripts instead of just deleting the backend
> > entries. So libxl__devices_destroy now uses the event library, and so does
> > libxl_domain_destroy.
>
> So we no longer have any function which will synchronously and
> immediately destroy a domain and throw away everything to do with it.
> Is it actually the case that you think such a function cannot be
> provided ?
The difference between remove and destroy is documented in libxl.h and
this patch does not change those definitions:
* libxl_device_<type>_remove(ctx, domid, device):
*
* Removes the given device from the specified domain by performing
* an orderly unplug with guest co-operation. This requires that the
* guest is running.
*
* This method is currently synchronous and therefore can block
* while interacting with the guest.
*
* libxl_device_<type>_destroy(ctx, domid, device):
*
* Removes the given device from the specified domain without guest
* co-operation. It is guest specific what affect this will have on
* a running guest.
*
* This function does not interact with the guest and therefore
* cannot block on the guest.
These descriptions must be updated to refer to the new async behaviour.
Does the new implementation of destroy meet these requirements (modulo
now being asynchronous)?
The documentation does not currently mention blocking on the backend
domain, could you please add a comment which describes what the new
requirements are in this respect. Likewise hotplug scripts are not
mentioned in these docs.
In principal I think I am happy with the possibility to block
temporarily with a timeout on a backend domain but ultimately we need a
timeout and a fallback to the "just kill it" mode.
What happens here in 4.3 when we split the hotplug state out according
to whatever becomes of the "Driver domains communication protocol
proposal" thread? At that point the hotplug script state will be
separate from the backend state and we can go back to the "just nuke it"
mode, can't we? Would there be a regression vs. xend for us to stick
with the nuke it mode for 4.2 and fix this properly in 4.3?
I think it is important in the context of this patch to be clear about
what is the desired long term behaviour compared with the short term
behaviour being implemented for 4.2 is. We should also be clear about
what is being done now in order to address xl regressions vs xend. We
should also be clear about what is just papering over an issue for 4.2
vs what the proper fix in 4.3 will be. We also need to know what is
actually new functionality or behaviour (i.e. not fixing an xl vs xend
regression). IOW we need to have clear descriptions of the reasons for
the changes not just what the changes.
I think all the above need to be written down explicitly in either the
commit message or the introductory email, otherwise the review of this
series is just going to continue to go round in circles -- the reasoning
behind these changes is just too complex for a reviewer (even one who is
familiar with all this stuff already) to hold in their head.
> > +SUBSYSTEM=="xen-backend", KERNEL=="tap*", ENV{HOTPLUG_GATE}="/libxl/$env{XENBUS_PATH}/disable_udev", RUN+="/etc/xen/scripts/blktap $env{ACTION}"
> ...
> > +# Hack to prevent the execution of hotplug scripts from udev if the domain
> > +# has been launched from libxl
> > +if [ -n "${HOTPLUG_GATE}" ] && \
> > + `xenstore-read "$HOTPLUG_GATE" >/dev/null 2>&1`; then
> > + exit 0
> > +fi
>
> Oh I see. Hmm. Why should this string not be fixed ? I'm not sure I
> like the idea of being able to pass some random other xenstore path.
>
> Also: this xenstore path should be a relative path, ie one relative to
> the xenstore home of domain running this part of the tools. That way
> the scripts can be easily and automatically disabled for dom0 and
> enabled in driver domains.
XENBUS_PATH contains elements for both the back- and frontend domains as
well as the specific device.
Or do you think the key should be global per-(backend-domain rather than
per-device?
> > +/* Action to pass to hotplug caller functions */
> > +typedef enum {
> > + CONNECT = 1,
> > + DISCONNECT = 2
> > +} libxl__hotplug_action;
>
> These names are rather too generic, I think.
enums should also be declared in lixl_types_internal.idl
Ian.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 0/5] libxl: call hotplug scripts from libxl
2012-04-23 16:25 ` Roger Pau Monne
@ 2012-04-24 9:18 ` Ian Campbell
0 siblings, 0 replies; 41+ messages in thread
From: Ian Campbell @ 2012-04-24 9:18 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Marek Marczykowski, Joanna Rutkowska, xen-devel
On Mon, 2012-04-23 at 17:25 +0100, Roger Pau Monne wrote:
> Ian Campbell escribió:
> > On Mon, 2012-04-23 at 14:31 +0100, Roger Pau Monne wrote:
> >> Marek Marczykowski escribió:
> >>> On 20.04.2012 15:23, Roger Pau Monne wrote:
> >>>> This series removes the use of udev rules to call hotplug scripts when using
> >>>> libxl. Scripts are directly called from the toolstack at the necessary points,
> >>>> making use of the new event library and it's fork support.
> >>> What about non-dom0 backends? There will be no simple way to execute script
> >>> there by libxl without help from udev...
> >> A new config option has been added on this series
> >> (disable_xl_vif_scripts) that allows the user to keep executing vif
> >> scripts from udev, so this functionality is not lost.
> >
> > In the long term (e.g. for 4.3) we intend to overhaul this in a way
> > which makes driver domains work without udev at all, see "Driver domains
> > communication protocol proposal" posted by Ian Jackson several weeks ago
> > -- it would be good to confirm that the scheme proposed there works well
> > for Qubes-OS too.
> >
> > In the short term (i.e. 4.2) we felt it was too late to be making these
> > sorts of changes (e.g. implementing that complete protocol) and
> > therefore the compromise is that xl will execute the scripts only in the
> > case that dom0 is also the backend domain while for driver domains we
> > retain the pre-4.2 behaviour of executing the hotplug scripts via udev
> > inside the driver domain. This was necessary in order to fix things such
> > as teardown of disks on NetBSD and teardown of NICs on openvswitch
> > (currently both are broken even with backend = dom0 due to short comings
> > in the previous approach) while not regressing the driver domain use
> > case.
> >
> > By default do we write the xenstore key to suppress udev running the
> > scripts regardless of which domain the backend is in or only for
> > backend=0?
>
> For vbd devices we write it everytime, for vifs devices we write it if
> disable_xl_vif_scripts is not set.
>
> > Or is it necessary to use the override config option for
> > driver domain?
>
> So the new proposal is to add a disable_xl_vbd_scripts and to detect if
> the device backend is different than dom0, if it is in fact different
> than dom0 don't execute hotplug scripts from xl (this will only work for
> vifs, because there's no support for setting the device domain for vbd
> devices yet).
If we can detect I don't see why we shouldn't, unless it turns out to
add complexity which we want to defer until 4.3.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 4/5] libxl: call hotplug scripts from libxl for vif
2012-04-23 16:59 ` Ian Jackson
@ 2012-04-24 9:18 ` Ian Campbell
0 siblings, 0 replies; 41+ messages in thread
From: Ian Campbell @ 2012-04-24 9:18 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel, Roger Pau Monne
On Mon, 2012-04-23 at 17:59 +0100, Ian Jackson wrote:
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index 5cf9708..0b2c832 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -343,6 +343,7 @@ libxl_device_nic = Struct("device_nic", [
> > ("ifname", string),
> > ("script", string),
> > ("nictype", libxl_nic_type),
> > + ("disable_xl_vif_script", integer),
> > ])
>
> We shouldn't have config options of the form "disable_...". They
> should all be phrased as "enable" or in this case "run". And I'm not
> convinced by "xl", but since there isn't any patch to the xl domain
> configuration doc for this new option I'm not sure. Of course
> documentation is needed...
Also this should either be a bool, or perhaps a libxl_defbool if we want
to do auto detection.
Ian.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd
2012-04-24 9:16 ` Ian Campbell
@ 2012-04-24 10:09 ` Ian Jackson
2012-04-24 10:17 ` Ian Campbell
0 siblings, 1 reply; 41+ messages in thread
From: Ian Jackson @ 2012-04-24 10:09 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel, Roger Pau Monne
Ian Campbell writes ("Re: [Xen-devel] [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd"):
> I think it is important in the context of this patch to be clear about
> what is the desired long term behaviour compared with the short term
> behaviour being implemented for 4.2 is. We should also be clear about
> what is being done now in order to address xl regressions vs xend. We
> should also be clear about what is just papering over an issue for 4.2
> vs what the proper fix in 4.3 will be. We also need to know what is
> actually new functionality or behaviour (i.e. not fixing an xl vs xend
> regression). IOW we need to have clear descriptions of the reasons for
> the changes not just what the changes.
Yes.
> I think all the above need to be written down explicitly in either the
> commit message or the introductory email, otherwise the review of this
> series is just going to continue to go round in circles -- the reasoning
> behind these changes is just too complex for a reviewer (even one who is
> familiar with all this stuff already) to hold in their head.
Quite...
> > Also: this xenstore path should be a relative path, ie one relative to
> > the xenstore home of domain running this part of the tools. That way
> > the scripts can be easily and automatically disabled for dom0 and
> > enabled in driver domains.
>
> XENBUS_PATH contains elements for both the back- and frontend domains as
> well as the specific device.
>
> Or do you think the key should be global per-(backend-domain rather than
> per-device?
The latter.
> > These names are rather too generic, I think.
>
> enums should also be declared in lixl_types_internal.idl
Surely an enum which doesn't escape from inside libxl and which never
needs to be printed can just be an enum ?
Ian.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd
2012-04-24 10:09 ` Ian Jackson
@ 2012-04-24 10:17 ` Ian Campbell
2012-04-24 10:27 ` Ian Jackson
0 siblings, 1 reply; 41+ messages in thread
From: Ian Campbell @ 2012-04-24 10:17 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel, Roger Pau Monne
On Tue, 2012-04-24 at 11:09 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd"):
> > > Also: this xenstore path should be a relative path, ie one relative to
> > > the xenstore home of domain running this part of the tools. That way
> > > the scripts can be easily and automatically disabled for dom0 and
> > > enabled in driver domains.
> >
> > XENBUS_PATH contains elements for both the back- and frontend domains as
> > well as the specific device.
> >
> > Or do you think the key should be global per-(backend-domain rather than
> > per-device?
>
> The latter.
So /libxl/$XENBUS_PATH is ok then?
> > > These names are rather too generic, I think.
> >
> > enums should also be declared in lixl_types_internal.idl
>
> Surely an enum which doesn't escape from inside libxl and which never
> needs to be printed can just be an enum ?
Sure.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd
2012-04-24 10:17 ` Ian Campbell
@ 2012-04-24 10:27 ` Ian Jackson
2012-04-24 10:29 ` Ian Campbell
0 siblings, 1 reply; 41+ messages in thread
From: Ian Jackson @ 2012-04-24 10:27 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel, Roger Pau Monne
Ian Campbell writes ("Re: [Xen-devel] [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd"):
> On Tue, 2012-04-24 at 11:09 +0100, Ian Jackson wrote:
> > Ian Campbell writes ("Re: [Xen-devel] [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd"):
>
> > > XENBUS_PATH contains elements for both the back- and frontend domains as
> > > well as the specific device.
> > >
> > > Or do you think the key should be global per-(backend-domain rather than
> > > per-device?
> >
> > The latter.
>
> So /libxl/$XENBUS_PATH is ok then?
?
I was suggesting that the key should be global, and XENBUS_PATH
contains too much.
Ian.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd
2012-04-24 10:27 ` Ian Jackson
@ 2012-04-24 10:29 ` Ian Campbell
2012-04-24 10:37 ` Ian Jackson
0 siblings, 1 reply; 41+ messages in thread
From: Ian Campbell @ 2012-04-24 10:29 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel, Roger Pau Monne
On Tue, 2012-04-24 at 11:27 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd"):
> > On Tue, 2012-04-24 at 11:09 +0100, Ian Jackson wrote:
> > > Ian Campbell writes ("Re: [Xen-devel] [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd"):
> >
> > > > XENBUS_PATH contains elements for both the back- and frontend domains as
> > > > well as the specific device.
> > > >
> > > > Or do you think the key should be global per-(backend-domain rather than
> > > > per-device?
> > >
> > > The latter.
> >
> > So /libxl/$XENBUS_PATH is ok then?
>
> ?
>
> I was suggesting that the key should be global, and XENBUS_PATH
> contains too much.
Ah, I saw "latter" and read the last bit of my sentence "per-device", my
mistake.
Ian.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd
2012-04-24 10:29 ` Ian Campbell
@ 2012-04-24 10:37 ` Ian Jackson
2012-04-25 13:53 ` Roger Pau Monne
0 siblings, 1 reply; 41+ messages in thread
From: Ian Jackson @ 2012-04-24 10:37 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel, Roger Pau Monne
Ian Campbell writes ("Re: [Xen-devel] [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd"):
> On Tue, 2012-04-24 at 11:27 +0100, Ian Jackson wrote:
> > I was suggesting that the key should be global, and XENBUS_PATH
> > contains too much.
>
> Ah, I saw "latter" and read the last bit of my sentence "per-device", my
> mistake.
Right.
The reason I think it should be global is that this is a transitional
bodge to make driver domains continue to work pending a proper
sorting-out in 4.3. So there is no need to make it fully general.
Ian.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 4/5] libxl: call hotplug scripts from libxl for vif
2012-04-20 13:23 ` [PATCH v3 4/5] libxl: call hotplug scripts from libxl for vif Roger Pau Monne
2012-04-23 16:59 ` Ian Jackson
@ 2012-04-25 11:19 ` Ian Campbell
2012-04-25 11:24 ` Roger Pau Monne
1 sibling, 1 reply; 41+ messages in thread
From: Ian Campbell @ 2012-04-25 11:19 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel
On Fri, 2012-04-20 at 14:23 +0100, Roger Pau Monne wrote:
>
> * Added fancy functions to fetch tap and vif names, now the prefix of
> the tap
> device has been saved in a constant, called TAP_DEVICE_PREFIX.
You missed the device name construction in
libxl__build_device_model_args_*.
Ian.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 4/5] libxl: call hotplug scripts from libxl for vif
2012-04-25 11:19 ` Ian Campbell
@ 2012-04-25 11:24 ` Roger Pau Monne
0 siblings, 0 replies; 41+ messages in thread
From: Roger Pau Monne @ 2012-04-25 11:24 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel
Ian Campbell escribió:
> On Fri, 2012-04-20 at 14:23 +0100, Roger Pau Monne wrote:
>> * Added fancy functions to fetch tap and vif names, now the prefix of
>> the tap
>> device has been saved in a constant, called TAP_DEVICE_PREFIX.
>
> You missed the device name construction in
> libxl__build_device_model_args_*.
Didn't you have that on your patch? Anyway, please take this functions
and merge them with your own series/patch, this is a mess.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 2/5] libxl: add libxl__xs_path_cleanup
2012-04-23 16:52 ` Ian Jackson
@ 2012-04-25 13:19 ` Roger Pau Monne
2012-05-02 9:44 ` Roger Pau Monne
1 sibling, 0 replies; 41+ messages in thread
From: Roger Pau Monne @ 2012-04-25 13:19 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel
Ian Jackson escribió:
> Roger Pau Monne writes ("[Xen-devel] [PATCH v3 2/5] libxl: add libxl__xs_path_cleanup"):
>> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
>> index c7e057d..36d58cd 100644
>> --- a/tools/libxl/libxl_device.c
>> +++ b/tools/libxl/libxl_device.c
>> @@ -356,7 +356,6 @@ int libxl__device_disk_dev_number(const char *virtpath, int *pdisk,
>> return -1;
>> }
>>
>> -
>
> Unrelated whitespace change.
Done.
>> +/*
>> + * Perfrom recursive cleanup of xenstore path, from top to bottom
>> + * just like xenstore-rm -t
>> + */
>> +_hidden int libxl__xs_path_cleanup(libxl__gc *gc, char *path);
>
> I think, following my confusion, that this needs some better
> documentation comment.
Ok, I've tried to change to something more self-explaining.
>> diff --git a/tools/libxl/libxl_xshelp.c b/tools/libxl/libxl_xshelp.c
>> index 3ea8d08..0b1b844 100644
>> --- a/tools/libxl/libxl_xshelp.c
>> +++ b/tools/libxl/libxl_xshelp.c
>> @@ -135,6 +135,28 @@ char *libxl__xs_libxl_path(libxl__gc *gc, uint32_t domid)
>> return s;
>> }
>>
>> +int libxl__xs_path_cleanup(libxl__gc *gc, char *path)
>> +{
>> + libxl_ctx *ctx = libxl__gc_owner(gc);
>> + unsigned int nb = 0;
>> + char *last;
>> +
>> + if (!path)
>> + return 0;
>> +
>> + xs_rm(ctx->xsh, XBT_NULL, path);
>> +
>> + for (last = strrchr(path, '/'); last != NULL; last = strrchr(path, '/')) {
>
> If the path is relative, this won't work correctly.
Are you sure? From what I've tested it seems to work ok.
>
> Also this whole thing needs to take place in a transaction, or it is
> racy. Probably a transaction supplied by the caller, in which case
> you should assert it.
Ok, will add another lock.
>> + *last = '\0';
>> + if (!libxl__xs_directory(gc, XBT_NULL, path,&nb))
>> + continue;
>
> If this fails, it should be a fatal error; we should not just blunder
> on.
>
> Ian.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd
2012-04-24 10:37 ` Ian Jackson
@ 2012-04-25 13:53 ` Roger Pau Monne
2012-04-25 14:59 ` Ian Campbell
0 siblings, 1 reply; 41+ messages in thread
From: Roger Pau Monne @ 2012-04-25 13:53 UTC (permalink / raw)
To: Ian Jackson; +Cc: Ian Campbell, xen-devel
Ian Jackson escribió:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd"):
>> On Tue, 2012-04-24 at 11:27 +0100, Ian Jackson wrote:
>>> I was suggesting that the key should be global, and XENBUS_PATH
>>> contains too much.
>> Ah, I saw "latter" and read the last bit of my sentence "per-device", my
>> mistake.
>
> Right.
>
> The reason I think it should be global is that this is a transitional
> bodge to make driver domains continue to work pending a proper
> sorting-out in 4.3. So there is no need to make it fully general.
>
> Ian.
So the new variable should be global, even between devices, so that we
either enable hotplug script calling from xl for all, or for none.
Something like:
/libxl/run_hotplug_scripts
Thanks.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd
2012-04-25 13:53 ` Roger Pau Monne
@ 2012-04-25 14:59 ` Ian Campbell
0 siblings, 0 replies; 41+ messages in thread
From: Ian Campbell @ 2012-04-25 14:59 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Ian Jackson, xen-devel
On Wed, 2012-04-25 at 14:53 +0100, Roger Pau Monne wrote:
> Ian Jackson escribió:
> > Ian Campbell writes ("Re: [Xen-devel] [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd"):
> >> On Tue, 2012-04-24 at 11:27 +0100, Ian Jackson wrote:
> >>> I was suggesting that the key should be global, and XENBUS_PATH
> >>> contains too much.
> >> Ah, I saw "latter" and read the last bit of my sentence "per-device", my
> >> mistake.
> >
> > Right.
> >
> > The reason I think it should be global is that this is a transitional
> > bodge to make driver domains continue to work pending a proper
> > sorting-out in 4.3. So there is no need to make it fully general.
> >
> > Ian.
>
> So the new variable should be global, even between devices, so that we
> either enable hotplug script calling from xl for all, or for none.
> Something like:
>
> /libxl/run_hotplug_scripts
It still needs to be per-backend domain, doesn't it?
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Fwd: Re: [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd
[not found] ` <4F982F18.2080902@citrix.com>
@ 2012-04-26 11:48 ` Roger Pau Monne
2012-04-26 12:06 ` Ian Campbell
2012-04-26 13:02 ` Ian Jackson
0 siblings, 2 replies; 41+ messages in thread
From: Roger Pau Monne @ 2012-04-26 11:48 UTC (permalink / raw)
To: Ian Jackson; +Cc: Ian Campbell, xen-devel
Ian Jackson escribió:
> Thanks for this mammoth patch. My comments are below.
>
> Roger Pau Monne writes ("[Xen-devel] [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd"):
>> This is a rather big change, that adds the necessary machinery to perform
>> hotplug script calling from libxl for Linux. This patch launches the necessary
>> scripts to attach and detach PHY and TAP backend types (Qemu doesn't
>> use hotplug scripts).
>>
>> Here's a list of the major changes introduced:
>
> Can you please wrap your commit messages to no more than 75
> characters ? Some vcs's indent them. And of course they get indented
> when we reply leading to wrap damage.
>
>> * libxl__devices_destroy no longer calls libxl__device_destroy, and instead
>> calls libxl__initiate_device_remove, so we can disconnect the device and
>> execute the necessary hotplug scripts instead of just deleting the backend
>> entries. So libxl__devices_destroy now uses the event library, and so does
>> libxl_domain_destroy.
>
> So we no longer have any function which will synchronously and
> immediately destroy a domain and throw away everything to do with it.
> Is it actually the case that you think such a function cannot be
> provided ?
It can be provided, but we should create another command or add an
option to "destroy" (like -f), that doesn't wait for backend
disconnection and just nukes both frontend/backend xenstore entries.
This will also prevent us from executing hotplug scripts, and could lead
to unexpected results.
With this series we wait for the backend to disconnect for 10s, and if
it doesn't respond we nuke the frontend and wait for another 10s. If
after that it fails to disconnect we nuke the remaining backend xenstore
entries.
>> * Added a check in xen-hotplug-common.sh, so scripts are only executed from
>> udev when using xend. xl adds a disable_udev=y to xenstore private directory
>> and with the modification of the udev rules every call from udev gets
>> HOTPLUG_GATE passed, that points to disable_udev. If HOTPLUG_GATE is passed
>> and points to a value, the hotplug script is not executed.
>
> WDYM "points to a value"? Did you just mean "is set to a nonempty
> value" ? I'm not convinced by the name of this variable. Shouldn't
> it have Xen in the name, and specify its own sense ? Eg,
> XEN_HOTPLUG_DISABLE or something ?
This was decided with Ian Campbell, but I have no strong feelings one
way or another, so I don't mind changing it, please read my comment
below regarding the naming.
>> +SUBSYSTEM=="xen-backend", KERNEL=="tap*", ENV{HOTPLUG_GATE}="/libxl/$env{XENBUS_PATH}/disable_udev", RUN+="/etc/xen/scripts/blktap $env{ACTION}"
> ...
>> +# Hack to prevent the execution of hotplug scripts from udev if the domain
>> +# has been launched from libxl
>> +if [ -n "${HOTPLUG_GATE}" ]&& \
>> + `xenstore-read "$HOTPLUG_GATE">/dev/null 2>&1`; then
>> + exit 0
>> +fi
>
> Oh I see. Hmm. Why should this string not be fixed ? I'm not sure I
> like the idea of being able to pass some random other xenstore path.
Anyway, I will have to pass an environmental variable when calling the
script from udev, I can rename this to UDEV_CALL=1 if that sounds better.
> Also: this xenstore path should be a relative path, ie one relative to
> the xenstore home of domain running this part of the tools. That way
> the scripts can be easily and automatically disabled for dom0 and
> enabled in driver domains.
Something like:
/libxl/<domid>/disable_udev?
So that it embraces the whole domain instead of just applying to a
single device?
>
>> + /* compatibility addon to keep udev rules */
>> + flexarray_append(private, "disable_udev");
>> + flexarray_append(private, "y");
>
> We would normally use "1" for true in xenstore.
>
>> libxl__device_generic_add(gc,&device,
>> - libxl__xs_kvs_of_flexarray(gc, back, back->count),
>> - libxl__xs_kvs_of_flexarray(gc, front, front->count));
>> + libxl__xs_kvs_of_flexarray(gc, back, back->count),
>> + libxl__xs_kvs_of_flexarray(gc, front, front->count),
>> + libxl__xs_kvs_of_flexarray(gc, private, private->count));
>> +
>> + switch(disk->backend) {
>> + case LIBXL_DISK_BACKEND_PHY:
>> + case LIBXL_DISK_BACKEND_TAP:
>> + rc = libxl__initiate_device_add(egc, ao,&device);
>> + if (rc) goto out_free;
>> + break;
>> + case LIBXL_DISK_BACKEND_QDISK:
>> + default:
>> + libxl__ao_complete(egc, ao, 0);
>> + break;
>> + }
>
> Does this really need no confirmation from qemu ?
Qemu is not even started here, and it doesn't use any kind of hotplug
scripts, so I think I can safely say yes.
>
>> out_free:
>> + flexarray_free(private);
>> +out_free_b:
>> flexarray_free(back);
>> +out_free_f:
>> flexarray_free(front);
>
> It would be much better to allocate these from the gc somehow.
> Failing that, perhaps we could initialise them to NULL and free
> them iff necessary on the exit path:
Ok.
> out:
> ...
> if (back) flexarray_free(back);
> if (front) flexarray_free(front);
> etc.
>
>> int libxl_device_disk_remove(libxl_ctx *ctx, uint32_t domid,
>> @@ -1442,7 +1484,18 @@ int libxl_device_disk_remove(libxl_ctx *ctx, uint32_t domid,
>> if (rc != 0) goto out;
>>
>> rc = libxl__initiate_device_remove(egc, ao,&device);
>> - if (rc) goto out;
>> + switch(rc) {
>> + case 1:
>> + /* event added */
>
> I think this terminology "event added" is confusingly wrong. What you
> mean is "event queued", I think. You should change this everywhere.
> But before you do that, please see what I write below about your
> approach to libxl__initiate_device_remove.
Ok.
>> @@ -1666,11 +1719,11 @@ 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);
>> + libxl_device_disk_add(ctx, domid, disk, 0);
>
> This needs a /* fixme-ao */ because inside-libxl callers are not
> allowed to invent an ao_how*, even NULL. You need to mark every
> occurrence of these that way.
Ok.
>> @@ -1884,8 +1937,9 @@ int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic)
>> flexarray_append(front, libxl__sprintf(gc,
>> LIBXL_MAC_FMT, LIBXL_MAC_BYTES(nic->mac)));
>> libxl__device_generic_add(gc,&device,
>> - libxl__xs_kvs_of_flexarray(gc, back, back->count),
>> - libxl__xs_kvs_of_flexarray(gc, front, front->count));
>> + libxl__xs_kvs_of_flexarray(gc, back, back->count),
>> + libxl__xs_kvs_of_flexarray(gc, front, front->count),
>> + NULL);
>
> Likewise. Also unintentional indentation change ? (In several more
> places, too.)
Ok, I will try to spot these.
>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
>> index 3675afe..de598ad 100644
>> --- a/tools/libxl/libxl_create.c
>> +++ b/tools/libxl/libxl_create.c
>> @@ -598,7 +598,7 @@ static int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
>> 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]);
>> + ret = libxl_device_disk_add(ctx, domid,&d_config->disks[i], 0);
>
> Here in xl simply passing 0 is correct.
>
>> +char *libxl__device_private_path(libxl__gc *gc, libxl__device *device)
>> +{
>> + return libxl__sprintf(gc, "/libxl/backend/%s/%u/%d",
>> + libxl__device_kind_to_string(device->backend_kind),
>> + device->domid, device->devid);
>> +}
>
> Did you want to use GCSPRINTF ?
Changed, thanks.
>
>> + rc = libxl__device_hotplug(gc, aorm->dev, DISCONNECT);
>
> This is not correct. You need to do something more with the exit
> status of the hotplug script than simply throwing it away as you do in
> hotplug_exited. libxl__device_hotplug needs to take a callback from
> the caller so that it can notify the caller when the hotplug script
> has exited.
>
> You need to not allow the ao to complete until the hotplug script has
> exited. Otherwise you will enter hotplug_exited after the
> libxl__hotplug_state has been destroyed with the ao.
Ok, I think I've changed most of this stuff, and unified device
addition/destruction on the same event, since it's just a matter of wait
-> execute hotplug.
> If it's too slow and you need to time out, you need to send the
> hotplug script a signal, and then wait for it to exit. If necessary
> that signal could be SIGKILL; SIGKILL can be relied on to cause the
> hotplug script to actually terminate and thus the hotplug_exited
> callback to occur.
>
Where is the best place to handle this? From what I see, we have no
helper for doing this, so I guess I will have to use waitpid or
something similar?
>> +/*
>> + * Return codes:
>> + * 1 Success and event(s) HAVE BEEN added
>> + * 0 Success and NO events were added
>> + *< 0 Error
>> + *
>> + * Since this function can be called several times, and several events
>> + * can be added to the same context, the user has to call
>> + * libxl__ao_complete when necessary (if no events are added).
>> + */
>> int libxl__initiate_device_remove(libxl__egc *egc, libxl__ao *ao,
>> libxl__device *dev)
>
> This comment should be in the header file near the declaration.
> And indeed if you had put it there you would have discovered another
> comment already there describing the old behaviour, which you have now
> left behind as an erroneous comment.
Done.
> And I think the new interface is entirely wrong. How does the caller
> know when to complete the ao if libxl__initiate_device_remove never
> calls back ?
>
> You need to split this into two functions: one with the current
> interface which is a simple wrapper, used for all the existing call
> sites, and one which never completes any ao but which always makes a
> callback when the operation is complete. That callback should be used
> by the caller of libxl__initiate_device_remove to do whatever it needs
> to, which might include completing the ao.
I understand what you are saying, but I have trouble thinking of a
correct way to do this, since multiple events can be added to the same
ao, and the libxl__initiate_device_remove "callback" will be called more
than once, how do I know when I have to call ao_complete?
> At the moment, AFAICT from reading your code, the caller's ao will be
> completed by the first nontrivial device remove to complete, ie a bug.
>
>> -int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
>> +int libxl__initiate_device_add(libxl__egc *egc, libxl__ao *ao,
>> + libxl__device *dev)
> ...
>> + rc = libxl__ev_devstate_wait(gc,&aorm->ds, device_add_callback,
>> + state_path, XenbusStateInitWait,
>> + LIBXL_INIT_TIMEOUT * 1000);
>> + if (rc) {
>> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to initialize device %"
>> + PRIu32, dev->devid);
>> + libxl__ev_devstate_cancel(gc,&aorm->ds);
>
> This cancel is not necessary, because device_add_cleanup will do
> this. (Or at least it should do; I haven't checked.)
Removed.
>> + goto out_fail;
>> + }
>> +
>> + return 0;
>> +
>> +out_fail:
>> + assert(rc);
>> + device_add_cleanup(gc, aorm);
>> + return rc;
>> +out_ok:
>
> ^ blank line needed after the return.
>
>> + rc = libxl__device_hotplug(gc, dev, CONNECT);
>> + libxl__ao_complete(egc, ao, 0);
>> + return rc;
>> +}
>
> Some of my comments about initiate_device_remove's callback probably
> apply here too. In particular, I have found it is often better to
> make a callback-queueing function always call the callback right away,
> recursively, on the error path. The result is that the function can
> return void, which simplifies callers considerably.
>
> Whichever way you do it you do need to document your choices about
> reentrancy.
>
>> +static void hotplug_exited(libxl__egc *egc, libxl__ev_child *child,
>> + pid_t pid, int status)
>> +{
>> + libxl__hotplug_state *hs = CONTAINER_OF(child, *hs, child);
>> + EGC_GC;
>> +
>> + if (status) {
>> + libxl_report_child_exitstatus(CTX, hs->rc ? LIBXL__LOG_ERROR
>> + : LIBXL__LOG_WARNING,
>> + "hotplug child", pid, status);
>> + }
>
> As I say, this needs to make a callback.
Sure.
>> +int libxl__hotplug_exec(libxl__gc *gc, char *arg0, char **args, char **env)
>> +{
>
> It would be better IMO to call this function "launch" or something,
> since it doesn't actually call exec in the parent (which is what I
> think a function called "exec" should do).
libxl__hotplug_launch it is.
>> + libxl__hotplug_state *hs;
>> +
>> + hs = libxl__zalloc(gc, sizeof(*hs));
>
> How about
> GCNEW(hs)
> ?
>
>> + if (!pid) {
>> + /* child */
>> + signal(SIGCHLD, SIG_DFL);
>
> What is this for ? Is the problem that children of libxl otherwise
> inherit a weird SIGCHLD disposition ? If so you need to fix this in
> libxl__exec, probably in a separate patch - and you probably need to
> sort out sigprocmask too in case the libxl caller has set up something
> weird. This is something that ought to go in a new patch.
>
>> + if (libxl__ev_child_inuse(&hs->child)) {
>> + hs->rc = rc;
>
> Under what circumstances will rc!=0 here ? Surely that is the success
> path? It might be easier simply to have "out:" be only the error
> path. The success path always has _inuse and the failure path never
> does.
>
>> +/* Action to pass to hotplug caller functions */
>> +typedef enum {
>> + CONNECT = 1,
>> + DISCONNECT = 2
>> +} libxl__hotplug_action;
>
> These names are rather too generic, I think.
I've changed them to HOTPLUG_CONNECT and HOTPLUG_DISCONNECT.
>> +typedef struct libxl__hotplug_state libxl__hotplug_state;
>> +
>> +struct libxl__hotplug_state {
>> + /* public, result, caller may only read in callback */
>> + int rc;
>> + /* private for implementation */
>> + libxl__ev_child child;
>> +};
>
> And this is where the callback member ought to go, in the public
> part, with a note saying it should be set by the caller.
>
> Is there any provision for timeout here ? Would here be a good place
> to do the timeout, rather than higher up the stack ?
>
> Also you might find it better to move the args and env and so forth
> into the hotplug_state. That way, for example, you can actually
> produce a useful message when the script fails.
>
>> +/*
>> + * libxl__hotplug_exec is an internal function that should not be used
>> + * directly, all hotplug script calls have to implement and use
>> + * libxl__device_hotplug.
>> + */
>> +_hidden int libxl__device_hotplug(libxl__gc *gc, libxl__device *dev,
>> + libxl__hotplug_action action);
>> +_hidden int libxl__hotplug_exec(libxl__gc *gc, char *arg0, char **args,
>> + char **env);
>
> Why does libxl__hotplug_exec need to be declared here at all then ?
> Could it be static in libxl_hotplug.c ?
No, because libxl_linux uses libxl_hotplug_launch, which is supposed to
be a common launcher for both Linux and NetBSD.
>
>> +/* Hotplug scripts helpers */
>> +static char **get_hotplug_env(libxl__gc *gc, libxl__device *dev)
>> +{
>> + libxl_ctx *ctx = libxl__gc_owner(gc);
>
> Why not use CTX ? For that matter, if you use my new LOG macros you
> don't need to refer to CTX at all.
>
>> + script = libxl__xs_read(gc, XBT_NULL,
>> + libxl__sprintf(gc, "%s/%s", be_path, "script"));
>> + if (!script) {
>> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to read script from %s",
>> + be_path);
>
> You need to log the errno.
>
>> + f_env = flexarray_make(9, 1);
>> + if (!f_env) {
>> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
>> + "unable to create environment array");
>> + return NULL;
>> + }
>
> Isn't this an allocation failure which should be dealt with by
> libxl__alloc_failed ?
Done, I think we should set a macro for
libxl__alloc_failed(ctx, __func__, 0,0);
Because I'm using that on more than one place.
>
>> + flexarray_set(f_env, nr++, "XENBUS_TYPE");
>> + flexarray_set(f_env, nr++, (char *)
>> + libxl__device_kind_to_string(dev->backend_kind));
>
> Why is the cast needed ?
Not sure, probably slipped from some other place, it's removed now.
>
>> + flexarray_set(f_env, nr++, "XENBUS_PATH");
>> + flexarray_set(f_env, nr++,
>> + libxl__sprintf(gc, "backend/%s/%u/%d",
>> + libxl__device_kind_to_string(dev->backend_kind),
>> + dev->domid, dev->devid));
>
> GCSPRINTF might help shorten this ?
Done.
>
> For that matter, you've called libxl__device_kind_to_string twice.
> Calling it only once would make this easier to read.
>
> I won't comment any more on places where I think GCSPRINTF,
> LOG/LOGE/LOGEV and CTX might usefully replace what you have written.
I'm trying to spot most of them, but it's quite difficult. Are we going
to do a massive replace of this at some point?
>
>> + env = get_hotplug_env(gc, dev);
>> + if (!env)
>> + return -1;
>
> Surely this should never fail as it just results in allocation failure ?
>
> This function seems to be supposed to return an rc value, so return -1
> is wrong.
>
>> + args = (char **) flexarray_contents(f_args);
>
> Why is the cast needed ?
>
>> + what = libxl__sprintf(gc, "%s %s", args[0],
>> + action == CONNECT ? "connect" : "disconnect");
>
> Surely
>
> action == CONNECT ? "connect" :
> action == DISCONNECT ? "disconnect" : abort()
>
> ?
Done!
>> + rc = libxl__hotplug_exec(gc, args[0], args, env);
>> + if (rc) {
>> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable execute hotplug scripts for "
>> + "device %"PRIu32, dev->devid);
>
> libxl__hotplug_exec ought to do all the logging. That means that all
> the information needed to do so should be passed to it, including the
> domid (which you don't currently log) and the device id. That should
> probably be filled in by its caller in the libxl__hotplug_state
> struct.
>
>> diff --git a/tools/python/xen/lowlevel/xl/xl.c b/tools/python/xen/lowlevel/xl/xl.c
>> index c4f7c52..ce7a2b2 100644
>> --- a/tools/python/xen/lowlevel/xl/xl.c
>> +++ b/tools/python/xen/lowlevel/xl/xl.c
>> @@ -447,7 +447,7 @@ static PyObject *pyxl_domain_destroy(XlObject *self, PyObject *args)
>> int domid;
>> if ( !PyArg_ParseTuple(args, "i",&domid) )
>> return NULL;
>> - if ( libxl_domain_destroy(self->ctx, domid) ) {
>> + if ( libxl_domain_destroy(self->ctx, domid, NULL) ) {
>
> I think this is correct. Or, at least, we don't currently provide any
> asynchronous capability in the Python code and I don't mind not doing
> so.
>
> Ian.
Thanks for the review!
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: Fwd: Re: [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd
2012-04-26 11:48 ` Fwd: " Roger Pau Monne
@ 2012-04-26 12:06 ` Ian Campbell
2012-04-26 12:12 ` Roger Pau Monne
2012-04-26 13:02 ` Ian Jackson
1 sibling, 1 reply; 41+ messages in thread
From: Ian Campbell @ 2012-04-26 12:06 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Ian Jackson, xen-devel
On Thu, 2012-04-26 at 12:48 +0100, Roger Pau Monne wrote:
> Ian Jackson escribió:
> > Thanks for this mammoth patch. My comments are below.
> >
> > Roger Pau Monne writes ("[Xen-devel] [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd"):
> >> This is a rather big change, that adds the necessary machinery to perform
> >> hotplug script calling from libxl for Linux. This patch launches the necessary
> >> scripts to attach and detach PHY and TAP backend types (Qemu doesn't
> >> use hotplug scripts).
> >>
> >> Here's a list of the major changes introduced:
> >
> > Can you please wrap your commit messages to no more than 75
> > characters ? Some vcs's indent them. And of course they get indented
> > when we reply leading to wrap damage.
> >
> >> * libxl__devices_destroy no longer calls libxl__device_destroy, and instead
> >> calls libxl__initiate_device_remove, so we can disconnect the device and
> >> execute the necessary hotplug scripts instead of just deleting the backend
> >> entries. So libxl__devices_destroy now uses the event library, and so does
> >> libxl_domain_destroy.
> >
> > So we no longer have any function which will synchronously and
> > immediately destroy a domain and throw away everything to do with it.
> > Is it actually the case that you think such a function cannot be
> > provided ?
>
> It can be provided, but we should create another command or add an
> option to "destroy" (like -f), that doesn't wait for backend
> disconnection and just nukes both frontend/backend xenstore entries.
> This will also prevent us from executing hotplug scripts, and could lead
> to unexpected results.
We have a plan of action to fix this properly in 4.3 (via the Driver Dom
protocol, which separates the backend from the hotplug info).
It would IMHO be acceptable for 4.2 to keep on simply nuking the
backend, and accepting the downsides which this entails. AFAIK this is
not a regression vs Xend -- xend also just nukes the backend (please
correct me if I am wrong about this).
> >> * Added a check in xen-hotplug-common.sh, so scripts are only executed from
> >> udev when using xend. xl adds a disable_udev=y to xenstore private directory
> >> and with the modification of the udev rules every call from udev gets
> >> HOTPLUG_GATE passed, that points to disable_udev. If HOTPLUG_GATE is passed
> >> and points to a value, the hotplug script is not executed.
> >
> > WDYM "points to a value"? Did you just mean "is set to a nonempty
> > value" ? I'm not convinced by the name of this variable. Shouldn't
> > it have Xen in the name, and specify its own sense ? Eg,
> > XEN_HOTPLUG_DISABLE or something ?
>
> This was decided with Ian Campbell,
Please do feel free to take my half-baked suggestions and make them
sensible ;-)
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: Fwd: Re: [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd
2012-04-26 12:06 ` Ian Campbell
@ 2012-04-26 12:12 ` Roger Pau Monne
0 siblings, 0 replies; 41+ messages in thread
From: Roger Pau Monne @ 2012-04-26 12:12 UTC (permalink / raw)
To: Ian Campbell; +Cc: Ian Jackson, xen-devel
Ian Campbell escribió:
> On Thu, 2012-04-26 at 12:48 +0100, Roger Pau Monne wrote:
>> Ian Jackson escribió:
>>> Thanks for this mammoth patch. My comments are below.
>>>
>>> Roger Pau Monne writes ("[Xen-devel] [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd"):
>>>> This is a rather big change, that adds the necessary machinery to perform
>>>> hotplug script calling from libxl for Linux. This patch launches the necessary
>>>> scripts to attach and detach PHY and TAP backend types (Qemu doesn't
>>>> use hotplug scripts).
>>>>
>>>> Here's a list of the major changes introduced:
>>> Can you please wrap your commit messages to no more than 75
>>> characters ? Some vcs's indent them. And of course they get indented
>>> when we reply leading to wrap damage.
>>>
>>>> * libxl__devices_destroy no longer calls libxl__device_destroy, and instead
>>>> calls libxl__initiate_device_remove, so we can disconnect the device and
>>>> execute the necessary hotplug scripts instead of just deleting the backend
>>>> entries. So libxl__devices_destroy now uses the event library, and so does
>>>> libxl_domain_destroy.
>>> So we no longer have any function which will synchronously and
>>> immediately destroy a domain and throw away everything to do with it.
>>> Is it actually the case that you think such a function cannot be
>>> provided ?
>> It can be provided, but we should create another command or add an
>> option to "destroy" (like -f), that doesn't wait for backend
>> disconnection and just nukes both frontend/backend xenstore entries.
>> This will also prevent us from executing hotplug scripts, and could lead
>> to unexpected results.
>
> We have a plan of action to fix this properly in 4.3 (via the Driver Dom
> protocol, which separates the backend from the hotplug info).
>
> It would IMHO be acceptable for 4.2 to keep on simply nuking the
> backend, and accepting the downsides which this entails. AFAIK this is
> not a regression vs Xend -- xend also just nukes the backend (please
> correct me if I am wrong about this).
This is ok if we execute hotplug scripts with udev, because they are
executed when the "physical" device is destroyed, but if we have to
launch the scripts from the toolstack, the only way to know that the
physical device is no longer in use is by watching xenstore backend
entries, that's why we cannot nuke them unless it is our last option.
>>>> * Added a check in xen-hotplug-common.sh, so scripts are only executed from
>>>> udev when using xend. xl adds a disable_udev=y to xenstore private directory
>>>> and with the modification of the udev rules every call from udev gets
>>>> HOTPLUG_GATE passed, that points to disable_udev. If HOTPLUG_GATE is passed
>>>> and points to a value, the hotplug script is not executed.
>>> WDYM "points to a value"? Did you just mean "is set to a nonempty
>>> value" ? I'm not convinced by the name of this variable. Shouldn't
>>> it have Xen in the name, and specify its own sense ? Eg,
>>> XEN_HOTPLUG_DISABLE or something ?
>> This was decided with Ian Campbell,
>
> Please do feel free to take my half-baked suggestions and make them
> sensible ;-)
>
> Ian.
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Fwd: Re: [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd
2012-04-26 11:48 ` Fwd: " Roger Pau Monne
2012-04-26 12:06 ` Ian Campbell
@ 2012-04-26 13:02 ` Ian Jackson
2012-04-26 13:09 ` Ian Campbell
1 sibling, 1 reply; 41+ messages in thread
From: Ian Jackson @ 2012-04-26 13:02 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Ian Campbell, xen-devel
Thanks for your reply. I see you didn't reply to all of my comments.
Do you plan to address the others later ?
Roger Pau Monne writes ("Fwd: Re: [Xen-devel] [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd"):
> Ian Jackson escribió:
> > So we no longer have any function which will synchronously and
> > immediately destroy a domain and throw away everything to do with it.
> > Is it actually the case that you think such a function cannot be
> > provided ?
>
> It can be provided, but we should create another command or add an
> option to "destroy" (like -f), that doesn't wait for backend
> disconnection and just nukes both frontend/backend xenstore entries.
> This will also prevent us from executing hotplug scripts, and could lead
> to unexpected results.
Quite so. But it is important to be able to disregard a
malfunctioning driver domain. That would make it possible to shut
down all the guests using it, destroy the driver domain itself, and
restart it in a hopefully good state, for example.
> With this series we wait for the backend to disconnect for 10s, and if
> it doesn't respond we nuke the frontend and wait for another 10s. If
> after that it fails to disconnect we nuke the remaining backend xenstore
> entries.
I think we need to think about these timeouts. At the very least
they're policy which should probably not be hardcoded in libxl; and
arguably people might want to be able to specify infinite ("I trust my
guest or driver domain and never want to pull the rug out from under
its feet") or zero ("this guest or driver domain has gone wrong and I
want it killed, now").
> >> +# Hack to prevent the execution of hotplug scripts from udev if the domain
> >> +# has been launched from libxl
> >> +if [ -n "${HOTPLUG_GATE}" ]&& \
> >> + `xenstore-read "$HOTPLUG_GATE">/dev/null 2>&1`; then
> >> + exit 0
> >> +fi
> >
> > Oh I see. Hmm. Why should this string not be fixed ? I'm not sure I
> > like the idea of being able to pass some random other xenstore path.
>
> Anyway, I will have to pass an environmental variable when calling the
> script from udev, I can rename this to UDEV_CALL=1 if that sounds better.
Perhaps it would be better to do things the other way around, and have
an env var for the case where we're _not_ calling the script from
udev ? After all, udev config is configuration (at least on my
distros) which the user may not update when the software is updated.
> > Also: this xenstore path should be a relative path, ie one relative to
> > the xenstore home of domain running this part of the tools. That way
> > the scripts can be easily and automatically disabled for dom0 and
> > enabled in driver domains.
>
> Something like:
>
> /libxl/<domid>/disable_udev?
>
> So that it embraces the whole domain instead of just applying to a
> single device?
Yes, except you want
/local/domain/<domid>/libxl/disable_udev
or some such so that <domid> can access it via the relative path
libxl/disable_udev
without needing to know its own domid.
> >> + switch(disk->backend) {
> >> + case LIBXL_DISK_BACKEND_PHY:
> >> + case LIBXL_DISK_BACKEND_TAP:
> >> + rc = libxl__initiate_device_add(egc, ao,&device);
> >> + if (rc) goto out_free;
> >> + break;
> >> + case LIBXL_DISK_BACKEND_QDISK:
> >> + default:
> >> + libxl__ao_complete(egc, ao, 0);
> >> + break;
> >> + }
> >
> > Does this really need no confirmation from qemu ?
>
> Qemu is not even started here, and it doesn't use any kind of hotplug
> scripts, so I think I can safely say yes.
Uh, what, LIBXL_DISK_BACKEND_QDISK but qemu is not started ? How is
that possible ?
> >> + rc = libxl__device_hotplug(gc, aorm->dev, DISCONNECT);
> >
> > This is not correct. You need to do something more with the exit
> > status of the hotplug script than simply throwing it away as you do in
> > hotplug_exited. libxl__device_hotplug needs to take a callback from
> > the caller so that it can notify the caller when the hotplug script
> > has exited.
> >
> > You need to not allow the ao to complete until the hotplug script has
> > exited. Otherwise you will enter hotplug_exited after the
> > libxl__hotplug_state has been destroyed with the ao.
>
> Ok, I think I've changed most of this stuff, and unified device
> addition/destruction on the same event, since it's just a matter of wait
> -> execute hotplug.
I'm not sure I quite understand your response, but OK :-).
> > If it's too slow and you need to time out, you need to send the
> > hotplug script a signal, and then wait for it to exit. If necessary
> > that signal could be SIGKILL; SIGKILL can be relied on to cause the
> > hotplug script to actually terminate and thus the hotplug_exited
> > callback to occur.
>
> Where is the best place to handle this? From what I see, we have no
> helper for doing this, so I guess I will have to use waitpid or
> something similar?
You must not use waitpid yourself. libxl__ev_child_fork will do all
of that and simply call yo back when the child exits.
You can call kill on the pid at any time (provided that 1. you have
the ctx lock held and 2. the libxl__ev_child state struct is still
"inuse" ie still has the pid in it).
> > And I think the new interface is entirely wrong. How does the caller
> > know when to complete the ao if libxl__initiate_device_remove never
> > calls back ?
> >
> > You need to split this into two functions: one with the current
> > interface which is a simple wrapper, used for all the existing call
> > sites, and one which never completes any ao but which always makes a
> > callback when the operation is complete. That callback should be used
> > by the caller of libxl__initiate_device_remove to do whatever it needs
> > to, which might include completing the ao.
>
> I understand what you are saying, but I have trouble thinking of a
> correct way to do this, since multiple events can be added to the same
> ao, and the libxl__initiate_device_remove "callback" will be called more
> than once, how do I know when I have to call ao_complete?
You have to maintain a data structure of some kind that tells you
whether you're expecting more callbacks and what they are.
> >> +/* Action to pass to hotplug caller functions */
> >> +typedef enum {
> >> + CONNECT = 1,
> >> + DISCONNECT = 2
> >> +} libxl__hotplug_action;
> >
> > These names are rather too generic, I think.
>
> I've changed them to HOTPLUG_CONNECT and HOTPLUG_DISCONNECT.
Sounds good.
> >> +/*
> >> + * libxl__hotplug_exec is an internal function that should not be used
> >> + * directly, all hotplug script calls have to implement and use
> >> + * libxl__device_hotplug.
> >> + */
> >> +_hidden int libxl__device_hotplug(libxl__gc *gc, libxl__device *dev,
> >> + libxl__hotplug_action action);
> >> +_hidden int libxl__hotplug_exec(libxl__gc *gc, char *arg0, char **args,
> >> + char **env);
> >
> > Why does libxl__hotplug_exec need to be declared here at all then ?
> > Could it be static in libxl_hotplug.c ?
>
> No, because libxl_linux uses libxl_hotplug_launch, which is supposed to
> be a common launcher for both Linux and NetBSD.
Ah right.
> >> + f_env = flexarray_make(9, 1);
> >> + if (!f_env) {
> >> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> >> + "unable to create environment array");
> >> + return NULL;
> >> + }
> >
> > Isn't this an allocation failure which should be dealt with by
> > libxl__alloc_failed ?
>
> Done, I think we should set a macro for
>
> libxl__alloc_failed(ctx, __func__, 0,0);
>
> Because I'm using that on more than one place.
Sure. A helper macro for that would be fine.
> > For that matter, you've called libxl__device_kind_to_string twice.
> > Calling it only once would make this easier to read.
> >
> > I won't comment any more on places where I think GCSPRINTF,
> > LOG/LOGE/LOGEV and CTX might usefully replace what you have written.
>
> I'm trying to spot most of them, but it's quite difficult. Are we going
> to do a massive replace of this at some point?
I'm not sure. I don't currently have any plans to do so before 4.2.
But in new code it would be better to use the shorter convenience
macros simply to reduce the amount of nearly- meaningless repetition.
Thanks,
Ian.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: Fwd: Re: [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd
2012-04-26 13:02 ` Ian Jackson
@ 2012-04-26 13:09 ` Ian Campbell
2012-05-11 16:06 ` Ian Jackson
0 siblings, 1 reply; 41+ messages in thread
From: Ian Campbell @ 2012-04-26 13:09 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel, Roger Pau Monne
On Thu, 2012-04-26 at 14:02 +0100, Ian Jackson wrote:
> Thanks for your reply. I see you didn't reply to all of my comments.
> Do you plan to address the others later ?
>
> Roger Pau Monne writes ("Fwd: Re: [Xen-devel] [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd"):
> > Ian Jackson escribió:
> > > So we no longer have any function which will synchronously and
> > > immediately destroy a domain and throw away everything to do with it.
> > > Is it actually the case that you think such a function cannot be
> > > provided ?
> >
> > It can be provided, but we should create another command or add an
> > option to "destroy" (like -f), that doesn't wait for backend
> > disconnection and just nukes both frontend/backend xenstore entries.
> > This will also prevent us from executing hotplug scripts, and could lead
> > to unexpected results.
>
> Quite so. But it is important to be able to disregard a
> malfunctioning driver domain. That would make it possible to shut
> down all the guests using it, destroy the driver domain itself, and
> restart it in a hopefully good state, for example.
>
> > With this series we wait for the backend to disconnect for 10s, and if
> > it doesn't respond we nuke the frontend and wait for another 10s. If
> > after that it fails to disconnect we nuke the remaining backend xenstore
> > entries.
>
> I think we need to think about these timeouts. At the very least
> they're policy which should probably not be hardcoded in libxl; and
> arguably people might want to be able to specify infinite ("I trust my
> guest or driver domain and never want to pull the rug out from under
> its feet") or zero ("this guest or driver domain has gone wrong and I
> want it killed, now").
Unless the same is going to be true of the eventual solution (which I
suppose it may well be?) we should be wary of over engineering the
interim solution for 4.2.
> > >> +# Hack to prevent the execution of hotplug scripts from udev if the domain
> > >> +# has been launched from libxl
> > >> +if [ -n "${HOTPLUG_GATE}" ]&& \
> > >> + `xenstore-read "$HOTPLUG_GATE">/dev/null 2>&1`; then
> > >> + exit 0
> > >> +fi
> > >
> > > Oh I see. Hmm. Why should this string not be fixed ? I'm not sure I
> > > like the idea of being able to pass some random other xenstore path.
> >
> > Anyway, I will have to pass an environmental variable when calling the
> > script from udev, I can rename this to UDEV_CALL=1 if that sounds better.
>
> Perhaps it would be better to do things the other way around, and have
> an env var for the case where we're _not_ calling the script from
> udev ? After all, udev config is configuration (at least on my
> distros) which the user may not update when the software is updated.
It was my suggestion to do it this way so that we don't end up with a
vestigial env var which must always be set for no real reason after
we've removed the udev path altogether.
I don't really mind though, we could live with that vestigial thing, or
have another, easier, transition a few releases down the line.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 2/5] libxl: add libxl__xs_path_cleanup
2012-04-23 16:52 ` Ian Jackson
2012-04-25 13:19 ` Roger Pau Monne
@ 2012-05-02 9:44 ` Roger Pau Monne
1 sibling, 0 replies; 41+ messages in thread
From: Roger Pau Monne @ 2012-05-02 9:44 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel
Ian Jackson escribió:
> Roger Pau Monne writes ("[Xen-devel] [PATCH v3 2/5] libxl: add libxl__xs_path_cleanup"):
>> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
>> index c7e057d..36d58cd 100644
>> --- a/tools/libxl/libxl_device.c
>> +++ b/tools/libxl/libxl_device.c
>> @@ -356,7 +356,6 @@ int libxl__device_disk_dev_number(const char *virtpath, int *pdisk,
>> return -1;
>> }
>>
>> -
>
> Unrelated whitespace change.
>
>> +/*
>> + * Perfrom recursive cleanup of xenstore path, from top to bottom
>> + * just like xenstore-rm -t
>> + */
>> +_hidden int libxl__xs_path_cleanup(libxl__gc *gc, char *path);
>
> I think, following my confusion, that this needs some better
> documentation comment.
>
>> diff --git a/tools/libxl/libxl_xshelp.c b/tools/libxl/libxl_xshelp.c
>> index 3ea8d08..0b1b844 100644
>> --- a/tools/libxl/libxl_xshelp.c
>> +++ b/tools/libxl/libxl_xshelp.c
>> @@ -135,6 +135,28 @@ char *libxl__xs_libxl_path(libxl__gc *gc, uint32_t domid)
>> return s;
>> }
>>
>> +int libxl__xs_path_cleanup(libxl__gc *gc, char *path)
>> +{
>> + libxl_ctx *ctx = libxl__gc_owner(gc);
>> + unsigned int nb = 0;
>> + char *last;
>> +
>> + if (!path)
>> + return 0;
>> +
>> + xs_rm(ctx->xsh, XBT_NULL, path);
>> +
>> + for (last = strrchr(path, '/'); last != NULL; last = strrchr(path, '/')) {
>
> If the path is relative, this won't work correctly.
>
> Also this whole thing needs to take place in a transaction, or it is
> racy. Probably a transaction supplied by the caller, in which case
> you should assert it.
This cannot be done inside of a transaction, because we cannot check
that the directory is empty if the remove has not actually taken place,
and checking that there are zero or one elements (the one we 'had'
removed) can lead to unexpected results, as someone might be deleting
elements on our back and we might actually delete a directory that still
has valid entries.
>
>> + *last = '\0';
>> + if (!libxl__xs_directory(gc, XBT_NULL, path,&nb))
>> + continue;
>
> If this fails, it should be a fatal error; we should not just blunder
> on.
>
> Ian.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: Fwd: Re: [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd
2012-04-26 13:09 ` Ian Campbell
@ 2012-05-11 16:06 ` Ian Jackson
2012-05-11 16:26 ` Ian Campbell
0 siblings, 1 reply; 41+ messages in thread
From: Ian Jackson @ 2012-05-11 16:06 UTC (permalink / raw)
To: Ian Campbell; +Cc: Roger Pau Monne, xen-devel
Ian Campbell writes ("Re: [Xen-devel] Fwd: Re: [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd"):
> On Thu, 2012-04-26 at 14:02 +0100, Ian Jackson wrote:
> > I think we need to think about these timeouts. At the very least
> > they're policy which should probably not be hardcoded in libxl; and
> > arguably people might want to be able to specify infinite ("I trust my
> > guest or driver domain and never want to pull the rug out from under
> > its feet") or zero ("this guest or driver domain has gone wrong and I
> > want it killed, now").
>
> Unless the same is going to be true of the eventual solution (which I
> suppose it may well be?) we should be wary of over engineering the
> interim solution for 4.2.
I guess. Also xend's poor behaviour in this area (failing hotplug
timeouts) provides something of an excuse...
> > Perhaps it would be better to do things the other way around, and have
> > an env var for the case where we're _not_ calling the script from
> > udev ? After all, udev config is configuration (at least on my
> > distros) which the user may not update when the software is updated.
>
> It was my suggestion to do it this way so that we don't end up with a
> vestigial env var which must always be set for no real reason after
> we've removed the udev path altogether.
When we have "removed the udev path altogether" we will still need to
have something to prevent trouble if the udev rules remain for some
reason.
> I don't really mind though, we could live with that vestigial thing, or
> have another, easier, transition a few releases down the line.
The vestigial thing can indeed eventually go away.
Ian.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: Fwd: Re: [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd
2012-05-11 16:06 ` Ian Jackson
@ 2012-05-11 16:26 ` Ian Campbell
0 siblings, 0 replies; 41+ messages in thread
From: Ian Campbell @ 2012-05-11 16:26 UTC (permalink / raw)
To: Ian Jackson; +Cc: Roger Pau Monne, xen-devel
On Fri, 2012-05-11 at 17:06 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] Fwd: Re: [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd"):
> > On Thu, 2012-04-26 at 14:02 +0100, Ian Jackson wrote:
> > > I think we need to think about these timeouts. At the very least
> > > they're policy which should probably not be hardcoded in libxl; and
> > > arguably people might want to be able to specify infinite ("I trust my
> > > guest or driver domain and never want to pull the rug out from under
> > > its feet") or zero ("this guest or driver domain has gone wrong and I
> > > want it killed, now").
> >
> > Unless the same is going to be true of the eventual solution (which I
> > suppose it may well be?) we should be wary of over engineering the
> > interim solution for 4.2.
>
> I guess. Also xend's poor behaviour in this area (failing hotplug
> timeouts) provides something of an excuse...
>
> > > Perhaps it would be better to do things the other way around, and have
> > > an env var for the case where we're _not_ calling the script from
> > > udev ? After all, udev config is configuration (at least on my
> > > distros) which the user may not update when the software is updated.
> >
> > It was my suggestion to do it this way so that we don't end up with a
> > vestigial env var which must always be set for no real reason after
> > we've removed the udev path altogether.
>
> When we have "removed the udev path altogether" we will still need to
> have something to prevent trouble if the udev rules remain for some
> reason.
>
> > I don't really mind though, we could live with that vestigial thing, or
> > have another, easier, transition a few releases down the line.
>
> The vestigial thing can indeed eventually go away.
I'm happy for Roger and you to agree whichever sense you think best for
the flag.
Ian.
^ permalink raw reply [flat|nested] 41+ messages in thread
end of thread, other threads:[~2012-05-11 16:26 UTC | newest]
Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-20 13:23 [PATCH v3 0/5] libxl: call hotplug scripts from libxl Roger Pau Monne
2012-04-20 13:23 ` [PATCH v3 1/5] libxl: allow libxl__exec to take a parameter containing the env variables Roger Pau Monne
2012-04-23 15:37 ` Ian Jackson
2012-04-20 13:23 ` [PATCH v3 2/5] libxl: add libxl__xs_path_cleanup Roger Pau Monne
2012-04-23 15:33 ` Ian Jackson
2012-04-23 15:42 ` Roger Pau Monne
2012-04-23 16:49 ` Ian Jackson
2012-04-23 16:52 ` Ian Jackson
2012-04-25 13:19 ` Roger Pau Monne
2012-05-02 9:44 ` Roger Pau Monne
2012-04-20 13:23 ` [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd Roger Pau Monne
2012-04-23 16:48 ` Ian Jackson
2012-04-24 9:16 ` Ian Campbell
2012-04-24 10:09 ` Ian Jackson
2012-04-24 10:17 ` Ian Campbell
2012-04-24 10:27 ` Ian Jackson
2012-04-24 10:29 ` Ian Campbell
2012-04-24 10:37 ` Ian Jackson
2012-04-25 13:53 ` Roger Pau Monne
2012-04-25 14:59 ` Ian Campbell
[not found] ` <4F982F18.2080902@citrix.com>
2012-04-26 11:48 ` Fwd: " Roger Pau Monne
2012-04-26 12:06 ` Ian Campbell
2012-04-26 12:12 ` Roger Pau Monne
2012-04-26 13:02 ` Ian Jackson
2012-04-26 13:09 ` Ian Campbell
2012-05-11 16:06 ` Ian Jackson
2012-05-11 16:26 ` Ian Campbell
2012-04-20 13:23 ` [PATCH v3 4/5] libxl: call hotplug scripts from libxl for vif Roger Pau Monne
2012-04-23 16:59 ` Ian Jackson
2012-04-24 9:18 ` Ian Campbell
2012-04-25 11:19 ` Ian Campbell
2012-04-25 11:24 ` Roger Pau Monne
2012-04-20 13:23 ` [PATCH v3 5/5] libxl: add "downscript=no" to Qemu call Roger Pau Monne
2012-04-23 15:38 ` Ian Jackson
2012-04-23 12:12 ` [PATCH v3 0/5] libxl: call hotplug scripts from libxl Marek Marczykowski
2012-04-23 13:31 ` Roger Pau Monne
2012-04-23 13:47 ` Marek Marczykowski
2012-04-23 13:59 ` Non-dom0 block backends (was: Re: [PATCH v3 0/5] libxl: call hotplug scripts from libxl) Joanna Rutkowska
2012-04-23 14:02 ` [PATCH v3 0/5] libxl: call hotplug scripts from libxl Ian Campbell
2012-04-23 16:25 ` Roger Pau Monne
2012-04-24 9:18 ` Ian Campbell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.