Roger Pau Monne
2011-Sep-15 13:03 UTC
[Xen-devel] [PATCH] libxl: add support for booting PV domains from NetBSD using raw files as disks. Fixed the shutdown race problem by checking "hotplug-status" instead of "state" xenstore variable in NetBSD
# HG changeset patch
# User Roger Pau Monne <roger.pau@entel.upc.edu>
# Date 1316091721 -7200
# Node ID 015617579cd36fc58318aaf350bec5f7cc07ef2f
# Parent 63e254468d6e8d81baeafaed68f05791dc21eb4e
libxl: add support for booting PV domains from NetBSD using raw files as disks.
Fixed the shutdown race problem by checking "hotplug-status" instead
of "state" xenstore variable in NetBSD.
Signed-off-by: Roger Pau Monne <roger.pau@entel.upc.edu>
diff -r 63e254468d6e -r 015617579cd3 tools/hotplug/NetBSD/block
--- a/tools/hotplug/NetBSD/block Wed Sep 14 14:18:40 2011 +0200
+++ b/tools/hotplug/NetBSD/block Thu Sep 15 15:02:01 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 015617579cd3 tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c Wed Sep 14 14:18:40 2011 +0200
+++ b/tools/libxl/libxl_device.c Thu Sep 15 15:02:01 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,16 +371,35 @@ 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;
+
+#ifdef HAVE_PHY_BACKEND_FILE_SUPPORT
+ 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;
+ }
+ }
+#else
if (atoi(state) != 4) {
xs_rm(ctx->xsh, XBT_NULL, be_path);
goto out;
}
-
+#endif
+
retry_transaction:
t = xs_transaction_start(ctx->xsh);
xs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/online",
be_path), "0", strlen("0"));
@@ -389,6 +413,13 @@ retry_transaction:
}
}
if (!force) {
+#ifdef HAVE_PHY_BACKEND_FILE_SUPPORT
+ if (strstr(be_path, "vbd")) {
+ xs_watch(ctx->xsh, hotplug_path, be_path);
+ rc = 1;
+ goto out;
+ }
+#endif
xs_watch(ctx->xsh, state_path, be_path);
rc = 1;
} else {
@@ -405,6 +436,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,7 +445,21 @@ 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]);
+#ifdef HAVE_PHY_BACKEND_FILE_SUPPORT
+ if (strstr(l1[XS_WATCH_PATH], "hotplug-status")) {
+ hotplug = libxl__xs_read(gc, XBT_NULL, l1[XS_WATCH_PATH]);
+ if (!hotplug || strcmp(hotplug, "disconnected") == 0)
{
+ 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;
+ }
+ free(l1);
+ return rc;
+ }
+#endif
+ 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]);
@@ -422,6 +468,9 @@ static int wait_for_dev_destroy(libxl__g
}
free(l1);
}
+ } else {
+ /* timeout reached */
+ rc = 0;
}
return rc;
}
@@ -482,7 +531,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 63e254468d6e -r 015617579cd3 tools/libxl/libxl_osdeps.h
--- a/tools/libxl/libxl_osdeps.h Wed Sep 14 14:18:40 2011 +0200
+++ b/tools/libxl/libxl_osdeps.h Thu Sep 15 15:02:01 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__)
diff -r 63e254468d6e -r 015617579cd3 tools/xenbackendd/xenbackendd.c
--- a/tools/xenbackendd/xenbackendd.c Wed Sep 14 14:18:40 2011 +0200
+++ b/tools/xenbackendd/xenbackendd.c Thu Sep 15 15:02:01 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:
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Campbell
2011-Sep-15 13:41 UTC
Re: [Xen-devel] [PATCH] libxl: add support for booting PV domains from NetBSD using raw files as disks. Fixed the shutdown race problem by checking "hotplug-status" instead of "state" xenstore variable in NetBSD
On Thu, 2011-09-15 at 09:03 -0400, Roger Pau Monne wrote:> # HG changeset patch > # User Roger Pau Monne <roger.pau@entel.upc.edu> > # Date 1316091721 -7200 > # Node ID 015617579cd36fc58318aaf350bec5f7cc07ef2f > # Parent 63e254468d6e8d81baeafaed68f05791dc21eb4e > libxl: add support for booting PV domains from NetBSD using raw files > as disks. Fixed the shutdown race problem by checking "hotplug-status" > instead of "state" xenstore variable in NetBSD.This was one long line which mercurial will use as a summary, it''s a good idea to make sure that the first line stands somewhat alone as a summary so the e.g. "hg log" is useful. Also this sounds on the face of it like two bugfixes, if that''s the case they should be submitted separately.> > Signed-off-by: Roger Pau Monne <roger.pau@entel.upc.edu> > > diff -r 63e254468d6e -r 015617579cd3 tools/hotplug/NetBSD/block > --- a/tools/hotplug/NetBSD/block Wed Sep 14 14:18:40 2011 +0200 > +++ b/tools/hotplug/NetBSD/block Thu Sep 15 15:02:01 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 disconnectedleftover debugging?> + xenstore-write $xpath/hotplug-status disconnected > xenstore-rm $xpath > exit 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); > + char *hotplug = libxl__xs_read(gc, XBT_NULL, hotplug_path); > int rc = 0; > > if (!state) > goto out; > + > +#ifdef HAVE_PHY_BACKEND_FILE_SUPPORT > + 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; > + } > + }Do the other backend types also write this node? It looks like Linux does, at least in some circumstances (tap and vbd AFAICT), in which case perhaps this is suitable as the only test here (i.e. drop the #ifdef and the #else case). That would remove a lot of the conditional code in this patch. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Roger Pau Monné
2011-Sep-15 15:19 UTC
Re: [Xen-devel] [PATCH] libxl: add support for booting PV domains from NetBSD using raw files as disks. Fixed the shutdown race problem by checking "hotplug-status" instead of "state" xenstore variable in NetBSD
2011/9/15 Ian Campbell <Ian.Campbell@citrix.com>:> On Thu, 2011-09-15 at 09:03 -0400, Roger Pau Monne wrote: >> # HG changeset patch >> # User Roger Pau Monne <roger.pau@entel.upc.edu> >> # Date 1316091721 -7200 >> # Node ID 015617579cd36fc58318aaf350bec5f7cc07ef2f >> # Parent 63e254468d6e8d81baeafaed68f05791dc21eb4e >> libxl: add support for booting PV domains from NetBSD using raw files >> as disks. Fixed the shutdown race problem by checking "hotplug-status" >> instead of "state" xenstore variable in NetBSD. > > This was one long line which mercurial will use as a summary, it''s a > good idea to make sure that the first line stands somewhat alone as a > summary so the e.g. "hg log" is useful. > Also this sounds on the face of it like two bugfixes, if that''s the case > they should be submitted separately.Well, it''s not a bugfix, because NetBSD was not supported by libxl, so I think it''s just part of the patch to add support for NetBSD to libxl. I will split the line, and the patch if it''s necessary, but one doesn''t make sense without the other, and I don''t really know what would be in each patch.>> >> Signed-off-by: Roger Pau Monne <roger.pau@entel.upc.edu> >> >> diff -r 63e254468d6e -r 015617579cd3 tools/hotplug/NetBSD/block >> --- a/tools/hotplug/NetBSD/block Wed Sep 14 14:18:40 2011 +0200 >> +++ b/tools/hotplug/NetBSD/block Thu Sep 15 15:02:01 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 > > leftover debugging?The block NetBSD script contains some echos, so I feel it would be interesting to add this one too.>> + xenstore-write $xpath/hotplug-status disconnected >> xenstore-rm $xpath >> exit 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); >> + char *hotplug = libxl__xs_read(gc, XBT_NULL, hotplug_path); >> int rc = 0; >> >> if (!state) >> goto out; >> + >> +#ifdef HAVE_PHY_BACKEND_FILE_SUPPORT >> + 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; >> + } >> + } > > Do the other backend types also write this node? It looks like Linux > does, at least in some circumstances (tap and vbd AFAICT), in which case > perhaps this is suitable as the only test here (i.e. drop the #ifdef and > the #else case). That would remove a lot of the conditional code in this > patch.If Linux also uses this, I will have to modify the Linux block script, so that it sets hotplug-status to "disconnected" also. This will be nice, I don''t like code with #ifdefs as it tends to get messy and it''s not easy to understand. Regards, Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Sep-15 15:40 UTC
Re: [Xen-devel] [PATCH] libxl: add support for booting PV domains from NetBSD using raw files as disks. Fixed the shutdown race problem by checking "hotplug-status" instead of "state" xenstore variable in NetBSD
On Thu, 2011-09-15 at 11:19 -0400, Roger Pau Monné wrote:> 2011/9/15 Ian Campbell <Ian.Campbell@citrix.com>: > > On Thu, 2011-09-15 at 09:03 -0400, Roger Pau Monne wrote: > >> # HG changeset patch > >> # User Roger Pau Monne <roger.pau@entel.upc.edu> > >> # Date 1316091721 -7200 > >> # Node ID 015617579cd36fc58318aaf350bec5f7cc07ef2f > >> # Parent 63e254468d6e8d81baeafaed68f05791dc21eb4e > >> libxl: add support for booting PV domains from NetBSD using raw files > >> as disks. Fixed the shutdown race problem by checking "hotplug-status" > >> instead of "state" xenstore variable in NetBSD. > > > > This was one long line which mercurial will use as a summary, it''s a > > good idea to make sure that the first line stands somewhat alone as a > > summary so the e.g. "hg log" is useful. > > Also this sounds on the face of it like two bugfixes, if that''s the case > > they should be submitted separately. > > Well, it''s not a bugfix, because NetBSD was not supported by libxl, so > I think it''s just part of the patch to add support for NetBSD to > libxl. I will split the line, and the patch if it''s necessary, but one > doesn''t make sense without the other, and I don''t really know what > would be in each patch.I guess I saw "add support" and "fix race" and assumed they were separate (albeit related) changes. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel