Skip to contentSkip to conversation

libxl: add support for hotplug script calling from libxl

PATCH Superseded Roger Pau Monne 3 participants 44 messages Dec 13, 2011 – Dec 19, 2011 Arm/actions semi architecture

v4 of 5 Superseded Latest is v5 →

lore
Share & tools
lore
#0 lore raw

PATCH v4 1/1 libxl: add support for hotplug script calling from libxl

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.

#1 lore raw

PATCH v4 1/1 xenbackendd: pass type of block device to hotplug script

# HG changeset patch
# User Roger Pau Monne <[email protected]> # 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.

tools/hotplug/NetBSD/block +1 -1
diff -r 1c58bb664d8d -r a0f9c898470c tools/hotplug/NetBSD/block
@@ -19,7 +19,7 @@ error() {
 
 xpath=$1
 xstatus=$2
-xtype=$(xenstore-read "$xpath/type")
+xtype=$3
 xparams=$(xenstore-read "$xpath/params")
 
 case $xstatus in
tools/xenbackendd/xenbackendd.c +35 -5
diff -r 1c58bb664d8d -r a0f9c898470c tools/xenbackendd/xenbackendd.c
@@ -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:
#3 lore raw

PATCH v4 1/1 libxl: add support for image files for NetBSD

In reply to Roger Pau Monne

# HG changeset patch
# User Roger Pau Monne <[email protected]> # 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.

tools/libxl/Makefile +9 -0
diff -r a0f9c898470c -r e3907ed91220 tools/libxl/Makefile
@@ -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 \
tools/libxl/libxl_device.c +7 -8
diff -r a0f9c898470c -r e3907ed91220 tools/libxl/libxl_device.c
@@ -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)) {
tools/libxl/libxl_internal.h +9 -0
diff -r a0f9c898470c -r e3907ed91220 tools/libxl/libxl_internal.h
@@ -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);
tools/libxl/libxl_linux.c new file +27 -0
diff -r a0f9c898470c -r e3907ed91220 tools/libxl/libxl_linux.c
@@ -0,0 +1,27 @@
+/*
+ * Copyright (C) 2011
+ * Author Roger Pau Monne <[email protected]>
+ *
+ * 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;
+}
tools/libxl/libxl_netbsd.c new file +26 -0
diff -r a0f9c898470c -r e3907ed91220 tools/libxl/libxl_netbsd.c
@@ -0,0 +1,26 @@
+/*
+ * Copyright (C) 2011
+ * Author Roger Pau Monne <[email protected]>
+ *
+ * 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;
+}
#5 lore raw

PATCH v4 1/1 libxl: add libxl__forkexec function to libxl_exec

In reply to Roger Pau Monne

# HG changeset patch
# User Roger Pau Monne <[email protected]> # 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.

tools/libxl/libxl_exec.c +33 -0
diff -r e3907ed91220 -r 573a246bf0c6 tools/libxl/libxl_exec.c
@@ -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
tools/libxl/libxl_internal.h +16 -0
diff -r e3907ed91220 -r 573a246bf0c6 tools/libxl/libxl_internal.h
@@ -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,
#9 lore raw

PATCH v4 1/1 libxl: introduce libxl__wait_for_device_state

In reply to Roger Pau Monne

# HG changeset patch
# User Roger Pau Monne <[email protected]> # 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.

tools/libxl/libxl_device.c +30 -12
diff -r 573a246bf0c6 -r d5f1ab565bf6 tools/libxl/libxl_device.c
@@ -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 {
tools/libxl/libxl_internal.h +28 -0
diff -r 573a246bf0c6 -r d5f1ab565bf6 tools/libxl/libxl_internal.h
@@ -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
#11 lore raw

PATCH v4 1/1 libxl: wait for devices to initialize upon addition to the domain

In reply to Roger Pau Monne

# HG changeset patch
# User Roger Pau Monne <[email protected]>
# 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.

tools/libxl/libxl.c +39 -1
diff -r d5f1ab565bf6 -r 148765a54f6e tools/libxl/libxl.c
@@ -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);
#12 lore raw

PATCH v4 1/1 libxl: execute hotplug scripts directly from libxl

In reply to Roger Pau Monne

# HG changeset patch
# User Roger Pau Monne <[email protected]> # 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.

tools/libxl/libxl.c +23 -0
diff -r 148765a54f6e -r a3a95ea16ca4 tools/libxl/libxl.c
@@ -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:
tools/libxl/libxl_device.c +26 -0
diff -r 148765a54f6e -r a3a95ea16ca4 tools/libxl/libxl_device.c
@@ -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);
 
tools/libxl/libxl_internal.h +40 -0
diff -r 148765a54f6e -r a3a95ea16ca4 tools/libxl/libxl_internal.h
@@ -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);
tools/libxl/libxl_linux.c +14 -0
diff -r 148765a54f6e -r a3a95ea16ca4 tools/libxl/libxl_linux.c
@@ -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;
+}
tools/libxl/libxl_netbsd.c +128 -0

137 lines not on this page · raw message

#14 lore raw

PATCH v4 1/1 hotplug NetBSD: pass an action instead of a state to hotplug scripts

In reply to Roger Pau Monne

# HG changeset patch
# User Roger Pau Monne <[email protected]>
# 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.

tools/hotplug/NetBSD/block +4 -4
diff -r a3a95ea16ca4 -r 51404e6e2d6f tools/hotplug/NetBSD/block
@@ -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
tools/hotplug/NetBSD/vif-bridge +4 -4
diff -r a3a95ea16ca4 -r 51404e6e2d6f tools/hotplug/NetBSD/vif-bridge
@@ -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")
tools/hotplug/NetBSD/vif-ip +4 -4
diff -r a3a95ea16ca4 -r 51404e6e2d6f tools/hotplug/NetBSD/vif-ip
@@ -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")
#17 lore raw

PATCH v4 1/1 xenbackendd: pass action to hotplug script

In reply to Roger Pau Monne

# HG changeset patch
# User Roger Pau Monne <[email protected]> # 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.

tools/xenbackendd/xenbackendd.c +8 -2
diff -r 51404e6e2d6f -r 30f2db9a780f tools/xenbackendd/xenbackendd.c
@@ -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;
#18 lore raw

PATCH v4 1/1 hotplug: remove debug messages from NetBSD hotplug scripts

In reply to Roger Pau Monne

# HG changeset patch
# User Roger Pau Monne <[email protected]> # 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.

tools/hotplug/NetBSD/block +0 -4
diff -r 30f2db9a780f -r 263959b9bc33 tools/hotplug/NetBSD/block
@@ -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
 	;;
tools/hotplug/NetBSD/vif-bridge +0 -3
diff -r 30f2db9a780f -r 263959b9bc33 tools/hotplug/NetBSD/vif-bridge
@@ -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
 	;;
 *)
tools/hotplug/NetBSD/vif-ip +0 -2
diff -r 30f2db9a780f -r 263959b9bc33 tools/hotplug/NetBSD/vif-ip
@@ -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
 	;;
 *)
#20 lore raw

PATCH v4 1/1 rc.d NetBSD: don't start xenbackendd by default, only when xend needs it

In reply to Roger Pau Monne

# HG changeset patch
# User Roger Pau Monne <[email protected]>
# 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.

tools/hotplug/NetBSD/rc.d/xenbackendd new file +27 -0
diff -r 263959b9bc33 -r d599451d870d tools/hotplug/NetBSD/rc.d/xenbackendd
@@ -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"
+
tools/hotplug/NetBSD/rc.d/xencommons +4 -25
diff -r 263959b9bc33 -r d599451d870d tools/hotplug/NetBSD/rc.d/xencommons
@@ -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
tools/hotplug/NetBSD/rc.d/xend +1 -1
diff -r 263959b9bc33 -r d599451d870d tools/hotplug/NetBSD/rc.d/xend
@@ -1,7 +1,7 @@
 #!/bin/sh
 #
 # PROVIDE: xend
-# REQUIRE: xencommons
+# REQUIRE: xencommons xenbackendd
 
 . /etc/rc.subr
#22 lore raw

PATCH v4 1/1 libxl: fix incorrect log message in libxl_domain_destroy

In reply to Roger Pau Monne

# HG changeset patch
# User Roger Pau Monne <[email protected]> # 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.

tools/libxl/libxl.c +1 -1
diff -r d599451d870d -r a36942bf253c tools/libxl/libxl.c
@@ -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)
#24 lore raw

PATCH v4 1/1 libxl: set frontend status to 6 on domain destroy

In reply to Roger Pau Monne

# HG changeset patch
# User Roger Pau Monne <[email protected]> # 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.

tools/libxl/libxl_device.c +1 -4
diff -r a36942bf253c -r 35c322e60205 tools/libxl/libxl_device.c
@@ -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);
#40 lore raw

PATCH v4 1/1 libxl: remove force parameter from libxl_domain_destroy

In reply to Roger Pau Monne

# HG changeset patch
# User Roger Pau Monne <[email protected]> # 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.

tools/libxl/libxl.c +2 -2
diff -r 35c322e60205 -r ea1a9fe2ef02 tools/libxl/libxl.c
@@ -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));
tools/libxl/libxl.h +1 -1
diff -r 35c322e60205 -r ea1a9fe2ef02 tools/libxl/libxl.h
@@ -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 */
tools/libxl/libxl_create.c +1 -1
diff -r 35c322e60205 -r ea1a9fe2ef02 tools/libxl/libxl_create.c
@@ -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;
 }
