[PATCH 00 of 14 v4] libxl: add support for hotplug script calling from libxl

[PATCH 00 of 14 v4] libxl: add support for hotplug script calling from libxl

[Roger Pau Monne]

This patch series adds support for hotplug script calling directly
from libxl, instead of relying on xenbackendd. Also some patches
contain general bug fixes.

Currently Linux hotplug script call functions are empty, so Linux
continues to use udev rules to call hotplug scripts.

Patches 1, 7, 8, 9, 10 are NetBSD specific, and basicaly pave the way
for the application of the bigger changes present in this series.

Patch 11 is a trivial update for an error message.

Patch 2 adds support for mounting raw image files using the vnd
device. Since NetBSD doesn't have qdisk or blktap support, the only
way to use raw images with guests is to mount the image and pass the
block device as a "PHY" backend. To check wheter an OS supports raw
images as "PHY" backends two new files are added to the project, to
avoid using #ifdefs, that contain a helper function. The file
to be included is decided during the compilation process.

Patch 3 adds a generic function to fork the current process and
execute a given file, which will be later used to execute hotplug
scripts synchronously.

Patches 4 and 5 add a new function to wait for a device to reach a
certain state, and replace wait_for_dev_destroy with this more generic
implementation. This function is also used to wait for device
initialization before calling hotplug scripts. The added function will
benefit from a rework after event support is added to libxl.

Patch 6 adds the calling of hotplug scripts when devices are
initializated and removed. Two new files are also added to support
hotplug scripts, since Linux and NetBSD hotplug scripts have different
call parameters. The path of the script to execute is retrieved
from xenstore. The file to include is also decided at compile
time.

Patches 12, 13, 14 sets frontend status to 6 when a domain is
destroyed, so devices are disconnected and then execute hotplug
scripts. Also the syntax of the libxl_domain_destroy and 
libxl__devices_destroy has been changed to always force the destruction.

Changes since v3:

 * Merged the destroy fixes with the hotplug series.

Please review, Roger.

[PATCH 01 of 14 v4] xenbackendd: pass type of block device to hotplug script

[Roger Pau Monne]

# HG changeset patch
# User Roger Pau Monne <roger.pau@entel.upc.edu>
# Date 1317386335 -7200
# Node ID a0f9c898470cedc6d9009fde182a5bd73e587a28
# Parent  1c58bb664d8d55e475d179cb5f81693991859fc8
xenbackendd: pass type of block device to hotplug script

Pass the type of block device to attach to the block script instead
of reading it from xenstore, since new Xen versions don't make a
difference between a block device or an image.

Signed-off-by: Roger Pau Monne <roger.pau@entel.upc.edu>

diff -r 1c58bb664d8d -r a0f9c898470c tools/hotplug/NetBSD/block
--- a/tools/hotplug/NetBSD/block	Thu Dec 08 17:15:16 2011 +0000
+++ b/tools/hotplug/NetBSD/block	Fri Sep 30 14:38:55 2011 +0200
@@ -19,7 +19,7 @@ error() {
 
 xpath=$1
 xstatus=$2
-xtype=$(xenstore-read "$xpath/type")
+xtype=$3
 xparams=$(xenstore-read "$xpath/params")
 
 case $xstatus in
diff -r 1c58bb664d8d -r a0f9c898470c tools/xenbackendd/xenbackendd.c
--- a/tools/xenbackendd/xenbackendd.c	Thu Dec 08 17:15:16 2011 +0000
+++ b/tools/xenbackendd/xenbackendd.c	Fri Sep 30 14:38:55 2011 +0200
@@ -89,15 +89,15 @@ dodebug(const char *fmt, ...)
 }
 
 static void
-doexec(const char *cmd, const char *arg1, const char *arg2)
+doexec(const char *cmd, const char *arg1, const char *arg2, const char *arg3)
 {
-	dodebug("exec %s %s %s", cmd, arg1, arg2);
+	dodebug("exec %s %s %s %s", cmd, arg1, arg2, arg3);
 	switch(vfork()) {
 	case -1:
 		dolog(LOG_ERR, "can't vfork: %s", strerror(errno));
 		break;
 	case 0:
-		execl(cmd, cmd, arg1, arg2, NULL);
+		execl(cmd, cmd, arg1, arg2, arg3, NULL);
 		dolog(LOG_ERR, "can't exec %s: %s", cmd, strerror(errno));
 		exit(EXIT_FAILURE);
 		/* NOTREACHED */
@@ -145,11 +145,14 @@ xen_setup(void)
 int
 main(int argc, char * const argv[])
 {
+	struct stat stab;
 	char **vec;
 	unsigned int num;
 	char *s;
 	int state;
 	char *sstate;
+	char *stype;
+	char *params;
 	char *p;
 	char buf[80];
 	int type;
@@ -297,11 +300,38 @@ main(int argc, char * const argv[])
 				    strerror(errno));
 				goto next2;
 			}
-			doexec(s, vec[XS_WATCH_PATH], sstate);
+			doexec(s, vec[XS_WATCH_PATH], sstate, NULL);
 			break;
 
 		case DEVTYPE_VBD:
-			doexec(vbd_script, vec[XS_WATCH_PATH], sstate);
+			/* check if given file is a block device or a raw image */
+			snprintf(buf, sizeof(buf), "%s/params", vec[XS_WATCH_PATH]);
+			params = xs_read(xs, XBT_NULL, buf, 0);
+			if(params == NULL) {
+				dolog(LOG_ERR,
+					"Failed to read %s (%s)", buf, strerror(errno));
+				goto next2;
+			}
+			if (stat(params, &stab) < 0) {
+				dolog(LOG_ERR,
+					"Failed to get info about %s (%s)", params,
+					strerror(errno));
+				goto next3;
+			}
+			stype = NULL;
+			if (S_ISBLK(stab.st_mode))
+				stype = "phy";
+			if (S_ISREG(stab.st_mode))
+				stype = "file";
+			if (stype == NULL) {
+				dolog(LOG_ERR,
+					"Failed to attach %s (not a block device or raw image)",
+					params, strerror(errno));
+				goto next3;
+			}
+			doexec(vbd_script, vec[XS_WATCH_PATH], sstate, stype);
+next3:
+			free(params);
 			break;
 
 		default:

Re: [PATCH 01 of 14 v4] xenbackendd: pass type of block device to hotplug script

[Ian Jackson]

Roger Pau Monne writes ("[Xen-devel] [PATCH 01 of 14 v4] xenbackendd: pass type of block device to hotplug script"):
> xenbackendd: pass type of block device to hotplug script
> 
> Pass the type of block device to attach to the block script instead
> of reading it from xenstore, since new Xen versions don't make a
> difference between a block device or an image.

Why can the hotplug script not use test(1) ?

Ian.

[PATCH 02 of 14 v4] libxl: add support for image files for NetBSD

[Roger Pau Monne]

# HG changeset patch
# User Roger Pau Monne <roger.pau@entel.upc.edu>
# Date 1317386335 -7200
# Node ID e3907ed912201e5270ff19265fab2c979b3929cc
# Parent  a0f9c898470cedc6d9009fde182a5bd73e587a28
libxl: add support for image files for NetBSD

Created a helper function to detect if the OS is capable of using
image files as phy backends. Create two OS specific files, and
changed the Makefile to choose the correct one at compile time.

Signed-off-by: Roger Pau Monne <roger.pau@entel.upc.edu>

diff -r a0f9c898470c -r e3907ed91220 tools/libxl/Makefile
--- a/tools/libxl/Makefile	Fri Sep 30 14:38:55 2011 +0200
+++ b/tools/libxl/Makefile	Fri Sep 30 14:38:55 2011 +0200
@@ -32,6 +32,15 @@ endif
 LIBXL_OBJS-$(CONFIG_X86) += libxl_cpuid.o
 LIBXL_OBJS-$(CONFIG_IA64) += libxl_nocpuid.o
 
+ifeq ($(CONFIG_NetBSD),y)
+LIBXL_OBJS-y += libxl_netbsd.o
+else ifeq ($(CONFIG_Linux),y)
+LIBXL_OBJS-y += libxl_linux.o
+else
+$(error Your Operating System is not supported by libxenlight, \
+please check libxl_linux.c and libxl_netbsd.c to see how to get it ported)
+endif
+
 LIBXL_LIBS += -lyajl
 
 LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \
diff -r a0f9c898470c -r e3907ed91220 tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c	Fri Sep 30 14:38:55 2011 +0200
+++ b/tools/libxl/libxl_device.c	Fri Sep 30 14:38:55 2011 +0200
@@ -138,15 +138,14 @@ static int disk_try_backend(disk_try_bac
               a->disk->format == LIBXL_DISK_FORMAT_EMPTY)) {
             goto bad_format;
         }
-        if (a->disk->format != LIBXL_DISK_FORMAT_EMPTY &&
-            !S_ISBLK(a->stab.st_mode)) {
-            LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend phy"
-                       " unsuitable as phys path not a block device",
-                       a->disk->vdev);
-            return 0;
-        }
 
