* [PATCH 0 of 2] Add NetBSD support for libxl PV DomU @ 2011-09-16 15:36 Roger Pau Monne 2011-09-16 15:36 ` [PATCH 1 of 2] hotplug: add hotplug-status disconnected Roger Pau Monne 2011-09-16 15:36 ` [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks Roger Pau Monne 0 siblings, 2 replies; 12+ messages in thread From: Roger Pau Monne @ 2011-09-16 15:36 UTC (permalink / raw) To: xen-devel This series of patches adds NetBSD support to libxl, using hotplug-status to synchronize the unplugging of vbd devices. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1 of 2] hotplug: add hotplug-status disconnected 2011-09-16 15:36 [PATCH 0 of 2] Add NetBSD support for libxl PV DomU Roger Pau Monne @ 2011-09-16 15:36 ` Roger Pau Monne 2011-09-21 9:41 ` Ian Campbell 2011-09-16 15:36 ` [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks Roger Pau Monne 1 sibling, 1 reply; 12+ messages in thread From: Roger Pau Monne @ 2011-09-16 15:36 UTC (permalink / raw) To: xen-devel # 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: ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1 of 2] hotplug: add hotplug-status disconnected 2011-09-16 15:36 ` [PATCH 1 of 2] hotplug: add hotplug-status disconnected Roger Pau Monne @ 2011-09-21 9:41 ` Ian Campbell 0 siblings, 0 replies; 12+ messages in thread From: Ian Campbell @ 2011-09-21 9:41 UTC (permalink / raw) To: Roger Pau Monne; +Cc: xen-devel 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks 2011-09-16 15:36 [PATCH 0 of 2] Add NetBSD support for libxl PV DomU Roger Pau Monne 2011-09-16 15:36 ` [PATCH 1 of 2] hotplug: add hotplug-status disconnected Roger Pau Monne @ 2011-09-16 15:36 ` Roger Pau Monne 2011-09-21 9:49 ` Ian Campbell 2011-09-27 17:01 ` Ian Jackson 1 sibling, 2 replies; 12+ messages in thread From: Roger Pau Monne @ 2011-09-16 15:36 UTC (permalink / raw) To: xen-devel # 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__) ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks 2011-09-16 15:36 ` [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks Roger Pau Monne @ 2011-09-21 9:49 ` Ian Campbell 2011-09-27 17:01 ` Ian Jackson 1 sibling, 0 replies; 12+ messages in thread From: Ian Campbell @ 2011-09-21 9:49 UTC (permalink / raw) To: Roger Pau Monne; +Cc: xen-devel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks 2011-09-16 15:36 ` [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks Roger Pau Monne 2011-09-21 9:49 ` Ian Campbell @ 2011-09-27 17:01 ` Ian Jackson 2011-09-28 8:07 ` Roger Pau Monné 2011-09-28 8:13 ` Ian Campbell 1 sibling, 2 replies; 12+ messages in thread From: Ian Jackson @ 2011-09-27 17:01 UTC (permalink / raw) To: Roger Pau Monne; +Cc: xen-devel 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks 2011-09-27 17:01 ` Ian Jackson @ 2011-09-28 8:07 ` Roger Pau Monné 2011-09-28 13:28 ` Ian Jackson 2011-09-28 8:13 ` Ian Campbell 1 sibling, 1 reply; 12+ messages in thread From: Roger Pau Monné @ 2011-09-28 8:07 UTC (permalink / raw) To: Ian Jackson; +Cc: xen-devel 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks 2011-09-28 8:07 ` Roger Pau Monné @ 2011-09-28 13:28 ` Ian Jackson 0 siblings, 0 replies; 12+ messages in thread From: Ian Jackson @ 2011-09-28 13:28 UTC (permalink / raw) To: Roger Pau Monné; +Cc: xen-devel 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks 2011-09-27 17:01 ` Ian Jackson 2011-09-28 8:07 ` Roger Pau Monné @ 2011-09-28 8:13 ` Ian Campbell 2011-09-28 13:32 ` Ian Jackson 1 sibling, 1 reply; 12+ messages in thread From: Ian Campbell @ 2011-09-28 8:13 UTC (permalink / raw) To: Ian Jackson; +Cc: Roger Pau Monne, xen-devel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks 2011-09-28 8:13 ` Ian Campbell @ 2011-09-28 13:32 ` Ian Jackson 2011-09-29 8:25 ` Roger Pau Monné 0 siblings, 1 reply; 12+ messages in thread From: Ian Jackson @ 2011-09-28 13:32 UTC (permalink / raw) To: Ian Campbell; +Cc: Roger Pau Monne, xen-devel 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks 2011-09-28 13:32 ` Ian Jackson @ 2011-09-29 8:25 ` Roger Pau Monné 2011-09-29 9:05 ` Ian Campbell 0 siblings, 1 reply; 12+ messages in thread From: Roger Pau Monné @ 2011-09-29 8:25 UTC (permalink / raw) To: Ian Jackson; +Cc: xen-devel, Ian Campbell > 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks 2011-09-29 8:25 ` Roger Pau Monné @ 2011-09-29 9:05 ` Ian Campbell 0 siblings, 0 replies; 12+ messages in thread From: Ian Campbell @ 2011-09-29 9:05 UTC (permalink / raw) To: Roger Pau Monné; +Cc: xen-devel, Ian Jackson 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2011-09-29 9:05 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-09-16 15:36 [PATCH 0 of 2] Add NetBSD support for libxl PV DomU Roger Pau Monne 2011-09-16 15:36 ` [PATCH 1 of 2] hotplug: add hotplug-status disconnected Roger Pau Monne 2011-09-21 9:41 ` Ian Campbell 2011-09-16 15:36 ` [PATCH 2 of 2] libxl: add support for booting PV domains from NetBSD using raw files as disks Roger Pau Monne 2011-09-21 9:49 ` Ian Campbell 2011-09-27 17:01 ` Ian Jackson 2011-09-28 8:07 ` Roger Pau Monné 2011-09-28 13:28 ` Ian Jackson 2011-09-28 8:13 ` Ian Campbell 2011-09-28 13:32 ` Ian Jackson 2011-09-29 8:25 ` Roger Pau Monné 2011-09-29 9:05 ` Ian Campbell
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.