tools/libxl/libxl_dm.c +1 -1
diff -r 35c322e60205 -r ea1a9fe2ef02 tools/libxl/libxl_dm.c
@@ -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 {
tools/libxl/xl_cmdimpl.c +6 -6
diff -r 35c322e60205 -r ea1a9fe2ef02 tools/libxl/xl_cmdimpl.c
@@ -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);
tools/python/xen/lowlevel/xl/xl.c +3 -3
diff -r 35c322e60205 -r ea1a9fe2ef02 tools/python/xen/lowlevel/xl/xl.c
@@ -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;
     }
#42 lore raw

PATCH v4 1/1 libxl: remove force parameter from libxl__devices_destroy

In reply to Roger Pau Monne

# HG changeset patch
# User Roger Pau Monne <[email protected]> # 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.

tools/libxl/libxl.c +1 -1
diff -r ea1a9fe2ef02 -r 8a84f5337686 tools/libxl/libxl.c
@@ -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));
tools/libxl/libxl_device.c +4 -41

70 lines not on this page · raw message

tools/libxl/libxl_internal.h +1 -1
diff -r ea1a9fe2ef02 -r 8a84f5337686 tools/libxl/libxl_internal.h
@@ -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 */
Acked-by on v4 1/1
#10 lore raw

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