-        return backend;
+        if (libxl__try_phy_backend(a->stab.st_mode))
+            return backend;
+
+        LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend phy"
+                   " unsuitable as phys path not a block device",
+                   a->disk->vdev);
+        return 0;
 
     case LIBXL_DISK_BACKEND_TAP:
         if (!libxl__blktap_enabled(a->gc)) {
diff -r a0f9c898470c -r e3907ed91220 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h	Fri Sep 30 14:38:55 2011 +0200
+++ b/tools/libxl/libxl_internal.h	Fri Sep 30 14:38:55 2011 +0200
@@ -252,6 +252,15 @@ _hidden int libxl__device_destroy(libxl_
 _hidden int libxl__devices_destroy(libxl__gc *gc, uint32_t domid, int force);
 _hidden int libxl__wait_for_backend(libxl__gc *gc, char *be_path, char *state);
 
+/*
+ * libxl__try_phy_backend - Check if there's support for the passed
+ * type of file using the PHY backend
+ * st_mode: mode_t of the file, as returned by stat function
+ *
+ * Returns 0 on success, and < 0 on error.
+ */
+_hidden int libxl__try_phy_backend(mode_t st_mode);
+
 /* from libxl_pci */
 
 _hidden int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, int starting);
diff -r a0f9c898470c -r e3907ed91220 tools/libxl/libxl_linux.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/libxl/libxl_linux.c	Fri Sep 30 14:38:55 2011 +0200
@@ -0,0 +1,27 @@
+/*
+ * Copyright (C) 2011
+ * Author Roger Pau Monne <roger.pau@entel.upc.edu>
+ *
+ * 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 <sys/stat.h>
+
+#include "libxl_internal.h"
+ 
+int libxl__try_phy_backend(mode_t st_mode)
+{
+    if (!S_ISBLK(st_mode)) {
+        return 0;
+    }
+
+    return 1;
+}
diff -r a0f9c898470c -r e3907ed91220 tools/libxl/libxl_netbsd.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/libxl/libxl_netbsd.c	Fri Sep 30 14:38:55 2011 +0200
@@ -0,0 +1,26 @@
+/*
+ * Copyright (C) 2011
+ * Author Roger Pau Monne <roger.pau@entel.upc.edu>
+ *
+ * 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 <sys/stat.h>
+
+#include "libxl_internal.h"
+
+int libxl__try_phy_backend(mode_t st_mode)
+{
+    if (S_ISREG(st_mode) || S_ISBLK(st_mode))
+        return 1;
+
+    return 0;
+}

Re: [PATCH 02 of 14 v4] libxl: add support for image files for NetBSD

[Ian Jackson]

Roger Pau Monne writes ("[Xen-devel] [PATCH 02 of 14 v4] libxl: add support for image files for NetBSD"):
> libxl: add support for image files for NetBSD
> 
> Created a helper function to detect if the OS is capable of using
> image files as phy backends. Create two OS specific files, and
> changed the Makefile to choose the correct one at compile time.
> 
> Signed-off-by: Roger Pau Monne <roger.pau@entel.upc.edu>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

[PATCH 03 of 14 v4] libxl: add libxl__forkexec function to libxl_exec

[Roger Pau Monne]

# HG changeset patch
# User Roger Pau Monne <roger.pau@entel.upc.edu>
# Date 1323766195 -3600
# Node ID 573a246bf0c6b3ad01473d350a2c3c5aad30c351
# Parent  e3907ed912201e5270ff19265fab2c979b3929cc
libxl: add libxl__forkexec function to libxl_exec

Add a new function to libxl_exec that performs a fork and executes
the passed arguments using libxl__exec.

Signed-off-by: Roger Pau Monne <roger.pau@entel.upc.edu>

diff -r e3907ed91220 -r 573a246bf0c6 tools/libxl/libxl_exec.c
--- a/tools/libxl/libxl_exec.c	Fri Sep 30 14:38:55 2011 +0200
+++ b/tools/libxl/libxl_exec.c	Tue Dec 13 09:49:55 2011 +0100
@@ -450,6 +450,39 @@ int libxl__spawn_check(libxl__gc *gc, li
     return ERROR_FAIL;
 }
 
+int libxl__forkexec(libxl__gc *gc, int stdinfd, int stdoutfd,
+                    int stderrfd, char **args)
+{
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+    int status;
+    int rc = 0;
+    pid_t pid = fork();
+
+    switch (pid) {
+    case -1:
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "fork failed\n");
+        rc = -1;
+        break;
+    case 0:
+        libxl__exec(stdinfd, stdoutfd, stderrfd, args[0], args);
+        /* libxl__exec never returns */
+    default:
+        while (waitpid(pid, &status, 0) < 0) {
+            if (errno != EINTR) {
+                LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "waitpid failed\n");
+                rc = -1;
+                break;
+            }
+        }
+        if (status)
+            libxl_report_child_exitstatus(ctx, LIBXL__LOG_ERROR,
+                                          args[0], pid, status);
+        rc = status;
+        break;
+    }
+    return rc;
+}
+
 /*
  * Local variables:
  * mode: C
diff -r e3907ed91220 -r 573a246bf0c6 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h	Fri Sep 30 14:38:55 2011 +0200
+++ b/tools/libxl/libxl_internal.h	Tue Dec 13 09:49:55 2011 +0100
@@ -400,6 +400,22 @@ _hidden int libxl__spawn_check(libxl__gc
 _hidden void libxl__exec(int stdinfd, int stdoutfd, int stderrfd,
                const char *arg0, char **args); // logs errors, never returns
 
+/*
+ * libxl__forkexec - Executes a file synchronously
+ * gc: allocation pool
+ * stdinfd, stdoutfd, stderrfd: fds to pass to libxl__exec
+ * args: file to execute and arguments to pass in the following format
+ *      args[0]: file to execute
+ *      args[1]: first argument to pass to the called program
+ *      ...
+ *      args[n-1]: (n-1)th argument to pass to the called program
+ *      args[n]: NULL
+ *
+ * Returns the exit status, as reported by waitpid.
+ */
+_hidden int libxl__forkexec(libxl__gc *gc, int stdinfd, int stdoutfd,
+                            int stderrfd, char **args);
+
 /* from xl_create */
 _hidden int libxl__domain_make(libxl__gc *gc, libxl_domain_create_info *info, uint32_t *domid);
 _hidden int libxl__domain_build(libxl__gc *gc,

Re: [PATCH 03 of 14 v4] libxl: add libxl__forkexec function to libxl_exec

[Ian Jackson]

Roger Pau Monne writes ("[Xen-devel] [PATCH 03 of 14 v4] libxl: add libxl__forkexec function to libxl_exec"):
> +int libxl__forkexec(libxl__gc *gc, int stdinfd, int stdoutfd,
> +                    int stderrfd, char **args)
> +{
...
> +        libxl__exec(stdinfd, stdoutfd, stderrfd, args[0], args);
...
> +            libxl_report_child_exitstatus(ctx, LIBXL__LOG_ERROR,
> +                                          args[0], pid, status);

I think this function should allow passing separate values for arg0
and args.  It would also be useful to allow passing a separate "what"
to libxl_report_child_exitstatus: for example, that would allow you to
distinguish the various call sites for running a hotplug script, so
that failures in the plug and unplug give different answers.  It also
allows the caller to use libxl__sprintf if they want to give
more information about the program.

> +/*
> + * libxl__forkexec - Executes a file synchronously

OK, but:

> + * gc: allocation pool
> + * stdinfd, stdoutfd, stderrfd: fds to pass to libxl__exec
> + * args: file to execute and arguments to pass in the following format
> + *      args[0]: file to execute
> + *      args[1]: first argument to pass to the called program
> + *      ...
> + *      args[n-1]: (n-1)th argument to pass to the called program
> + *      args[n]: NULL

IMO all of the above is obvious and should be eliminated.  (This is
exactly the kind of "fd is the file descriptor" stuff that I was
complaining about in another recent thread.)

> + * Returns the exit status, as reported by waitpid.

And this fails to go into enough detail.  Indeed it is false.
In particular, it fails to mention:
  - what the function does if some system call fails
  - whether any messages are logged

Ian.

Re: [PATCH 03 of 14 v4] libxl: add libxl__forkexec function to libxl_exec

[Ian Campbell]

On Tue, 2011-12-13 at 15:14 +0000, Ian Jackson wrote:
> 
> > + * gc: allocation pool
> > + * stdinfd, stdoutfd, stderrfd: fds to pass to libxl__exec
> > + * args: file to execute and arguments to pass in the following
> format
> > + *      args[0]: file to execute
> > + *      args[1]: first argument to pass to the called program
> > + *      ...
> > + *      args[n-1]: (n-1)th argument to pass to the called program
> > + *      args[n]: NULL
> 
> IMO all of the above is obvious and should be eliminated.  (This is
> exactly the kind of "fd is the file descriptor" stuff that I was
> complaining about in another recent thread.) 

The definition of args[0] as the actual executable (or argv0) and the
requirement for args[n] == NULL are interesting enough to mention I
think (the NULL thing in particular often trips people up). But you
could also just reference execvp(3).

Ian.

Re: [PATCH 03 of 14 v4] libxl: add libxl__forkexec function to libxl_exec

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 03 of 14 v4] libxl: add libxl__forkexec function	to libxl_exec"):
> On Tue, 2011-12-13 at 15:14 +0000, Ian Jackson wrote:
> > IMO all of the above is obvious and should be eliminated.  (This is
> > exactly the kind of "fd is the file descriptor" stuff that I was
> > complaining about in another recent thread.) 
> 
> The definition of args[0] as the actual executable (or argv0) and the
> requirement for args[n] == NULL are interesting enough to mention I
> think (the NULL thing in particular often trips people up). But you
> could also just reference execvp(3).

I think it would be better to reference execvp.

Ian.

[PATCH 04 of 14 v4] libxl: introduce libxl__wait_for_device_state

[Roger Pau Monne]

# HG changeset patch
# User Roger Pau Monne <roger.pau@entel.upc.edu>
# Date 1323766195 -3600
# Node ID d5f1ab565bf64c98c04720bafe50fa4cb6b1592f
# Parent  573a246bf0c6b3ad01473d350a2c3c5aad30c351
libxl: introduce libxl__wait_for_device_state

This is a generic function, that waits for xs watches to reach a
certain state and then executes the passed helper function. Removed
wait_for_dev_destroy and used this new function instead. This
function will also be used by future patches that need to wait for
the initialization of devices before executing hotplug scripts.

Signed-off-by: Roger Pau Monne <roger.pau@entel.upc.edu>

diff -r 573a246bf0c6 -r d5f1ab565bf6 tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c	Tue Dec 13 09:49:55 2011 +0100
+++ b/tools/libxl/libxl_device.c	Tue Dec 13 09:49:55 2011 +0100
@@ -370,7 +370,9 @@ int libxl__device_disk_dev_number(const 
  * Returns 0 if a device is removed, ERROR_* if an error
  * or timeout occurred.
  */
-static int wait_for_dev_destroy(libxl__gc *gc, struct timeval *tv)
+int libxl__wait_for_device_state(libxl__gc *gc, struct timeval *tv,
+                                 XenbusState state,
+                                 libxl__device_state_handler handler)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     int nfds, rc;
@@ -395,17 +397,14 @@ start:
         default:
             l1 = xs_read_watch(ctx->xsh, &n);
             if (l1 != NULL) {
-                char *state = libxl__xs_read(gc, XBT_NULL,
+                char *sstate = libxl__xs_read(gc, XBT_NULL,
                                              l1[XS_WATCH_PATH]);
-                if (!state || atoi(state) == 6) {
-                    xs_unwatch(ctx->xsh, l1[0], l1[1]);
-                    xs_rm(ctx->xsh, XBT_NULL, l1[XS_WATCH_TOKEN]);
-                    LIBXL__LOG(ctx, LIBXL__LOG_DEBUG,
-                               "Destroyed device backend at %s",
-                               l1[XS_WATCH_TOKEN]);
-                    rc = 0;
+                if (!sstate || atoi(sstate) == state) {
+                    /* Call handler function if present */
+                    if (handler)
+                        rc = handler(gc, l1, sstate);
                 } else {
-                    /* State is not "disconnected", continue waiting... */
+                    /* State is different than expected, continue waiting... */
                     goto start;
                 }
                 free(l1);
@@ -418,6 +417,23 @@ start:
 }
 
 /*
+ * Handler function for device destruction to be passed to
+ * libxl__wait_for_device_state
+ */
+static int destroy_device(libxl__gc *gc, char **l1, char *state)
+{
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+
+    xs_unwatch(ctx->xsh, l1[0], l1[1]);
+    xs_rm(ctx->xsh, XBT_NULL, l1[XS_WATCH_TOKEN]);
+    LIBXL__LOG(ctx, LIBXL__LOG_DEBUG,
+               "Destroyed device backend at %s",
+               l1[XS_WATCH_TOKEN]);
+
+    return 0;
+}
+
+/*
  * Returns 0 (device already destroyed) or 1 (caller must
  * wait_for_dev_destroy) on success, ERROR_* on fail.
  */
@@ -458,7 +474,8 @@ retry_transaction:
         struct timeval tv;
         tv.tv_sec = LIBXL_DESTROY_TIMEOUT;
         tv.tv_usec = 0;
-        rc = wait_for_dev_destroy(gc, &tv);
+        rc = libxl__wait_for_device_state(gc, &tv, XenbusStateClosed,
+                                          destroy_device);
         if (rc < 0) /* an error or timeout occurred, clear watches */
             xs_unwatch(ctx->xsh, state_path, be_path);
         xs_rm(ctx->xsh, XBT_NULL, libxl__device_frontend_path(gc, dev));
@@ -566,7 +583,8 @@ int libxl__devices_destroy(libxl__gc *gc
         tv.tv_sec = LIBXL_DESTROY_TIMEOUT;
         tv.tv_usec = 0;
         while (n_watches > 0) {
-            if (wait_for_dev_destroy(gc, &tv) < 0) {
+            if (libxl__wait_for_device_state(gc, &tv, XenbusStateClosed,
+                                             destroy_device) < 0) {
                 /* function returned ERROR_* */
                 break;
             } else {
diff -r 573a246bf0c6 -r d5f1ab565bf6 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h	Tue Dec 13 09:49:55 2011 +0100
+++ b/tools/libxl/libxl_internal.h	Tue Dec 13 09:49:55 2011 +0100
@@ -20,11 +20,14 @@
 #include <stdint.h>
 #include <stdarg.h>
 #include <stdlib.h>
+#include <sys/time.h>
 
 #include <xs.h>
 #include <xenctrl.h>
 #include "xentoollog.h"
 
+#include <xen/io/xenbus.h>
+
 #if __GNUC__ > 3 || (__GNUC__ == 3 && __GNUC_MINOR__ >= 1)
 #define _hidden __attribute__((visibility("hidden")))
 #define _protected __attribute__((visibility("protected")))
@@ -252,6 +255,31 @@ _hidden int libxl__device_destroy(libxl_
 _hidden int libxl__devices_destroy(libxl__gc *gc, uint32_t domid, int force);
 _hidden int libxl__wait_for_backend(libxl__gc *gc, char *be_path, char *state);
 
+/* Handler for the libxl__wait_for_device_state callback */
+/*
+ * libxl__device_state_handler - Handler for the libxl__wait_for_device_state
+ * gc: allocation pool
+ * l1: array containing the path and token
+ * state: string that contains the state of the device
+ *
+ * Returns 0 on success, and < 0 on error.
+ */
+typedef int libxl__device_state_handler(libxl__gc *gc, char **l1, char *state);
+
+/*
+ * libxl__wait_for_device_state - waits a given time for a device to
+ * reach a given state
+ * gc: allocation pool
+ * tv: timeval struct containing the maximum time to wait
+ * state: state to wait for (check xen/io/xenbus.h)
+ * handler: callback function to execute when state is reached
+ *
+ * Returns 0 on success, and < 0 on error.
+ */
+_hidden int libxl__wait_for_device_state(libxl__gc *gc, struct timeval *tv,
+                                         XenbusState state,
+                                         libxl__device_state_handler handler);
+
 /*
  * libxl__try_phy_backend - Check if there's support for the passed
  * type of file using the PHY backend

Re: [PATCH 04 of 14 v4] libxl: introduce libxl__wait_for_device_state

[Ian Jackson]

Roger Pau Monne writes ("[Xen-devel] [PATCH 04 of 14 v4] libxl: introduce libxl__wait_for_device_state"):
> libxl: introduce libxl__wait_for_device_state
> 
> This is a generic function, that waits for xs watches to reach a
> certain state and then executes the passed helper function. Removed
> wait_for_dev_destroy and used this new function instead. This
> function will also be used by future patches that need to wait for
> the initialization of devices before executing hotplug scripts.

I think this is a reasonable step forwards.

However, given my plans for new asynchronous event handling, this
function will have to be turned half-inside-out by me shortly.  So
under the circumstances I don't propose to review it very closely
now.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Thanks,
Ian.

[PATCH 05 of 14 v4] libxl: wait for devices to initialize upon addition to the domain

[Roger Pau Monne]

# HG changeset patch
# User Roger Pau Monne <roger.pau@entel.upc.edu>
# Date 1323766195 -3600
# Node ID 148765a54f6ed77fb83ea6c8788e420a0781f225
# Parent  d5f1ab565bf64c98c04720bafe50fa4cb6b1592f
libxl: wait for devices to initialize upon addition to the domain.

Block waiting for devices to initialize (switch to state 2). This is
necessary because we need to call the hotplug scripts when state is
2, otherwise the execution fails. Make use of the newly introduced
function libxl__wait_for_device_state.

Signed-off-by: Roger Pau Monne <roger.pau@entel.upc.edu>

diff -r d5f1ab565bf6 -r 148765a54f6e tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Tue Dec 13 09:49:55 2011 +0100
+++ b/tools/libxl/libxl.c	Tue Dec 13 09:49:55 2011 +0100
@@ -972,6 +972,8 @@ int libxl_device_disk_add(libxl_ctx *ctx
     flexarray_t *front;
     flexarray_t *back;
     char *dev;
+    char *be_path, *state_path, *state;
+    struct timeval tv;
     libxl__device device;
     int major, minor, rc;
 
@@ -1076,6 +1078,23 @@ int libxl_device_disk_add(libxl_ctx *ctx
                              libxl__xs_kvs_of_flexarray(&gc, back, back->count),
                              libxl__xs_kvs_of_flexarray(&gc, front, front->count));
 
+    be_path = libxl__device_backend_path(&gc, &device);
+    state_path = libxl__sprintf(&gc, "%s/state", be_path);
+    state = libxl__xs_read(&gc, XBT_NULL, state_path);
+
+    if (atoi(state) != XenbusStateInitWait) {
+        xs_watch(ctx->xsh, state_path, be_path);
+        tv.tv_sec = LIBXL_DESTROY_TIMEOUT;
+        tv.tv_usec = 0;
+        rc = libxl__wait_for_device_state(&gc, &tv, XenbusStateInitWait, NULL);
+        xs_unwatch(ctx->xsh, state_path, be_path);
+        if (rc < 0) {
+            LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                       "unable to initialize disk device: %s\n",
+                       disk->pdev_path);
+            goto out_free;
+        }
+    }
     rc = 0;
 
 out_free:
@@ -1451,6 +1470,8 @@ int libxl_device_nic_add(libxl_ctx *ctx,
     flexarray_t *back;
     libxl__device device;
     char *dompath, **l;
+    char *be_path, *state_path, *state;
+    struct timeval tv;
     unsigned int nb, rc;
 
     front = flexarray_make(16, 1);
@@ -1519,8 +1540,25 @@ int libxl_device_nic_add(libxl_ctx *ctx,
                              libxl__xs_kvs_of_flexarray(&gc, back, back->count),
                              libxl__xs_kvs_of_flexarray(&gc, front, front->count));
 
-    /* FIXME: wait for plug */
+    be_path = libxl__device_backend_path(&gc, &device);
+    state_path = libxl__sprintf(&gc, "%s/state", be_path);
+    state = libxl__xs_read(&gc, XBT_NULL, state_path);
+
+    if (atoi(state) != XenbusStateInitWait) {
+        xs_watch(ctx->xsh, state_path, be_path);
+        tv.tv_sec = LIBXL_DESTROY_TIMEOUT;
+        tv.tv_usec = 0;
+        rc = libxl__wait_for_device_state(&gc, &tv, XenbusStateInitWait, NULL);
+        xs_unwatch(ctx->xsh, state_path, be_path);
+        if (rc < 0) {
+            LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                       "unable to initialize nic device: %s\n",
+                       nic->ifname);
+            goto out_free;
+        }
+    }
     rc = 0;
+
 out_free:
     flexarray_free(back);
     flexarray_free(front);

[PATCH 06 of 14 v4] libxl: execute hotplug scripts directly from libxl

[Roger Pau Monne]

# HG changeset patch
# User Roger Pau Monne <roger.pau@entel.upc.edu>
# Date 1317386335 -7200
# Node ID a3a95ea16ca4e34c8213522d4aae854ec16b6057
# Parent  148765a54f6ed77fb83ea6c8788e420a0781f225
libxl: execute hotplug scripts directly from libxl.

Added the necessary handlers to execute hotplug scripts when necessary
from libxl. Split NetBSD and Linux hotplug calls into two separate
files, because parameters for hotplug scripts are different. Linux
has empty functions, since the calling of hotplug scripts is still
done using udev.

Signed-off-by: Roger Pau Monne <roger.pau@entel.upc.edu>

diff -r 148765a54f6e -r a3a95ea16ca4 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Tue Dec 13 09:49:55 2011 +0100
+++ b/tools/libxl/libxl.c	Fri Sep 30 14:38:55 2011 +0200
@@ -1019,6 +1019,11 @@ int libxl_device_disk_add(libxl_ctx *ctx
             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:
@@ -1095,6 +1100,15 @@ int libxl_device_disk_add(libxl_ctx *ctx
             goto out_free;
         }
     }
+
+    /* Call hotplug scripts to attach device */
+    if (libxl__device_execute_hotplug(&gc, &device, CONNECT) < 0) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to execute hotplug script for disk: %s\n",
+                   disk->pdev_path);
+        rc = -1;
+        goto out_free;
+    }
+
     rc = 0;
 
 out_free:
@@ -1557,6 +1571,15 @@ int libxl_device_nic_add(libxl_ctx *ctx,
             goto out_free;
         }
     }
+
+    /* Call hotplug scripts to attach device */
+    if (libxl__device_execute_hotplug(&gc, &device, CONNECT) < 0) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to execute hotplug script for nic: %s\n",
+                   nic->ifname);
+        rc = -1;
+        goto out_free;
+    }
+
     rc = 0;
 
 out_free:
diff -r 148765a54f6e -r a3a95ea16ca4 tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c	Tue Dec 13 09:49:55 2011 +0100
+++ b/tools/libxl/libxl_device.c	Fri Sep 30 14:38:55 2011 +0200
@@ -68,6 +68,25 @@ int libxl__parse_backend_path(libxl__gc 
     return libxl__device_kind_from_string(strkind, &dev->backend_kind);
 }
 
+int libxl__device_execute_hotplug(libxl__gc *gc, libxl__device *dev,
+                                  libxl__hotplug_action action)
+{
+    int rc = 0;
+
+    switch(dev->kind) {
+    case LIBXL__DEVICE_KIND_VIF:
+        rc = libxl__nic_hotplug(gc, dev, action);
+        break;
+    case LIBXL__DEVICE_KIND_VBD:
+        rc = libxl__disk_hotplug(gc, dev, action);
+        break;
+    default:
+        break;
+    }
+
+    return rc;
+}
+
 int libxl__device_generic_add(libxl__gc *gc, libxl__device *device,
                              char **bents, char **fents)
 {
@@ -449,6 +468,7 @@ int libxl__device_remove(libxl__gc *gc, 
     if (!state)
         goto out;
     if (atoi(state) != 4) {
+        libxl__device_execute_hotplug(gc, dev, DISCONNECT);
         libxl__device_destroy_tapdisk(gc, be_path);
         xs_rm(ctx->xsh, XBT_NULL, be_path);
         goto out;
@@ -493,6 +513,12 @@ int libxl__device_destroy(libxl__gc *gc,
     char *be_path = libxl__device_backend_path(gc, dev);
     char *fe_path = libxl__device_frontend_path(gc, dev);
 
+    /* 
+     * Run hotplug scripts, which will probably not be able to
+     * execute successfully since the device may still be plugged
+     */
+    libxl__device_execute_hotplug(gc, dev, DISCONNECT);
+
     xs_rm(ctx->xsh, XBT_NULL, be_path);
     xs_rm(ctx->xsh, XBT_NULL, fe_path);
 
diff -r 148765a54f6e -r a3a95ea16ca4 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h	Tue Dec 13 09:49:55 2011 +0100
+++ b/tools/libxl/libxl_internal.h	Fri Sep 30 14:38:55 2011 +0200
@@ -289,6 +289,46 @@ _hidden int libxl__wait_for_device_state
  */
 _hidden int libxl__try_phy_backend(mode_t st_mode);
 
+/* hotplug functions */
+
+/* Action to pass to hotplug caller functions */
+typedef enum {
+    CONNECT = 1,
+    DISCONNECT = 2
+} libxl__hotplug_action;
+
+/*
+ * libxl__device_execute_hotplug - generic function to execute hotplug scripts
+ * gc: allocation pool
+ * dev: reference to the device that executes the hotplug scripts
+ * action: action to execute
+ *
+ * Returns 0 on success, and < 0 on error.
+ */
+_hidden int libxl__device_execute_hotplug(libxl__gc *gc, libxl__device *dev,
+                                          libxl__hotplug_action action);
+
+/*
+ * libxl__disk_hotplug - execute hotplug script for a disk type device
+ * gc: allocation pool
+ * dev: reference to the disk device
+ * action: action to execute
+ *
+ * Returns 0 on success, and < 0 on error.
+ */
+_hidden int libxl__disk_hotplug(libxl__gc *gc, libxl__device *dev,
+                                libxl__hotplug_action action);
+
+/*
+ * libxl__nic_hotplug - execute hotplug script for a nic type device
+ * gc: allocation pool
+ * dev: reference to the nic device
+ *
+ * Returns 0 on success, and < 0 on error.
+ */
+_hidden int libxl__nic_hotplug(libxl__gc *gc, libxl__device *dev,
+                               libxl__hotplug_action action);
+
 /* from libxl_pci */
 
 _hidden int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, int starting);
diff -r 148765a54f6e -r a3a95ea16ca4 tools/libxl/libxl_linux.c
--- a/tools/libxl/libxl_linux.c	Tue Dec 13 09:49:55 2011 +0100
+++ b/tools/libxl/libxl_linux.c	Fri Sep 30 14:38:55 2011 +0200
@@ -25,3 +25,17 @@ int libxl__try_phy_backend(mode_t st_mod
 
     return 1;
 }
+
+/* Hotplug scripts caller functions */
+
+int libxl_disk_hotplug(libxl__gc *gc, libxl__device *dev,
+                       libxl__hotplug_action action)
+{
+    return 0;
+}
+
+int libxl_nic_hotplug_connect(libxl__gc *gc, libxl__device *dev,
+                              libxl__hotplug_action action)
+{
+    return 0;
+}
diff -r 148765a54f6e -r a3a95ea16ca4 tools/libxl/libxl_netbsd.c
--- a/tools/libxl/libxl_netbsd.c	Tue Dec 13 09:49:55 2011 +0100
+++ b/tools/libxl/libxl_netbsd.c	Fri Sep 30 14:38:55 2011 +0200
@@ -14,6 +14,7 @@
  */
  
 #include <sys/stat.h>
+#include <sys/wait.h>
 
 #include "libxl_internal.h"
 
@@ -24,3 +25,130 @@ int libxl__try_phy_backend(mode_t st_mod
 
     return 0;
 }
+
+/* Hotplug scripts caller functions */
+
+int libxl__disk_hotplug(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);
+    struct stat stab;
+    char *stype, *sstate, *script, *params;
+    char **args;
+    int status, nr = 0;
+    int rc = -1;
+    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;
+    }
+
+    params = libxl__xs_read(gc, XBT_NULL,
+                            libxl__sprintf(gc, "%s/%s", be_path, "params"));
+    if (!params)
+        return -1;
+
+    sstate = libxl__xs_read(gc, XBT_NULL,
+                            libxl__sprintf(gc, "%s/%s", be_path, "state"));
+    if (!sstate) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Unable to read state from %s",
+                   be_path);
+        return -1;
+    }
+
+    if (stat(params, &stab) < 0) {
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "failed to get stat info\n");
+        return -1;
+    }
+    if (S_ISBLK(stab.st_mode))
+        stype = "phy";
+    if (S_ISREG(stab.st_mode))
+        stype = "file";
+    if (stype == NULL) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Not block or regular file");
+        return -1;
+    }
+
+    f_args = flexarray_make(5, 1);
+    if (!f_args)
+        return -1;
+
+    flexarray_set(f_args, nr++, script);
+    flexarray_set(f_args, nr++, be_path);
+    flexarray_set(f_args, nr++, libxl__sprintf(gc, "%d", action));
+    flexarray_set(f_args, nr++, stype);
+    flexarray_set(f_args, nr++, NULL);
+
+    args = (char **) flexarray_contents(f_args);
+
+    LIBXL__LOG(ctx, LIBXL__LOG_DEBUG,
+               "Calling disk hotplug script: %s %s %s %s",
+               args[0], args[1], args[2], args[3]);
+    status = libxl__forkexec(gc, -1, -1, -1, args);
+    if (!WIFEXITED(status) || WEXITSTATUS(status) == EXIT_FAILURE) {
+        rc = -1;
+        goto out;
+    }
+    rc = 0;
+
+out:
+    free(args);
+    return rc;
+}
+
+int libxl__nic_hotplug(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 *sstate, *script;
+    char **args;
+    int status, nr = 0;
+    int rc = -1;
+    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;
+    }
+
+    sstate = libxl__xs_read(gc, XBT_NULL,
+                            libxl__sprintf(gc, "%s/%s", be_path, "state"));
+    if (!sstate) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Unable to read state from %s",
+                                          be_path);
+        return -1;
+    }
+
+    f_args = flexarray_make(4, 1);
+    if (!f_args)
+        return -1;
+
+    flexarray_set(f_args, nr++, script);
+    flexarray_set(f_args, nr++, be_path);
+    flexarray_set(f_args, nr++, libxl__sprintf(gc, "%d", action));
+    flexarray_set(f_args, nr++, NULL);
+
+    args = (char **) flexarray_contents(f_args);
+
+    LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Calling nic hotplug script: %s %s %s",
+               args[0], args[1], args[2]);
+    status = libxl__forkexec(gc, -1, -1, -1, args);
+    if (!WIFEXITED(status) || WEXITSTATUS(status) == EXIT_FAILURE) {
+        rc = -1;
+        goto out;
+    }
+    rc = 0;
+
+out:
+    free(args);
+    return rc;
+}

