[PATCH v2 0/5] NetBSD: minor fixes and hotplug execution

[PATCH v2 0/5] NetBSD: minor fixes and hotplug execution

[Roger Pau Monne]

This series contains minor fixes to build xen on NetBSD and adds 
hotplug support to NetBSD. After this, xen-unstable should build 
and work out-of-the-box on NetBSD.

Changes are available in the git repository at:
  git://xenbits.xen.org/people/royger/xen.git netbsd.v2

Roger Pau Monne (5):
      tools/build: fix pygrub linking
      libxl: react correctly to POLLHUP
     *hotplug/NetBSD: check type of file to attach from params
     *libxl: call hotplug scripts from xl for NetBSD
      init/NetBSD: move xenbackendd to xend init script

*Acked

[PATCH v2 1/5] tools/build: fix pygrub linking

[Roger Pau Monne]

Prevent creating a symlink to $(DESTDIR)/$(BINDIR) if it is the same
as $(PRIVATE_BINDIR)

This fixes NetBSD install, where $(DESTDIR)/$(BINDIR) ==
$(PRIVATE_BINDIR).

Changes since v1:

 * Do the check in shell instead of Makefile.

Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Christoph Egger <Christoph.Egger@amd.com>
Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
---
 tools/pygrub/Makefile |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/tools/pygrub/Makefile b/tools/pygrub/Makefile
index bd22dd4..8c99e11 100644
--- a/tools/pygrub/Makefile
+++ b/tools/pygrub/Makefile
@@ -14,7 +14,10 @@ install: all
 		$(PYTHON_PREFIX_ARG) --root="$(DESTDIR)" \
 		--install-scripts=$(PRIVATE_BINDIR) --force
 	$(INSTALL_DIR) $(DESTDIR)/var/run/xend/boot
-	ln -sf $(PRIVATE_BINDIR)/pygrub $(DESTDIR)/$(BINDIR)
+	set -e; if [ `readlink -f $(DESTDIR)/$(BINDIR)` != \
+	             `readlink -f $(PRIVATE_BINDIR)` ]; then \
+	    ln -sf $(PRIVATE_BINDIR)/pygrub $(DESTDIR)/$(BINDIR); \
+	fi
 
 .PHONY: clean
 clean:
-- 
1.7.7.5 (Apple Git-26)

[PATCH v2 2/5] libxl: react correctly to POLLHUP

[Roger Pau Monne]

When received POLLHUP on datacopier_readable/writable, kill the
datacopier and call the callback with onwrite=-2. On the bootloader
callback kill the bootloader process and wait for the callback, but
without setting the bl->rc error code.

This is because NetBSD returns POLLHUP when the process on the other
end of the pty exits (even when exiting normally).

Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
---
 tools/libxl/libxl_aoutils.c    |   16 ++++++++++++++++
 tools/libxl/libxl_bootloader.c |    9 ++++++---
 tools/libxl/libxl_internal.h   |    1 +
 3 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
index 99972a2..2ea9b2c 100644
--- a/tools/libxl/libxl_aoutils.c
+++ b/tools/libxl/libxl_aoutils.c
@@ -102,6 +102,14 @@ static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev,
     libxl__datacopier_state *dc = CONTAINER_OF(ev, *dc, toread);
     STATE_AO_GC(dc->ao);
 
+    if (revents & POLLHUP) {
+        LOG(DEBUG, "received POLLHUP on %s during copy of %s, "
+                   "killing the bootloader process",
+                   dc->readwhat, dc->copywhat);
+        datacopier_callback(egc, dc, -2, 0);
+        return;
+    }
+
     if (revents & ~POLLIN) {
         LOG(ERROR, "unexpected poll event 0x%x (should be POLLIN)"
             " on %s during copy of %s", revents, dc->readwhat, dc->copywhat);
@@ -163,6 +171,14 @@ static void datacopier_writable(libxl__egc *egc, libxl__ev_fd *ev,
     libxl__datacopier_state *dc = CONTAINER_OF(ev, *dc, towrite);
     STATE_AO_GC(dc->ao);
 
+    if (revents & POLLHUP) {
+        LOG(DEBUG, "received POLLHUP on %s during copy of %s, "
+                   "killing the bootloader process",
+                   dc->writewhat, dc->copywhat);
+        datacopier_callback(egc, dc, -2, 0);
+        return;
+    }
+
     if (revents & ~POLLOUT) {
         LOG(ERROR, "unexpected poll event 0x%x (should be POLLOUT)"
             " on %s during copy of %s", revents, dc->writewhat, dc->copywhat);
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index ef5a91b..f3d92e2 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -285,8 +285,8 @@ static void bootloader_abort(libxl__egc *egc,
     libxl__datacopier_kill(&bl->display);
     if (libxl__ev_child_inuse(&bl->child)) {
         r = kill(bl->child.pid, SIGTERM);
-        if (r) LOGE(WARN, "after failure, failed to kill bootloader [%lu]",
-                    (unsigned long)bl->child.pid);
+        if (r) LOGE(WARN, "%sfailed to kill bootloader [%lu]",
+                    rc ? "after failure, " : "", (unsigned long)bl->child.pid);
     }
     bl->rc = rc;
 }
@@ -567,7 +567,10 @@ static void bootloader_copyfail(libxl__egc *egc, const char *which,
     STATE_AO_GC(bl->ao);
     if (!onwrite && !errnoval)
         LOG(ERROR, "unexpected eof copying %s", which);
-    bootloader_abort(egc, bl, ERROR_FAIL);
+    if (onwrite == -2)
+        bootloader_abort(egc, bl, 0);
+    else
+        bootloader_abort(egc, bl, ERROR_FAIL);
 }
 static void bootloader_keystrokes_copyfail(libxl__egc *egc,
        libxl__datacopier_state *dc, int onwrite, int errnoval)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 3ee3a09..286aa19 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2073,6 +2073,7 @@ typedef struct libxl__datacopier_buf libxl__datacopier_buf;
  *     errnoval==0 means we got eof and all data was written
  *     errnoval!=0 means we had a read error, logged
  * onwrite==-1 means some other internal failure, errnoval not valid, logged
+ * onwrite==-2 means we got a POLLHUP, errnoval not valid, logged
  * in all cases copier is killed before calling this callback */
 typedef void libxl__datacopier_callback(libxl__egc *egc,
      libxl__datacopier_state *dc, int onwrite, int errnoval);
-- 
1.7.7.5 (Apple Git-26)

[PATCH v2 3/5] hotplug/NetBSD: check type of file to attach from params

[Roger Pau Monne]

xend used to set the xenbus backend entry "type" to either "phy" or
"file", but now libxl sets it to "phy" for both file and block device.
We have to manually check for the type of the "param" filed in order
to detect if we are trying to attach a file or a block device.

Changes since v1:

 * Check that file is either a block special file or a regular file
   and report error otherwise.

Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Christoph Egger <Christoph.Egger@amd.com>
Acked-by: Christoph Egger <Christoph.Egger@amd.com>
Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
---
 tools/hotplug/NetBSD/block |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/tools/hotplug/NetBSD/block b/tools/hotplug/NetBSD/block
index cf5ff3a..31d9998 100644
--- a/tools/hotplug/NetBSD/block
+++ b/tools/hotplug/NetBSD/block
@@ -19,8 +19,14 @@ error() {
 
 xpath=$1
 xstatus=$2
-xtype=$(xenstore-read "$xpath/type")
 xparams=$(xenstore-read "$xpath/params")
+if [ -b $xparams ]; then
+	xtype="phy"
+elif [ -f $xparams ]; then
+	xtype="file"
+else
+	error "invalid file type"
+fi
 
 case $xstatus in
 6)
-- 
1.7.7.5 (Apple Git-26)

[PATCH v2 4/5] libxl: call hotplug scripts from xl for NetBSD

[Roger Pau Monne]

Add the missing NetBSD functions to call hotplug scripts, and disable
xenbackendd if libxl/disable_udev is not set.

Cc: Christoph Egger <Christoph.Egger@amd.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
---
 tools/libxl/libxl_netbsd.c      |   57 ++++++++++++++++++++++++++++++++++++++-
 tools/xenbackendd/xenbackendd.c |    8 +++++-
 2 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
index 28cdf21..9587833 100644
--- a/tools/libxl/libxl_netbsd.c
+++ b/tools/libxl/libxl_netbsd.c
@@ -32,10 +32,65 @@ char *libxl__devid_to_localdev(libxl__gc *gc, int devid)
 }
 
 /* Hotplug scripts caller functions */
+static int libxl__hotplug(libxl__gc *gc, libxl__device *dev, char ***args,
+                          libxl__device_action action)
+{
+    char *be_path = libxl__device_backend_path(gc, dev);
+    char *script;
+    int nr = 0, rc = 0, arraysize = 4;
+
+    script = libxl__xs_read(gc, XBT_NULL,
+                            GCSPRINTF("%s/%s", be_path, "script"));
+    if (!script) {
+        LOGEV(ERROR, errno, "unable to read script from %s", be_path);
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    GCNEW_ARRAY(*args, arraysize);
+    (*args)[nr++] = script;
+    (*args)[nr++] = be_path;
+    (*args)[nr++] = GCSPRINTF("%d", action == DEVICE_CONNECT ?
+                                    XenbusStateInitWait : XenbusStateClosed);
+    (*args)[nr++] = NULL;
+    assert(nr == arraysize);
+
+out:
+    return rc;
+}
+
 int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
                                    char ***args, char ***env,
                                    libxl__device_action action,
                                    int num_exec)
 {
-    return 0;
+    char *disable_udev = libxl__xs_read(gc, XBT_NULL, DISABLE_UDEV_PATH);
+    int rc;
+
+    /* Check if we have to run hotplug scripts */
+    if (!disable_udev || num_exec > 0) {
+        rc = 0;
+        goto out;
+    }
+
+    switch (dev->backend_kind) {
+    case LIBXL__DEVICE_KIND_VBD:
+    case LIBXL__DEVICE_KIND_VIF:
+        if (num_exec != 0) {
+            rc = 0;
+            goto out;
+        }
+        rc = libxl__hotplug(gc, dev, args, action);
+        if (!rc) rc = 1;
+        break;
+    default:
+        /* If no need to execute any hotplug scripts,
+         * call the callback manually
+         */
+        rc = 0;
+        break;
+    }
+
+out:
+    return rc;
 }
diff --git a/tools/xenbackendd/xenbackendd.c b/tools/xenbackendd/xenbackendd.c
index 6b5bb8e..5381a2a 100644
--- a/tools/xenbackendd/xenbackendd.c
+++ b/tools/xenbackendd/xenbackendd.c
@@ -33,6 +33,7 @@
 #define DEVTYPE_UNKNOWN 0
 #define DEVTYPE_VIF 1
 #define DEVTYPE_VBD 2
+#define DISABLE_EXEC "libxl/disable_udev"
 
 #define DOMAIN_PATH "/local/domain/0"
 
@@ -149,7 +150,7 @@ main(int argc, char * const argv[])
 	unsigned int num;
 	char *s;
 	int state;
-	char *sstate;
+	char *sstate, *sdisable;
 	char *p;
 	char buf[80];
 	int type;
@@ -245,6 +246,10 @@ main(int argc, char * const argv[])
 			continue;
 		}
 
+		sdisable = xs_read(xs, XBT_NULL, DISABLE_EXEC, 0);
+		if (sdisable)
+			goto next1;
+
 		if (strlen(vec[XS_WATCH_PATH]) < sizeof("state"))
 			goto next1;
 
@@ -314,6 +319,7 @@ next2:
 		free(sstate);
 
 next1:
+		free(sdisable);
 		free(vec);
 	}
 