In reply to Roger Pau Monne

Roger Pau Monne writes ("[Xen-devel] [PATCH 04 of 14 v4] libxl: introduce libxl__wait_for_device_state"):

Quoted 7 lines

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.

Thanks,
Ian.

Acked-by on v4 1/1
#19 lore raw

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

In reply to Roger Pau Monne

Roger Pau Monne writes ("[Xen-devel] [PATCH 09 of 14 v4] hotplug: remove debug messages from NetBSD hotplug scripts"):

Quoted 4 lines

hotplug: remove debug messages from NetBSD hotplug scripts

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

#2 lore raw

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

In reply to Roger Pau Monne

Roger Pau Monne writes ("[Xen-devel] [PATCH 01 of 14 v4] xenbackendd: pass type of block device to hotplug script"):

Quoted 5 lines

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.

#25 lore raw

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

In reply to Roger Pau Monne

Roger Pau Monne writes ("[Xen-devel] [PATCH 12 of 14 v4] libxl: set frontend status to 6 on domain destroy"):

Quoted 4 lines

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.

Quoted 1 line

+ 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 ...

Quoted 1 line
     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.

Acked-by on v4 1/1
#41 lore raw

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

In reply to Roger Pau Monne

Roger Pau Monne writes ("[Xen-devel] [PATCH 13 of 14 v4] libxl: remove force parameter from libxl_domain_destroy"):

Quoted 7 lines

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.

#6 lore raw

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

In reply to Roger Pau Monne

Roger Pau Monne writes ("[Xen-devel] [PATCH 03 of 14 v4] libxl: add libxl__forkexec function to libxl_exec"):