Re: [PATCH 06 of 14 v4] libxl: execute hotplug scripts directly from libxl

[Ian Jackson]

Roger Pau Monne writes ("[Xen-devel] [PATCH 06 of 14 v4] libxl: execute hotplug scripts directly from libxl"):
> libxl: execute hotplug scripts directly from libxl.
> 
> Added the necessary handlers to execute hotplug scripts when necessary
> >from libxl. Split NetBSD and Linux hotplug calls into two separate
> files, because parameters for hotplug scripts are different. Linux
> has empty functions, since the calling of hotplug scripts is still
> done using udev.

I think this would be improved by an explanation of what the job of
the hotplug scripts are and what their calling conventions are, on
NetBSD.  I'm afraid as it is I don't understand what the hotplug
scripts do on NetBSD.

I know what they do on Linux for vifs.  On Linux we are missing script
support for block devices, but it's not clear that they are really
"hotplug" scripts.

Also your patch has some long lines.

Ian.

[PATCH 07 of 14 v4] hotplug NetBSD: pass an action instead of a state to hotplug scripts

[Roger Pau Monne]

# HG changeset patch
# User Roger Pau Monne <roger.pau@entel.upc.edu>
# Date 1323766195 -3600
# Node ID 51404e6e2d6f4d53a14deadca4b62748d449782b
# Parent  a3a95ea16ca4e34c8213522d4aae854ec16b6057
hotplug NetBSD: pass an action instead of a state to hotplug scripts