-- 
1.7.7.5 (Apple Git-26)

[PATCH v2 5/5] init/NetBSD: move xenbackendd to xend init script

[Roger Pau Monne]

xenbackendd is not needed by the xl toolstack, so move it's launch to
the xend script.

We have to iterate until we are sure there are no xend processes left,
since doing a single pkill usually leaves xend processes running.

Changes since v1:

 * Use pgrep and pkill instead of the convoluted shell expression.

Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Christoph Egger <Christoph.Egger@amd.com>
Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
---
 tools/hotplug/NetBSD/rc.d/xencommons |   29 ++---------------
 tools/hotplug/NetBSD/rc.d/xend       |   55 +++++++++++++++++++++++++++++++--
 2 files changed, 55 insertions(+), 29 deletions(-)

diff --git a/tools/hotplug/NetBSD/rc.d/xencommons b/tools/hotplug/NetBSD/rc.d/xencommons
index c0d87bf..fe4c9ac 100644
--- a/tools/hotplug/NetBSD/rc.d/xencommons
+++ b/tools/hotplug/NetBSD/rc.d/xencommons
@@ -29,8 +29,6 @@ XENBACKENDD_PIDFILE="/var/run/xenbackendd.pid"
 
 xen_precmd()
 {
-	mkdir -p /var/run/xend || exit 1
-	mkdir -p /var/run/xend/boot || exit 1
 	mkdir -p /var/run/xenstored || exit 1
 }
 
@@ -46,7 +44,7 @@ xen_startcmd()
 			XENSTORED_ROOTDIR="/var/lib/xenstored"
 		fi
 		rm -f ${XENSTORED_ROOTDIR}/tdb* >/dev/null 2>&1
-		printf "Starting xenservices: xenstored, xenconsoled, xenbackendd."
+		printf "Starting xenservices: xenstored, xenconsoled."
 		XENSTORED_ARGS=" --pid-file ${XENSTORED_PIDFILE}"
 		if [ -n "${XENSTORED_TRACE}" ]; then
 			XENSTORED_ARGS="${XENSTORED_ARGS} -T /var/log/xen/xenstored-trace.log"
@@ -58,7 +56,7 @@ xen_startcmd()
 			sleep 1
 		done
 	else
-		printf "Starting xenservices: xenconsoled, xenbackendd."
+		printf "Starting xenservices: xenconsoled."
 	fi
 
 	XENCONSOLED_ARGS=""
@@ -68,13 +66,6 @@ xen_startcmd()
 
 	${SBINDIR}/xenconsoled ${XENCONSOLED_ARGS}
 
-	XENBACKENDD_ARGS=""
-	if [ -n "${XENBACKENDD_DEBUG}" ]; then
-		XENBACKENDD_ARGS="${XENBACKENDD_ARGS} -d"
-	fi
-
-	${SBINDIR}/xenbackendd ${XENBACKENDD_ARGS}
-
 	printf "\n"
 
 	printf "Setting domain 0 name.\n"
@@ -87,8 +78,6 @@ xen_stop()
 	printf "Stopping xencommons.\n"
 	printf "WARNING: Not stopping xenstored, as it cannot be restarted.\n"
 
-	rc_pid=$(check_pidfile ${XENBACKENDD_PIDFILE} ${SBINDIR}/xenbackendd)
-	pids="$pids $rc_pid"
 	rc_pid=$(check_pidfile ${XENCONSOLED_PIDFILE} ${SBINDIR}/xenconsoled)
 	pids="$pids $rc_pid"
 
@@ -108,17 +97,12 @@ xen_status()
 		pids="$pids $xenconsoled_pid"
 	fi
 
-	xenbackend_pid=$(check_pidfile ${XENBACKENDD_PIDFILE} ${SBINDIR}/xenbackendd)
-	if test -n ${xenbackend_pid}; then
-		pids="$pids $xenbackend_pid"
-	fi
-
-	if test -n "$xenbackend_pid" -a -n "$xenconsoled_pid" -a -n "$xenstored_pid";
+	if test -n "$xenconsoled_pid" -a -n "$xenstored_pid";
 	then
 		echo "xencommons are running as pids $pids."
 		return 0
 	fi
-	if test -z "$xenbackend_pid" -a -z "$xenconsoled_pid" -a -z "$xenstored_pid";
+	if test -z "$xenconsoled_pid" -a -z "$xenstored_pid";
 	then
 		echo "xencommons are not running."
 		return 0
@@ -134,11 +118,6 @@ xen_status()
 	else
 		echo "xenconsoled is not running."
 	fi
-	if test -n $xenbackend_pid; then
-		echo "xenbackendd is running as pid $xenbackend_pid."
-	else
-		echo "xenbackendd is not running."
-	fi
 }
 
 load_rc_config $name
diff --git a/tools/hotplug/NetBSD/rc.d/xend b/tools/hotplug/NetBSD/rc.d/xend
index ead9ee0..ac5f2ca 100644
--- a/tools/hotplug/NetBSD/rc.d/xend
+++ b/tools/hotplug/NetBSD/rc.d/xend
@@ -15,10 +15,57 @@ export PATH
 
 name="xend"
 rcvar=$name
