Add NetBSD support for libxl PV DomU

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

tools/xenbackendd/xenbackendd.c +19 -2

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

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

tools/libxl/libxl_device.c +1 -0

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

tools/libxl/libxl_device.c +39 -8

+        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;
 }

diff -r 00949e363f6f -r aff396042176 tools/libxl/libxl_osdeps.h

tools/libxl/libxl_osdeps.h +1 -0

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

libxl: add support for booting PV domains from NetBSD using raw files as disks.

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

Quoted 1 line

libxl: add support for booting PV domains from NetBSD using raw files as disks.

Thanks, I have some comments:

Quoted 6 lines

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

tools/libxl/libxl_device.c int libxl__device_destroy(libxl__gc *gc, · line 374

Quoted 6 lines

tools/libxl/libxl_device.c +1 -0

@@ -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 ?

tools/libxl/libxl_device.c int libxl__devices_destroy(libxl__gc *gc · line 525

Quoted 10 lines

tools/libxl/libxl_device.c +1 -1

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

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

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.

Quoted 11 lines

tools/libxl/libxl_device.c int libxl__devices_destroy(libxl__gc *gc · line 525

Quoted 9 lines

tools/libxl/libxl_device.c +1 -1

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

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

Quoted 1 line

libxl: add support for booting PV domains from NetBSD using raw files as disks.

Thanks, I have some comments:

tools/libxl/libxl_device.c static int disk_try_backend(disk_try_bac · line 143

Quoted 5 lines

tools/libxl/libxl_device.c +5 -0

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

tools/libxl/libxl_device.c int libxl__device_destroy(libxl__gc *gc, · line 374

Quoted 5 lines

tools/libxl/libxl_device.c +1 -0

@@ -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 ?

tools/libxl/libxl_device.c int libxl__devices_destroy(libxl__gc *gc · line 525

Quoted 9 lines

tools/libxl/libxl_device.c +1 -1

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

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

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.

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

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.

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 7 lines

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.