change second parameter of NetBSD hotplug scripts to take an action
(CONNECT/DISCONNECT) instead of a xenbus state.

Signed-off-by: Roger Pau Monne <roger.pau@entel.upc.edu>

diff -r a3a95ea16ca4 -r 51404e6e2d6f tools/hotplug/NetBSD/block
--- a/tools/hotplug/NetBSD/block	Fri Sep 30 14:38:55 2011 +0200
+++ b/tools/hotplug/NetBSD/block	Tue Dec 13 09:49:55 2011 +0100
@@ -18,12 +18,12 @@ error() {
 	
 
 xpath=$1
-xstatus=$2
+xaction=$2
 xtype=$3
 xparams=$(xenstore-read "$xpath/params")
 
-case $xstatus in
-6)
+case $xaction in
+2)
 	# device removed
 	case $xtype in
 	file)
@@ -41,7 +41,7 @@ 6)
 	xenstore-rm $xpath
 	exit 0
 	;;
-2)
+1)
 	case $xtype in
 	file)
 		# Store the list of available vnd(4) devices in
diff -r a3a95ea16ca4 -r 51404e6e2d6f tools/hotplug/NetBSD/vif-bridge
--- a/tools/hotplug/NetBSD/vif-bridge	Fri Sep 30 14:38:55 2011 +0200
+++ b/tools/hotplug/NetBSD/vif-bridge	Tue Dec 13 09:49:55 2011 +0100
@@ -11,15 +11,15 @@ PATH=${BINDIR}:${SBINDIR}:${LIBEXEC}:${P
 export PATH
 
 xpath=$1
-xstatus=$2
+xaction=$2
 
-case $xstatus in
-6)
+case $xaction in
+2)
 	# device removed
 	xenstore-rm $xpath
 	exit 0
 	;;
-2)
+1)
 	xbridge=$(xenstore-read "$xpath/bridge")
 	xfid=$(xenstore-read "$xpath/frontend-id")
 	xhandle=$(xenstore-read "$xpath/handle")
diff -r a3a95ea16ca4 -r 51404e6e2d6f tools/hotplug/NetBSD/vif-ip
--- a/tools/hotplug/NetBSD/vif-ip	Fri Sep 30 14:38:55 2011 +0200
+++ b/tools/hotplug/NetBSD/vif-ip	Tue Dec 13 09:49:55 2011 +0100
@@ -11,15 +11,15 @@ PATH=${BINDIR}:${SBINDIR}:${LIBEXEC}:${P
 export PATH
 
 xpath=$1
-xstatus=$2
+xaction=$2
 
-case $xstatus in
-6)
+case $xaction in
+2)
 	# device removed
 	xenstore-rm $xpath
 	exit 0
 	;;
-2)
+1)
 	xip=$(xenstore-read "$xpath/ip")
 	xfid=$(xenstore-read "$xpath/frontend-id")
 	xhandle=$(xenstore-read "$xpath/handle")

Re: [PATCH 07 of 14 v4] hotplug NetBSD: pass an action instead of a state to hotplug scripts

[Ian Jackson]

Roger Pau Monne writes ("[Xen-devel] [PATCH 07 of 14 v4] hotplug NetBSD: pass an action instead of a state to hotplug scripts"):
> hotplug NetBSD: pass an action instead of a state to hotplug scripts
> 
> change second parameter of NetBSD hotplug scripts to take an action
> (CONNECT/DISCONNECT) instead of a xenbus state.

Aren't these hotplug scripts generally installed in /etc, where people
and tools will avoid simply overwriting old versions with new ?

In which case this apparently-subtle but actually-radical change to
their API might be a problem.

Or am I wrong ?

Ian.

[PATCH 08 of 14 v4] xenbackendd: pass action to hotplug script

[Roger Pau Monne]

# HG changeset patch
# User Roger Pau Monne <roger.pau@entel.upc.edu>
# Date 1323766195 -3600
# Node ID 30f2db9a780fe36eeacf6dabd76bb2577248edea
# Parent  51404e6e2d6f4d53a14deadca4b62748d449782b
xenbackendd: pass action to hotplug script

Pass an action to hotplug scripts instead of a xenbus state.

Signed-off-by: Roger Pau Monne <roger.pau@entel.upc.edu>

diff -r 51404e6e2d6f -r 30f2db9a780f tools/xenbackendd/xenbackendd.c
--- a/tools/xenbackendd/xenbackendd.c	Tue Dec 13 09:49:55 2011 +0100
+++ b/tools/xenbackendd/xenbackendd.c	Tue Dec 13 09:49:55 2011 +0100
@@ -34,6 +34,9 @@
 #define DEVTYPE_VIF 1
 #define DEVTYPE_VBD 2
 
+#define CONNECT "1"
+#define DISCONNECT "2"
+
 #define DOMAIN_PATH "/local/domain/0"
 
 #ifndef XEN_SCRIPT_DIR
@@ -150,6 +153,7 @@ main(int argc, char * const argv[])
 	unsigned int num;
 	char *s;
 	int state;
+	char *action;
 	char *sstate;
 	char *stype;
 	char *params;
@@ -300,7 +304,8 @@ main(int argc, char * const argv[])
 				    strerror(errno));
 				goto next2;
 			}
-			doexec(s, vec[XS_WATCH_PATH], sstate, NULL);
+			action = (state == 6 ? DISCONNECT : CONNECT);
+			doexec(s, vec[XS_WATCH_PATH], action, NULL);
 			break;
 
 		case DEVTYPE_VBD:
@@ -329,7 +334,8 @@ main(int argc, char * const argv[])
 					params, strerror(errno));
 				goto next3;
 			}
-			doexec(vbd_script, vec[XS_WATCH_PATH], sstate, stype);
+			action = (state == 6 ? DISCONNECT : CONNECT);
+			doexec(vbd_script, vec[XS_WATCH_PATH], action, stype);
 next3:
 			free(params);
 			break;

Re: [PATCH 07 of 14 v4] hotplug NetBSD: pass an action instead of a state to hotplug scripts [and 1 more messages]

[Ian Jackson]

Roger Pau Monne writes ("[Xen-devel] [PATCH 07 of 14 v4] hotplug NetBSD: pass an action instead of a state to hotplug scripts"):
> hotplug NetBSD: pass an action instead of a state to hotplug scripts
> 
> change second parameter of NetBSD hotplug scripts to take an action
> (CONNECT/DISCONNECT) instead of a xenbus state.

Roger Pau Monne writes ("[Xen-devel] [PATCH 08 of 14 v4] xenbackendd: pass action to hotplug script"):
> xenbackendd: pass action to hotplug script
> 
> Pass an action to hotplug scripts instead of a xenbus state.
> 
> Signed-off-by: Roger Pau Monne <roger.pau@entel.upc.edu>

Also I have just spotted that you seem to have made two halves of a
mutually-incompatble change in different patches.

Don't do that, because it breaks bisectability.  The tree should be
buildable and useable at every point.

Thanks,
Ian.

[PATCH 09 of 14 v4] hotplug: remove debug messages from NetBSD hotplug scripts

[Roger Pau Monne]

# HG changeset patch
# User Roger Pau Monne <roger.pau@entel.upc.edu>
# Date 1323766195 -3600
# Node ID 263959b9bc339a60da119c09c4fe229d29d7911e
# Parent  30f2db9a780fe36eeacf6dabd76bb2577248edea
hotplug: remove debug messages from NetBSD hotplug scripts

Remove unecessary debug messages from NetBSD hotplug scripts, left
error messages only.

Signed-off-by: Roger Pau Monne <roger.pau@entel.upc.edu>

diff -r 30f2db9a780f -r 263959b9bc33 tools/hotplug/NetBSD/block
--- a/tools/hotplug/NetBSD/block	Tue Dec 13 09:49:55 2011 +0100
+++ b/tools/hotplug/NetBSD/block	Tue Dec 13 09:49:55 2011 +0100
@@ -64,14 +64,12 @@ 1)
 			if [ "$status" = "free" ] && \
 			    vnconfig /dev/${disk}d $xparams >/dev/null; then
 				device=/dev/${disk}d
-				echo vnconfig /dev/${disk}d $xparams
 				break	
 			fi
 		done
 		if [ x$device = x ] ; then
 			error "no available vnd device"
 		fi
-		echo xenstore-write $xpath/vnd $device
 		xenstore-write $xpath/vnd $device
 		;;
 	phy)
@@ -79,9 +77,7 @@ 1)
 		;;
 	esac
 	physical_device=$(stat -f '%r' "$device")
-	echo xenstore-write $xpath/physical-device $physical_device
 	xenstore-write $xpath/physical-device $physical_device
-	echo xenstore-write $xpath/hotplug-status connected
 	xenstore-write $xpath/hotplug-status connected
 	exit 0
 	;;