Quoted 3 lines
tools/libxl/libxl_exec.c +3 -0
+int libxl__forkexec(libxl__gc *gc, int stdinfd, int stdoutfd,
+                    int stderrfd, char **args)
+{

...

Quoted 1 line

+ libxl__exec(stdinfd, stdoutfd, stderrfd, args[0], args);

...

tools/libxl/libxl_exec.c int libxl__spawn_check(libxl__gc *gc, li · line 479

Quoted 2 lines
tools/libxl/libxl_exec.c +2 -0
+            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.

Quoted 2 lines

+/*
+ * libxl__forkexec - Executes a file synchronously

OK, but:

Quoted 8 lines

+ * 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.)

Quoted 1 line

+ * 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.

#13 lore raw

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

In reply to Roger Pau Monne

Roger Pau Monne writes ("[Xen-devel] [PATCH 06 of 14 v4] libxl: execute hotplug scripts directly from libxl"):

Quoted 7 lines

libxl: execute hotplug scripts directly from libxl.

Added the necessary handlers to execute hotplug scripts when necessary

Quoted 1 line

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.

#21 lore raw

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

In reply to Roger Pau Monne

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"):

Quoted 5 lines

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.

#15 lore raw

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

In reply to Roger Pau Monne

Roger Pau Monne writes ("[Xen-devel] [PATCH 07 of 14 v4] hotplug NetBSD: pass an action instead of a state to hotplug scripts"):

Quoted 4 lines

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.

Acked-by on v4 1/1
#23 lore raw

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

In reply to Roger Pau Monne

Roger Pau Monne writes ("[Xen-devel] [PATCH 11 of 14 v4] libxl: fix incorrect lo> libxl: fix incorrect log message in libxl_domain_destroy

Quoted 5 lines

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

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

Ian.

Acked-by on v4 1/1
#43 lore raw

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

In reply to Roger Pau Monne

Roger Pau Monne writes ("[Xen-devel] [PATCH 14 of 14 v4] libxl: remove force parameter from libxl__devices_destroy"):

Quoted 3 lines

libxl: remove force parameter from libxl__devices_destroy

Remove the force flag, and always use forced destruction.

Acked-by on v4 1/1
#4 lore raw

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

In reply to Roger Pau Monne

Roger Pau Monne writes ("[Xen-devel] [PATCH 02 of 14 v4] libxl: add support for image files for NetBSD"):

Quoted 7 lines

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.

#16 lore raw

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

In reply to Roger Pau Monne

Roger Pau Monne writes ("[Xen-devel] [PATCH 07 of 14 v4] hotplug NetBSD: pass an action instead of a state to hotplug scripts"):

Quoted 4 lines

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"):

Quoted 5 lines

xenbackendd: pass action to hotplug script

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

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.

#7 lore raw

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

In reply to Ian Jackson
Quoted 11 lines · Ian Jackson
Quoted 4 lines

+ * gc: allocation pool
+ * stdinfd, stdoutfd, stderrfd: fds to pass to libxl__exec
+ * args: file to execute and arguments to pass in the following

format

Quoted 5 lines
+ *      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
Quoted 3 lines

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.

#8 lore raw

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

Ian Campbell writes ("Re: [Xen-devel] [PATCH 03 of 14 v4] libxl: add libxl__forkexec function to libxl_exec"):

Quoted 9 lines · Ian Jackson
Quoted 3 lines

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.

#26 lore raw

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

In reply to Ian Jackson

2011/12/13 Ian Jackson <[email protected]>:

Quoted 6 lines

Roger Pau Monne writes ("[Xen-devel] [PATCH 12 of 14 v4] libxl: set frontend status to 6 on domain destroy"):

Quoted 4 lines

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.

Quoted 6 lines

There seems to be a race here.

Quoted 1 line

+    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).

Quoted 8 lines
Quoted 1 line

     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
[email protected]
http://lists.xensource.com/xen-devel

#27 lore raw

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

Quoted 11 lines · Roger Pau Monné

2011/12/13 Ian Jackson <[email protected]>:

Quoted 10 lines

Roger Pau Monne writes ("[Xen-devel] [PATCH 12 of 14 v4] libxl: set frontend status to 6 on domain destroy"):

Quoted 4 lines

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.

Quoted 1 line

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

Quoted 7 lines
Quoted 2 lines

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?

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 11 lines

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

Quoted 7 lines
Quoted 1 line
     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 ?

Quoted 6 lines
Quoted 1 line

Ian.

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#28 lore raw

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

2011/12/14 Ian Campbell <[email protected]>:

Quoted 4 lines

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.

Quoted 8 lines

Or is the issue only in the userspace portions?

Quoted 3 lines

 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).

Quoted 6 lines

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?

Quoted 8 lines
Quoted 3 lines

 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
[email protected]
http://lists.xensource.com/xen-devel

#39 lore raw

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

In reply to Roger Pau Monné

Roger Pau Monné writes ("Re: [Xen-devel] [PATCH 12 of 14 v4] libxl: set frontend status to 6 on domain destroy"):

Quoted 8 lines

2011/12/13 Ian Jackson <[email protected]>:

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 2 lines

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.

#29 lore raw

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

In reply to Roger Pau Monné

Roger Pau Monné writes ("Re: [Xen-devel] [PATCH 12 of 14 v4] libxl: set frontend status to 6 on domain destroy"):

Quoted 11 lines

2011/12/14 Ian Campbell <[email protected]>:

Quoted 4 lines

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.

#35 lore raw

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

In reply to Roger Pau Monné
Quoted 11 lines · Roger Pau Monné

2011/12/14 Ian Campbell <[email protected]>:

Quoted 4 lines

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.

Quoted 6 lines
Quoted 6 lines

Or is the issue only in the userspace portions?

Quoted 3 lines

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.

Quoted 8 lines
Quoted 2 lines

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.

Quoted 10 lines
Quoted 6 lines

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
[email protected]
http://lists.xensource.com/xen-devel

#30 lore raw

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

In reply to Ian Jackson

2011/12/14 Ian Jackson <[email protected]>:

Quoted 9 lines
Quoted 5 lines

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
[email protected]
http://lists.xensource.com/xen-devel

#36 lore raw

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

In reply to Ian Campbell

2011/12/14 Ian Campbell <[email protected]>:

Quoted 13 lines · Roger Pau Monné
Quoted 11 lines

2011/12/14 Ian Campbell <[email protected]>:

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 10 lines
Quoted 10 lines
Quoted 9 lines

Or is the issue only in the userspace portions?

Quoted 3 lines

 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?

Quoted 8 lines
Quoted 5 lines

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.

Quoted 7 lines
Quoted 7 lines
Quoted 6 lines

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.

Quoted 5 lines
Quoted 3 lines

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
[email protected]
http://lists.xensource.com/xen-devel

#31 lore raw

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

In reply to Roger Pau Monné
Quoted 11 lines · Roger Pau Monné

2011/12/14 Ian Jackson <[email protected]>:

Quoted 9 lines
Quoted 5 lines

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.

Quoted 6 lines

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
[email protected]
http://lists.xensource.com/xen-devel

#37 lore raw

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

In reply to Roger Pau Monné
Quoted 4 lines · Roger Pau Monné

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
[email protected]
http://lists.xensource.com/xen-devel

#38 lore raw

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

2011/12/14 Ian Campbell <[email protected]>:

Quoted 7 lines · Roger Pau Monné
Quoted 4 lines

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
[email protected]
http://lists.xensource.com/xen-devel

#32 lore raw

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

In reply to Ian Campbell

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.

#33 lore raw

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

Quoted 12 lines · 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).

Quoted 6 lines

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.

Quoted 4 lines

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

Ian.

#34 lore raw

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

2011/12/16 Ian Campbell <[email protected]>:

Quoted 20 lines · Ian Jackson
Quoted 12 lines

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).

Quoted 6 lines

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.

Quoted 7 lines

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.

Quoted 7 lines

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).

Quoted 3 lines

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.

Quoted 13 lines

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.

Quoted 5 lines

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.

Quoted 6 lines

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).

Quoted 10 lines

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
[email protected]
http://lists.xensource.com/xen-devel