[PATCH 0 of 2] Add NetBSD support for libxl PV DomU

[PATCH 0 of 2] Add NetBSD support for libxl PV DomU

[Roger Pau Monne]

This series of patches adds NetBSD support to libxl, using hotplug-status to synchronize the unplugging of vbd devices.

[PATCH 1 of 2] hotplug: add hotplug-status disconnected

[Roger Pau Monne]

# HG changeset patch
# User Roger Pau Monne <roger.pau@entel.upc.edu>
# Date 1316186985 -7200
# Node ID 00949e363f6f2c70001da548403475628df93b97
# Parent  63e254468d6e8d81baeafaed68f05791dc21eb4e
hotplug: add hotplug-status disconnected

Added hotplug-status disconnected when vbd are removed.

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

diff -r 63e254468d6e -r 00949e363f6f tools/hotplug/Linux/block
--- a/tools/hotplug/Linux/block	Wed Sep 14 14:18:40 2011 +0200
+++ b/tools/hotplug/Linux/block	Fri Sep 16 17:29:45 2011 +0200
@@ -321,6 +321,7 @@ mount it read-write in a guest domain."
   remove)
     case $t in 
       phy)
+	xenstore_write "$XENBUS_PATH/hotplug-status" "disconnected"
 	exit 0
 	;;
 
@@ -329,6 +330,7 @@ mount it read-write in a guest domain."
         node=$(xenstore_read "$XENBUS_PATH/node")
 	losetup -d "$node"
         release_lock "block"
+	xenstore_write "$XENBUS_PATH/hotplug-status" "disconnected"
 	exit 0
 	;;
 
diff -r 63e254468d6e -r 00949e363f6f tools/hotplug/NetBSD/block
--- a/tools/hotplug/NetBSD/block	Wed Sep 14 14:18:40 2011 +0200
+++ b/tools/hotplug/NetBSD/block	Fri Sep 16 17:29:45 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
@@ -38,6 +38,8 @@ 6)
 		echo "unknown type $xtype" >&2
 		;;
 	esac
+	echo xenstore-write $xpath/hotplug-status disconnected
+	xenstore-write $xpath/hotplug-status disconnected
 	xenstore-rm $xpath
 	exit 0
 	;;
diff -r 63e254468d6e -r 00949e363f6f tools/xenbackendd/xenbackendd.c
--- a/tools/xenbackendd/xenbackendd.c	Wed Sep 14 14:18:40 2011 +0200
+++ b/tools/xenbackendd/xenbackendd.c	Fri Sep 16 17:29:45 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;
@@ -169,7 +172,7 @@ main(int argc, char * const argv[])
 			log_file = optarg;
 			break;
 		case 'p':
-			pidfile = pidfile;
+			pidfile = optarg;
 		case 's':
 			vbd_script = optarg;
 			break;
@@ -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:

[PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks

[Roger Pau Monne]

# HG changeset patch
# User Roger Pau Monne <roger.pau@entel.upc.edu>
# Date 1316187362 -7200
# Node ID aff3960421768180410c8d553acf8881435bc3b4
# Parent  00949e363f6f2c70001da548403475628df93b97
libxl: add support for booting PV domains from NetBSD using raw files as disks.

Used the hotplug-status attribute in xenstore for vbd to check when the device is offline.

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

diff -r 00949e363f6f -r aff396042176 tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c	Fri Sep 16 17:29:45 2011 +0200
+++ b/tools/libxl/libxl_device.c	Fri Sep 16 17:36:02 2011 +0200
@@ -136,15 +136,20 @@ 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 (S_ISBLK(a->stab.st_mode))
+                return backend;
+#ifdef HAVE_PHY_BACKEND_FILE_SUPPORT
+        if (S_ISREG(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 or"
+                   " raw image", a->disk->vdev);
+#else
+        LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend phy"
+                   " unsuitable as phys path not a block device",
+                   a->disk->vdev);
+#endif
+        return 0;
 
     case LIBXL_DISK_BACKEND_TAP:
         if (!libxl__blktap_enabled(a->gc)) {
@@ -366,14 +371,26 @@ int libxl__device_destroy(libxl__gc *gc,
     libxl_ctx *ctx = libxl__gc_owner(gc);
     xs_transaction_t t;
     char *state_path = libxl__sprintf(gc, "%s/state", be_path);
+    char *hotplug_path = libxl__sprintf(gc, "%s/hotplug-status", be_path);
     char *state = libxl__xs_read(gc, XBT_NULL, state_path);
+    char *hotplug = libxl__xs_read(gc, XBT_NULL, hotplug_path);
     int rc = 0;
 
     if (!state)
         goto out;
-    if (atoi(state) != 4) {
-        xs_rm(ctx->xsh, XBT_NULL, be_path);
-        goto out;
+
+    if (!strstr(be_path, "vbd")) {
+        if (atoi(state) != 4) {
+            xs_rm(ctx->xsh, XBT_NULL, be_path);
+            goto out;
+        }
+    } else {
+        if (!hotplug)
+            goto out;
+        if (!strcmp(hotplug, "disconnected")) {
+            xs_rm(ctx->xsh, XBT_NULL, be_path);
+            goto out;
+        }
     }
 
 retry_transaction:
@@ -389,8 +406,13 @@ retry_transaction:
         }
     }
     if (!force) {
-        xs_watch(ctx->xsh, state_path, be_path);
-        rc = 1;
+        if (strstr(be_path, "vbd")) {
+            xs_watch(ctx->xsh, hotplug_path, be_path);
+            rc = 1;
+        } else {
+            xs_watch(ctx->xsh, state_path, be_path);
+            rc = 1;
+        }
     } else {
         xs_rm(ctx->xsh, XBT_NULL, be_path);
     }
@@ -405,6 +427,7 @@ static int wait_for_dev_destroy(libxl__g
     unsigned int n;
     fd_set rfds;
     char **l1 = NULL;
+    char *state, *hotplug;
 
     rc = 1;
     nfds = xs_fileno(ctx->xsh) + 1;
@@ -413,15 +436,29 @@ static int wait_for_dev_destroy(libxl__g
     if (select(nfds, &rfds, NULL, NULL, tv) > 0) {
         l1 = xs_read_watch(ctx->xsh, &n);
         if (l1 != NULL) {
-            char *state = 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 (strstr(l1[XS_WATCH_PATH], "hotplug-status")) {
+                hotplug = libxl__xs_read(gc, XBT_NULL, l1[XS_WATCH_PATH]);
+                if (!hotplug || !strcmp(hotplug, "disconnected")) {
+                    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 hotplug-status: %s", 
+                               l1[XS_WATCH_TOKEN], hotplug);
+                    rc = 0;
+                }
+            } else {
+                state = 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;
+                }
             }
             free(l1);
         }
+    } else {
+        /* timeout reached */
+        rc = 0;
     }
     return rc;
 }
@@ -482,7 +519,7 @@ int libxl__devices_destroy(libxl__gc *gc
         tv.tv_usec = 0;
         while (n_watches > 0) {
             if (wait_for_dev_destroy(gc, &tv)) {
-                break;
+                continue;
             } else {
                 n_watches--;
             }
diff -r 00949e363f6f -r aff396042176 tools/libxl/libxl_osdeps.h
--- a/tools/libxl/libxl_osdeps.h	Fri Sep 16 17:29:45 2011 +0200
+++ b/tools/libxl/libxl_osdeps.h	Fri Sep 16 17:36:02 2011 +0200
@@ -25,6 +25,7 @@
 
 #if defined(__NetBSD__) || defined(__OpenBSD__)
 #include <util.h>
+#define HAVE_PHY_BACKEND_FILE_SUPPORT
 #elif defined(__linux__)
 #include <pty.h>
 #elif defined(__sun__)

Re: [PATCH 1 of 2] hotplug: add hotplug-status disconnected

[Ian Campbell]

On Fri, 2011-09-16 at 16:36 +0100, Roger Pau Monne wrote:
> # HG changeset patch
> # User Roger Pau Monne <roger.pau@entel.upc.edu>
> # Date 1316186985 -7200
> # Node ID 00949e363f6f2c70001da548403475628df93b97
> # Parent  63e254468d6e8d81baeafaed68f05791dc21eb4e
> hotplug: add hotplug-status disconnected
> 
> Added hotplug-status disconnected when vbd are removed.
> 
> Signed-off-by: Roger Pau Monne <roger.pau@entel.upc.edu>
> 

Generally looks good, but I don't think you've got the logical
separation between the two patches quite right:

> diff -r 63e254468d6e -r 00949e363f6f tools/xenbackendd/xenbackendd.c
> --- a/tools/xenbackendd/xenbackendd.c	Wed Sep 14 14:18:40 2011 +0200
> +++ b/tools/xenbackendd/xenbackendd.c	Fri Sep 16 17:29:45 2011 +0200

> @@ -169,7 +172,7 @@ main(int argc, char * const argv[])
>  			log_file = optarg;
>  			break;
>  		case 'p':
> -			pidfile = pidfile;
> +			pidfile = optarg;

This is an unrelated bugfix? If so please post it as such.

>  		case 's':
>  			vbd_script = optarg;
>  			break;
> @@ -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);

Isn't this more like part of patch 2/2? It doesn't seem to relate to the
addition of the hotplug-status xenstore node.

Ian.

Re: [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks

[Ian Campbell]

On Fri, 2011-09-16 at 16:36 +0100, Roger Pau Monne wrote:
> # HG changeset patch
> # User Roger Pau Monne <roger.pau@entel.upc.edu>
> # Date 1316187362 -7200
> # Node ID aff3960421768180410c8d553acf8881435bc3b4
> # Parent  00949e363f6f2c70001da548403475628df93b97
> libxl: add support for booting PV domains from NetBSD using raw files as disks.
> 
> Used the hotplug-status attribute in xenstore for vbd to check when the device is offline.
> 
> Signed-off-by: Roger Pau Monne <roger.pau@entel.upc.edu>

Again this generally looks good but I think the functional split between
the patches isn't quite right.

> 
> diff -r 00949e363f6f -r aff396042176 tools/libxl/libxl_device.c
> --- a/tools/libxl/libxl_device.c	Fri Sep 16 17:29:45 2011 +0200
> +++ b/tools/libxl/libxl_device.c	Fri Sep 16 17:36:02 2011 +0200
> @@ -366,14 +371,26 @@ int libxl__device_destroy(libxl__gc *gc,


This bit (and most of this patch) goes along with the hotplug script
changes in patch 1/2, doesn't it?

>      libxl_ctx *ctx = libxl__gc_owner(gc);
>      xs_transaction_t t;
>      char *state_path = libxl__sprintf(gc, "%s/state", be_path);
> +    char *hotplug_path = libxl__sprintf(gc, "%s/hotplug-status", be_path);
>      char *state = libxl__xs_read(gc, XBT_NULL, state_path);
> +    char *hotplug = libxl__xs_read(gc, XBT_NULL, hotplug_path);
>      int rc = 0;
>  
>      if (!state)
>          goto out;
> -    if (atoi(state) != 4) {
> -        xs_rm(ctx->xsh, XBT_NULL, be_path);
> -        goto out;
> +
> +    if (!strstr(be_path, "vbd")) {

I've got a WIP patch which passes a libxl__device to functions like this
which will be helpful for removing these strstr things, since you can
just use the DEVICE_* enum.

> +        if (atoi(state) != 4) {
> +            xs_rm(ctx->xsh, XBT_NULL, be_path);
> +            goto out;
> +        }
> +    } else {
> +        if (!hotplug)
> +            goto out;
> +        if (!strcmp(hotplug, "disconnected")) {
> +            xs_rm(ctx->xsh, XBT_NULL, be_path);
> +            goto out;
> +        }
>      }
>  
>  retry_transaction:

> @@ -389,8 +406,13 @@ retry_transaction:
>          }
>      }
>      if (!force) {
> -        xs_watch(ctx->xsh, state_path, be_path);
> -        rc = 1;
> +        if (strstr(be_path, "vbd")) {
> +            xs_watch(ctx->xsh, hotplug_path, be_path);
> +            rc = 1;
> +        } else {
> +            xs_watch(ctx->xsh, state_path, be_path);
> +            rc = 1;
> +        }
>      } else {
>          xs_rm(ctx->xsh, XBT_NULL, be_path);
>      }
> @@ -405,6 +427,7 @@ static int wait_for_dev_destroy(libxl__g
>      unsigned int n;
>      fd_set rfds;
>      char **l1 = NULL;
> +    char *state, *hotplug;
>  
>      rc = 1;
>      nfds = xs_fileno(ctx->xsh) + 1;
> @@ -413,15 +436,29 @@ static int wait_for_dev_destroy(libxl__g
>      if (select(nfds, &rfds, NULL, NULL, tv) > 0) {
>          l1 = xs_read_watch(ctx->xsh, &n);
>          if (l1 != NULL) {
> -            char *state = 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 (strstr(l1[XS_WATCH_PATH], "hotplug-status")) {
> +                hotplug = libxl__xs_read(gc, XBT_NULL, l1[XS_WATCH_PATH]);
> +                if (!hotplug || !strcmp(hotplug, "disconnected")) {
> +                    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 hotplug-status: %s", 
> +                               l1[XS_WATCH_TOKEN], hotplug);
> +                    rc = 0;
> +                }
> +            } else {
> +                state = 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;
> +                }
>              }
>              free(l1);
>          }
> +    } else {
> +        /* timeout reached */
> +        rc = 0;
>      }
>      return rc;
>  }
> @@ -482,7 +519,7 @@ int libxl__devices_destroy(libxl__gc *gc
>          tv.tv_usec = 0;
>          while (n_watches > 0) {
>              if (wait_for_dev_destroy(gc, &tv)) {
> -                break;
> +                continue;
>              } else {
>                  n_watches--;
>              }

This looks like an independent bug fix?

> diff -r 00949e363f6f -r aff396042176 tools/libxl/libxl_osdeps.h
> --- a/tools/libxl/libxl_osdeps.h	Fri Sep 16 17:29:45 2011 +0200
> +++ b/tools/libxl/libxl_osdeps.h	Fri Sep 16 17:36:02 2011 +0200
> @@ -25,6 +25,7 @@
>  
>  #if defined(__NetBSD__) || defined(__OpenBSD__)
>  #include <util.h>
> +#define HAVE_PHY_BACKEND_FILE_SUPPORT
>  #elif defined(__linux__)
>  #include <pty.h>
>  #elif defined(__sun__)
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks

[Ian Jackson]

Roger Pau Monne writes ("[Xen-devel] [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks"):
> libxl: add support for booting PV domains from NetBSD using raw files as disks.

Thanks, I have some comments:

> +        if (S_ISBLK(a->stab.st_mode))
> +                return backend;
> +#ifdef HAVE_PHY_BACKEND_FILE_SUPPORT
> +        if (S_ISREG(a->stab.st_mode))
> +            return backend;

I think we would prefer not to have #ifdefs in the code.  That can
make the logic quite hard to follow.  Instead, invent a helper
function which answers the "does the phy backend support files" which
is provided in two versions, from osdep.c.

> @@ -366,14 +371,26 @@ int libxl__device_destroy(libxl__gc *gc,
>      libxl_ctx *ctx = libxl__gc_owner(gc);
>      xs_transaction_t t;
>      char *state_path = libxl__sprintf(gc, "%s/state", be_path);
> +    char *hotplug_path = libxl__sprintf(gc, "%s/hotplug-status", be_path);

We want to get away from the hotplug scripts for disks at least on
Linux with libxl.  Rather, any scripts that are needed should be run
from libxl directly.

How does that fit with NetBSD's disk backend approach ?
What goes wrong on NetBSD without this additional code ?

> @@ -482,7 +519,7 @@ int libxl__devices_destroy(libxl__gc *gc
>          tv.tv_usec = 0;
>          while (n_watches > 0) {
>              if (wait_for_dev_destroy(gc, &tv)) {
> -                break;
> +                continue;
>              } else {
>                  n_watches--;
>              }

I'm not sure I understand this change, or why it's needed.

Ian.

Re: [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks

[Roger Pau Monné]

2011/9/27 Ian Jackson <Ian.Jackson@eu.citrix.com>:
> Roger Pau Monne writes ("[Xen-devel] [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks"):
>> libxl: add support for booting PV domains from NetBSD using raw files as disks.
>
> Thanks, I have some comments:

This is an old version of the patch, I've split this change across
several patches, that are on the list also, and include more exact
descriptions of every change.

>
>> +        if (S_ISBLK(a->stab.st_mode))
>> +                return backend;
>> +#ifdef HAVE_PHY_BACKEND_FILE_SUPPORT
>> +        if (S_ISREG(a->stab.st_mode))
>> +            return backend;
>
> I think we would prefer not to have #ifdefs in the code.  That can
> make the logic quite hard to follow.  Instead, invent a helper
> function which answers the "does the phy backend support files" which
> is provided in two versions, from osdep.c.

Ok, I don't like ifdefs also.

>
>> @@ -366,14 +371,26 @@ int libxl__device_destroy(libxl__gc *gc,
>>      libxl_ctx *ctx = libxl__gc_owner(gc);
>>      xs_transaction_t t;
>>      char *state_path = libxl__sprintf(gc, "%s/state", be_path);
>> +    char *hotplug_path = libxl__sprintf(gc, "%s/hotplug-status", be_path);
>
> We want to get away from the hotplug scripts for disks at least on
> Linux with libxl.  Rather, any scripts that are needed should be run
> from libxl directly.
>
> How does that fit with NetBSD's disk backend approach ?
> What goes wrong on NetBSD without this additional code ?

NetBSD still uses the "loop" ("vnd" on NetBSD) device for files,
because it doesn't have support for qdisk or blktap. If we don't check
the hotplug-status before removing the vbd from xenstore (and only
look at state) it might be removed before the hotplug scripts are
executed, and the disk is never unmounted. This is why we need to
check the hotplug-status before removing vbd from xenstore. Of course,
I could call the hotplug scripts from libxl directly (for disk and
inet interfaces), and we could get rid of xenbackendd.

>
>> @@ -482,7 +519,7 @@ int libxl__devices_destroy(libxl__gc *gc
>>          tv.tv_usec = 0;
>>          while (n_watches > 0) {
>>              if (wait_for_dev_destroy(gc, &tv)) {
>> -                break;
>> +                continue;
>>              } else {
>>                  n_watches--;
>>              }
>
> I'm not sure I understand this change, or why it's needed.

This change is more explained in the series, basically libxl was not
waiting for all devices to disconnect, because when it returned from
wait_for_dev_destroy exited the loop immediately, even if we where
watching for more than one device to disconnect.

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

Re: [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks

[Ian Campbell]

On Tue, 2011-09-27 at 18:01 +0100, Ian Jackson wrote:
> Roger Pau Monne writes ("[Xen-devel] [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks"):
> > libxl: add support for booting PV domains from NetBSD using raw files as disks.
> 
> Thanks, I have some comments:
> 
> > +        if (S_ISBLK(a->stab.st_mode))
> > +                return backend;
> > +#ifdef HAVE_PHY_BACKEND_FILE_SUPPORT
> > +        if (S_ISREG(a->stab.st_mode))
> > +            return backend;
> 
> I think we would prefer not to have #ifdefs in the code.  That can
> make the logic quite hard to follow.  Instead, invent a helper
> function which answers the "does the phy backend support files" which
> is provided in two versions, from osdep.c.

This was my suggestion but you are right that a helper function is a
much better idea.

> > @@ -366,14 +371,26 @@ int libxl__device_destroy(libxl__gc *gc,
> >      libxl_ctx *ctx = libxl__gc_owner(gc);
> >      xs_transaction_t t;
> >      char *state_path = libxl__sprintf(gc, "%s/state", be_path);
> > +    char *hotplug_path = libxl__sprintf(gc, "%s/hotplug-status", be_path);
> 
> We want to get away from the hotplug scripts for disks at least on
> Linux with libxl.  Rather, any scripts that are needed should be run
> from libxl directly.

xenbackendd is the component in NetBSD which runs the "hotplug" scripts,
triggered off the xenstore state node transitions. (I presume the NetBSD
kernel driver doesn't generate hotplug events)

AIUI the issue is the synchronisation between the kernel device, libxl
and NetBSD's xenbackendd. Effectively libxl and xenbackendd are racing
on the teardown (both are watching the state node in xenstore). If
xenbackendd loses then it cannot do its cleanup because libxl has
already nuked the necessary info from xenstore. The fix which Roger has
made is to have only xenbackendd watch "state" and set
"hotplug-status=disconnected" and libxl to watch "hotplug-status". On
Linux the equivalent is to have the hotplug script write the
"hotplug-status=disconnected".

I think strictly speaking the Linux hotplug scripts have a similar race
but it just happens that there is no actual work on the teardown path so
the race is benign.

> How does that fit with NetBSD's disk backend approach ?

I expect that if we get rid of hotplug scripts on Linux that this will
be equivalent to removing xenbackendd on NetBSD, the same approximate
scheme should work for both, I think.

I think you've explained the scheme you have in mind for deprecating
hotplug scripts before but could you remind me (and for the lists
benefit)? Is it simply a case of shelling out to the vbd's configured
"script=" at the right points on attach and detach?

Would we then need special handling to turn "file:<X>" into
"phy:<X>,script=block-loop"?

I seem to remember something about setting up a faked out backend area
for each backend and running the scripts against that instead of the
real one.

> What goes wrong on NetBSD without this additional code ?

NetBSD doesn't have the option of falling back to blktap for file://
type disk devices so these simply don't work at the moment. This is a
pretty serious blocker for NetBSD moving to xl.

There was a subtle difference between NetBSD's and Linux's handling of
these with xend. On Linux xend used to setup and manage the loopback
device and create a simple phy backend referencing it. On NetBSD xend
just sets up a file or phy backend as appropriate and the scripts which
run out of xenbackendd take care of setting up the loopback as necessary
and filing in the real device in xenstore. On teardown the loopback
device needs to be cleaned up (and this is what currently fails).

For libxl on Linux we decided to avoid managing loopback devices in
libxl and instead just used blktap to meet that need but as I say this
isn't an option on BSD.

Roger has indicated that he is working on blktap-in-userspace
functionality, which would solve this problem longer term, so I think we
just need a stopgap measure to allow NetBSD users to switch to xl in the
meantime. How much work do you expect deprecating the hotplug scripts to
be, is it (or at least the subset necessary to solve this issue)
something which can be achieved in the short term?

> > @@ -482,7 +519,7 @@ int libxl__devices_destroy(libxl__gc *gc
> >          tv.tv_usec = 0;
> >          while (n_watches > 0) {
> >              if (wait_for_dev_destroy(gc, &tv)) {
> > -                break;
> > +                continue;
> >              } else {
> >                  n_watches--;
> >              }
> 
> I'm not sure I understand this change, or why it's needed.

This was an unrelated fixup. Roger subsequently posted it again as a
separate change. (as he did this whole series with clearer separation of
different fixes)

Ian.

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

Re: [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks

[Ian Jackson]

Roger Pau Monné writes ("Re: [Xen-devel] [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks"):
> NetBSD still uses the "loop" ("vnd" on NetBSD) device for files,
> because it doesn't have support for qdisk or blktap. If we don't check
> the hotplug-status before removing the vbd from xenstore (and only
> look at state) it might be removed before the hotplug scripts are
> executed, and the disk is never unmounted. This is why we need to
> check the hotplug-status before removing vbd from xenstore. Of course,
> I could call the hotplug scripts from libxl directly (for disk and
> inet interfaces), and we could get rid of xenbackendd.

Right.

Yes, I think you should call the hotplug scripts from libxl and get
rid of xenbackendd.  That is the way we want the Linux world to go,
although of course Linux needs to call hotplug scripts in fewer cases.

> >> @@ -482,7 +519,7 @@ int libxl__devices_destroy(libxl__gc *gc
> >>          tv.tv_usec = 0;
> >>          while (n_watches > 0) {
> >>              if (wait_for_dev_destroy(gc, &tv)) {
> >> -                break;
> >> +                continue;
> >>              } else {
> >>                  n_watches--;
> >>              }
> >
> > I'm not sure I understand this change, or why it's needed.
> 
> This change is more explained in the series, basically libxl was not
> waiting for all devices to disconnect, because when it returned from
> wait_for_dev_destroy exited the loop immediately, even if we where
> watching for more than one device to disconnect.

Right.

Ian.

Re: [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks"):
> I think you've explained the scheme you have in mind for deprecating
> hotplug scripts before but could you remind me (and for the lists
> benefit)? Is it simply a case of shelling out to the vbd's configured
> "script=" at the right points on attach and detach?

Yes.

The other elements of the block hotplug scripts don't do anything any
more on Linux I think, and currently libxl does not cope with script=
being set, so from a Linux pov this is a new feature for libxl.

> Would we then need special handling to turn "file:<X>" into
> "phy:<X>,script=block-loop"?

I think this should be done behind the scenes.  The backend-specific
code in libxl should call some kind of "invoke this script" function
which would also be used for explicit "script=".

On NetBSD, how do "more exciting" script= things work ?  Eg, iscsi or
nbd.  On Linux the idea is that the hotplug script sets up your nbd
which causes a real block device to appear, and that block device is
used by blkback.

> I seem to remember something about setting up a faked out backend area
> for each backend and running the scripts against that instead of the
> real one.

We would need to do that to support (eg) pygrub over nbd.

> There was a subtle difference between NetBSD's and Linux's handling of
> these with xend. On Linux xend used to setup and manage the loopback
> device and create a simple phy backend referencing it. On NetBSD xend
> just sets up a file or phy backend as appropriate and the scripts which
> run out of xenbackendd take care of setting up the loopback as necessary
> and filing in the real device in xenstore. On teardown the loopback
> device needs to be cleaned up (and this is what currently fails).

I'm not a fan of these approaches with a separate daemon.  We should
avoid that if we can as it produces a lot of complication.

Ian.

Re: [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks

[Roger Pau Monné]

> Yes.
>
> The other elements of the block hotplug scripts don't do anything any
> more on Linux I think, and currently libxl does not cope with script=
> being set, so from a Linux pov this is a new feature for libxl.
>
>> Would we then need special handling to turn "file:<X>" into
>> "phy:<X>,script=block-loop"?
>
> I think this should be done behind the scenes.  The backend-specific
> code in libxl should call some kind of "invoke this script" function
> which would also be used for explicit "script=".
>
> On NetBSD, how do "more exciting" script= things work ?  Eg, iscsi or
> nbd.

NetBSD only has block and vif scripts currently, nothing fancy.

> On Linux the idea is that the hotplug script sets up your nbd
> which causes a real block device to appear, and that block device is
> used by blkback.

NetBSD uses this approach to mount virtual images, the image is
mounted on the vnd device, and then this physical device is handled as
a regular 'phy' backend.

>
>> I seem to remember something about setting up a faked out backend area
>> for each backend and running the scripts against that instead of the
>> real one.
>
> We would need to do that to support (eg) pygrub over nbd.
>
>> There was a subtle difference between NetBSD's and Linux's handling of
>> these with xend. On Linux xend used to setup and manage the loopback
>> device and create a simple phy backend referencing it. On NetBSD xend
>> just sets up a file or phy backend as appropriate and the scripts which
>> run out of xenbackendd take care of setting up the loopback as necessary
>> and filing in the real device in xenstore. On teardown the loopback
>> device needs to be cleaned up (and this is what currently fails).
>
> I'm not a fan of these approaches with a separate daemon.  We should
> avoid that if we can as it produces a lot of complication.

I also think it's stupid to have two different programs watching
xenstore, because that's what xenbackendd does. Instead, the code in
xenbackendd could be move to libxl without much problem. The proper
calls to the block, vif and other scripts can be added to
libxl__device_destroy function to unplug vbd and network interfaces,
but I don't know if that's the best place to put them, because I still
don't have enough knowledge of libxl to decide that. As for the
startup script, I have to look at the source to decide where to put
them. Some advice about where would be the best place to put this
calls is welcome.

Regards, Roger.

Re: [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks

[Ian Campbell]

On Thu, 2011-09-29 at 09:25 +0100, Roger Pau Monné wrote:
> I also think it's stupid to have two different programs watching
> xenstore, because that's what xenbackendd does. Instead, the code in
> xenbackendd could be move to libxl without much problem. The proper
> calls to the block, vif and other scripts can be added to
> libxl__device_destroy function to unplug vbd and network interfaces,
> but I don't know if that's the best place to put them, because I still
> don't have enough knowledge of libxl to decide that.

I think libxl__device_destroy is where they have to go right now,
because there are destroy code paths which don't go through code with
device-specific knowledge (specifically libxl__devices_destroy).

Now, I'd like to change that in the long run (so all destroy paths knows
about device specifics) but for now libxl__device_destroy will do.

>  As for the
> startup script, I have to look at the source to decide where to put
> them. Some advice about where would be the best place to put this
> calls is welcome.

I think libxl_device_disk_add() and libxl_device_nic_add() are the right
places.

The disk case is pretty trivial, I think, since you run the script
before setting up the backend and after tearing it down again.

The network case is a little trickier since you need to run the script
after the backend driver has created the actual device in dom0 so the
script can configure it (I presume this is much the same on NetBSD as
Linux). The existing hotplug scripts are obviously pretty good for this
(at least on Linux) since they are called at precisely the right time.
How does NetBSD manage that synchronisation today in xenbackendd? I'm
not sure how this can be done in an OS independent way and/or compatibly
with the existing backend implementations for each platform .

I'd have no problem with just tackling the block half now since it is
the more critical bit for NetBSD users.

There's a secondary issue of doing the right thing for
libxl_device_disk_local_(attach|detach) (to allow e.g. pygrub) but again
I think that could be left for the time being, fixing block devices for
regular use by domains is more important.

With regard to disabling the hotplug scripts/xenbackendd when this last
came up we decided that /etc/init.d/xencommons should be opting in on a
per-toolstack basis by touching a file (in /var/run or so) which the
hotplug script check before actually doing anything. In the NetBSD case
instead of touching a file I guess you start xenbackendd or not as
appropriate (or pass the right parameters etc).

That thread is "add a way to disable xen's udev script." from June,
you'll probably want to skip to the end, specifically
<19963.36234.366877.468293@mariner.uk.xensource.com>

Ian.