diff -r 30f2db9a780f -r 263959b9bc33 tools/hotplug/NetBSD/vif-bridge
--- a/tools/hotplug/NetBSD/vif-bridge	Tue Dec 13 09:49:55 2011 +0100
+++ b/tools/hotplug/NetBSD/vif-bridge	Tue Dec 13 09:49:55 2011 +0100
@@ -24,12 +24,9 @@ 1)
 	xfid=$(xenstore-read "$xpath/frontend-id")
 	xhandle=$(xenstore-read "$xpath/handle")
 	iface=$(xenstore-read "$xpath/vifname")
-	echo ifconfig $iface up
 	ifconfig $iface up
 	brconfig $xbridge add $iface
-	echo brconfig $xbridge add $iface
 	xenstore-write $xpath/hotplug-status connected
-	echo xenstore-write $xpath/hotplug-status connected
 	exit 0
 	;;
 *)
diff -r 30f2db9a780f -r 263959b9bc33 tools/hotplug/NetBSD/vif-ip
--- a/tools/hotplug/NetBSD/vif-ip	Tue Dec 13 09:49:55 2011 +0100
+++ b/tools/hotplug/NetBSD/vif-ip	Tue Dec 13 09:49:55 2011 +0100
@@ -24,10 +24,8 @@ 1)
 	xfid=$(xenstore-read "$xpath/frontend-id")
 	xhandle=$(xenstore-read "$xpath/handle")
 	iface=$(xenstore-read "$xpath/vifname")
-	echo ifconfig $iface $xip up
 	ifconfig $iface $xip up
 	xenstore-write $xpath/hotplug-status connected
-	echo xenstore-write $xpath/hotplug-status connected
 	exit 0
 	;;
 *)

Re: [PATCH 09 of 14 v4] hotplug: remove debug messages from NetBSD hotplug scripts

[Ian Jackson]

Roger Pau Monne writes ("[Xen-devel] [PATCH 09 of 14 v4] hotplug: remove debug messages from NetBSD hotplug scripts"):
> hotplug: remove debug messages from NetBSD hotplug scripts
> 
> Remove unecessary debug messages from NetBSD hotplug scripts, left
> error messages only.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

[PATCH 10 of 14 v4] rc.d NetBSD: don't start xenbackendd by default, only when xend needs it

[Roger Pau Monne]

# HG changeset patch
# User Roger Pau Monne <roger.pau@entel.upc.edu>
# Date 1317386335 -7200
# Node ID d599451d870d77e05c6b12b5560f45192f6c046e
# Parent  263959b9bc339a60da119c09c4fe229d29d7911e
rc.d NetBSD: don't start xenbackendd by default, only when xend needs it.

With the move of hotplug scripts from xenbackendd to libxl
xenbackendd is no longer needed by libxl, only start it when xend is
started.

Signed-off-by: Roger Pau Monne <roger.pau@entel.upc.edu>

diff -r 263959b9bc33 -r d599451d870d tools/hotplug/NetBSD/rc.d/xenbackendd
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/hotplug/NetBSD/rc.d/xenbackendd	Fri Sep 30 14:38:55 2011 +0200
@@ -0,0 +1,27 @@
+#!/bin/sh
+#
+# PROVIDE: xenbackendd
+# REQUIRE: xencommons
+
+. /etc/rc.subr
+
+DIR=$(dirname "$0")
+. "${DIR}/xen-hotplugpath.sh"
+
+LD_LIBRARY_PATH="${LIBDIR}"
+export LD_LIBRARY_PATH PYTHONPATH
+PATH="${PATH}:${SBINDIR}"
+export PATH
+
+name="xenbackendd"
+rcvar=$name
+command="${SBINDIR}/xenbackendd"
+if [ -n "${XENBACKENDD_DEBUG}" ]; then
+	command_args="${XENBACKENDD_ARGS} -d"
+else
+	command_args="${XENBACKENDD_ARGS}"
+fi
+
+load_rc_config $name
+run_rc_command "$1"
+
diff -r 263959b9bc33 -r d599451d870d tools/hotplug/NetBSD/rc.d/xencommons
--- a/tools/hotplug/NetBSD/rc.d/xencommons	Tue Dec 13 09:49:55 2011 +0100
+++ b/tools/hotplug/NetBSD/rc.d/xencommons	Fri Sep 30 14:38:55 2011 +0200
@@ -22,8 +22,6 @@ required_files="/kern/xen/privcmd"
 
 XENSTORED_PIDFILE="/var/run/xenstored.pid"
 XENCONSOLED_PIDFILE="/var/run/xenconsoled.pid"
-XENBACKENDD_PIDFILE="/var/run/xenbackendd.pid"
-#XENBACKENDD_DEBUG=1
 #XENCONSOLED_TRACE="/var/log/xen/xenconsole-trace.log"
 #XENSTORED_TRACE="/var/log/xen/xenstore-trace.log"
 
@@ -46,7 +44,7 @@ xen_startcmd()
 			XENSTORED_ROOTDIR="/var/lib/xenstored"
 		fi
 		rm -f ${XENSTORED_ROOTDIR}/tdb* >/dev/null 2>&1
-		printf "Starting xenservices: xenstored, xenconsoled, xenbackendd."
+		printf "Starting xenservices: xenstored, xenconsoled."
 		XENSTORED_ARGS=" --pid-file ${XENSTORED_PIDFILE}"
 		if [ -n "${XENSTORED_TRACE}" ]; then
 			XENSTORED_ARGS="${XENSTORED_ARGS} -T /var/log/xen/xenstored-trace.log"
@@ -58,7 +56,7 @@ xen_startcmd()
 			sleep 1
 		done
 	else
-		printf "Starting xenservices: xenconsoled, xenbackendd."
+		printf "Starting xenservices: xenconsoled."
 	fi
 
 	XENCONSOLED_ARGS=""
@@ -68,13 +66,6 @@ xen_startcmd()
 
 	${SBINDIR}/xenconsoled ${XENCONSOLED_ARGS}
 
-	XENBACKENDD_ARGS=""
-	if [ -n "${XENBACKENDD_DEBUG}" ]; then
-		XENBACKENDD_ARGS="${XENBACKENDD_ARGS} -d"
-	fi
-
-	${SBINDIR}/xenbackendd ${XENBACKENDD_ARGS}
-
 	printf "\n"
 
 	printf "Setting domain 0 name.\n"
@@ -87,8 +78,6 @@ xen_stop()
 	printf "Stopping xencommons.\n"
 	printf "WARNING: Not stopping xenstored, as it cannot be restarted.\n"
 
-	rc_pid=$(check_pidfile ${XENBACKENDD_PIDFILE} ${SBINDIR}/xenbackendd)
-	pids="$pids $rc_pid"
 	rc_pid=$(check_pidfile ${XENCONSOLED_PIDFILE} ${SBINDIR}/xenconsoled)
 	pids="$pids $rc_pid"
 
@@ -108,17 +97,12 @@ xen_status()
 		pids="$pids $xenconsoled_pid"
 	fi
 
-	xenbackend_pid=$(check_pidfile ${XENBACKENDD_PIDFILE} ${SBINDIR}/xenbackendd)
-	if test -n ${xenbackend_pid}; then
-		pids="$pids $xenbackend_pid"
-	fi
-
-	if test -n "$xenbackend_pid" -a -n "$xenconsoled_pid" -a -n "$xenstored_pid";
+	if test -n "$xenconsoled_pid" -a -n "$xenstored_pid";
 	then
 		echo "xencommons are running as pids $pids."
 		return 0
 	fi
-	if test -z "$xenbackend_pid" -a -z "$xenconsoled_pid" -a -z "$xenstored_pid";
+	if test -z "$xenconsoled_pid" -a -z "$xenstored_pid";
 	then
 		echo "xencommons are not running."
 		return 0
@@ -134,11 +118,6 @@ xen_status()
 	else
 		echo "xenconsoled is not running."
 	fi
-	if test -n $xenbackend_pid; then
-		echo "xenbackendd is running as pid $xenbackend_pid."
-	else
-		echo "xenbackendd is not running."
-	fi
 }
 
 load_rc_config $name
diff -r 263959b9bc33 -r d599451d870d tools/hotplug/NetBSD/rc.d/xend
--- a/tools/hotplug/NetBSD/rc.d/xend	Tue Dec 13 09:49:55 2011 +0100
+++ b/tools/hotplug/NetBSD/rc.d/xend	Fri Sep 30 14:38:55 2011 +0200
@@ -1,7 +1,7 @@
 #!/bin/sh
 #
 # PROVIDE: xend
-# REQUIRE: xencommons
+# REQUIRE: xencommons xenbackendd
 
 . /etc/rc.subr

Re: [PATCH 10 of 14 v4] rc.d NetBSD: don't start xenbackendd by default, only when xend needs it

[Ian Jackson]

Roger Pau Monne writes ("[Xen-devel] [PATCH 10 of 14 v4] rc.d NetBSD: don't start xenbackendd by default, only when xend needs it"):
> rc.d NetBSD: don't start xenbackendd by default, only when xend needs it.
> 
> With the move of hotplug scripts from xenbackendd to libxl
> xenbackendd is no longer needed by libxl, only start it when xend is
> started.

This looks plausible given the other patches in the series.

Ian.

[PATCH 11 of 14 v4] libxl: fix incorrect log message in libxl_domain_destroy

[Roger Pau Monne]

# HG changeset patch
# User Roger Pau Monne <roger.pau@entel.upc.edu>
# Date 1323766195 -3600
# Node ID a36942bf253c957179d4fb406ac0a45c0ddd2b28
# Parent  d599451d870d77e05c6b12b5560f45192f6c046e
libxl: fix incorrect log message in libxl_domain_destroy

Fix a log message that was referring to libxl_devices_dispose instead
of libxl__devices_destroy.

Signed-off-by: Roger Pau Monne <roger.pau@entel.upc.edu>

diff -r d599451d870d -r a36942bf253c tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Fri Sep 30 14:38:55 2011 +0200
+++ b/tools/libxl/libxl.c	Tue Dec 13 09:49:55 2011 +0100
@@ -769,7 +769,7 @@ int libxl_domain_destroy(libxl_ctx *ctx,
         libxl__qmp_cleanup(&gc, domid);
     }
     if (libxl__devices_destroy(&gc, domid, force) < 0)
-        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "libxl_devices_dispose failed for %d", domid);
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "libxl__devices_destroy failed for %d", domid);
 
     vm_path = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/vm", dom_path));
     if (vm_path)

Re: [PATCH 11 of 14 v4] libxl: fix incorrect log message in libxl_domain_destroy

[Ian Jackson]

