* [PATCH v2 1/5] libxl: allow libxl__exec to take a parameter containing the env variables
2012-04-17 15:51 [PATCH v2 0/5] libxl: call hotplug scripts from libxl Roger Pau Monne
@ 2012-04-17 15:51 ` Roger Pau Monne
2012-04-17 15:51 ` [PATCH v2 2/5] libxl: add libxl__xs_path_cleanup Roger Pau Monne
` (3 subsequent siblings)
4 siblings, 0 replies; 22+ messages in thread
From: Roger Pau Monne @ 2012-04-17 15:51 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 59992b6..16ebef3 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 fdeddc6..a09b38d 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -991,7 +991,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 d486af2..2181774 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -949,8 +949,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] 22+ messages in thread* [PATCH v2 2/5] libxl: add libxl__xs_path_cleanup
2012-04-17 15:51 [PATCH v2 0/5] libxl: call hotplug scripts from libxl Roger Pau Monne
2012-04-17 15:51 ` [PATCH v2 1/5] libxl: allow libxl__exec to take a parameter containing the env variables Roger Pau Monne
@ 2012-04-17 15:51 ` Roger Pau Monne
2012-04-18 8:51 ` Ian Campbell
2012-04-17 15:51 ` [PATCH v2 3/5] libxl: call hotplug scripts from libxl for vbd Roger Pau Monne
` (2 subsequent siblings)
4 siblings, 1 reply; 22+ messages in thread
From: Roger Pau Monne @ 2012-04-17 15:51 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.
Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
---
tools/libxl/libxl_device.c | 21 +++++++++++++++++++++
1 files changed, 21 insertions(+), 0 deletions(-)
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index c7e057d..fe4bcc6 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -356,6 +356,27 @@ int libxl__device_disk_dev_number(const char *virtpath, int *pdisk,
return -1;
}
+static 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;
+}
typedef struct {
libxl__ao *ao;
--
1.7.7.5 (Apple Git-26)
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 2/5] libxl: add libxl__xs_path_cleanup
2012-04-17 15:51 ` [PATCH v2 2/5] libxl: add libxl__xs_path_cleanup Roger Pau Monne
@ 2012-04-18 8:51 ` Ian Campbell
0 siblings, 0 replies; 22+ messages in thread
From: Ian Campbell @ 2012-04-18 8:51 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel
On Tue, 2012-04-17 at 16:51 +0100, Roger Pau Monne wrote:
> 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.
Can you put this in libxl_xshelp.c and a prototype in libxl_internal.h,
please. Also it should include a comment in the header describing the
function and what it does.
>
> Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
> ---
> tools/libxl/libxl_device.c | 21 +++++++++++++++++++++
> 1 files changed, 21 insertions(+), 0 deletions(-)
>
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index c7e057d..fe4bcc6 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -356,6 +356,27 @@ int libxl__device_disk_dev_number(const char *virtpath, int *pdisk,
> return -1;
> }
>
> +static 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;
> +}
>
> typedef struct {
> libxl__ao *ao;
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 3/5] libxl: call hotplug scripts from libxl for vbd
2012-04-17 15:51 [PATCH v2 0/5] libxl: call hotplug scripts from libxl Roger Pau Monne
2012-04-17 15:51 ` [PATCH v2 1/5] libxl: allow libxl__exec to take a parameter containing the env variables Roger Pau Monne
2012-04-17 15:51 ` [PATCH v2 2/5] libxl: add libxl__xs_path_cleanup Roger Pau Monne
@ 2012-04-17 15:51 ` Roger Pau Monne
2012-04-18 9:25 ` Ian Campbell
2012-04-17 15:51 ` [PATCH v2 4/5] libxl: call hotplug scripts from libxl for vif Roger Pau Monne
2012-04-17 15:51 ` [PATCH v2 5/5] libxl: add "downscript=no" to Qemu call Roger Pau Monne
4 siblings, 1 reply; 22+ messages in thread
From: Roger Pau Monne @ 2012-04-17 15:51 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 device backend,
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 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>
---
tools/hotplug/Linux/xen-backend.rules | 6 +-
tools/hotplug/Linux/xen-hotplug-common.sh | 6 ++
tools/libxl/Makefile | 3 +-
tools/libxl/libxl.c | 107 +++++++++++++++++++++----
tools/libxl/libxl.h | 7 +-
tools/libxl/libxl_create.c | 4 +-
tools/libxl/libxl_device.c | 124 ++++++++++++++++++++++++++---
tools/libxl/libxl_dm.c | 4 +-
tools/libxl/libxl_hotplug.c | 67 ++++++++++++++++
tools/libxl/libxl_internal.h | 43 ++++++++++-
tools/libxl/libxl_linux.c | 114 ++++++++++++++++++++++++++
tools/libxl/xl_cmdimpl.c | 14 ++--
tools/python/xen/lowlevel/xl/xl.c | 5 +-
13 files changed, 458 insertions(+), 46 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..ef8ef8e 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}="$XENBUS_PATH/disable_udev", RUN+="/etc/xen/scripts/blktap $env{ACTION}"
+SUBSYSTEM=="xen-backend", KERNEL=="vbd*", ENV{HOTPLUG_GATE}="$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}="$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 b30d11f..ac8b810 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 16ebef3..06438ea 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,9 +1317,11 @@ 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;
char *dev;
@@ -1330,7 +1336,7 @@ 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;
@@ -1361,6 +1367,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 +1385,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:
@@ -1406,6 +1422,9 @@ int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *dis
flexarray_append(back, disk->readwrite ? "w" : "r");
flexarray_append(back, "device-type");
flexarray_append(back, disk->is_cdrom ? "cdrom" : "disk");
+ /* compatibility addon to keep udev rules */
+ flexarray_append(back, "disable_udev");
+ flexarray_append(back, "y");
flexarray_append(front, "backend-id");
flexarray_append(front, libxl__sprintf(gc, "%d", disk->backend_domid));
@@ -1420,14 +1439,26 @@ int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *dis
libxl__xs_kvs_of_flexarray(gc, back, back->count),
libxl__xs_kvs_of_flexarray(gc, front, front->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(back);
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 +1473,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 +1708,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++)
@@ -1909,7 +1951,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;
@@ -2256,7 +2309,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;
@@ -2389,7 +2453,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 50bae2f..b7347be 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -394,7 +394,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 */
@@ -523,7 +524,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 fe4bcc6..f352beb 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -381,22 +381,70 @@ static int libxl__xs_path_cleanup(libxl__gc *gc, char *path)
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__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)
{
@@ -413,7 +461,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;
}
@@ -434,6 +481,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,
@@ -441,7 +489,7 @@ retry_transaction:
LIBXL_DESTROY_TIMEOUT * 1000);
if (rc) goto out_fail;
- return 0;
+ return 1;
out_fail:
assert(rc);
@@ -449,31 +497,74 @@ 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);
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);
- 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__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;
@@ -503,11 +594,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 a09b38d..4779a78 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -790,7 +790,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;
}
@@ -1044,7 +1044,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 2181774..a39346c 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"
@@ -455,6 +456,37 @@ _hidden bool libxl__xs_mkdir(libxl__gc *gc, xs_transaction_t t,
_hidden char *libxl__xs_libxl_path(libxl__gc *gc, uint32_t domid);
+/*
+ * 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
@@ -740,7 +772,8 @@ _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);
/*
@@ -763,6 +796,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..c5583af 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -25,3 +25,117 @@ 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;
+ 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);
+
+ return (char **) flexarray_contents(f_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/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 08815a3..01ff363 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1301,7 +1301,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:
@@ -1862,7 +1862,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)
@@ -2339,7 +2339,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); }
}
@@ -2613,7 +2613,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);
}
@@ -2846,7 +2846,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);
@@ -2964,7 +2964,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);
@@ -5011,7 +5011,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 bbbf2a9..a41a720 100644
--- a/tools/python/xen/lowlevel/xl/xl.c
+++ b/tools/python/xen/lowlevel/xl/xl.c
@@ -444,9 +444,10 @@ static PyObject *pyxl_domain_reboot(XlObject *self, PyObject *args)
static PyObject *pyxl_domain_destroy(XlObject *self, PyObject *args)
{
int domid;
- if ( !PyArg_ParseTuple(args, "i", &domid) )
+ const libxl_asyncop_how *ao_how;
+ if ( !PyArg_ParseTuple(args, "ii", &domid, &ao_how) )
return NULL;
- if ( libxl_domain_destroy(self->ctx, domid) ) {
+ if ( libxl_domain_destroy(self->ctx, domid, ao_how) ) {
PyErr_SetString(xl_error_obj, "cannot destroy domain");
return NULL;
}
--
1.7.7.5 (Apple Git-26)
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 3/5] libxl: call hotplug scripts from libxl for vbd
2012-04-17 15:51 ` [PATCH v2 3/5] libxl: call hotplug scripts from libxl for vbd Roger Pau Monne
@ 2012-04-18 9:25 ` Ian Campbell
0 siblings, 0 replies; 22+ messages in thread
From: Ian Campbell @ 2012-04-18 9:25 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel
On Tue, 2012-04-17 at 16:51 +0100, Roger Pau Monne wrote:
> diff --git a/tools/python/xen/lowlevel/xl/xl.c b/tools/python/xen/lowlevel/xl/xl.c
> index bbbf2a9..a41a720 100644
> --- a/tools/python/xen/lowlevel/xl/xl.c
> +++ b/tools/python/xen/lowlevel/xl/xl.c
> @@ -444,9 +444,10 @@ static PyObject *pyxl_domain_reboot(XlObject *self, PyObject *args)
> static PyObject *pyxl_domain_destroy(XlObject *self, PyObject *args)
> {
> int domid;
> - if ( !PyArg_ParseTuple(args, "i", &domid) )
> + const libxl_asyncop_how *ao_how;
> + if ( !PyArg_ParseTuple(args, "ii", &domid, &ao_how) )
> return NULL;
> - if ( libxl_domain_destroy(self->ctx, domid) ) {
> + if ( libxl_domain_destroy(self->ctx, domid, ao_how) ) {
Without a bunch more infrastructure to allow python to create and
managed ao_how and callbacks etc there's not really much point in this
and you might as well just declare that the python bindings as
synchronous (i.e. pass NULL here).
Otherwise the poatch looks good:
Acked-by: Ian Campbell <ian.campbell@citrix.com>
> PyErr_SetString(xl_error_obj, "cannot destroy domain");
> return NULL;
> }
> --
> 1.7.7.5 (Apple Git-26)
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 4/5] libxl: call hotplug scripts from libxl for vif
2012-04-17 15:51 [PATCH v2 0/5] libxl: call hotplug scripts from libxl Roger Pau Monne
` (2 preceding siblings ...)
2012-04-17 15:51 ` [PATCH v2 3/5] libxl: call hotplug scripts from libxl for vbd Roger Pau Monne
@ 2012-04-17 15:51 ` Roger Pau Monne
2012-04-18 9:54 ` Ian Campbell
2012-04-17 15:51 ` [PATCH v2 5/5] libxl: add "downscript=no" to Qemu call Roger Pau Monne
4 siblings, 1 reply; 22+ messages in thread
From: Roger Pau Monne @ 2012-04-17 15:51 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 mantain
the simmetry.
libxl__initiate_device_remove has been changed a little, to nuke the frontend
xenstore path before trying to perform device teardown, this allows for
unitialized 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 gobal 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 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/hotplug/Linux/xen-backend.rules | 6 +-
tools/libxl/libxl.c | 90 +++++++-------------
tools/libxl/libxl.h | 3 +-
tools/libxl/libxl_create.c | 22 +++++-
tools/libxl/libxl_device.c | 77 +++++++++++++++--
tools/libxl/libxl_dm.c | 2 +-
tools/libxl/libxl_internal.h | 6 ++
tools/libxl/libxl_linux.c | 150 ++++++++++++++++++++++++++++++++-
tools/libxl/libxl_types.idl | 1 +
tools/libxl/xl.c | 4 +
tools/libxl/xl.h | 1 +
tools/libxl/xl_cmdimpl.c | 15 +++-
13 files changed, 308 insertions(+), 76 deletions(-)
diff --git a/docs/man/xl.conf.pod.5 b/docs/man/xl.conf.pod.5
index 8bd45ea..cf2c477 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_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/hotplug/Linux/xen-backend.rules b/tools/hotplug/Linux/xen-backend.rules
index ef8ef8e..35c4e13 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}="$XENBUS_PATH/disabl
SUBSYSTEM=="xen-backend", KERNEL=="vbd*", ENV{HOTPLUG_GATE}="$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}="$XENBUS_PATH/disable_udev", ACTION=="online", RUN+="/etc/xen/scripts/vif-setup online type_if=vif"
+SUBSYSTEM=="xen-backend", KERNEL=="vif-*", ENV{HOTPLUG_GATE}="$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}="$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=="xentap*", ACTION=="add", ENV{HOTPLUG_GATE}="$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 06438ea..d0d6968 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)
@@ -1831,23 +1791,10 @@ int libxl__device_nic_setdefault(libxl__gc *gc, libxl_device_nic *nic)
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;
-
- 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;
libxl__device device;
@@ -1862,7 +1809,7 @@ 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;
@@ -1915,6 +1862,13 @@ int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic)
flexarray_append(back, libxl__strdup(gc, nic->bridge));
flexarray_append(back, "handle");
flexarray_append(back, libxl__sprintf(gc, "%d", nic->devid));
+ flexarray_append(back, "type");
+ flexarray_append(back, libxl__sprintf(gc, "%d", nic->nictype));
+ /* compatibility addon to keep udev rules */
+ if (!nic->disable_vif_script) {
+ flexarray_append(back, "disable_udev");
+ flexarray_append(back, "y");
+ }
flexarray_append(front, "backend-id");
flexarray_append(front, libxl__sprintf(gc, "%d", nic->backend_domid));
@@ -1929,14 +1883,30 @@ int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic)
libxl__xs_kvs_of_flexarray(gc, back, back->count),
libxl__xs_kvs_of_flexarray(gc, front, front->count));
- /* FIXME: wait for plug */
+ switch(nic->nictype) {
+ case LIBXL_NIC_TYPE_VIF:
+ if (!nic->disable_vif_script) {
+ rc = libxl__initiate_device_add(egc, ao, &device);
+ if (rc) goto out_free;
+ }
+ break;
+ case LIBXL_NIC_TYPE_IOEMU:
+ /*
+ * Don't execute hotplug scripts for IOEMU interfaces yet,
+ * we need to launch the device model first.
+ */
+ default:
+ libxl__ao_complete(egc, ao, 0);
+ break;
+ }
+
rc = 0;
out_free:
flexarray_free(back);
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,
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index b7347be..6da6107 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -551,7 +551,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..a1f5731 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_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 f352beb..033f23c 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -249,6 +249,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;
@@ -450,22 +504,29 @@ int libxl__initiate_device_remove(libxl__egc *egc, libxl__ao *ao,
{
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;
+ /*
+ * 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
+ */
+ libxl__xs_path_cleanup(gc, libxl__device_frontend_path(gc, dev));
+
+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)) {
@@ -476,6 +537,8 @@ 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);
@@ -497,8 +560,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);
return 0;
}
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 4779a78..04f76c2 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -795,7 +795,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 a39346c..3787217 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -756,6 +756,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,
diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index c5583af..1ef282f 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -27,11 +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, *be_path;
+
+ be_path = libxl__device_backend_path(gc, dev);
+
+ snictype = libxl__xs_read(gc, XBT_NULL,
+ libxl__sprintf(gc, "%s/%s", be_path, "type"));
+ if (!snictype) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to read nictype from %s",
+ be_path);
+ return -1;
+ }
+
+ return atoi(snictype);
+}
+
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;
+ char *script, *ifname;
flexarray_t *f_env;
int nr = 0;
@@ -43,7 +62,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");
@@ -62,6 +81,35 @@ 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) {
+ ifname = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s",
+ be_path,
+ "vifname"));
+ switch (get_nic_type(gc, dev)) {
+ case LIBXL_NIC_TYPE_IOEMU:
+ flexarray_set(f_env, nr++, "INTERFACE");
+ if (ifname) {
+ flexarray_set(f_env, nr++, libxl__sprintf(gc, "xentap-%s",
+ ifname));
+ } else {
+ flexarray_set(f_env, nr++,
+ libxl__sprintf(gc, "xentap%u.%d",
+ dev->domid, dev->devid));
+ }
+ case LIBXL_NIC_TYPE_VIF:
+ flexarray_set(f_env, nr++, "vif");
+ if (ifname) {
+ flexarray_set(f_env, nr++, ifname);
+ } else {
+ flexarray_set(f_env, nr++,
+ libxl__sprintf(gc, "vif%u.%d",
+ dev->domid, dev->devid));
+ }
+ break;
+ default:
+ return NULL;
+ }
+ }
flexarray_set(f_env, nr++, NULL);
@@ -123,6 +171,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)
{
@@ -132,6 +275,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 09089b2..ac3e79f 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_vif_script", integer),
])
libxl_device_pci = Struct("device_pci", [
diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
index a6ffd25..2a705b8 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_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_vif_scripts", &l, 0))
+ disable_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..13619e7 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_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 01ff363..41230a6 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -846,6 +846,19 @@ static void parse_config_data(const char *configfile_filename_report,
nic->script = strdup(default_vifscript);
}
+ /* Set default nic type for PV guests correctly */
+ if (b_info->type == LIBXL_DOMAIN_TYPE_PV) {
+ nic->nictype = LIBXL_NIC_TYPE_VIF;
+ }
+
+ /*
+ * Disable nic network script calling, to allow the user
+ * to attach the nic backend from a different domain.
+ */
+ if (disable_vif_scripts) {
+ nic->disable_vif_script = 1;
+ }
+
if (default_bridge) {
free(nic->bridge);
nic->bridge = strdup(default_bridge);
@@ -4901,7 +4914,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] 22+ messages in thread* Re: [PATCH v2 4/5] libxl: call hotplug scripts from libxl for vif
2012-04-17 15:51 ` [PATCH v2 4/5] libxl: call hotplug scripts from libxl for vif Roger Pau Monne
@ 2012-04-18 9:54 ` Ian Campbell
2012-04-18 11:22 ` Roger Pau Monne
2012-04-18 11:58 ` Roger Pau Monne
0 siblings, 2 replies; 22+ messages in thread
From: Ian Campbell @ 2012-04-18 9:54 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel
On Tue, 2012-04-17 at 16:51 +0100, Roger Pau Monne wrote:
> 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 mantain
> the simmetry.
"maintain" and "symmetry"
These are pure code motion or did the code actually change too?
> libxl__initiate_device_remove has been changed a little, to nuke the frontend
> xenstore path before trying to perform device teardown, this allows for
> unitialized devices to be closed gracefully, specially vif interfaces added to
uninitialized (or -ised)
> 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 gobal option, called disable_vif_scripts has been added to allow the user
global
> decide if he wants to execute the hotplug scripts from xl or from udev. This has
> been documented in the xl.conf man page.
Did you do this for block too?
>
> 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/hotplug/Linux/xen-backend.rules | 6 +-
> tools/libxl/libxl.c | 90 +++++++-------------
> tools/libxl/libxl.h | 3 +-
> tools/libxl/libxl_create.c | 22 +++++-
> tools/libxl/libxl_device.c | 77 +++++++++++++++--
> tools/libxl/libxl_dm.c | 2 +-
> tools/libxl/libxl_internal.h | 6 ++
> tools/libxl/libxl_linux.c | 150 ++++++++++++++++++++++++++++++++-
> tools/libxl/libxl_types.idl | 1 +
> tools/libxl/xl.c | 4 +
> tools/libxl/xl.h | 1 +
> tools/libxl/xl_cmdimpl.c | 15 +++-
> 13 files changed, 308 insertions(+), 76 deletions(-)
>
> diff --git a/docs/man/xl.conf.pod.5 b/docs/man/xl.conf.pod.5
> index 8bd45ea..cf2c477 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_vif_scripts=BOOLEAN>
> +
> +If enabled xl will not execute nic hotplug scripts itself, and instead
> +relegate this task to udev.
> +
> +Default: C<0>
Please can you also add a commented out version
to ./tools/examples/xl.conf, with the value of the default.
This doesn't actually disable the scripts entirely, but rather only from
udev, disable_udev_vif_scripts perhaps?
> =item B<lockfile="PATH">
>
> Sets the path to the lock file used by xl to serialise certain
> @@ -1929,14 +1883,30 @@ int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic)
> libxl__xs_kvs_of_flexarray(gc, back, back->count),
> libxl__xs_kvs_of_flexarray(gc, front, front->count));
>
> - /* FIXME: wait for plug */
> + switch(nic->nictype) {
> + case LIBXL_NIC_TYPE_VIF:
> + if (!nic->disable_vif_script) {
> + rc = libxl__initiate_device_add(egc, ao, &device);
> + if (rc) goto out_free;
> + }
You don't need an ao_complete in the else case?
> + break;
> + case LIBXL_NIC_TYPE_IOEMU:
> + /*
> + * Don't execute hotplug scripts for IOEMU interfaces yet,
> + * we need to launch the device model first.
> + */
It'd be useful to add a reference to where these are run, at first I
thought this comment (and the patch to xen-backend.rules) meant they
weren't being run at all.
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index de598ad..a1f5731 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_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);
I should have asked this in 3/5 but does this function not need to be
async, since the script might take a while and we need to wait for it to
complete before continuing?
> + 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++)
[...]
> @@ -450,22 +504,29 @@ int libxl__initiate_device_remove(libxl__egc *egc, libxl__ao *ao,
> {
> 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;
>
> + /*
> + * 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
> + */
I'm still not happy with this. The _remove functions are supposed to be
a graceful + co-operative remove, that means prodding the front and
backend into doing the controlled teardown dance.
This may well take some time and may even fail after some timeout
depending on the device type, guest configuration and co-operation etc,
but that is expected and should be handled.
Forcing things in this way is appropriate for device_destroy only IMHO
since that is the function which is provided for use when the guest is
not co-operating.
> + libxl__xs_path_cleanup(gc, libxl__device_frontend_path(gc, dev));
> +
> +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)) {
> @@ -476,6 +537,8 @@ 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);
>
> @@ -497,8 +560,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);
> return 0;
> }
> diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
> index c5583af..1ef282f 100644
> --- a/tools/libxl/libxl_linux.c
> +++ b/tools/libxl/libxl_linux.c
> @@ -27,11 +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, *be_path;
> +
> + be_path = libxl__device_backend_path(gc, dev);
> +
> + snictype = libxl__xs_read(gc, XBT_NULL,
> + libxl__sprintf(gc, "%s/%s", be_path, "type"));
This really ought to be under some private libxl path relating to the
device rather than under the backend/frontend visible dirs.
Actually, now I think of it, that is also true of the udev_disable key?
You could also use the handy enum to/from_strings functions to leave one
less magic number in xenstore.
> + if (!snictype) {
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to read nictype from %s",
> + be_path);
> + return -1;
> + }
> +
> + return atoi(snictype);
> +}
> +
> @@ -62,6 +81,35 @@ 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) {
> + ifname = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s",
> + be_path,
> + "vifname"));
> + switch (get_nic_type(gc, dev)) {
> + case LIBXL_NIC_TYPE_IOEMU:
> + flexarray_set(f_env, nr++, "INTERFACE");
> + if (ifname) {
> + flexarray_set(f_env, nr++, libxl__sprintf(gc, "xentap-%s",
> + ifname));
> + } else {
> + flexarray_set(f_env, nr++,
> + libxl__sprintf(gc, "xentap%u.%d",
> + dev->domid, dev->devid));
This is cut-n-paste of some other code, perhaps create a function to get
the tap name from the vif name and or dom/devid?
> + }
> + case LIBXL_NIC_TYPE_VIF:
> + flexarray_set(f_env, nr++, "vif");
> + if (ifname) {
> + flexarray_set(f_env, nr++, ifname);
> + } else {
> + flexarray_set(f_env, nr++,
> + libxl__sprintf(gc, "vif%u.%d",
> + dev->domid, dev->devid));
> + }
Likewise
> + break;
> + default:
> + return NULL;
> + }
> + }
>
> flexarray_set(f_env, nr++, NULL);
>
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 01ff363..41230a6 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -846,6 +846,19 @@ static void parse_config_data(const char *configfile_filename_report,
> nic->script = strdup(default_vifscript);
> }
>
> + /* Set default nic type for PV guests correctly */
> + if (b_info->type == LIBXL_DOMAIN_TYPE_PV) {
> + nic->nictype = LIBXL_NIC_TYPE_VIF;
> + }
Hrm, really the lib ought to be taking care of that for us...
libxl__device_nic_setdefault has a domid so it should be able to query
the domain type with libxl__domain_type.
> + /*
> + * Disable nic network script calling, to allow the user
> + * to attach the nic backend from a different domain.
> + */
> + if (disable_vif_scripts) {
> + nic->disable_vif_script = 1;
> + }
This is equivalent to :
nic->disable_vif_script = disable_vif_scripts
> +
> if (default_bridge) {
> free(nic->bridge);
> nic->bridge = strdup(default_bridge);
> @@ -4901,7 +4914,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)
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 4/5] libxl: call hotplug scripts from libxl for vif
2012-04-18 9:54 ` Ian Campbell
@ 2012-04-18 11:22 ` Roger Pau Monne
2012-04-18 12:31 ` Ian Campbell
2012-04-18 11:58 ` Roger Pau Monne
1 sibling, 1 reply; 22+ messages in thread
From: Roger Pau Monne @ 2012-04-18 11:22 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel
On 18 Apr 2012, at 10:54, Ian Campbell wrote:
> On Tue, 2012-04-17 at 16:51 +0100, Roger Pau Monne wrote:
>> 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
>> mantain
>> the simmetry.
>
> "maintain" and "symmetry"
>
> These are pure code motion or did the code actually change too?
The code didn't change, I just moved it around.
>
>> libxl__initiate_device_remove has been changed a little, to nuke the
>> frontend
>> xenstore path before trying to perform device teardown, this allows
>> for
>> unitialized devices to be closed gracefully, specially vif interfaces
>> added to
>
> uninitialized (or -ised)
>
>> 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 gobal option, called disable_vif_scripts has been added to
>> allow the user
>
> global
>
>> decide if he wants to execute the hotplug scripts from xl or from
>> udev. This has
>> been documented in the xl.conf man page.
>
> Did you do this for block too?
Nope, only for vif interfaces, that are the only ones that have some
kind of limited support for the driver domains thing when called from
udev.
>>
>> 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/hotplug/Linux/xen-backend.rules | 6 +-
>> tools/libxl/libxl.c | 90 +++++++-------------
>> tools/libxl/libxl.h | 3 +-
>> tools/libxl/libxl_create.c | 22 +++++-
>> tools/libxl/libxl_device.c | 77 +++++++++++++++--
>> tools/libxl/libxl_dm.c | 2 +-
>> tools/libxl/libxl_internal.h | 6 ++
>> tools/libxl/libxl_linux.c | 150
>> ++++++++++++++++++++++++++++++++-
>> tools/libxl/libxl_types.idl | 1 +
>> tools/libxl/xl.c | 4 +
>> tools/libxl/xl.h | 1 +
>> tools/libxl/xl_cmdimpl.c | 15 +++-
>> 13 files changed, 308 insertions(+), 76 deletions(-)
>>
>> diff --git a/docs/man/xl.conf.pod.5 b/docs/man/xl.conf.pod.5
>> index 8bd45ea..cf2c477 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_vif_scripts=BOOLEAN>
>> +
>> +If enabled xl will not execute nic hotplug scripts itself, and
>> instead
>> +relegate this task to udev.
>> +
>> +Default: C<0>
>
> Please can you also add a commented out version
> to ./tools/examples/xl.conf, with the value of the default.
>
> This doesn't actually disable the scripts entirely, but rather only
> from
> udev, disable_udev_vif_scripts perhaps?
It actually disables script calling from libxl, so I think it might be
best to call it disable_xl_vif_scripts?
>
>> =item B<lockfile="PATH">
>>
>> Sets the path to the lock file used by xl to serialise certain
>> @@ -1929,14 +1883,30 @@ int libxl_device_nic_add(libxl_ctx *ctx,
>> uint32_t domid, libxl_device_nic *nic)
>> libxl__xs_kvs_of_flexarray(gc, back,
>> back->count),
>> libxl__xs_kvs_of_flexarray(gc, front,
>> front->count));
>>
>> - /* FIXME: wait for plug */
>> + switch(nic->nictype) {
>> + case LIBXL_NIC_TYPE_VIF:
>> + if (!nic->disable_vif_script) {
>> + rc = libxl__initiate_device_add(egc, ao, &device);
>> + if (rc) goto out_free;
>> + }
>
> You don't need an ao_complete in the else case?
Sure, good one, I think this kind of mechanism is prone to errors…
AO_INPROGRESS should call ao_complete if no events have been added.
>> + break;
>> + case LIBXL_NIC_TYPE_IOEMU:
>> + /*
>> + * Don't execute hotplug scripts for IOEMU interfaces yet,
>> + * we need to launch the device model first.
>> + */
>
> It'd be useful to add a reference to where these are run, at first I
> thought this comment (and the patch to xen-backend.rules) meant they
> weren't being run at all.
Done I've added a small note saying where they are called.
>
>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
>> index de598ad..a1f5731 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_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);
>
> I should have asked this in 3/5 but does this function not need to be
> async, since the script might take a while and we need to wait for it
> to
> complete before continuing?
Are you sure we need to wait for it to finish? There wasn't any kind of
synchronization in the past when we called the scripts from udev, so I
guess we don't have to wait either for them to finish.
>
>> + 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++)
> [...]
>> @@ -450,22 +504,29 @@ int libxl__initiate_device_remove(libxl__egc
>> *egc, libxl__ao *ao,
>> {
>> 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;
>>
>> + /*
>> + * 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
>> + */
>
> I'm still not happy with this. The _remove functions are supposed to
> be
> a graceful + co-operative remove, that means prodding the front and
> backend into doing the controlled teardown dance.
>
> This may well take some time and may even fail after some timeout
> depending on the device type, guest configuration and co-operation
> etc,
> but that is expected and should be handled.
>
> Forcing things in this way is appropriate for device_destroy only IMHO
> since that is the function which is provided for use when the guest is
> not co-operating.
Before this patch, we used to just nuke both front and back xenstore
directories (because we always called libxl__device_destroy), so I think
this is quite and improvement in term of co-operation than what we had
before. We only used this kind of negotiation when detaching a block
from a live guest.
>
>> + libxl__xs_path_cleanup(gc, libxl__device_frontend_path(gc,
>> dev));
>> +
>> +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)) {
>> @@ -476,6 +537,8 @@ 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);
>>
>> @@ -497,8 +560,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);
>> return 0;
>> }
>> diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
>> index c5583af..1ef282f 100644
>> --- a/tools/libxl/libxl_linux.c
>> +++ b/tools/libxl/libxl_linux.c
>> @@ -27,11 +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, *be_path;
>> +
>> + be_path = libxl__device_backend_path(gc, dev);
>> +
>> + snictype = libxl__xs_read(gc, XBT_NULL,
>> + libxl__sprintf(gc, "%s/%s", be_path,
>> "type"));
>
> This really ought to be under some private libxl path relating to the
> device rather than under the backend/frontend visible dirs.
Well, this is currently only used by libxl, but the type of the backend
used I think should be inside the backend device directory, since other
applications might also want to know this.
> Actually, now I think of it, that is also true of the udev_disable
> key?
This is probably true for udev_disable, something like
/libxl/devices/<domid>/<devid>/udev_disable? I will have to modify
libxl__device_generic_add to do this, because it should be done on the
same transaction when the device is added. I'm not sure if we need to
add so much overhead for just one xenstore entry, that will go away in
the next release probably.
> You could also use the handy enum to/from_strings functions to leave
> one
> less magic number in xenstore.
Yes.
>
>> + if (!snictype) {
>> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to read nictype
>> from %s",
>> + be_path);
>> + return -1;
>> + }
>> +
>> + return atoi(snictype);
>> +}
>> +
>> @@ -62,6 +81,35 @@ 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) {
>> + ifname = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc,
>> "%s/%s",
>> +
>> be_path,
>> +
>> "vifname"));
>> + switch (get_nic_type(gc, dev)) {
>> + case LIBXL_NIC_TYPE_IOEMU:
>> + flexarray_set(f_env, nr++, "INTERFACE");
>> + if (ifname) {
>> + flexarray_set(f_env, nr++, libxl__sprintf(gc,
>> "xentap-%s",
>> +
>> ifname));
>> + } else {
>> + flexarray_set(f_env, nr++,
>> + libxl__sprintf(gc, "xentap%u.%d",
>> + dev->domid, dev->devid));
>
> This is cut-n-paste of some other code, perhaps create a function to
> get
> the tap name from the vif name and or dom/devid?
Are you going to add it to your patch or I should add it to mine?
Something like:
char *libxl__device_tap_name(libxl__gc *gc, libxl__device *dev)
>> + }
>> + case LIBXL_NIC_TYPE_VIF:
>> + flexarray_set(f_env, nr++, "vif");
>> + if (ifname) {
>> + flexarray_set(f_env, nr++, ifname);
>> + } else {
>> + flexarray_set(f_env, nr++,
>> + libxl__sprintf(gc, "vif%u.%d",
>> + dev->domid, dev->devid));
>> + }
>
> Likewise
char *libxl__device_vif_name(libxl__gc *gc, libxl__device *dev)
Both this functions should return the correct vifname if one is set, or
the default one otherwise.
>> + break;
>> + default:
>> + return NULL;
>> + }
>> + }
>>
>> flexarray_set(f_env, nr++, NULL);
>>
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index 01ff363..41230a6 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -846,6 +846,19 @@ static void parse_config_data(const char
>> *configfile_filename_report,
>> nic->script = strdup(default_vifscript);
>> }
>>
>> + /* Set default nic type for PV guests correctly */
>> + if (b_info->type == LIBXL_DOMAIN_TYPE_PV) {
>> + nic->nictype = LIBXL_NIC_TYPE_VIF;
>> + }
>
> Hrm, really the lib ought to be taking care of that for us...
>
> libxl__device_nic_setdefault has a domid so it should be able to query
> the domain type with libxl__domain_type.
Yes.
>> + /*
>> + * Disable nic network script calling, to allow the user
>> + * to attach the nic backend from a different domain.
>> + */
>> + if (disable_vif_scripts) {
>> + nic->disable_vif_script = 1;
>> + }
>
> This is equivalent to :
> nic->disable_vif_script = disable_vif_scripts
Yes, and it's one line less.
>
>> +
>> if (default_bridge) {
>> free(nic->bridge);
>> nic->bridge = strdup(default_bridge);
>> @@ -4901,7 +4914,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)
>>
>>
>> _______________________________________________
>> 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] 22+ messages in thread* Re: [PATCH v2 4/5] libxl: call hotplug scripts from libxl for vif
2012-04-18 11:22 ` Roger Pau Monne
@ 2012-04-18 12:31 ` Ian Campbell
2012-04-18 13:28 ` Roger Pau Monne
0 siblings, 1 reply; 22+ messages in thread
From: Ian Campbell @ 2012-04-18 12:31 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel
> >> decide if he wants to execute the hotplug scripts from xl or from
> >> udev. This has
> >> been documented in the xl.conf man page.
> >
> > Did you do this for block too?
>
> Nope, only for vif interfaces, that are the only ones that have some
> kind of limited support for the driver domains thing when called from
> udev.
OK, makes sense.
> >> @@ -55,6 +55,13 @@ default.
> >>
> >> Default: C<1>
> >>
> >> +=item B<disable_vif_scripts=BOOLEAN>
> >> +
> >> +If enabled xl will not execute nic hotplug scripts itself, and
> >> instead
> >> +relegate this task to udev.
> >> +
> >> +Default: C<0>
> >
> > Please can you also add a commented out version
> > to ./tools/examples/xl.conf, with the value of the default.
> >
> > This doesn't actually disable the scripts entirely, but rather only
> > from > udev, disable_udev_vif_scripts perhaps?
>
> It actually disables script calling from libxl, so I think it might be
> best to call it disable_xl_vif_scripts?
Oh, I was confusing it with the xenstore key used by the scripts! I
think your existing name is fine then.
> >
> >> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> >> index de598ad..a1f5731 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_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);
> >
> > I should have asked this in 3/5 but does this function not need to be
> > async, since the script might take a while and we need to wait for it
> > to
> > complete before continuing?
>
> Are you sure we need to wait for it to finish?
Not at all..
> There wasn't any kind of
> synchronization in the past when we called the scripts from udev, so I
> guess we don't have to wait either for them to finish.
You are right, I don't think xl currently waits.
xend used to (IIRC) wait for the hotplug-status node to be written and
used this to do some basic error handling, waiting doe that node is
roughly equivalent to waiting for the script to finish.
Given that xl today doesn't wait I guess I'd be happy to defer that to
the future.
> >> @@ -450,22 +504,29 @@ int libxl__initiate_device_remove(libxl__egc
> >> *egc, libxl__ao *ao,
> >> {
> >> 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;
> >>
> >> + /*
> >> + * 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
> >> + */
> >
> > I'm still not happy with this. The _remove functions are supposed to
> > be
> > a graceful + co-operative remove, that means prodding the front and
> > backend into doing the controlled teardown dance.
> >
> > This may well take some time and may even fail after some timeout
> > depending on the device type, guest configuration and co-operation
> > etc,
> > but that is expected and should be handled.
> >
> > Forcing things in this way is appropriate for device_destroy only IMHO
> > since that is the function which is provided for use when the guest is
> > not co-operating.
>
> Before this patch, we used to just nuke both front and back xenstore
> directories (because we always called libxl__device_destroy),
Not always, the call to destroy depended on the current state.
In the case where the guest is alive and responsive we have to do the
hot remove in the graceful way. If the guest is alive but not responsive
then the correct approach is to try the graceful method with a timeout
which triggers the big hammer approach.
> so I think
> this is quite and improvement in term of co-operation than what we had
> before. We only used this kind of negotiation when detaching a block
> from a live guest.
>
> >
> >> + libxl__xs_path_cleanup(gc, libxl__device_frontend_path(gc,
> >> dev));
> >> +
> >> +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)) {
> >> @@ -476,6 +537,8 @@ 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);
> >>
> >> @@ -497,8 +560,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);
> >> return 0;
> >> }
> >> diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
> >> index c5583af..1ef282f 100644
> >> --- a/tools/libxl/libxl_linux.c
> >> +++ b/tools/libxl/libxl_linux.c
> >> @@ -27,11 +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, *be_path;
> >> +
> >> + be_path = libxl__device_backend_path(gc, dev);
> >> +
> >> + snictype = libxl__xs_read(gc, XBT_NULL,
> >> + libxl__sprintf(gc, "%s/%s", be_path,
> >> "type"));
> >
> > This really ought to be under some private libxl path relating to the
> > device rather than under the backend/frontend visible dirs.
>
> Well, this is currently only used by libxl, but the type of the backend
> used I think should be inside the backend device directory, since other
> applications might also want to know this.
Hang on, can't you infer the type from the backend path, one should
contains vif and the other something else (tap)? Or is this because of
the stupid sharing of the vif dir for both vif and tap from the hotplug
scripts point of view?
It's probably too late in the 4.2 cycle to direct the tap hotplug script
to a different backend dir so I think the best thing to do for now is to
put this key somewhere else so that it doesn't become a guest visible
API (which is what happens with where you have put it). The same place
as udev_disable would work fine.
> > Actually, now I think of it, that is also true of the udev_disable
> > key?
>
> This is probably true for udev_disable, something like
> /libxl/devices/<domid>/<devid>/udev_disable? I will have to modify
> libxl__device_generic_add to do this, because it should be done on the
> same transaction when the device is added. I'm not sure if we need to
> add so much overhead for just one xenstore entry, that will go away in
> the next release probably.
I think this will be around for more than one release (these
deprecations always take time) so it is worth doing right.
> >
> >> + if (!snictype) {
> >> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to read nictype
> >> from %s",
> >> + be_path);
> >> + return -1;
> >> + }
> >> +
> >> + return atoi(snictype);
> >> +}
> >> +
> >> @@ -62,6 +81,35 @@ 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) {
> >> + ifname = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc,
> >> "%s/%s",
> >> +
> >> be_path,
> >> +
> >> "vifname"));
> >> + switch (get_nic_type(gc, dev)) {
> >> + case LIBXL_NIC_TYPE_IOEMU:
> >> + flexarray_set(f_env, nr++, "INTERFACE");
> >> + if (ifname) {
> >> + flexarray_set(f_env, nr++, libxl__sprintf(gc,
> >> "xentap-%s",
> >> +
> >> ifname));
> >> + } else {
> >> + flexarray_set(f_env, nr++,
> >> + libxl__sprintf(gc, "xentap%u.%d",
> >> + dev->domid, dev->devid));
> >
> > This is cut-n-paste of some other code, perhaps create a function to
> > get
> > the tap name from the vif name and or dom/devid?
>
>
> Are you going to add it to your patch or I should add it to mine?
I'll let you do it if that's alright.
> Something like:
>
> char *libxl__device_tap_name(libxl__gc *gc, libxl__device *dev)
[...]
> char *libxl__device_vif_name(libxl__gc *gc, libxl__device *dev)
>
> Both this functions should return the correct vifname if one is set, or
> the default one otherwise.
Yep.
Ian.
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 4/5] libxl: call hotplug scripts from libxl for vif
2012-04-18 12:31 ` Ian Campbell
@ 2012-04-18 13:28 ` Roger Pau Monne
2012-04-18 13:31 ` Ian Campbell
0 siblings, 1 reply; 22+ messages in thread
From: Roger Pau Monne @ 2012-04-18 13:28 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel
On 18 Apr 2012, at 13:31, Ian Campbell wrote:
>>>> @@ -55,6 +55,13 @@ default.
>>>>
>>>> Default: C<1>
>>>>
>>>> +=item B<disable_vif_scripts=BOOLEAN>
>>>> +
>>>> +If enabled xl will not execute nic hotplug scripts itself, and
>>>> instead
>>>> +relegate this task to udev.
>>>> +
>>>> +Default: C<0>
>>>
>>> Please can you also add a commented out version
>>> to ./tools/examples/xl.conf, with the value of the default.
>>>
>>> This doesn't actually disable the scripts entirely, but rather only
>>> from > udev, disable_udev_vif_scripts perhaps?
>>
>> It actually disables script calling from libxl, so I think it might
>> be
>> best to call it disable_xl_vif_scripts?
>
> Oh, I was confusing it with the xenstore key used by the scripts! I
> think your existing name is fine then.
I've changed it to disable_xl_vif_scripts, which I think is even more
clear.
>>>> @@ -450,22 +504,29 @@ int libxl__initiate_device_remove(libxl__egc
>>>> *egc, libxl__ao *ao,
>>>> {
>>>> 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;
>>>>
>>>> + /*
>>>> + * 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
>>>> + */
>>>
>>> I'm still not happy with this. The _remove functions are supposed to
>>> be
>>> a graceful + co-operative remove, that means prodding the front and
>>> backend into doing the controlled teardown dance.
>>>
>>> This may well take some time and may even fail after some timeout
>>> depending on the device type, guest configuration and co-operation
>>> etc,
>>> but that is expected and should be handled.
>>>
>>> Forcing things in this way is appropriate for device_destroy only
>>> IMHO
>>> since that is the function which is provided for use when the guest
>>> is
>>> not co-operating.
>>
>> Before this patch, we used to just nuke both front and back xenstore
>> directories (because we always called libxl__device_destroy),
>
> Not always, the call to destroy depended on the current state.
>
> In the case where the guest is alive and responsive we have to do the
> hot remove in the graceful way. If the guest is alive but not
> responsive
> then the correct approach is to try the graceful method with a timeout
> which triggers the big hammer approach.
Ok, I will add and extra parameter to libxl__initiate_device_remove that
turns on or off the destruction of the front xenstore entries. We will
first call it without removing the fronted, and if that fails we will
call libxl__initiate_device_remove from the callback this time forcing
the removal of the frontend.
>
>> so I think
>> this is quite and improvement in term of co-operation than what we
>> had
>> before. We only used this kind of negotiation when detaching a block
>> from a live guest.
>>
>>>
>>>> + libxl__xs_path_cleanup(gc, libxl__device_frontend_path(gc,
>>>> dev));
>>>> +
>>>> +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)) {
>>>> @@ -476,6 +537,8 @@ 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);
>>>>
>>>> @@ -497,8 +560,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);
>>>> return 0;
>>>> }
>>>> diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
>>>> index c5583af..1ef282f 100644
>>>> --- a/tools/libxl/libxl_linux.c
>>>> +++ b/tools/libxl/libxl_linux.c
>>>> @@ -27,11 +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, *be_path;
>>>> +
>>>> + be_path = libxl__device_backend_path(gc, dev);
>>>> +
>>>> + snictype = libxl__xs_read(gc, XBT_NULL,
>>>> + libxl__sprintf(gc, "%s/%s", be_path,
>>>> "type"));
>>>
>>> This really ought to be under some private libxl path relating to
>>> the
>>> device rather than under the backend/frontend visible dirs.
>>
>> Well, this is currently only used by libxl, but the type of the
>> backend
>> used I think should be inside the backend device directory, since
>> other
>> applications might also want to know this.
>
> Hang on, can't you infer the type from the backend path, one should
> contains vif and the other something else (tap)? Or is this because of
> the stupid sharing of the vif dir for both vif and tap from the
> hotplug
> scripts point of view?
Nope, tap doesn't have a backend xenstore entry, only vifs have, so this
is kind of a hack because I was marking a vifs path as tap...
> It's probably too late in the 4.2 cycle to direct the tap hotplug
> script
> to a different backend dir so I think the best thing to do for now is
> to
> put this key somewhere else so that it doesn't become a guest visible
> API (which is what happens with where you have put it). The same place
> as udev_disable would work fine.
Does this paths sound ok:
/libxl/devices/<domid>/nic/<devid>/udev_disable
/libxl/devices/<domid>/nic/<devid>/type
>>> Actually, now I think of it, that is also true of the udev_disable
>>> key?
>>
>> This is probably true for udev_disable, something like
>> /libxl/devices/<domid>/<devid>/udev_disable? I will have to modify
>> libxl__device_generic_add to do this, because it should be done on
>> the
>> same transaction when the device is added. I'm not sure if we need to
>> add so much overhead for just one xenstore entry, that will go away
>> in
>> the next release probably.
>
> I think this will be around for more than one release (these
> deprecations always take time) so it is worth doing right.
>
>>>
>>>> + if (!snictype) {
>>>> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to read nictype
>>>> from %s",
>>>> + be_path);
>>>> + return -1;
>>>> + }
>>>> +
>>>> + return atoi(snictype);
>>>> +}
>>>> +
>>>> @@ -62,6 +81,35 @@ 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) {
>>>> + ifname = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc,
>>>> "%s/%s",
>>>> +
>>>> be_path,
>>>> +
>>>> "vifname"));
>>>> + switch (get_nic_type(gc, dev)) {
>>>> + case LIBXL_NIC_TYPE_IOEMU:
>>>> + flexarray_set(f_env, nr++, "INTERFACE");
>>>> + if (ifname) {
>>>> + flexarray_set(f_env, nr++, libxl__sprintf(gc,
>>>> "xentap-%s",
>>>> +
>>>> ifname));
>>>> + } else {
>>>> + flexarray_set(f_env, nr++,
>>>> + libxl__sprintf(gc, "xentap%u.%d",
>>>> + dev->domid, dev->devid));
>>>
>>> This is cut-n-paste of some other code, perhaps create a function to
>>> get
>>> the tap name from the vif name and or dom/devid?
>>
>>
>> Are you going to add it to your patch or I should add it to mine?
>
> I'll let you do it if that's alright.
>> Something like:
>>
>> char *libxl__device_tap_name(libxl__gc *gc, libxl__device *dev)
> [...]
>> char *libxl__device_vif_name(libxl__gc *gc, libxl__device *dev)
>>
>> Both this functions should return the correct vifname if one is set,
>> or
>> the default one otherwise.
>
> Yep.
>
> Ian.
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 4/5] libxl: call hotplug scripts from libxl for vif
2012-04-18 13:28 ` Roger Pau Monne
@ 2012-04-18 13:31 ` Ian Campbell
2012-04-18 13:48 ` Roger Pau Monne
2012-04-18 16:24 ` Roger Pau Monne
0 siblings, 2 replies; 22+ messages in thread
From: Ian Campbell @ 2012-04-18 13:31 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel
> Ok, I will add and extra parameter to libxl__initiate_device_remove that
> turns on or off the destruction of the front xenstore entries. We will
> first call it without removing the fronted, and if that fails we will
> call libxl__initiate_device_remove from the callback this time forcing
> the removal of the frontend.
If libxl__initiate_device_remove fails then you should be calling
libxl__initiate_device_destroy, I think, so no need for a param to
_remove?
> > Hang on, can't you infer the type from the backend path, one should
> > contains vif and the other something else (tap)? Or is this because of
> > the stupid sharing of the vif dir for both vif and tap from the
> > hotplug
> > scripts point of view?
>
> Nope, tap doesn't have a backend xenstore entry, only vifs have, so this
> is kind of a hack because I was marking a vifs path as tap...
Right, that's the stupid sharing I was referring too (which IIRC I
added :-/)
>
> > It's probably too late in the 4.2 cycle to direct the tap hotplug
> > script
> > to a different backend dir so I think the best thing to do for now is
> > to
> > put this key somewhere else so that it doesn't become a guest visible
> > API (which is what happens with where you have put it). The same place
> > as udev_disable would work fine.
>
> Does this paths sound ok:
>
> /libxl/devices/<domid>/nic/<devid>/udev_disable
> /libxl/devices/<domid>/nic/<devid>/type
Works for me.
Ian.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 4/5] libxl: call hotplug scripts from libxl for vif
2012-04-18 13:31 ` Ian Campbell
@ 2012-04-18 13:48 ` Roger Pau Monne
2012-04-18 15:30 ` Ian Campbell
2012-04-18 16:24 ` Roger Pau Monne
1 sibling, 1 reply; 22+ messages in thread
From: Roger Pau Monne @ 2012-04-18 13:48 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel
On 18 Apr 2012, at 14:31, Ian Campbell wrote:
>> Ok, I will add and extra parameter to libxl__initiate_device_remove
>> that
>> turns on or off the destruction of the front xenstore entries. We
>> will
>> first call it without removing the fronted, and if that fails we will
>> call libxl__initiate_device_remove from the callback this time
>> forcing
>> the removal of the frontend.
>
> If libxl__initiate_device_remove fails then you should be calling
> libxl__initiate_device_destroy, I think, so no need for a param to
> _remove?
Not really… That's how I think the removal path should be:
1- Wait for backend to turn to state 6 -----> if ok then execute
hotplug, and remove front/backend
2- If timeout: nuke frontend and wait for backend state 6 -----> if ok
then execute hotplug and remove front/back
3- If timeout: nuke front/backend
>>> Hang on, can't you infer the type from the backend path, one should
>>> contains vif and the other something else (tap)? Or is this because
>>> of
>>> the stupid sharing of the vif dir for both vif and tap from the
>>> hotplug
>>> scripts point of view?
>>
>> Nope, tap doesn't have a backend xenstore entry, only vifs have, so
>> this
>> is kind of a hack because I was marking a vifs path as tap...
>
> Right, that's the stupid sharing I was referring too (which IIRC I
> added :-/)
>
>>
>>> It's probably too late in the 4.2 cycle to direct the tap hotplug
>>> script
>>> to a different backend dir so I think the best thing to do for now
>>> is
>>> to
>>> put this key somewhere else so that it doesn't become a guest
>>> visible
>>> API (which is what happens with where you have put it). The same
>>> place
>>> as udev_disable would work fine.
>>
>> Does this paths sound ok:
>>
>> /libxl/devices/<domid>/nic/<devid>/udev_disable
>> /libxl/devices/<domid>/nic/<devid>/type
>
> Works for me.
>
> Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 4/5] libxl: call hotplug scripts from libxl for vif
2012-04-18 13:48 ` Roger Pau Monne
@ 2012-04-18 15:30 ` Ian Campbell
2012-04-18 15:40 ` Roger Pau Monne
0 siblings, 1 reply; 22+ messages in thread
From: Ian Campbell @ 2012-04-18 15:30 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel
On Wed, 2012-04-18 at 14:48 +0100, Roger Pau Monne wrote:
> On 18 Apr 2012, at 14:31, Ian Campbell wrote:
>
> >> Ok, I will add and extra parameter to libxl__initiate_device_remove
> >> that
> >> turns on or off the destruction of the front xenstore entries. We
> >> will
> >> first call it without removing the fronted, and if that fails we will
> >> call libxl__initiate_device_remove from the callback this time
> >> forcing
> >> the removal of the frontend.
> >
> > If libxl__initiate_device_remove fails then you should be calling
> > libxl__initiate_device_destroy, I think, so no need for a param to
> > _remove?
>
> Not really… That's how I think the removal path should be:
>
> 1- Wait for backend to turn to state 6 -----> if ok then execute
> hotplug, and remove front/backend
> 2- If timeout: nuke frontend and wait for backend state 6 -----> if ok
> then execute hotplug and remove front/back
> 3- If timeout: nuke front/backend
That could work, but what does doing step 2 buy you? Why not skip
straight to step 3?
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 4/5] libxl: call hotplug scripts from libxl for vif
2012-04-18 15:30 ` Ian Campbell
@ 2012-04-18 15:40 ` Roger Pau Monne
2012-04-20 12:47 ` Ian Campbell
0 siblings, 1 reply; 22+ messages in thread
From: Roger Pau Monne @ 2012-04-18 15:40 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel
On 18 Apr 2012, at 16:30, Ian Campbell wrote:
> On Wed, 2012-04-18 at 14:48 +0100, Roger Pau Monne wrote:
>> On 18 Apr 2012, at 14:31, Ian Campbell wrote:
>>
>>>> Ok, I will add and extra parameter to libxl__initiate_device_remove
>>>> that
>>>> turns on or off the destruction of the front xenstore entries. We
>>>> will
>>>> first call it without removing the fronted, and if that fails we
>>>> will
>>>> call libxl__initiate_device_remove from the callback this time
>>>> forcing
>>>> the removal of the frontend.
>>>
>>> If libxl__initiate_device_remove fails then you should be calling
>>> libxl__initiate_device_destroy, I think, so no need for a param to
>>> _remove?
>>
>> Not really… That's how I think the removal path should be:
>>
>> 1- Wait for backend to turn to state 6 -----> if ok then execute
>> hotplug, and remove front/backend
>> 2- If timeout: nuke frontend and wait for backend state 6 -----> if
>> ok
>> then execute hotplug and remove front/back
>> 3- If timeout: nuke front/backend
>
> That could work, but what does doing step 2 buy you? Why not skip
> straight to step 3?
hotplug scripts usually require the device to be on state 6 for them to
work, nuking the frontend forces the backend disconnection, and gets us
a state 6.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 4/5] libxl: call hotplug scripts from libxl for vif
2012-04-18 15:40 ` Roger Pau Monne
@ 2012-04-20 12:47 ` Ian Campbell
0 siblings, 0 replies; 22+ messages in thread
From: Ian Campbell @ 2012-04-20 12:47 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel
On Wed, 2012-04-18 at 16:40 +0100, Roger Pau Monne wrote:
> On 18 Apr 2012, at 16:30, Ian Campbell wrote:
>
> > On Wed, 2012-04-18 at 14:48 +0100, Roger Pau Monne wrote:
> >> On 18 Apr 2012, at 14:31, Ian Campbell wrote:
> >>
> >>>> Ok, I will add and extra parameter to libxl__initiate_device_remove
> >>>> that
> >>>> turns on or off the destruction of the front xenstore entries. We
> >>>> will
> >>>> first call it without removing the fronted, and if that fails we
> >>>> will
> >>>> call libxl__initiate_device_remove from the callback this time
> >>>> forcing
> >>>> the removal of the frontend.
> >>>
> >>> If libxl__initiate_device_remove fails then you should be calling
> >>> libxl__initiate_device_destroy, I think, so no need for a param to
> >>> _remove?
> >>
> >> Not really… That's how I think the removal path should be:
> >>
> >> 1- Wait for backend to turn to state 6 -----> if ok then execute
> >> hotplug, and remove front/backend
> >> 2- If timeout: nuke frontend and wait for backend state 6 -----> if
> >> ok
> >> then execute hotplug and remove front/back
> >> 3- If timeout: nuke front/backend
> >
> > That could work, but what does doing step 2 buy you? Why not skip
> > straight to step 3?
>
> hotplug scripts usually require the device to be on state 6 for them to
> work, nuking the frontend forces the backend disconnection, and gets us
> a state 6.
OK, that makes sense. You'd need to have two levels of timeout/callbacks
though?
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 4/5] libxl: call hotplug scripts from libxl for vif
2012-04-18 13:31 ` Ian Campbell
2012-04-18 13:48 ` Roger Pau Monne
@ 2012-04-18 16:24 ` Roger Pau Monne
2012-04-18 17:22 ` Ian Campbell
1 sibling, 1 reply; 22+ messages in thread
From: Roger Pau Monne @ 2012-04-18 16:24 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel
On 18 Apr 2012, at 14:31, Ian Campbell wrote:
>>> It's probably too late in the 4.2 cycle to direct the tap hotplug
>>> script
>>> to a different backend dir so I think the best thing to do for now
>>> is
>>> to
>>> put this key somewhere else so that it doesn't become a guest
>>> visible
>>> API (which is what happens with where you have put it). The same
>>> place
>>> as udev_disable would work fine.
>>
>> Does this paths sound ok:
>>
>> /libxl/devices/<domid>/nic/<devid>/udev_disable
>> /libxl/devices/<domid>/nic/<devid>/type
>
> Works for me.
I've just realized that it's not possible to use exactly this paths,
because then I won't be able to set the HOTPLUG_GATE env from the udev
rules file, so I think something like:
/libxl/backend/vif/7/0
Is one of the only options left. Here are the env variable available
when calling a hotplug script from udev:
UDEV_LOG=3
ACTION=remove
DEVPATH=/devices/vif-7-0
SUBSYSTEM=xen-backend
XENBUS_TYPE=vif
XENBUS_PATH=backend/vif/7/0
XENBUS_BASE_PATH=backend
SEQNUM=1382
So I would set ENV{HOTPLUG_GATE} as /libxl/$XENBUS_PATH
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 4/5] libxl: call hotplug scripts from libxl for vif
2012-04-18 16:24 ` Roger Pau Monne
@ 2012-04-18 17:22 ` Ian Campbell
0 siblings, 0 replies; 22+ messages in thread
From: Ian Campbell @ 2012-04-18 17:22 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel
On Wed, 2012-04-18 at 17:24 +0100, Roger Pau Monne wrote:
> On 18 Apr 2012, at 14:31, Ian Campbell wrote:
>
> >>> It's probably too late in the 4.2 cycle to direct the tap hotplug
> >>> script
> >>> to a different backend dir so I think the best thing to do for now
> >>> is
> >>> to
> >>> put this key somewhere else so that it doesn't become a guest
> >>> visible
> >>> API (which is what happens with where you have put it). The same
> >>> place
> >>> as udev_disable would work fine.
> >>
> >> Does this paths sound ok:
> >>
> >> /libxl/devices/<domid>/nic/<devid>/udev_disable
> >> /libxl/devices/<domid>/nic/<devid>/type
> >
> > Works for me.
>
> I've just realized that it's not possible to use exactly this paths,
> because then I won't be able to set the HOTPLUG_GATE env from the udev
> rules file, so I think something like:
>
> /libxl/backend/vif/7/0
>
> Is one of the only options left. Here are the env variable available
> when calling a hotplug script from udev:
>
> UDEV_LOG=3
> ACTION=remove
> DEVPATH=/devices/vif-7-0
> SUBSYSTEM=xen-backend
> XENBUS_TYPE=vif
> XENBUS_PATH=backend/vif/7/0
> XENBUS_BASE_PATH=backend
> SEQNUM=1382
>
> So I would set ENV{HOTPLUG_GATE} as /libxl/$XENBUS_PATH
That seems reasonable to me.
Ian.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 4/5] libxl: call hotplug scripts from libxl for vif
2012-04-18 9:54 ` Ian Campbell
2012-04-18 11:22 ` Roger Pau Monne
@ 2012-04-18 11:58 ` Roger Pau Monne
2012-04-18 12:34 ` Ian Campbell
1 sibling, 1 reply; 22+ messages in thread
From: Roger Pau Monne @ 2012-04-18 11:58 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel
On 18 Apr 2012, at 10:54, Ian Campbell wrote:
> On Tue, 2012-04-17 at 16:51 +0100, Roger Pau Monne wrote:
>> 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
>> mantain
>> the simmetry.
>
> "maintain" and "symmetry"
>
> These are pure code motion or did the code actually change too?
>
>> libxl__initiate_device_remove has been changed a little, to nuke the
>> frontend
>> xenstore path before trying to perform device teardown, this allows
>> for
>> unitialized devices to be closed gracefully, specially vif interfaces
>> added to
>
> uninitialized (or -ised)
>
>> 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 gobal option, called disable_vif_scripts has been added to
>> allow the user
>
> global
>
>> decide if he wants to execute the hotplug scripts from xl or from
>> udev. This has
>> been documented in the xl.conf man page.
>
> Did you do this for block too?
>
>>
>> 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/hotplug/Linux/xen-backend.rules | 6 +-
>> tools/libxl/libxl.c | 90 +++++++-------------
>> tools/libxl/libxl.h | 3 +-
>> tools/libxl/libxl_create.c | 22 +++++-
>> tools/libxl/libxl_device.c | 77 +++++++++++++++--
>> tools/libxl/libxl_dm.c | 2 +-
>> tools/libxl/libxl_internal.h | 6 ++
>> tools/libxl/libxl_linux.c | 150
>> ++++++++++++++++++++++++++++++++-
>> tools/libxl/libxl_types.idl | 1 +
>> tools/libxl/xl.c | 4 +
>> tools/libxl/xl.h | 1 +
>> tools/libxl/xl_cmdimpl.c | 15 +++-
>> 13 files changed, 308 insertions(+), 76 deletions(-)
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index 01ff363..41230a6 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -846,6 +846,19 @@ static void parse_config_data(const char
>> *configfile_filename_report,
>> nic->script = strdup(default_vifscript);
>> }
>>
>> + /* Set default nic type for PV guests correctly */
>> + if (b_info->type == LIBXL_DOMAIN_TYPE_PV) {
>> + nic->nictype = LIBXL_NIC_TYPE_VIF;
>> + }
>
> Hrm, really the lib ought to be taking care of that for us...
>
> libxl__device_nic_setdefault has a domid so it should be able to query
> the domain type with libxl__domain_type.
int libxl__device_nic_setdefault(libxl__gc *gc, libxl_device_nic *nic)
is the prototype, and libxl_device_nic doesn't have a domid, so I'm not
sure where should I get it from.
>> + /*
>> + * Disable nic network script calling, to allow the user
>> + * to attach the nic backend from a different domain.
>> + */
>> + if (disable_vif_scripts) {
>> + nic->disable_vif_script = 1;
>> + }
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 4/5] libxl: call hotplug scripts from libxl for vif
2012-04-18 11:58 ` Roger Pau Monne
@ 2012-04-18 12:34 ` Ian Campbell
0 siblings, 0 replies; 22+ messages in thread
From: Ian Campbell @ 2012-04-18 12:34 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel
> >> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> >> index 01ff363..41230a6 100644
> >> --- a/tools/libxl/xl_cmdimpl.c
> >> +++ b/tools/libxl/xl_cmdimpl.c
> >> @@ -846,6 +846,19 @@ static void parse_config_data(const char
> >> *configfile_filename_report,
> >> nic->script = strdup(default_vifscript);
> >> }
> >>
> >> + /* Set default nic type for PV guests correctly */
> >> + if (b_info->type == LIBXL_DOMAIN_TYPE_PV) {
> >> + nic->nictype = LIBXL_NIC_TYPE_VIF;
> >> + }
> >
> > Hrm, really the lib ought to be taking care of that for us...
> >
> > libxl__device_nic_setdefault has a domid so it should be able to query
> > the domain type with libxl__domain_type.
>
> int libxl__device_nic_setdefault(libxl__gc *gc, libxl_device_nic *nic)
> is the prototype, and libxl_device_nic doesn't have a domid, so I'm not
> sure where should I get it from.
Urk, oh dear.
I think it would be ok to add parameters which are necessary for
setdefault to make its decision.
Other wise perhaps make the default be LIBXL_NIC_TYPE_VIF
unconditionally? That is the option which has a chance of working
regardless of guest type, TYPE_IOEMU is a bit of an odd choice for the
default.
Ian.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 5/5] libxl: add "downscript=no" to Qemu call
2012-04-17 15:51 [PATCH v2 0/5] libxl: call hotplug scripts from libxl Roger Pau Monne
` (3 preceding siblings ...)
2012-04-17 15:51 ` [PATCH v2 4/5] libxl: call hotplug scripts from libxl for vif Roger Pau Monne
@ 2012-04-17 15:51 ` Roger Pau Monne
2012-04-18 9:56 ` Ian Campbell
4 siblings, 1 reply; 22+ messages in thread
From: Roger Pau Monne @ 2012-04-17 15:51 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 04f76c2..d9c1786 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++;
}
}
@@ -461,10 +468,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] 22+ messages in thread* Re: [PATCH v2 5/5] libxl: add "downscript=no" to Qemu call
2012-04-17 15:51 ` [PATCH v2 5/5] libxl: add "downscript=no" to Qemu call Roger Pau Monne
@ 2012-04-18 9:56 ` Ian Campbell
0 siblings, 0 replies; 22+ messages in thread
From: Ian Campbell @ 2012-04-18 9:56 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel
On Tue, 2012-04-17 at 16:51 +0100, Roger Pau Monne wrote:
> 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>
Acked-by: Ian Campbell <ian.campbell@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 04f76c2..d9c1786 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++;
> }
> }
> @@ -461,10 +468,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++;
> }
> }
^ permalink raw reply [flat|nested] 22+ messages in thread