-command="${SBINDIR}/xend"
-command_args="start"
-command_interpreter=`head -n 1 ${command} | awk '{ print substr($0,3) }'`
-sig_stop="SIGKILL"
+start_precmd="xend_precmd"
+start_cmd="xend_startcmd"
+stop_cmd="xend_stop"
+status_cmd="xend_status"
+extra_commands="status"
+required_files="/kern/xen/privcmd"
+
+XENBACKENDD_PIDFILE="/var/run/xenbackendd.pid"
+#XENBACKENDD_DEBUG=1
+
+xend_precmd()
+{
+	mkdir -p /var/run/xend || exit 1
+	mkdir -p /var/run/xend/boot || exit 1
+}
+
+xend_startcmd()
+{
+	printf "Starting xenbackendd.\n"
+
+	XENBACKENDD_ARGS=""
+	if [ -n "${XENBACKENDD_DEBUG}" ]; then
+		XENBACKENDD_ARGS="${XENBACKENDD_ARGS} -d"
+	fi
+
+	${SBINDIR}/xenbackendd ${XENBACKENDD_ARGS}
+
+	printf "Starting xend.\n"
+	${SBINDIR}/xend start >/dev/null 2>&1
+}
+
+xend_stop()
+{
+	printf "Stopping xenbackendd, xend\n"
+	xb_pid=$(check_pidfile ${XENBACKENDD_PIDFILE} ${SBINDIR}/xenbackendd)
+	if test -n "$xb_pid";
+	then
+		kill -${sig_stop:-TERM} $xb_pid
+	fi
+	while pgrep -f ${SBINDIR}/xend >/dev/null 2>&1; do
+		pkill -${sig_stop:-KILL} -f ${SBINDIR}/xend
+	done
+	wait_for_pids $xb_pid
+	rm -f /var/lock/subsys/xend /var/lock/xend /var/run/xenbackendd.pid
+}
+
+xend_status()
+{
+	${SBINDIR}/xend status
+}
 
 load_rc_config $name
 run_rc_command "$1"
+
-- 
1.7.7.5 (Apple Git-26)

Re: [PATCH v2 1/5] tools/build: fix pygrub linking

[Ian Campbell]

On Thu, 2012-07-26 at 20:54 +0100, Roger Pau Monne wrote:
> Prevent creating a symlink to $(DESTDIR)/$(BINDIR) if it is the same
> as $(PRIVATE_BINDIR)
> 
> This fixes NetBSD install, where $(DESTDIR)/$(BINDIR) ==
> $(PRIVATE_BINDIR).

OOI why does netbsd install these internal utilities, which includes
things like libxl-save-helper (which absolutely is no use to call as a
user) in the regular bin dir rather than libexec or some other
acceptable place?

I'm sure there are some things which are wrongly in PRIVATE_BINDIR
instead of BINDIR but the right answer there would be to move them.

> Changes since v1:
> 
>  * Do the check in shell instead of Makefile.
> 
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Christoph Egger <Christoph.Egger@amd.com>
> Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
>  tools/pygrub/Makefile |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/tools/pygrub/Makefile b/tools/pygrub/Makefile
> index bd22dd4..8c99e11 100644
> --- a/tools/pygrub/Makefile
> +++ b/tools/pygrub/Makefile
> @@ -14,7 +14,10 @@ install: all
>  		$(PYTHON_PREFIX_ARG) --root="$(DESTDIR)" \
>  		--install-scripts=$(PRIVATE_BINDIR) --force
>  	$(INSTALL_DIR) $(DESTDIR)/var/run/xend/boot
> -	ln -sf $(PRIVATE_BINDIR)/pygrub $(DESTDIR)/$(BINDIR)
> +	set -e; if [ `readlink -f $(DESTDIR)/$(BINDIR)` != \
> +	             `readlink -f $(PRIVATE_BINDIR)` ]; then \
> +	    ln -sf $(PRIVATE_BINDIR)/pygrub $(DESTDIR)/$(BINDIR); \
> +	fi
>  
>  .PHONY: clean
>  clean:

Re: [PATCH v2 2/5] libxl: react correctly to POLLHUP

[Ian Campbell]

On Thu, 2012-07-26 at 20:54 +0100, Roger Pau Monne wrote:
> When received POLLHUP on datacopier_readable/writable, kill the
> datacopier and call the callback with onwrite=-2. On the bootloader
> callback kill the bootloader process and wait for the callback, but
> without setting the bl->rc error code.
> 
> This is because NetBSD returns POLLHUP when the process on the other
> end of the pty exits (even when exiting normally).
> 
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
> ---
>  tools/libxl/libxl_aoutils.c    |   16 ++++++++++++++++
>  tools/libxl/libxl_bootloader.c |    9 ++++++---
>  tools/libxl/libxl_internal.h   |    1 +
>  3 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
> index 99972a2..2ea9b2c 100644
> --- a/tools/libxl/libxl_aoutils.c
> +++ b/tools/libxl/libxl_aoutils.c
> @@ -102,6 +102,14 @@ static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev,
>      libxl__datacopier_state *dc = CONTAINER_OF(ev, *dc, toread);
>      STATE_AO_GC(dc->ao);
>  
> +    if (revents & POLLHUP) {
> +        LOG(DEBUG, "received POLLHUP on %s during copy of %s, "
> +                   "killing the bootloader process",

The datacopier module is completely independent from the bootloader one,
so mentioning bootloader in this message is inappropriate. Especially
since nothing in the immediate vicinity of this code is actually killing
anything.

I'll leave commenting on the semantics of treating POLLHUP this way for
Ian to think about.

Re: [PATCH v2 4/5] libxl: call hotplug scripts from xl for NetBSD

[Christoph Egger]

On 07/26/12 21:54, Roger Pau Monne wrote:

> Add the missing NetBSD functions to call hotplug scripts, and disable
> xenbackendd if libxl/disable_udev is not set.
> 
> Cc: Christoph Egger <Christoph.Egger@amd.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>


Acked-by: Christoph Egger <Christoph.Egger@amd.com>

> Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
> ---
>  tools/libxl/libxl_netbsd.c      |   57 ++++++++++++++++++++++++++++++++++++++-
>  tools/xenbackendd/xenbackendd.c |    8 +++++-
>  2 files changed, 63 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
> index 28cdf21..9587833 100644
> --- a/tools/libxl/libxl_netbsd.c
> +++ b/tools/libxl/libxl_netbsd.c
> @@ -32,10 +32,65 @@ char *libxl__devid_to_localdev(libxl__gc *gc, int devid)
>  }
>  
>  /* Hotplug scripts caller functions */
> +static int libxl__hotplug(libxl__gc *gc, libxl__device *dev, char ***args,
> +                          libxl__device_action action)
> +{
> +    char *be_path = libxl__device_backend_path(gc, dev);
> +    char *script;
> +    int nr = 0, rc = 0, arraysize = 4;
> +
> +    script = libxl__xs_read(gc, XBT_NULL,
> +                            GCSPRINTF("%s/%s", be_path, "script"));
> +    if (!script) {
> +        LOGEV(ERROR, errno, "unable to read script from %s", be_path);
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    GCNEW_ARRAY(*args, arraysize);
> +    (*args)[nr++] = script;
> +    (*args)[nr++] = be_path;
> +    (*args)[nr++] = GCSPRINTF("%d", action == DEVICE_CONNECT ?
> +                                    XenbusStateInitWait : XenbusStateClosed);
> +    (*args)[nr++] = NULL;
> +    assert(nr == arraysize);
> +
> +out:
> +    return rc;
> +}
> +
>  int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
>                                     char ***args, char ***env,
>                                     libxl__device_action action,
>                                     int num_exec)
>  {
> -    return 0;
> +    char *disable_udev = libxl__xs_read(gc, XBT_NULL, DISABLE_UDEV_PATH);
> +    int rc;
> +
> +    /* Check if we have to run hotplug scripts */
> +    if (!disable_udev || num_exec > 0) {
> +        rc = 0;
> +        goto out;
> +    }
> +
> +    switch (dev->backend_kind) {
> +    case LIBXL__DEVICE_KIND_VBD:
> +    case LIBXL__DEVICE_KIND_VIF:
> +        if (num_exec != 0) {
> +            rc = 0;
> +            goto out;
> +        }
> +        rc = libxl__hotplug(gc, dev, args, action);
> +        if (!rc) rc = 1;
> +        break;
> +    default:
> +        /* If no need to execute any hotplug scripts,
> +         * call the callback manually
> +         */
> +        rc = 0;
> +        break;
> +    }
> +
> +out:
> +    return rc;
>  }
> diff --git a/tools/xenbackendd/xenbackendd.c b/tools/xenbackendd/xenbackendd.c
> index 6b5bb8e..5381a2a 100644
> --- a/tools/xenbackendd/xenbackendd.c
> +++ b/tools/xenbackendd/xenbackendd.c
> @@ -33,6 +33,7 @@
>  #define DEVTYPE_UNKNOWN 0
>  #define DEVTYPE_VIF 1
>  #define DEVTYPE_VBD 2
> +#define DISABLE_EXEC "libxl/disable_udev"
>  
>  #define DOMAIN_PATH "/local/domain/0"
>  
> @@ -149,7 +150,7 @@ main(int argc, char * const argv[])
>  	unsigned int num;
>  	char *s;
>  	int state;
> -	char *sstate;
> +	char *sstate, *sdisable;
>  	char *p;
>  	char buf[80];
>  	int type;
> @@ -245,6 +246,10 @@ main(int argc, char * const argv[])
>  			continue;
>  		}
>  
> +		sdisable = xs_read(xs, XBT_NULL, DISABLE_EXEC, 0);
> +		if (sdisable)
> +			goto next1;
> +
>  		if (strlen(vec[XS_WATCH_PATH]) < sizeof("state"))
>  			goto next1;
>  
> @@ -314,6 +319,7 @@ next2:
>  		free(sstate);
>  
>  next1:
> +		free(sdisable);
>  		free(vec);
>  	}
>  