Roger Pau Monne writes ("[Xen-devel] [PATCH 11 of 14 v4] libxl: fix incorrect lo> libxl: fix incorrect log message in libxl_domain_destroy
> 
> Fix a log message that was referring to libxl_devices_dispose instead
> of libxl__devices_destroy.
> 
> Signed-off-by: Roger Pau Monne <roger.pau@entel.upc.edu>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

(Although it would be nice if you'd wrapped the long line while you
were there.)

Ian.

[PATCH 12 of 14 v4] libxl: set frontend status to 6 on domain destroy

[Roger Pau Monne]

# HG changeset patch
# User Roger Pau Monne <roger.pau@entel.upc.edu>
# Date 1323766195 -3600
# Node ID 35c322e6020501447936e56cafceb8bbc1b2e980
# Parent  a36942bf253c957179d4fb406ac0a45c0ddd2b28
libxl: set frontend status to 6 on domain destroy

Set frontend status to 6 on domain destruction and wait for devices to
be disconnected before executing hotplug scripts.

Signed-off-by: Roger Pau Monne <roger.pau@entel.upc.edu>

diff -r a36942bf253c -r 35c322e60205 tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c	Tue Dec 13 09:49:55 2011 +0100
+++ b/tools/libxl/libxl_device.c	Tue Dec 13 09:49:55 2011 +0100
@@ -513,10 +513,7 @@ int libxl__device_destroy(libxl__gc *gc,
     char *be_path = libxl__device_backend_path(gc, dev);
     char *fe_path = libxl__device_frontend_path(gc, dev);
 
-    /* 
-     * Run hotplug scripts, which will probably not be able to
-     * execute successfully since the device may still be plugged
-     */
+    libxl__xs_write(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s", fe_path, "state"), "6");
     libxl__device_execute_hotplug(gc, dev, DISCONNECT);
 
     xs_rm(ctx->xsh, XBT_NULL, be_path);

Re: [PATCH 12 of 14 v4] libxl: set frontend status to 6 on domain destroy

[Ian Jackson]

Roger Pau Monne writes ("[Xen-devel] [PATCH 12 of 14 v4] libxl: set frontend status to 6 on domain destroy"):
> libxl: set frontend status to 6 on domain destroy
> 
> Set frontend status to 6 on domain destruction and wait for devices to
> be disconnected before executing hotplug scripts.

There seems to be a race here.

> +    libxl__xs_write(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s", fe_path, "state"), "6");

So here, the kernel or backendd start racing, and you hope that they
win the race and close the device before ...

>      libxl__device_execute_hotplug(gc, dev, DISCONNECT);

... the hotplug script tries to remove it.

Is there something we can do to make sure that we always get this
right ?

Ian.

Re: [PATCH 12 of 14 v4] libxl: set frontend status to 6 on domain destroy

[Roger Pau Monné]

2011/12/13 Ian Jackson <Ian.Jackson@eu.citrix.com>:
> Roger Pau Monne writes ("[Xen-devel] [PATCH 12 of 14 v4] libxl: set frontend status to 6 on domain destroy"):
>> libxl: set frontend status to 6 on domain destroy
>>
>> Set frontend status to 6 on domain destruction and wait for devices to
>> be disconnected before executing hotplug scripts.
>
> There seems to be a race here.
>
>> +    libxl__xs_write(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s", fe_path, "state"), "6");
>
> So here, the kernel or backendd start racing, and you hope that they
> win the race and close the device before ...

From my experience in NetBSD, the kernel only closes the device when
it's frontend state is set to 6, since we destroy the domain, it is
unable to set the status to 6, and thus the kernel doesn't detach the
devices. I've added some libxl__wait_for_device_state logic here, to
assure the backend state is set to 6 before trying to execute hotplug
scripts. The truth is that I had it in previous versions of my patch,
but it seems the kernel always switches contexts and detaches the
devices before executing hotplug scripts (it might just be luck).

Also this patch speeds domain destruction a lot (which is also quite
slow under Linux from what I saw).

>>      libxl__device_execute_hotplug(gc, dev, DISCONNECT);
>
> ... the hotplug script tries to remove it.
>
> Is there something we can do to make sure that we always get this
> right ?
>
> Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH 12 of 14 v4] libxl: set frontend status to 6 on domain destroy

[Ian Campbell]

On Wed, 2011-12-14 at 09:13 +0000, Roger Pau Monné wrote:
> 2011/12/13 Ian Jackson <Ian.Jackson@eu.citrix.com>:
> > Roger Pau Monne writes ("[Xen-devel] [PATCH 12 of 14 v4] libxl: set frontend status to 6 on domain destroy"):
> >> libxl: set frontend status to 6 on domain destroy
> >>
> >> Set frontend status to 6 on domain destruction and wait for devices to
> >> be disconnected before executing hotplug scripts.
> >
> > There seems to be a race here.
> >
> >> +    libxl__xs_write(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s", fe_path, "state"), "6");
> >
> > So here, the kernel or backendd start racing, and you hope that they
> > win the race and close the device before ...
> 
> From my experience in NetBSD, the kernel only closes the device when
> it's frontend state is set to 6, since we destroy the domain, it is
> unable to set the status to 6, and thus the kernel doesn't detach the
> devices.

So if you rm the backend directory the NetBSD does not take that as a
sign to tear down the device? That sounds like a bug in the NetBSD
backend -- it should treat deletion of the backend state dir as if it
were reading state = "6" and shut everything down.

Or is the issue only in the userspace portions?

>  I've added some libxl__wait_for_device_state logic here, to
> assure the backend state is set to 6 before trying to execute hotplug
> scripts.

But that will always be true with this patch since you set it that way
just before, right?

If you go down this path then I think you need to set the state to
"5" (Closing) in order to prompt the backend to transition to
"6" (Closed) itself. However you need to be careful about adding a
synchronous wait to the device destroy function. This should eventually
work even if the frontend and backend are not co-operating. That starts
to look a bit like calling libxl__device_remove instead.

>  The truth is that I had it in previous versions of my patch,
> but it seems the kernel always switches contexts and detaches the
> devices before executing hotplug scripts (it might just be luck).

Probably just luck and partly due e.g. to your presumably system being
only very lightly loaded.

Ian.

> 
> Also this patch speeds domain destruction a lot (which is also quite
> slow under Linux from what I saw).
> 
> >>      libxl__device_execute_hotplug(gc, dev, DISCONNECT);
> >
> > ... the hotplug script tries to remove it.
> >
> > Is there something we can do to make sure that we always get this
> > right ?
> >
> > Ian.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH 12 of 14 v4] libxl: set frontend status to 6 on domain destroy

[Roger Pau Monné]

2011/12/14 Ian Campbell <Ian.Campbell@citrix.com>:
> So if you rm the backend directory the NetBSD does not take that as a
> sign to tear down the device? That sounds like a bug in the NetBSD
> backend -- it should treat deletion of the backend state dir as if it
> were reading state = "6" and shut everything down.

Yes, if I delete the frontend from xenstore, the devices are detached.
Anyway, doing it one way or another (deletion or setting state to 6)
doesn't really make a difference, you still have to wait for the
backend to be disconnected (detached) before executing hotplug
scripts.

> Or is the issue only in the userspace portions?
>
>>  I've added some libxl__wait_for_device_state logic here, to
>> assure the backend state is set to 6 before trying to execute hotplug
>> scripts.
>
> But that will always be true with this patch since you set it that way
> just before, right?

I set frontend status to 6, and I suggest to wait for backend status
to be 6 also, then execute hotplug scripts. It won't be true, because
frontend and backend status are not tied (in the NetBSD case backend
status is set by the Dom0 kernel or hotplug scripts, while frontend
status is set by the DomU).

> If you go down this path then I think you need to set the state to
> "5" (Closing) in order to prompt the backend to transition to
> "6" (Closed) itself. However you need to be careful about adding a
> synchronous wait to the device destroy function. This should eventually
> work even if the frontend and backend are not co-operating. That starts
> to look a bit like calling libxl__device_remove instead.

Do you mean to set backend status to 5 instead of setting frontend to
6, and then wait for the kernel to do the transition from 5 to 6,
instead of setting the frontend to 6?

>>  The truth is that I had it in previous versions of my patch,
>> but it seems the kernel always switches contexts and detaches the
>> devices before executing hotplug scripts (it might just be luck).
>
> Probably just luck and partly due e.g. to your presumably system being
> only very lightly loaded.
>
> Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH 12 of 14 v4] libxl: set frontend status to 6 on domain destroy

[Ian Jackson]

Roger Pau Monné writes ("Re: [Xen-devel] [PATCH 12 of 14 v4] libxl: set frontend status to 6 on domain destroy"):
> 2011/12/14 Ian Campbell <Ian.Campbell@citrix.com>:
> > So if you rm the backend directory the NetBSD does not take that as a
> > sign to tear down the device? That sounds like a bug in the NetBSD
> > backend -- it should treat deletion of the backend state dir as if it
> > were reading state = "6" and shut everything down.
> 
> Yes, if I delete the frontend from xenstore, the devices are detached.
> Anyway, doing it one way or another (deletion or setting state to 6)
> doesn't really make a difference, you still have to wait for the
> backend to be disconnected (detached) before executing hotplug
> scripts.

So device destruction could be achieved by simply deleting the backend
directory.  But, we would need some other way to detect when the
backend has actually closed the device so that we can run the script.

Ian.

Re: [PATCH 12 of 14 v4] libxl: set frontend status to 6 on domain destroy

[Roger Pau Monné]

2011/12/14 Ian Jackson <Ian.Jackson@eu.citrix.com>:
>> Yes, if I delete the frontend from xenstore, the devices are detached.
>> Anyway, doing it one way or another (deletion or setting state to 6)
>> doesn't really make a difference, you still have to wait for the
>> backend to be disconnected (detached) before executing hotplug
>> scripts.
>
> So device destruction could be achieved by simply deleting the backend
> directory.  But, we would need some other way to detect when the
> backend has actually closed the device so that we can run the script.

Device detach, will in turn allows destruction, can be achieved in two ways:

 * Set frontend status to 6.
 * Remove frontend entries.

There is no need to change backend entries to force detach a device.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH 12 of 14 v4] libxl: set frontend status to 6 on domain destroy

[Ian Campbell]

On Wed, 2011-12-14 at 12:12 +0000, Roger Pau Monné wrote:
> 2011/12/14 Ian Jackson <Ian.Jackson@eu.citrix.com>:
> >> Yes, if I delete the frontend from xenstore, the devices are detached.
> >> Anyway, doing it one way or another (deletion or setting state to 6)
> >> doesn't really make a difference, you still have to wait for the
> >> backend to be disconnected (detached) before executing hotplug
> >> scripts.
> >
> > So device destruction could be achieved by simply deleting the backend
> > directory.  But, we would need some other way to detect when the
> > backend has actually closed the device so that we can run the script.
> 
> Device detach, will in turn allows destruction, can be achieved in two ways:
> 
>  * Set frontend status to 6.
>  * Remove frontend entries.
> 
> There is no need to change backend entries to force detach a device.

The correct method for forcibly destroying a device is to remove the
backend directory.

The correct method to perform a graceful remove is to set the backend
state to "5" and wait for it to coordinate with the guest to reach state
"6". However this is not a forced operation and may not complete in a
timely manner or indeed at all.

Some backends also support an "online" node which, if left set to 1 when
setting the backend state to 5" for a graceful shutdown, causes the
backend to detach but remain active (as opposed to self destructing) in
the expectation of a frontend reconnecting again later -- this is used
for example when kexecing a PV domain.

In no case should the toolstack be messing with the frontend state,
although obviously in the destroy case it is at liberty shortly
afterwards to also nuke the frontend directory.

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH 12 of 14 v4] libxl: set frontend status to 6 on domain destroy

[Ian Jackson]

I have been thinking about this whole area.

Originally my opinion was that the block and network setup scripts
(eg, setting up loopback devices or iscsi or bridging whatever) should
be executed directly by xl.  This naturally gives the best error
handling and makes logging easiest.

However, if we are serious about supporting driver domains, or indeed
any kind of backend not running in the same domain as libxl, then
something in the driver domain needs to be responsible for executing
these scripts (or equivalent).

The obvious way to communicate this information to the driver domain
is via xenstore.

What we should be discussing is exactly how the driver domain
translates that into script execution.  Currently on Linux this is
mostly done with udev, and AIUI on BSD using xenbackendd.

So sorry for leading this discussion in what I now think may be a
wrong direction.

Ian.

Re: [PATCH 12 of 14 v4] libxl: set frontend status to 6 on domain destroy

[Ian Campbell]

On Thu, 2011-12-15 at 17:44 +0000, Ian Jackson wrote:
> I have been thinking about this whole area.
> 
> Originally my opinion was that the block and network setup scripts
> (eg, setting up loopback devices or iscsi or bridging whatever) should
> be executed directly by xl.  This naturally gives the best error
> handling and makes logging easiest.
> 
> However, if we are serious about supporting driver domains, or indeed
> any kind of backend not running in the same domain as libxl, then
> something in the driver domain needs to be responsible for executing
> these scripts (or equivalent).
> 
> The obvious way to communicate this information to the driver domain
> is via xenstore.
> 
> What we should be discussing is exactly how the driver domain
> translates that into script execution.  Currently on Linux this is
> mostly done with udev, and AIUI on BSD using xenbackendd.

xenbackendd is (AFAIK) watching for events (e.g. creation and deletion
of backend state nodes) in xenstore so at least in concept it is
portable to all OSes, whereas udev is pretty Linux specific.

One nice effect of the udev approach is that you get an explicit event
e.g. when a block device is torn down. This makes it trivial to avoid
problems like racing to teardown a loopback device.

Since xenbackendd is mostly inferring things by watching the backend
nodes used by the kernel side drivers it struggles due to the model
where we rm the backend's xenstore directory on destroy which nukes
state required by xenbackendd. 
The xenbackendd model seems slightly preferable to me, it's simpler to
setup and more portable. Doing things differently on different OSes
doesn't help with the confusion here!

Perhaps the core functionality could be in libxl such that a toolstack
can choose to integrate the functionality or run it as a separate daemon
as necessary?

The main issue with the xenbackendd model is the manner in which it has
to guess what is going on and try to synchronise with what the drivers
are going. This does also effect the udev driven way of doing things too
since the scripts often want to look in xenstore for parameters and on
destroy they are often missing.

We currently get this wrong under Linux at the minute and the network
teardown hotplug script doesn't work right because the necessary bits
are gone from xenstore. We mostly get away with this for bridging since
the device is automatically remove from the bridge but for vswitch we do
need to actually do some reconfiguration at this point. We also appear
to leak iptables rules under some circumstances.

Perhaps we would be better off splitting the hotplug stuff into its own
place in xenstore and have the toolstack explicitly trigger events at
the right time by writing to it? This could be compatible with existing
scripts since they use an environment variable to find their xenstore
base path.

We need to be aware that the script models (or at least the sequencing
wrt the xenstore state transitions) vary slightly with the device type
since for block devices you typically need to run the hotplug script
before creating the backend whereas with network devices you create the
device first and then run the script to configure it. vice versa for
destroy.

We also need to consider Network tap devices. Not all tap devices are
part of Xen's world which is a wrinkle which needs thought.

I tried to write down my understanding of the events and sequencing
under Linux but that mostly showed the gaps in my understanding...

AIUI XCP does a lot more of this stuff via xenstored and have been
(successfully) playing with driver domains -- we should ask for their
input.

> So sorry for leading this discussion in what I now think may be a
> wrong direction.
> 
> Ian.

Re: [PATCH 12 of 14 v4] libxl: set frontend status to 6 on domain destroy

[Roger Pau Monné]

2011/12/16 Ian Campbell <Ian.Campbell@citrix.com>:
> On Thu, 2011-12-15 at 17:44 +0000, Ian Jackson wrote:
>> I have been thinking about this whole area.
>>
>> Originally my opinion was that the block and network setup scripts
>> (eg, setting up loopback devices or iscsi or bridging whatever) should
>> be executed directly by xl.  This naturally gives the best error
>> handling and makes logging easiest.
>>
>> However, if we are serious about supporting driver domains, or indeed
>> any kind of backend not running in the same domain as libxl, then
>> something in the driver domain needs to be responsible for executing
>> these scripts (or equivalent).
>>
>> The obvious way to communicate this information to the driver domain
>> is via xenstore.
>>
>> What we should be discussing is exactly how the driver domain
>> translates that into script execution.  Currently on Linux this is
>> mostly done with udev, and AIUI on BSD using xenbackendd.
>
> xenbackendd is (AFAIK) watching for events (e.g. creation and deletion
> of backend state nodes) in xenstore so at least in concept it is
> portable to all OSes, whereas udev is pretty Linux specific.
>
> One nice effect of the udev approach is that you get an explicit event
> e.g. when a block device is torn down. This makes it trivial to avoid
> problems like racing to teardown a loopback device.

I agree that executing scripts from xl is much more comfortable, and
it would be good to do it that way whenever possible. It allows easier
error detection and control of when hotplug scripts are executed.

> Since xenbackendd is mostly inferring things by watching the backend
> nodes used by the kernel side drivers it struggles due to the model
> where we rm the backend's xenstore directory on destroy which nukes
> state required by xenbackendd.
> The xenbackendd model seems slightly preferable to me, it's simpler to
> setup and more portable. Doing things differently on different OSes
> doesn't help with the confusion here!

xenbackend is a quite simple implementation, it only watches backend
changes and reacts upon them. It is easy to implement, but it is
difficult to debug, since hotplug execution is tied to xenstore
events, and right now there's no way to synchronize the toolstack with
xenbackend (xenbackend is only synchronized with the kernel).

> Perhaps the core functionality could be in libxl such that a toolstack
> can choose to integrate the functionality or run it as a separate daemon
> as necessary?

It would be interesting to implement all hotplug call functions into
libxl, and have a configuration option at xl.conf that specifies if
hotplug scripts should be executed from xl directly or delegate the
execution to xenbackend, that should be rewritten to use the same
libxl functions, to avoid having duplicate code.

> The main issue with the xenbackendd model is the manner in which it has
> to guess what is going on and try to synchronise with what the drivers
> are going. This does also effect the udev driven way of doing things too
> since the scripts often want to look in xenstore for parameters and on
> destroy they are often missing.
>
> We currently get this wrong under Linux at the minute and the network
> teardown hotplug script doesn't work right because the necessary bits
> are gone from xenstore. We mostly get away with this for bridging since
> the device is automatically remove from the bridge but for vswitch we do
> need to actually do some reconfiguration at this point. We also appear
> to leak iptables rules under some circumstances.
>
> Perhaps we would be better off splitting the hotplug stuff into its own
> place in xenstore and have the toolstack explicitly trigger events at
> the right time by writing to it? This could be compatible with existing
> scripts since they use an environment variable to find their xenstore
> base path.

Currently the only possible way to synchronize xenbackend and the
toolstack is to watch the hotplug-status xenbackend entry, I've posted
a patch a long time ago, that tried to synchronize xl and xenbackend
using the hotplug-status xenstore entry.

> We need to be aware that the script models (or at least the sequencing
> wrt the xenstore state transitions) vary slightly with the device type
> since for block devices you typically need to run the hotplug script
> before creating the backend whereas with network devices you create the
> device first and then run the script to configure it. vice versa for
> destroy.

I think that the easier way to deal with hotplug scripts is to execute
them when backend state is 2, that is true for both network and block
scripts (at least on NetBSD).

> We also need to consider Network tap devices. Not all tap devices are
> part of Xen's world which is a wrinkle which needs thought.
>
> I tried to write down my understanding of the events and sequencing
> under Linux but that mostly showed the gaps in my understanding...
>
> AIUI XCP does a lot more of this stuff via xenstored and have been
> (successfully) playing with driver domains -- we should ask for their
> input.
>

After all this, what seems most suitable to me is to have hotplug
script calling functions inside libxl, modify xenbackend to use libxl
API, and give the user the option to execute hotplug scripts directly
from xl (calling libxl hotplug functions) or delegate the work to
xenbackend (writing a value to xenstore that xenbackend can use to
synchronize and triggers execution).

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH 12 of 14 v4] libxl: set frontend status to 6 on domain destroy

