Roger Pau Monne
2012-Jul-26 19:54 UTC
[PATCH v2 0/5] NetBSD: minor fixes and hotplug execution
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
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)
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)
Roger Pau Monne
2012-Jul-26 19:54 UTC
[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. 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)
Roger Pau Monne
2012-Jul-26 19:54 UTC
[PATCH v2 4/5] libxl: call hotplug scripts from xl for NetBSD
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)
Roger Pau Monne
2012-Jul-26 19:54 UTC
[PATCH v2 5/5] init/NetBSD: move xenbackendd to xend init script
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)
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:
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.
Christoph Egger
2012-Jul-27 12:49 UTC
Re: [PATCH v2 4/5] libxl: call hotplug scripts from xl for NetBSD
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
Christoph Egger
2012-Jul-27 12:50 UTC
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>> 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
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.
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.
Ian Jackson
2012-Jul-27 14:28 UTC
Re: [PATCH v2 3/5] hotplug/NetBSD: check type of file to attach from params
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" > +fiThis 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.
Ian Jackson
2012-Jul-27 14:29 UTC
Re: [PATCH v2 5/5] init/NetBSD: move xenbackendd to xend init script
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>
Christoph Egger
2012-Jul-27 14:49 UTC
Re: [PATCH v2 5/5] init/NetBSD: move xenbackendd to xend init script
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
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)
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.
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.
Ian Campbell
2012-Aug-01 11:47 UTC
Re: [PATCH v2 5/5] init/NetBSD: move xenbackendd to xend init script
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.> > > > >
Ian Campbell
2012-Aug-01 11:47 UTC
Re: [PATCH v2 4/5] libxl: call hotplug scripts from xl for NetBSD
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.
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
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); \ > > > + fiThis 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
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