-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [PATCH v2 5/5] init/NetBSD: move xenbackendd to xend init script

[Christoph Egger]

On 07/26/12 21:54, Roger Pau Monne wrote:

> xenbackendd is not needed by the xl toolstack, so move it's launch to
> the xend script.
> 
> We have to iterate until we are sure there are no xend processes left,
> since doing a single pkill usually leaves xend processes running.
> 
> Changes since v1:
> 
>  * Use pgrep and pkill instead of the convoluted shell expression.
> 
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Christoph Egger <Christoph.Egger@amd.com>


Acked-by: Christoph Egger <Christoph.Egger@amd.com>

> Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
> ---
>  tools/hotplug/NetBSD/rc.d/xencommons |   29 ++---------------
>  tools/hotplug/NetBSD/rc.d/xend       |   55 +++++++++++++++++++++++++++++++--
>  2 files changed, 55 insertions(+), 29 deletions(-)
> 
> diff --git a/tools/hotplug/NetBSD/rc.d/xencommons b/tools/hotplug/NetBSD/rc.d/xencommons
> index c0d87bf..fe4c9ac 100644
> --- a/tools/hotplug/NetBSD/rc.d/xencommons
> +++ b/tools/hotplug/NetBSD/rc.d/xencommons
> @@ -29,8 +29,6 @@ XENBACKENDD_PIDFILE="/var/run/xenbackendd.pid"
>  
>  xen_precmd()
>  {
> -	mkdir -p /var/run/xend || exit 1
> -	mkdir -p /var/run/xend/boot || exit 1
>  	mkdir -p /var/run/xenstored || exit 1
>  }
>  
> @@ -46,7 +44,7 @@ xen_startcmd()
>  			XENSTORED_ROOTDIR="/var/lib/xenstored"
>  		fi
>  		rm -f ${XENSTORED_ROOTDIR}/tdb* >/dev/null 2>&1
> -		printf "Starting xenservices: xenstored, xenconsoled, xenbackendd."
> +		printf "Starting xenservices: xenstored, xenconsoled."
>  		XENSTORED_ARGS=" --pid-file ${XENSTORED_PIDFILE}"
>  		if [ -n "${XENSTORED_TRACE}" ]; then
>  			XENSTORED_ARGS="${XENSTORED_ARGS} -T /var/log/xen/xenstored-trace.log"
> @@ -58,7 +56,7 @@ xen_startcmd()
>  			sleep 1
>  		done
>  	else
> -		printf "Starting xenservices: xenconsoled, xenbackendd."
> +		printf "Starting xenservices: xenconsoled."
>  	fi
>  
>  	XENCONSOLED_ARGS=""
> @@ -68,13 +66,6 @@ xen_startcmd()
>  
>  	${SBINDIR}/xenconsoled ${XENCONSOLED_ARGS}
>  
> -	XENBACKENDD_ARGS=""
> -	if [ -n "${XENBACKENDD_DEBUG}" ]; then
> -		XENBACKENDD_ARGS="${XENBACKENDD_ARGS} -d"
> -	fi
> -
> -	${SBINDIR}/xenbackendd ${XENBACKENDD_ARGS}
> -
>  	printf "\n"
>  
>  	printf "Setting domain 0 name.\n"
> @@ -87,8 +78,6 @@ xen_stop()
>  	printf "Stopping xencommons.\n"
>  	printf "WARNING: Not stopping xenstored, as it cannot be restarted.\n"
>  
> -	rc_pid=$(check_pidfile ${XENBACKENDD_PIDFILE} ${SBINDIR}/xenbackendd)
> -	pids="$pids $rc_pid"
>  	rc_pid=$(check_pidfile ${XENCONSOLED_PIDFILE} ${SBINDIR}/xenconsoled)
>  	pids="$pids $rc_pid"
>  
> @@ -108,17 +97,12 @@ xen_status()
>  		pids="$pids $xenconsoled_pid"
>  	fi
>  
> -	xenbackend_pid=$(check_pidfile ${XENBACKENDD_PIDFILE} ${SBINDIR}/xenbackendd)
> -	if test -n ${xenbackend_pid}; then
> -		pids="$pids $xenbackend_pid"
> -	fi
> -
> -	if test -n "$xenbackend_pid" -a -n "$xenconsoled_pid" -a -n "$xenstored_pid";
> +	if test -n "$xenconsoled_pid" -a -n "$xenstored_pid";
>  	then
>  		echo "xencommons are running as pids $pids."
>  		return 0
>  	fi
> -	if test -z "$xenbackend_pid" -a -z "$xenconsoled_pid" -a -z "$xenstored_pid";
> +	if test -z "$xenconsoled_pid" -a -z "$xenstored_pid";
>  	then
>  		echo "xencommons are not running."
>  		return 0
> @@ -134,11 +118,6 @@ xen_status()
>  	else
>  		echo "xenconsoled is not running."
>  	fi
> -	if test -n $xenbackend_pid; then
> -		echo "xenbackendd is running as pid $xenbackend_pid."
> -	else
> -		echo "xenbackendd is not running."
> -	fi
>  }
>  
>  load_rc_config $name
> diff --git a/tools/hotplug/NetBSD/rc.d/xend b/tools/hotplug/NetBSD/rc.d/xend
> index ead9ee0..ac5f2ca 100644
> --- a/tools/hotplug/NetBSD/rc.d/xend
> +++ b/tools/hotplug/NetBSD/rc.d/xend
> @@ -15,10 +15,57 @@ export PATH
>  
>  name="xend"
>  rcvar=$name
> -command="${SBINDIR}/xend"
> -command_args="start"
> -command_interpreter=`head -n 1 ${command} | awk '{ print substr($0,3) }'`
> -sig_stop="SIGKILL"
> +start_precmd="xend_precmd"
> +start_cmd="xend_startcmd"
> +stop_cmd="xend_stop"
> +status_cmd="xend_status"
> +extra_commands="status"
> +required_files="/kern/xen/privcmd"
> +
> +XENBACKENDD_PIDFILE="/var/run/xenbackendd.pid"
> +#XENBACKENDD_DEBUG=1
> +
> +xend_precmd()
> +{
> +	mkdir -p /var/run/xend || exit 1
> +	mkdir -p /var/run/xend/boot || exit 1
> +}
> +
> +xend_startcmd()
> +{
> +	printf "Starting xenbackendd.\n"
> +
> +	XENBACKENDD_ARGS=""
> +	if [ -n "${XENBACKENDD_DEBUG}" ]; then
> +		XENBACKENDD_ARGS="${XENBACKENDD_ARGS} -d"
> +	fi
> +
> +	${SBINDIR}/xenbackendd ${XENBACKENDD_ARGS}
> +
> +	printf "Starting xend.\n"
> +	${SBINDIR}/xend start >/dev/null 2>&1
> +}
> +
> +xend_stop()
> +{
> +	printf "Stopping xenbackendd, xend\n"
> +	xb_pid=$(check_pidfile ${XENBACKENDD_PIDFILE} ${SBINDIR}/xenbackendd)
> +	if test -n "$xb_pid";
> +	then
> +		kill -${sig_stop:-TERM} $xb_pid
> +	fi
> +	while pgrep -f ${SBINDIR}/xend >/dev/null 2>&1; do
> +		pkill -${sig_stop:-KILL} -f ${SBINDIR}/xend
> +	done
> +	wait_for_pids $xb_pid
> +	rm -f /var/lock/subsys/xend /var/lock/xend /var/run/xenbackendd.pid
> +}
> +
> +xend_status()
> +{
> +	${SBINDIR}/xend status
> +}
>  
>  load_rc_config $name
>  run_rc_command "$1"
> +