[Ian Campbell]

On Wed, 2011-12-14 at 11:12 +0000, Roger Pau Monné wrote:
> 2011/12/14 Ian Campbell <Ian.Campbell@citrix.com>:
> > So if you rm the backend directory the NetBSD does not take that as a
> > sign to tear down the device? That sounds like a bug in the NetBSD
> > backend -- it should treat deletion of the backend state dir as if it
> > were reading state = "6" and shut everything down.
> 
> Yes, if I delete the frontend from xenstore, the devices are detached.
> Anyway, doing it one way or another (deletion or setting state to 6)
> doesn't really make a difference, you still have to wait for the
> backend to be disconnected (detached) before executing hotplug
> scripts.

Right. This might actually be tricky in the case where the backend dir
has been removed since I expect there will be nowhere else which
indicates this.

> > Or is the issue only in the userspace portions?
> >
> >>  I've added some libxl__wait_for_device_state logic here, to
> >> assure the backend state is set to 6 before trying to execute hotplug
> >> scripts.
> >
> > But that will always be true with this patch since you set it that way
> > just before, right?
> 
> I set frontend status to 6, and I suggest to wait for backend status
> to be 6 also, then execute hotplug scripts. It won't be true, because
> frontend and backend status are not tied (in the NetBSD case backend
> status is set by the Dom0 kernel or hotplug scripts, while frontend
> status is set by the DomU).

I think (but I'm not sure) that it is not permissible for the toolstack
to mess with the frontend state like that.

> > If you go down this path then I think you need to set the state to
> > "5" (Closing) in order to prompt the backend to transition to
> > "6" (Closed) itself. However you need to be careful about adding a
> > synchronous wait to the device destroy function. This should eventually
> > work even if the frontend and backend are not co-operating. That starts
> > to look a bit like calling libxl__device_remove instead.
> 
> Do you mean to set backend status to 5 instead of setting frontend to
> 6, and then wait for the kernel to do the transition from 5 to 6,
> instead of setting the frontend to 6?

Yes.

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH 12 of 14 v4] libxl: set frontend status to 6 on domain destroy

[Roger Pau Monné]

2011/12/14 Ian Campbell <Ian.Campbell@citrix.com>:
> On Wed, 2011-12-14 at 11:12 +0000, Roger Pau Monné wrote:
>> 2011/12/14 Ian Campbell <Ian.Campbell@citrix.com>:
>> > So if you rm the backend directory the NetBSD does not take that as a
>> > sign to tear down the device? That sounds like a bug in the NetBSD
>> > backend -- it should treat deletion of the backend state dir as if it
>> > were reading state = "6" and shut everything down.
>>
>> Yes, if I delete the frontend from xenstore, the devices are detached.
>> Anyway, doing it one way or another (deletion or setting state to 6)
>> doesn't really make a difference, you still have to wait for the
>> backend to be disconnected (detached) before executing hotplug
>> scripts.
>
> Right. This might actually be tricky in the case where the backend dir
> has been removed since I expect there will be nowhere else which
> indicates this.

No, there's no other easy way to know if the device has been detached.
If hotplug scripts are executed from xl, I think it is safe to assume
that nobody else is messing with xenstore entries. Backend folder
should not be removed untill hotplug script execution has happened.

>
>> > Or is the issue only in the userspace portions?
>> >
>> >>  I've added some libxl__wait_for_device_state logic here, to
>> >> assure the backend state is set to 6 before trying to execute hotplug
>> >> scripts.
>> >
>> > But that will always be true with this patch since you set it that way
>> > just before, right?
>>
>> I set frontend status to 6, and I suggest to wait for backend status
>> to be 6 also, then execute hotplug scripts. It won't be true, because
>> frontend and backend status are not tied (in the NetBSD case backend
>> status is set by the Dom0 kernel or hotplug scripts, while frontend
>> status is set by the DomU).
>
> I think (but I'm not sure) that it is not permissible for the toolstack
> to mess with the frontend state like that.

Well, xl already removes frontend xenstore entries, so I assumed
setting frontend status to 6 was not a problem. If it is, I could
always do the following sequence to force the detach:

1. Delete frontend xenstore dir.
2. Wait for backend xenstore to switch to disconnected state (6).
3. Execute hotplug script.
4. Remove backend.

>> > If you go down this path then I think you need to set the state to
>> > "5" (Closing) in order to prompt the backend to transition to
>> > "6" (Closed) itself. However you need to be careful about adding a
>> > synchronous wait to the device destroy function. This should eventually
>> > work even if the frontend and backend are not co-operating. That starts
>> > to look a bit like calling libxl__device_remove instead.
>>
>> Do you mean to set backend status to 5 instead of setting frontend to
>> 6, and then wait for the kernel to do the transition from 5 to 6,
>> instead of setting the frontend to 6?
>
> Yes.

No luck, setting backend state to 5 did not make the kernel detach the
device. It seems like the kernel is only watching frontend xenstore
entries.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH 12 of 14 v4] libxl: set frontend status to 6 on domain destroy

[Ian Campbell]

On Wed, 2011-12-14 at 12:18 +0000, Roger Pau Monné wrote:
> 
> No luck, setting backend state to 5 did not make the kernel detach the
> device. It seems like the kernel is only watching frontend xenstore
> entries.

Does hot removal of devices work? This uses the same mechanism.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH 12 of 14 v4] libxl: set frontend status to 6 on domain destroy

[Roger Pau Monné]

2011/12/14 Ian Campbell <Ian.Campbell@citrix.com>:
> On Wed, 2011-12-14 at 12:18 +0000, Roger Pau Monné wrote:
>>
>> No luck, setting backend state to 5 did not make the kernel detach the
>> device. It seems like the kernel is only watching frontend xenstore
>> entries.
>
> Does hot removal of devices work? This uses the same mechanism.

Attaching of device doesn't work very well because of kernel problems,
and detaching sometimes makes the kernel crash.

I've tried the following procedure for domain destruction, and it works ok:

1. remove frontend.
2. wait for backend to switch to disconnected state.
3. execute hotplug scripts.
4. remove backend.

Is this acceptable?

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH 12 of 14 v4] libxl: set frontend status to 6 on domain destroy

[Ian Jackson]

Roger Pau Monné writes ("Re: [Xen-devel] [PATCH 12 of 14 v4] libxl: set frontend status to 6 on domain destroy"):
> 2011/12/13 Ian Jackson <Ian.Jackson@eu.citrix.com>:
> > So here, the kernel or backendd start racing, and you hope that they
> > win the race and close the device before ...
> 
> From my experience in NetBSD, the kernel only closes the device when
> it's frontend state is set to 6, since we destroy the domain, it is
> unable to set the status to 6, and thus the kernel doesn't detach the
> devices.

This is no good, because you need to be able to force unplug a device
without guest cooperation, and the frontend state is controlled by the
guest.  So I think there is a NetBSD kernel bug in this area.

It needs to be possible to tell the backend to shut down without
regard to the frontend state.

> I've added some libxl__wait_for_device_state logic here, to
> assure the backend state is set to 6 before trying to execute hotplug
> scripts. The truth is that I had it in previous versions of my patch,
> but it seems the kernel always switches contexts and detaches the
> devices before executing hotplug scripts (it might just be luck).

Well that's very nice but of course not reliable.

> Also this patch speeds domain destruction a lot (which is also quite
> slow under Linux from what I saw).

You're saying that not waiting for the backends to close speeds up
domain destruction ?  Where is the time going ?

Ian.

[PATCH 13 of 14 v4] libxl: remove force parameter from libxl_domain_destroy

[Roger Pau Monne]

# HG changeset patch
# User Roger Pau Monne <roger.pau@entel.upc.edu>
# Date 1323766195 -3600
# Node ID ea1a9fe2ef02f0737fa6f0c884a5ebaeb2b08872
# Parent  35c322e6020501447936e56cafceb8bbc1b2e980
libxl: remove force parameter from libxl_domain_destroy

Since a destroy is considered a forced shutdown, there's no point in
passing a force parameter. All the occurences of this function have
been replaced with the proper syntax.

Signed-off-by: Roger Pau Monne <roger.pau@entel.upc.edu>

diff -r 35c322e60205 -r ea1a9fe2ef02 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Tue Dec 13 09:49:55 2011 +0100
+++ b/tools/libxl/libxl.c	Tue Dec 13 09:49:55 2011 +0100
@@ -719,7 +719,7 @@ int libxl_event_get_disk_eject_info(libx
     return 1;
 }
 
-int libxl_domain_destroy(libxl_ctx *ctx, uint32_t domid, int force)
+int libxl_domain_destroy(libxl_ctx *ctx, uint32_t domid)
 {
     libxl__gc gc = LIBXL_INIT_GC(ctx);
     libxl_dominfo dominfo;
@@ -768,7 +768,7 @@ int libxl_domain_destroy(libxl_ctx *ctx,
 
         libxl__qmp_cleanup(&gc, domid);
     }
-    if (libxl__devices_destroy(&gc, domid, force) < 0)
+    if (libxl__devices_destroy(&gc, domid, 1) < 0)
         LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "libxl__devices_destroy failed for %d", domid);
 
     vm_path = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/vm", dom_path));
diff -r 35c322e60205 -r ea1a9fe2ef02 tools/libxl/libxl.h
--- a/tools/libxl/libxl.h	Tue Dec 13 09:49:55 2011 +0100
+++ b/tools/libxl/libxl.h	Tue Dec 13 09:49:55 2011 +0100
@@ -269,7 +269,7 @@ int libxl_domain_suspend(libxl_ctx *ctx,
                           uint32_t domid, int fd);
 int libxl_domain_resume(libxl_ctx *ctx, uint32_t domid);
 int libxl_domain_shutdown(libxl_ctx *ctx, uint32_t domid, int req);
-int libxl_domain_destroy(libxl_ctx *ctx, uint32_t domid, int force);
+int libxl_domain_destroy(libxl_ctx *ctx, uint32_t domid);
 int libxl_domain_preserve(libxl_ctx *ctx, uint32_t domid, libxl_domain_create_info *info, const char *name_suffix, libxl_uuid new_uuid);
 
 /* get max. number of cpus supported by hypervisor */
diff -r 35c322e60205 -r ea1a9fe2ef02 tools/libxl/libxl_create.c
--- a/tools/libxl/libxl_create.c	Tue Dec 13 09:49:55 2011 +0100
+++ b/tools/libxl/libxl_create.c	Tue Dec 13 09:49:55 2011 +0100
@@ -664,7 +664,7 @@ static int do_domain_create(libxl__gc *g
 
 error_out:
     if (domid)
-        libxl_domain_destroy(ctx, domid, 0);
+        libxl_domain_destroy(ctx, domid);
 
     return ret;
 }
diff -r 35c322e60205 -r ea1a9fe2ef02 tools/libxl/libxl_dm.c
--- a/tools/libxl/libxl_dm.c	Tue Dec 13 09:49:55 2011 +0100
+++ b/tools/libxl/libxl_dm.c	Tue Dec 13 09:49:55 2011 +0100
@@ -919,7 +919,7 @@ int libxl__destroy_device_model(libxl__g
             goto out;
         }
         LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Device model is a stubdom, domid=%d", stubdomid);
-        ret = libxl_domain_destroy(ctx, stubdomid, 0);
+        ret = libxl_domain_destroy(ctx, stubdomid);
         if (ret)
             goto out;
     } else {
diff -r 35c322e60205 -r ea1a9fe2ef02 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Tue Dec 13 09:49:55 2011 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Tue Dec 13 09:49:55 2011 +0100
@@ -1283,7 +1283,7 @@ static int handle_domain_death(libxl_ctx
         /* 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, 0);
+        libxl_domain_destroy(ctx, domid);
         break;
 
     case LIBXL_ACTION_ON_SHUTDOWN_COREDUMP_DESTROY:
@@ -1786,7 +1786,7 @@ start:
 error_out:
     release_lock();
     if (libxl_domid_valid_guest(domid))
-        libxl_domain_destroy(ctx, domid, 0);
+        libxl_domain_destroy(ctx, domid);
 
 out:
     if (logfile != 2)
@@ -2256,7 +2256,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, 0);
+    rc = libxl_domain_destroy(ctx, domid);
     if (rc) { fprintf(stderr,"destroy failed (rc=%d)\n",rc); exit(-1); }
 }
 
@@ -2502,7 +2502,7 @@ static int save_domain(const char *p, co
     if (checkpoint)
         libxl_domain_unpause(ctx, domid);
     else
-        libxl_domain_destroy(ctx, domid, 0);
+        libxl_domain_destroy(ctx, domid);
 
     exit(0);
 }
@@ -2735,7 +2735,7 @@ static void migrate_domain(const char *d
     }
 
     fprintf(stderr, "migration sender: Target reports successful startup.\n");
-    libxl_domain_destroy(ctx, domid, 1); /* bang! */
+    libxl_domain_destroy(ctx, domid); /* bang! */
     fprintf(stderr, "Migration successful.\n");
     exit(0);
 
@@ -2852,7 +2852,7 @@ static void migrate_receive(int debug, i
     if (rc) {
         fprintf(stderr, "migration target: Failure, destroying our copy.\n");
 
-        rc2 = libxl_domain_destroy(ctx, domid, 1);
+        rc2 = libxl_domain_destroy(ctx, domid);
         if (rc2) {
             fprintf(stderr, "migration target: Failed to destroy our copy"
                     " (code %d).\n", rc2);
diff -r 35c322e60205 -r ea1a9fe2ef02 tools/python/xen/lowlevel/xl/xl.c
--- a/tools/python/xen/lowlevel/xl/xl.c	Tue Dec 13 09:49:55 2011 +0100
+++ b/tools/python/xen/lowlevel/xl/xl.c	Tue Dec 13 09:49:55 2011 +0100
@@ -437,10 +437,10 @@ static PyObject *pyxl_domain_shutdown(Xl
 
 static PyObject *pyxl_domain_destroy(XlObject *self, PyObject *args)
 {
-    int domid, force = 1;
-    if ( !PyArg_ParseTuple(args, "i|i", &domid, &force) )
+    int domid;
+    if ( !PyArg_ParseTuple(args, "i", &domid) )
         return NULL;
-    if ( libxl_domain_destroy(self->ctx, domid, force) ) {
+    if ( libxl_domain_destroy(self->ctx, domid) ) {
         PyErr_SetString(xl_error_obj, "cannot destroy domain");
         return NULL;
     }

Re: [PATCH 13 of 14 v4] libxl: remove force parameter from libxl_domain_destroy

[Ian Jackson]

Roger Pau Monne writes ("[Xen-devel] [PATCH 13 of 14 v4] libxl: remove force parameter from libxl_domain_destroy"):
> libxl: remove force parameter from libxl_domain_destroy
> 
> Since a destroy is considered a forced shutdown, there's no point in
> passing a force parameter. All the occurences of this function have
> been replaced with the proper syntax.
> 
> Signed-off-by: Roger Pau Monne <roger.pau@entel.upc.edu>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

[PATCH 14 of 14 v4] libxl: remove force parameter from libxl__devices_destroy

[Roger Pau Monne]

# HG changeset patch
# User Roger Pau Monne <roger.pau@entel.upc.edu>
# Date 1323766195 -3600
# Node ID 8a84f53376862427f254a017cb52c928dbdd3d32
# Parent  ea1a9fe2ef02f0737fa6f0c884a5ebaeb2b08872
libxl: remove force parameter from libxl__devices_destroy

Remove the force flag, and always use forced destruction.

Signed-off-by: Roger Pau Monne <roger.pau@entel.upc.edu>

diff -r ea1a9fe2ef02 -r 8a84f5337686 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Tue Dec 13 09:49:55 2011 +0100
+++ b/tools/libxl/libxl.c	Tue Dec 13 09:49:55 2011 +0100
@@ -768,7 +768,7 @@ int libxl_domain_destroy(libxl_ctx *ctx,
 
         libxl__qmp_cleanup(&gc, domid);
     }
-    if (libxl__devices_destroy(&gc, domid, 1) < 0)
+    if (libxl__devices_destroy(&gc, domid) < 0)
         LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "libxl__devices_destroy failed for %d", domid);
 
     vm_path = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/vm", dom_path));
diff -r ea1a9fe2ef02 -r 8a84f5337686 tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c	Tue Dec 13 09:49:55 2011 +0100
+++ b/tools/libxl/libxl_device.c	Tue Dec 13 09:49:55 2011 +0100
@@ -524,13 +524,13 @@ int libxl__device_destroy(libxl__gc *gc,
     return 0;
 }
 
-int libxl__devices_destroy(libxl__gc *gc, uint32_t domid, int force)
+int libxl__devices_destroy(libxl__gc *gc, uint32_t domid)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     char *path;
     unsigned int num_kinds, num_devs;
     char **kinds = NULL, **devs = NULL;
-    int i, j, n_watches = 0;
+    int i, j;
     libxl__device dev;
     libxl__device_kind kind;
 
@@ -561,16 +561,7 @@ int libxl__devices_destroy(libxl__gc *gc
                 dev.kind = kind;
                 dev.devid = atoi(devs[j]);
 
-                if (force) {
-                    libxl__device_destroy(gc, &dev);
-                } else {
-                    int rc = libxl__device_remove(gc, &dev, 0);
-                    if (rc < 0)
-                        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
-                                   "cannot remove device %s\n", path);
-                    else
-                        n_watches += rc;
-                }
+                libxl__device_destroy(gc, &dev);
             }
         }
     }
@@ -584,37 +575,9 @@ int libxl__devices_destroy(libxl__gc *gc
         dev.kind = LIBXL__DEVICE_KIND_CONSOLE;
         dev.devid = 0;
 
-        if (force) {
-            libxl__device_destroy(gc, &dev);
-        } else {
-            int rc = libxl__device_remove(gc, &dev, 0);
-            if (rc < 0)
-                LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
-                           "cannot remove device %s\n", path);
-            else
-                n_watches += rc;
-        }
+        libxl__device_destroy(gc, &dev);
     }
 
-    if (!force) {
-        /* Linux-ism. Most implementations leave the timeout
-         * untouched after select. Linux, however, will chip
-         * away the elapsed time from it, which is what we
-         * need to enforce a single time span waiting for
-         * device destruction. */
-        struct timeval tv;
-        tv.tv_sec = LIBXL_DESTROY_TIMEOUT;
-        tv.tv_usec = 0;
-        while (n_watches > 0) {
-            if (libxl__wait_for_device_state(gc, &tv, XenbusStateClosed,
-                                             destroy_device) < 0) {
-                /* function returned ERROR_* */
-                break;
-            } else {
-                n_watches--;
-            }
-        }
-    }
 out:
     return 0;
 }
diff -r ea1a9fe2ef02 -r 8a84f5337686 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h	Tue Dec 13 09:49:55 2011 +0100
+++ b/tools/libxl/libxl_internal.h	Tue Dec 13 09:49:55 2011 +0100
@@ -252,7 +252,7 @@ _hidden int libxl__parse_backend_path(li
                                       libxl__device *dev);
 _hidden int libxl__device_remove(libxl__gc *gc, libxl__device *dev, int wait);
 _hidden int libxl__device_destroy(libxl__gc *gc, libxl__device *dev);
-_hidden int libxl__devices_destroy(libxl__gc *gc, uint32_t domid, int force);
+_hidden int libxl__devices_destroy(libxl__gc *gc, uint32_t domid);
 _hidden int libxl__wait_for_backend(libxl__gc *gc, char *be_path, char *state);
 
 /* Handler for the libxl__wait_for_device_state callback */

Re: [PATCH 14 of 14 v4] libxl: remove force parameter from libxl__devices_destroy

[Ian Jackson]

Roger Pau Monne writes ("[Xen-devel] [PATCH 14 of 14 v4] libxl: remove force parameter from libxl__devices_destroy"):
> libxl: remove force parameter from libxl__devices_destroy
> 
> Remove the force flag, and always use forced destruction.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>