-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [PATCH v2 2/5] libxl: react correctly to POLLHUP

[Ian Jackson]

Roger Pau Monne writes ("[PATCH v2 2/5] libxl: react correctly to POLLHUP"):
> When received POLLHUP on datacopier_readable/writable, kill the
> datacopier and call the callback with onwrite=-2. On the bootloader
> callback kill the bootloader process and wait for the callback, but
> without setting the bl->rc error code.
...
> +    if (revents & POLLHUP) {
> +        LOG(DEBUG, "received POLLHUP on %s during copy of %s, "
> +                   "killing the bootloader process",
> +                   dc->readwhat, dc->copywhat);

The datacopier logic is correct here as far as it goes I think, but as
Ian says, the message is wrong.

However I think you need also to check for POLLHUP on reading the
pty.  There are four fd events registered:
  - reading bootloader pty master, POLLHUP expected
  - writing bootloader pty master, POLLHUP expected
  - reading xenconsole client pty master, POLLHUP not expected
  - writing xenconsole client pty master, POLLHUP not expected
(The writing events may not always be registered.)

When the POLLHUP occurs on the bootloader pty master it should be
handled the same way whether we first notice it with respect to the
reading or writing event.  (These correspond to different poll slots
and there is no particular reason why it we would process one of these
before the other - although of course we the writing one is only
present if we have data to write.)

I'm assuming that libxl doesn't get a POLLHUP on the xenconsole pty
master when the client disconnects, because (a) if it did that would
be impossible for libxl to handle properly and (b) xenconsoled doesn't
have any code for this - although it uses select so it's not 100%
analogous.  So in this case I think the POLLHUP should be an error, as
before.


So having thought about it this way, I don't think this interface:

> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -2073,6 +2073,7 @@ typedef struct libxl__datacopier_buf libxl__datacopier_buf;
>   *     errnoval==0 means we got eof and all data was written
>   *     errnoval!=0 means we had a read error, logged
>   * onwrite==-1 means some other internal failure, errnoval not valid, logged
> + * onwrite==-2 means we got a POLLHUP, errnoval not valid, logged
>   * in all cases copier is killed before calling this callback */
>  typedef void libxl__datacopier_callback(libxl__egc *egc,
>       libxl__datacopier_state *dc, int onwrite, int errnoval);

is suitable.  We need to be able to distinguish POLLHUP on the writing
fd from POLLHUP on the reading fd.  That means that you shouldn't
abuse "onwrite" like this.

I can see two possible options:

1. Since we have "int errnoval" and errno values are always positive
you could signal this with a specific -ve value for errnoval.  That
is, call datacopier's callers must now be prepared to deal with this
`magic' value of errnoval which is not a proper errno value.  That is
going to be annoying for other users who will need special error
handling behaviour.

2. Provide a new callback function pointer for POLLHUP:
    struct libxl__datacopier_state {
        ...
        libxl__datacopier_callback *callback;
        libxl__datacopier_callback *callback_pollhup;
        ...
    }
with something like the following spec:
   /* If callback_pollhup!=NULL, POLLHUP will be reported
    * by calling callback_pollhup->(..., onwrite, -1).
    * If callback_pollhup==NULL, POLLHUP will be reported
    * as an internal failure.
    * in all cases copier is killed before calling these callbacks */
and make sure that the other users have callback_pollhup==0.


Thanks,
Ian.

Re: [PATCH v2 2/5] libxl: react correctly to POLLHUP

[Ian Jackson]

Roger Pau Monne writes ("[PATCH v2 2/5] libxl: react correctly to POLLHUP"):
> When received POLLHUP on datacopier_readable/writable, kill the
> datacopier and call the callback with onwrite=-2. On the bootloader
> callback kill the bootloader process and wait for the callback, but
> without setting the bl->rc error code.
...al)
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 3ee3a09..286aa19 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -2073,6 +2073,7 @@ typedef struct libxl__datacopier_buf libxl__datacopier_buf;
>   *     errnoval==0 means we got eof and all data was written
>   *     errnoval!=0 means we had a read error, logged
>   * onwrite==-1 means some other internal failure, errnoval not valid, logged
> + * onwrite==-2 means we got a POLLHUP, errnoval not valid, logged
>   * in all cases copier is killed before calling this callback */
>  typedef void libxl__datacopier_callback(libxl__egc *egc,
>       libxl__datacopier_state *dc, int onwrite, int errnoval);

Also, when you added this you obviously didn't search for callers
which needed to be updated, or you would have found one.

Ian.

Re: [PATCH v2 3/5] hotplug/NetBSD: check type of file to attach from params

[Ian Jackson]

Roger Pau Monne writes ("[PATCH v2 3/5] hotplug/NetBSD: check type of file to attach from params"):
> xend used to set the xenbus backend entry "type" to either "phy" or
> "file", but now libxl sets it to "phy" for both file and block device.
> We have to manually check for the type of the "param" filed in order
> to detect if we are trying to attach a file or a block device.
...
> diff --git a/tools/hotplug/NetBSD/block b/tools/hotplug/NetBSD/block
> index cf5ff3a..31d9998 100644
> --- a/tools/hotplug/NetBSD/block
> +++ b/tools/hotplug/NetBSD/block
> @@ -19,8 +19,14 @@ error() {
>  
>  xpath=$1
>  xstatus=$2
> -xtype=$(xenstore-read "$xpath/type")
>  xparams=$(xenstore-read "$xpath/params")
> +if [ -b $xparams ]; then
> +	xtype="phy"
> +elif [ -f $xparams ]; then
> +	xtype="file"
> +else
> +	error "invalid file type"
> +fi

This seems to be likely to print strange messages as well as the
intended message if xparams=''.  And you should print the type you are
complaining about in the error message.

Ian.

Re: [PATCH v2 5/5] init/NetBSD: move xenbackendd to xend init script

[Ian Jackson]

Christoph Egger writes ("Re: [PATCH v2 5/5] init/NetBSD: move xenbackendd to xend init script"):
> On 07/26/12 21:54, Roger Pau Monne wrote:
> 
> > xenbackendd is not needed by the xl toolstack, so move it's launch to
> > the xend script.
> > 
> > We have to iterate until we are sure there are no xend processes left,
> > since doing a single pkill usually leaves xend processes running.
> > 
> > Changes since v1:
> > 
> >  * Use pgrep and pkill instead of the convoluted shell expression.
> > 
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Christoph Egger <Christoph.Egger@amd.com>
> 
> 
> Acked-by: Christoph Egger <Christoph.Egger@amd.com>

Thanks for the review.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [PATCH v2 5/5] init/NetBSD: move xenbackendd to xend init script

[Christoph Egger]

On 07/27/12 16:29, Ian Jackson wrote:

> Christoph Egger writes ("Re: [PATCH v2 5/5] init/NetBSD: move xenbackendd to xend init script"):
>> On 07/26/12 21:54, Roger Pau Monne wrote:
>>
>>> xenbackendd is not needed by the xl toolstack, so move it's launch to
>>> the xend script.
>>>
>>> We have to iterate until we are sure there are no xend processes left,
>>> since doing a single pkill usually leaves xend processes running.
>>>
>>> Changes since v1:
>>>
>>>  * Use pgrep and pkill instead of the convoluted shell expression.
>>>
>>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>>> Cc: Christoph Egger <Christoph.Egger@amd.com>
>>
>>
>> Acked-by: Christoph Egger <Christoph.Egger@amd.com>
> 
> Thanks for the review.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>


I can also give a

Tested-by: Christoph Egger <Christoph.Egger@amd.com>

 



-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [PATCH v2 2/5] libxl: react correctly to POLLHUP

[Ian Jackson]

Ian Jackson writes ("Re: [PATCH v2 2/5] libxl: react correctly to POLLHUP"):
> 2. Provide a new callback function pointer for POLLHUP:
>     struct libxl__datacopier_state {
>         ...
>         libxl__datacopier_callback *callback;
>         libxl__datacopier_callback *callback_pollhup;
>         ...
>     }

I have done this.  The result (untested, but compiles) is below.  This
should not be applied until someone has been able to test it on NetBSD.

From: Ian Jackson <ian.jackson@eu.citrix.com>
Subject: [PATCH v3] libxl: react correctly to bootloader pty master POLLHUP

Receive POLLHUP on the bootloader master pty is not an error.
Hopefully it means that the bootloader has exited and therefore the
pty slave side has no process group any more.  (At least NetBSD
indicates POLLHUP on the master in this case.)

So send the bootloader SIGKILL; if it has already exited then this has
no effect (except that on some versions of NetBSD it erroneously
returns ESRCH and we print a harmless warning) and we will then
collect the bootloader's exit status and be satisfied.

In order to implement this we need to provide a way for users of
datacopier to handle POLLHUP rather than treating it as fatal.

We rename bootloader_abort to bootloader_stop since it now no longer
only applies to error situations.

Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

-
Changes in v3:
 * datacopier provides new interface for handling POLLHUP
 * Do not ignore errors on the xenconsole pty
 * Rename bootloader_abort.

---
 tools/libxl/libxl_aoutils.c    |   23 +++++++++++++++++++++++
 tools/libxl/libxl_bootloader.c |   34 ++++++++++++++++++++++++----------
 tools/libxl/libxl_internal.h   |    5 ++++-
 3 files changed, 51 insertions(+), 11 deletions(-)

diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
index 99972a2..4bd5484 100644
--- a/tools/libxl/libxl_aoutils.c
+++ b/tools/libxl/libxl_aoutils.c
@@ -97,11 +97,31 @@ void libxl__datacopier_prefixdata(libxl__egc *egc, libxl__datacopier_state *dc,
     LIBXL_TAILQ_INSERT_TAIL(&dc->bufs, buf, entry);
 }
 
+static int datacopier_pollhup_handled(libxl__egc *egc,
+                                      libxl__datacopier_state *dc,
+                                      short revents, int onwrite)
+{
+    STATE_AO_GC(dc->ao);
+
+    if (dc->callback_pollhup && (revents & POLLHUP)) {
+        LOG(DEBUG, "received POLLHUP on %s during copy of %s",
+            onwrite ? dc->writewhat : dc->readwhat,
+            dc->copywhat);
+        libxl__datacopier_kill(dc);
+        dc->callback(egc, dc, onwrite, -1);
+        return 1;
+    }
+    return 0;
+}
+
 static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev,
                                 int fd, short events, short revents) {
     libxl__datacopier_state *dc = CONTAINER_OF(ev, *dc, toread);
     STATE_AO_GC(dc->ao);
 
+    if (datacopier_pollhup_handled(egc, dc, revents, 0))
+        return;
+
     if (revents & ~POLLIN) {
         LOG(ERROR, "unexpected poll event 0x%x (should be POLLIN)"
             " on %s during copy of %s", revents, dc->readwhat, dc->copywhat);
@@ -163,6 +183,9 @@ static void datacopier_writable(libxl__egc *egc, libxl__ev_fd *ev,
     libxl__datacopier_state *dc = CONTAINER_OF(ev, *dc, towrite);
     STATE_AO_GC(dc->ao);
 
+    if (datacopier_pollhup_handled(egc, dc, revents, 1))
+        return;
+
     if (revents & ~POLLOUT) {
         LOG(ERROR, "unexpected poll event 0x%x (should be POLLOUT)"
             " on %s during copy of %s", revents, dc->writewhat, dc->copywhat);
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index ef5a91b..9e9a14a 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -275,7 +275,7 @@ static void bootloader_local_detached_cb(libxl__egc *egc,
 }
 
 /* might be called at any time, provided it's init'd */
-static void bootloader_abort(libxl__egc *egc,
+static void bootloader_stop(libxl__egc *egc,
                              libxl__bootloader_state *bl, int rc)
 {
     STATE_AO_GC(bl->ao);
@@ -285,8 +285,8 @@ static void bootloader_abort(libxl__egc *egc,
     libxl__datacopier_kill(&bl->display);
     if (libxl__ev_child_inuse(&bl->child)) {
         r = kill(bl->child.pid, SIGTERM);
-        if (r) LOGE(WARN, "after failure, failed to kill bootloader [%lu]",
-                    (unsigned long)bl->child.pid);
+        if (r) LOGE(WARN, "%sfailed to kill bootloader [%lu]",
+                    rc ? "after failure, " : "", (unsigned long)bl->child.pid);
     }
     bl->rc = rc;
 }
@@ -508,7 +508,10 @@ static void bootloader_gotptys(libxl__egc *egc, libxl__openpty_state *op)
     bl->keystrokes.maxsz = BOOTLOADER_BUF_OUT;
     bl->keystrokes.copywhat =
         GCSPRINTF("bootloader input for domain %"PRIu32, bl->domid);
-    bl->keystrokes.callback = bootloader_keystrokes_copyfail;
+    bl->keystrokes.callback =         bootloader_keystrokes_copyfail;
+    bl->keystrokes.callback_pollhup = bootloader_keystrokes_copyfail;
+        /* pollhup gets called with errnoval==-1 which is not otherwise
+         * possible since errnos are nonnegative, so it's unambiguous */
     rc = libxl__datacopier_start(&bl->keystrokes);
     if (rc) goto out;
 
@@ -516,7 +519,8 @@ static void bootloader_gotptys(libxl__egc *egc, libxl__openpty_state *op)
     bl->display.maxsz = BOOTLOADER_BUF_IN;
     bl->display.copywhat =
         GCSPRINTF("bootloader output for domain %"PRIu32, bl->domid);
-    bl->display.callback = bootloader_display_copyfail;
+    bl->display.callback =         bootloader_display_copyfail;
+    bl->display.callback_pollhup = bootloader_display_copyfail;
     rc = libxl__datacopier_start(&bl->display);
     if (rc) goto out;
 
@@ -562,30 +566,40 @@ static void bootloader_gotptys(libxl__egc *egc, libxl__openpty_state *op)
 
 /* perhaps one of these will be called, but perhaps not */
 static void bootloader_copyfail(libxl__egc *egc, const char *which,
-       libxl__bootloader_state *bl, int onwrite, int errnoval)
+        libxl__bootloader_state *bl, int ondisplay, int onwrite, int errnoval)
 {
     STATE_AO_GC(bl->ao);
+    int rc = ERROR_FAIL;
+
+    if (errnoval==-1) {
+        /* POLLHUP */
+        if (!!ondisplay != !!onwrite)
+            rc = 0;
+        else
+            LOG(ERROR, "unexpected POLLHUP on %s", which);
+    }
     if (!onwrite && !errnoval)
         LOG(ERROR, "unexpected eof copying %s", which);
-    bootloader_abort(egc, bl, ERROR_FAIL);
+
+    bootloader_stop(egc, bl, rc);
 }
 static void bootloader_keystrokes_copyfail(libxl__egc *egc,
        libxl__datacopier_state *dc, int onwrite, int errnoval)
 {
     libxl__bootloader_state *bl = CONTAINER_OF(dc, *bl, keystrokes);
-    bootloader_copyfail(egc, "bootloader input", bl, onwrite, errnoval);
+    bootloader_copyfail(egc, "bootloader input", bl, 0, onwrite, errnoval);
 }
 static void bootloader_display_copyfail(libxl__egc *egc,
        libxl__datacopier_state *dc, int onwrite, int errnoval)
 {
     libxl__bootloader_state *bl = CONTAINER_OF(dc, *bl, display);
-    bootloader_copyfail(egc, "bootloader output", bl, onwrite, errnoval);
+    bootloader_copyfail(egc, "bootloader output", bl, 1, onwrite, errnoval);
 }
 
 static void bootloader_domaindeath(libxl__egc *egc, libxl__domaindeathcheck *dc)
 {
     libxl__bootloader_state *bl = CONTAINER_OF(dc, *bl, deathcheck);
-    bootloader_abort(egc, bl, ERROR_FAIL);
+    bootloader_stop(egc, bl, ERROR_FAIL);
 }
 
 static void bootloader_finished(libxl__egc *egc, libxl__ev_child *child,
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 691b4f6..f0c94e8 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2070,7 +2070,9 @@ typedef struct libxl__datacopier_buf libxl__datacopier_buf;
  *     errnoval==0 means we got eof and all data was written
  *     errnoval!=0 means we had a read error, logged
  * onwrite==-1 means some other internal failure, errnoval not valid, logged
- * in all cases copier is killed before calling this callback */
+ * If we get POLLHUP, we call callback_pollhup(..., onwrite, -1);
+ * or if callback_pollhup==0 this is an internal failure, as above.
+ * In all cases copier is killed before calling this callback */
 typedef void libxl__datacopier_callback(libxl__egc *egc,
      libxl__datacopier_state *dc, int onwrite, int errnoval);
 
@@ -2089,6 +2091,7 @@ struct libxl__datacopier_state {
     const char *copywhat, *readwhat, *writewhat; /* for error msgs */
     FILE *log; /* gets a copy of everything */
     libxl__datacopier_callback *callback;
+    libxl__datacopier_callback *callback_pollhup;
     /* remaining fields are private to datacopier */
     libxl__ev_fd toread, towrite;
     ssize_t used;
-- 
tg: (bbf53f4..) t/xen/xl.pty-pollhup (depends on: t/xen/gitignore)

Re: [PATCH v2 2/5] libxl: react correctly to POLLHUP

[Ian Campbell]

On Fri, 2012-07-27 at 18:01 +0100, Ian Jackson wrote:
> Ian Jackson writes ("Re: [PATCH v2 2/5] libxl: react correctly to POLLHUP"):
> > 2. Provide a new callback function pointer for POLLHUP:
> >     struct libxl__datacopier_state {
> >         ...
> >         libxl__datacopier_callback *callback;
> >         libxl__datacopier_callback *callback_pollhup;
> >         ...
> >     }
> 
> I have done this.  The result (untested, but compiles) is below.  This
> should not be applied until someone has been able to test it on NetBSD.
> 
> From: Ian Jackson <ian.jackson@eu.citrix.com>
> Subject: [PATCH v3] libxl: react correctly to bootloader pty master POLLHUP
> 
> Receive POLLHUP on the bootloader master pty is not an error.
> Hopefully it means that the bootloader has exited and therefore the
> pty slave side has no process group any more.  (At least NetBSD
> indicates POLLHUP on the master in this case.)
> 
> So send the bootloader SIGKILL; if it has already exited then this has
> no effect (except that on some versions of NetBSD it erroneously
> returns ESRCH and we print a harmless warning) and we will then
> collect the bootloader's exit status and be satisfied.
> 
> In order to implement this we need to provide a way for users of
> datacopier to handle POLLHUP rather than treating it as fatal.
> 
> We rename bootloader_abort to bootloader_stop since it now no longer
> only applies to error situations.
> 
> Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> -
> Changes in v3:
>  * datacopier provides new interface for handling POLLHUP
>  * Do not ignore errors on the xenconsole pty
>  * Rename bootloader_abort.
> 
> ---
>  tools/libxl/libxl_aoutils.c    |   23 +++++++++++++++++++++++
>  tools/libxl/libxl_bootloader.c |   34 ++++++++++++++++++++++++----------
>  tools/libxl/libxl_internal.h   |    5 ++++-
>  3 files changed, 51 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
> index 99972a2..4bd5484 100644
> --- a/tools/libxl/libxl_aoutils.c
> +++ b/tools/libxl/libxl_aoutils.c
> @@ -97,11 +97,31 @@ void libxl__datacopier_prefixdata(libxl__egc *egc, libxl__datacopier_state *dc,
>      LIBXL_TAILQ_INSERT_TAIL(&dc->bufs, buf, entry);
>  }
>  
> +static int datacopier_pollhup_handled(libxl__egc *egc,
> +                                      libxl__datacopier_state *dc,
> +                                      short revents, int onwrite)
> +{
> +    STATE_AO_GC(dc->ao);
> +
> +    if (dc->callback_pollhup && (revents & POLLHUP)) {
> +        LOG(DEBUG, "received POLLHUP on %s during copy of %s",
> +            onwrite ? dc->writewhat : dc->readwhat,
> +            dc->copywhat);
> +        libxl__datacopier_kill(dc);
> +        dc->callback(egc, dc, onwrite, -1);

Shouldn't this be dc->callback_poolhup?

> @@ -508,7 +508,10 @@ static void bootloader_gotptys(libxl__egc *egc, libxl__openpty_state *op)
>      bl->keystrokes.maxsz = BOOTLOADER_BUF_OUT;
>      bl->keystrokes.copywhat =
>          GCSPRINTF("bootloader input for domain %"PRIu32, bl->domid);
> -    bl->keystrokes.callback = bootloader_keystrokes_copyfail;
> +    bl->keystrokes.callback =         bootloader_keystrokes_copyfail;
> +    bl->keystrokes.callback_pollhup = bootloader_keystrokes_copyfail;
> +        /* pollhup gets called with errnoval==-1 which is not otherwise
> +         * possible since errnos are nonnegative, so it's unambiguous */

So the split into two callbacks is just for convenience of some future
user, rather than something needed in this patch? Or have I missed
somewhere which sets callback but not callback_pollhup?

Ian.

Re: [PATCH v2 2/5] libxl: react correctly to POLLHUP

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH v2 2/5] libxl: react correctly to POLLHUP"):
> On Fri, 2012-07-27 at 18:01 +0100, Ian Jackson wrote:

> > +static int datacopier_pollhup_handled(libxl__egc *egc,
> > +                                      libxl__datacopier_state *dc,
> > +                                      short revents, int onwrite)
> > +{
> > +    STATE_AO_GC(dc->ao);
> > +
> > +    if (dc->callback_pollhup && (revents & POLLHUP)) {
> > +        LOG(DEBUG, "received POLLHUP on %s during copy of %s",
> > +            onwrite ? dc->writewhat : dc->readwhat,
> > +            dc->copywhat);
> > +        libxl__datacopier_kill(dc);
> > +        dc->callback(egc, dc, onwrite, -1);
> 
> Shouldn't this be dc->callback_poolhup?

Yes.  I did say I hadn't executed it :-).

> > @@ -508,7 +508,10 @@ static void bootloader_gotptys(libxl__egc *egc, libxl__openpty_state *op)
> >      bl->keystrokes.maxsz = BOOTLOADER_BUF_OUT;
> >      bl->keystrokes.copywhat =
> >          GCSPRINTF("bootloader input for domain %"PRIu32, bl->domid);
> > -    bl->keystrokes.callback = bootloader_keystrokes_copyfail;
> > +    bl->keystrokes.callback =         bootloader_keystrokes_copyfail;
> > +    bl->keystrokes.callback_pollhup = bootloader_keystrokes_copyfail;
> > +        /* pollhup gets called with errnoval==-1 which is not otherwise
> > +         * possible since errnos are nonnegative, so it's unambiguous */
> 
> So the split into two callbacks is just for convenience of some future
> user, rather than something needed in this patch? Or have I missed
> somewhere which sets callback but not callback_pollhup?

Yes, you have.  datacopier is used for the qemu state file during
save/restore, and naturally wants to treat pollhup as fatal.

Ian.

Re: [PATCH v2 5/5] init/NetBSD: move xenbackendd to xend init script

[Ian Campbell]

On Fri, 2012-07-27 at 15:49 +0100, Christoph Egger wrote:
> On 07/27/12 16:29, Ian Jackson wrote:
> 
> > Christoph Egger writes ("Re: [PATCH v2 5/5] init/NetBSD: move xenbackendd to xend init script"):
> >> On 07/26/12 21:54, Roger Pau Monne wrote:
> >>
> >>> xenbackendd is not needed by the xl toolstack, so move it's launch to
> >>> the xend script.
> >>>
> >>> We have to iterate until we are sure there are no xend processes left,
> >>> since doing a single pkill usually leaves xend processes running.
> >>>
> >>> Changes since v1:
> >>>
> >>>  * Use pgrep and pkill instead of the convoluted shell expression.
> >>>
> >>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> >>> Cc: Christoph Egger <Christoph.Egger@amd.com>
> >>
> >>
> >> Acked-by: Christoph Egger <Christoph.Egger@amd.com>
> > 
> > Thanks for the review.
> > 
> > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> 
> I can also give a
> 
> Tested-by: Christoph Egger <Christoph.Egger@amd.com>

Applied, thanks everyone.

> 
>  
> 
> 
> 

Re: [PATCH v2 4/5] libxl: call hotplug scripts from xl for NetBSD

[Ian Campbell]

On Fri, 2012-07-27 at 13:49 +0100, Christoph Egger wrote:
> On 07/26/12 21:54, Roger Pau Monne wrote:
> 
> > Add the missing NetBSD functions to call hotplug scripts, and disable
> > xenbackendd if libxl/disable_udev is not set.
> > 
> > Cc: Christoph Egger <Christoph.Egger@amd.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> 
> Acked-by: Christoph Egger <Christoph.Egger@amd.com>

Applied, thanks.

Re: [PATCH v2 1/5] tools/build: fix pygrub linking

[Ian Campbell]

On Fri, 2012-07-27 at 09:48 +0100, Ian Campbell wrote:
> On Thu, 2012-07-26 at 20:54 +0100, Roger Pau Monne wrote:
> > Prevent creating a symlink to $(DESTDIR)/$(BINDIR) if it is the same
> > as $(PRIVATE_BINDIR)
> > 
> > This fixes NetBSD install, where $(DESTDIR)/$(BINDIR) ==
> > $(PRIVATE_BINDIR).
> 
> [...]
> > Changes since v1:
> > 
> >  * Do the check in shell instead of Makefile.
> > 
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Christoph Egger <Christoph.Egger@amd.com>
> > Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Applied, thanks.

> 
> > ---
> >  tools/pygrub/Makefile |    5 ++++-
> >  1 files changed, 4 insertions(+), 1 deletions(-)
> > 
> > diff --git a/tools/pygrub/Makefile b/tools/pygrub/Makefile
> > index bd22dd4..8c99e11 100644
> > --- a/tools/pygrub/Makefile
> > +++ b/tools/pygrub/Makefile
> > @@ -14,7 +14,10 @@ install: all
> >  		$(PYTHON_PREFIX_ARG) --root="$(DESTDIR)" \
> >  		--install-scripts=$(PRIVATE_BINDIR) --force
> >  	$(INSTALL_DIR) $(DESTDIR)/var/run/xend/boot
> > -	ln -sf $(PRIVATE_BINDIR)/pygrub $(DESTDIR)/$(BINDIR)
> > +	set -e; if [ `readlink -f $(DESTDIR)/$(BINDIR)` != \
> > +	             `readlink -f $(PRIVATE_BINDIR)` ]; then \
> > +	    ln -sf $(PRIVATE_BINDIR)/pygrub $(DESTDIR)/$(BINDIR); \
> > +	fi
> >  
> >  .PHONY: clean
> >  clean:
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH v2 1/5] tools/build: fix pygrub linking

[Olaf Hering]

On Wed, Aug 01, Ian Campbell wrote:

> > > diff --git a/tools/pygrub/Makefile b/tools/pygrub/Makefile
> > > index bd22dd4..8c99e11 100644
> > > --- a/tools/pygrub/Makefile
> > > +++ b/tools/pygrub/Makefile
> > > @@ -14,7 +14,10 @@ install: all
> > >  		$(PYTHON_PREFIX_ARG) --root="$(DESTDIR)" \
> > >  		--install-scripts=$(PRIVATE_BINDIR) --force
> > >  	$(INSTALL_DIR) $(DESTDIR)/var/run/xend/boot
> > > -	ln -sf $(PRIVATE_BINDIR)/pygrub $(DESTDIR)/$(BINDIR)
> > > +	set -e; if [ `readlink -f $(DESTDIR)/$(BINDIR)` != \
> > > +	             `readlink -f $(PRIVATE_BINDIR)` ]; then \
> > > +	    ln -sf $(PRIVATE_BINDIR)/pygrub $(DESTDIR)/$(BINDIR); \
> > > +	fi

This needs quoting for the shell, like shown below:

[  148s] set -e; if [ `readlink -f /home/abuild/rpmbuild/BUILD/xen-4.2.25700/non-dbg/dist/install//usr/bin` != \
[  148s]              `readlink -f /usr/lib64/xen/bin` ]; then \
[  148s]     ln -sf /usr/lib64/xen/bin/pygrub /home/abuild/rpmbuild/BUILD/xen-4.2.25700/non-dbg/dist/install//usr/bin; \
[  148s] fi
[  148s] /bin/sh: line 0: [: /home/abuild/rpmbuild/BUILD/xen-4.2.25700/non-dbg/dist/install/usr/bin: unary operator expected

diff -r 3d622e2c7cfb tools/pygrub/Makefile
--- a/tools/pygrub/Makefile
+++ b/tools/pygrub/Makefile
@@ -14,8 +14,8 @@ install: all
 		$(PYTHON_PREFIX_ARG) --root="$(DESTDIR)" \
 		--install-scripts=$(PRIVATE_BINDIR) --force
 	$(INSTALL_DIR) $(DESTDIR)/var/run/xend/boot
-	set -e; if [ `readlink -f $(DESTDIR)/$(BINDIR)` != \
-	             `readlink -f $(PRIVATE_BINDIR)` ]; then \
+	set -e; if [ "`readlink -f $(DESTDIR)/$(BINDIR)`" != \
+	             "`readlink -f $(PRIVATE_BINDIR)`" ]; then \
 	    ln -sf $(PRIVATE_BINDIR)/pygrub $(DESTDIR)/$(BINDIR); \
 	fi
 
Olaf

Re: [PATCH v2 1/5] tools/build: fix pygrub linking

[Ian Campbell]

On Thu, 2012-08-02 at 06:42 +0100, Olaf Hering wrote:
> On Wed, Aug 01, Ian Campbell wrote:
> 
> > > > diff --git a/tools/pygrub/Makefile b/tools/pygrub/Makefile
> > > > index bd22dd4..8c99e11 100644
> > > > --- a/tools/pygrub/Makefile
> > > > +++ b/tools/pygrub/Makefile
> > > > @@ -14,7 +14,10 @@ install: all
> > > >  		$(PYTHON_PREFIX_ARG) --root="$(DESTDIR)" \
> > > >  		--install-scripts=$(PRIVATE_BINDIR) --force
> > > >  	$(INSTALL_DIR) $(DESTDIR)/var/run/xend/boot
> > > > -	ln -sf $(PRIVATE_BINDIR)/pygrub $(DESTDIR)/$(BINDIR)
> > > > +	set -e; if [ `readlink -f $(DESTDIR)/$(BINDIR)` != \
> > > > +	             `readlink -f $(PRIVATE_BINDIR)` ]; then \
> > > > +	    ln -sf $(PRIVATE_BINDIR)/pygrub $(DESTDIR)/$(BINDIR); \
> > > > +	fi
> 
> This needs quoting for the shell, like shown below:

Right, thanks.

Can you provide a S-o-b and I'll fabricate up a changelog as I commit
it.

> 
> [  148s] set -e; if [ `readlink -f /home/abuild/rpmbuild/BUILD/xen-4.2.25700/non-dbg/dist/install//usr/bin` != \
> [  148s]              `readlink -f /usr/lib64/xen/bin` ]; then \
> [  148s]     ln -sf /usr/lib64/xen/bin/pygrub /home/abuild/rpmbuild/BUILD/xen-4.2.25700/non-dbg/dist/install//usr/bin; \
> [  148s] fi
> [  148s] /bin/sh: line 0: [: /home/abuild/rpmbuild/BUILD/xen-4.2.25700/non-dbg/dist/install/usr/bin: unary operator expected
> 
> diff -r 3d622e2c7cfb tools/pygrub/Makefile
> --- a/tools/pygrub/Makefile
> +++ b/tools/pygrub/Makefile
> @@ -14,8 +14,8 @@ install: all
>  		$(PYTHON_PREFIX_ARG) --root="$(DESTDIR)" \
>  		--install-scripts=$(PRIVATE_BINDIR) --force
>  	$(INSTALL_DIR) $(DESTDIR)/var/run/xend/boot
> -	set -e; if [ `readlink -f $(DESTDIR)/$(BINDIR)` != \
> -	             `readlink -f $(PRIVATE_BINDIR)` ]; then \
> +	set -e; if [ "`readlink -f $(DESTDIR)/$(BINDIR)`" != \
> +	             "`readlink -f $(PRIVATE_BINDIR)`" ]; then \
>  	    ln -sf $(PRIVATE_BINDIR)/pygrub $(DESTDIR)/$(BINDIR); \
>  	fi
>  
> Olaf