[PATCH v7 0/8] xen: add systemd support

[PATCH v7 0/8] xen: add systemd support

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

This v7 series is submitted after making a few small changes based on review
from the v6 series, skips the already merged patches and is rebased on top of
today's xen tip tree.

The null character stuff was removed since after a bit more testing witout it I
was not able to see issues without it. Then we had the autoconf juju stuff
which to the best of my ability I wrestled with. The only pending item I am
aware of on this series is the question of whether or not to merge the ocaml C
extensions for systemd now, or wait until this is baked properly into Ocaml
libraries somewhere and then I guess those propagated to Linux distributions.
Someone will have to make that call -- but it certainly would be odd to see
such small implementation have to barred from being merged because Ocaml lacks
a proper implementation on systemd. That would essentially impede progress on
Xen due to Ocaml limitations, and lets face it, Linux should evolve faster than
Ocaml, and people are using C, not Ocaml for generic interfaces. This is a
rather more core observation but I feel the need to highlight this as an
expectation so that developers are really aware, it should be perhaps also
something discussed at the xen developer summit.

Luis R. Rodriguez (8):
  xenstored: enable usage of config.h on both xenstored and oxenstored
  cxenstored: add support for systemd active sockets
  oxenstored: add support for systemd active sockets
  oxenstored: force FD_CLOEXEC with Unix.set_close_on_exec on LSB init
  autoconf: xen: move standard path variables to config/Paths.mk.in
  xencommons: move module list into a generic place
  autoconf: xen: enable explicit preference option for xenstored
    preference
  systemd: add xen systemd service and module files

 .gitignore                                         |   6 +
 Makefile                                           |   6 +-
 README                                             |  67 +++++++++
 config/Linux.modules                               |  20 +++
 config/Paths.mk.in                                 |  37 +++++
 config/Stubdom.mk.in                               |   1 +
 config/Tools.mk.in                                 |   6 +
 configure.ac                                       |   8 +-
 m4/README.source                                   |   8 ++
 m4/paths.m4                                        |  61 +++++++++
 m4/systemd.m4                                      | 123 +++++++++++++++++
 m4/xenstored.m4                                    |  56 ++++++++
 tools/Rules.mk                                     |   1 +
 tools/configure.ac                                 |  30 +++-
 tools/hotplug/Linux/Makefile                       |  42 +++++-
 ...ysconfig.xencommons => sysconfig.xencommons.in} |  13 +-
 .../Linux/init.d/{xencommons => xencommons.in.in}  |  24 +---
 tools/hotplug/Linux/systemd/Makefile               |  67 +++++++++
 tools/hotplug/Linux/systemd/proc-xen.mount.in      |   9 ++
 .../Linux/systemd/var-lib-xenstored.mount.in       |  13 ++
 .../systemd/xen-qemu-dom0-disk-backend.service.in  |  22 +++
 .../hotplug/Linux/systemd/xen-watchdog.service.in  |  13 ++
 tools/hotplug/Linux/systemd/xenconsoled.service.in |  20 +++
 tools/hotplug/Linux/systemd/xendomains.service.in  |  16 +++
 tools/hotplug/Linux/systemd/xenstored.service.in   |  27 ++++
 tools/hotplug/Linux/systemd/xenstored.socket.in    |  11 ++
 tools/hotplug/Linux/systemd/xenstored_ro.socket.in |  11 ++
 tools/ocaml/xenstored/Makefile                     |  15 +-
 tools/ocaml/xenstored/systemd.ml                   |  17 +++
 tools/ocaml/xenstored/systemd.mli                  |  24 ++++
 tools/ocaml/xenstored/systemd_stubs.c              | 152 +++++++++++++++++++++
 tools/ocaml/xenstored/utils.ml                     |  21 ++-
 tools/ocaml/xenstored/xenstored.ml                 |   2 +
 tools/xenstore/Makefile                            |   7 +
 tools/xenstore/xenstored_core.c                    | 104 +++++++++++++-
 35 files changed, 1017 insertions(+), 43 deletions(-)
 create mode 100644 config/Linux.modules
 create mode 100644 config/Paths.mk.in
 create mode 100644 m4/paths.m4
 create mode 100644 m4/systemd.m4
 create mode 100644 m4/xenstored.m4
 rename tools/hotplug/Linux/init.d/{sysconfig.xencommons => sysconfig.xencommons.in} (63%)
 rename tools/hotplug/Linux/init.d/{xencommons => xencommons.in.in} (82%)
 create mode 100644 tools/hotplug/Linux/systemd/Makefile
 create mode 100644 tools/hotplug/Linux/systemd/proc-xen.mount.in
 create mode 100644 tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in
 create mode 100644 tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in
 create mode 100644 tools/hotplug/Linux/systemd/xen-watchdog.service.in
 create mode 100644 tools/hotplug/Linux/systemd/xenconsoled.service.in
 create mode 100644 tools/hotplug/Linux/systemd/xendomains.service.in
 create mode 100644 tools/hotplug/Linux/systemd/xenstored.service.in
 create mode 100644 tools/hotplug/Linux/systemd/xenstored.socket.in
 create mode 100644 tools/hotplug/Linux/systemd/xenstored_ro.socket.in
 create mode 100644 tools/ocaml/xenstored/systemd.ml
 create mode 100644 tools/ocaml/xenstored/systemd.mli
 create mode 100644 tools/ocaml/xenstored/systemd_stubs.c

-- 
2.0.1

[PATCH v7 1/8] xenstored: enable usage of config.h on both xenstored and oxenstored

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

This will be used later for dynamic configuration paths on C code.

Acked-by: Ian Campbell <ian.campbell@citrix.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 tools/ocaml/xenstored/Makefile | 2 ++
 tools/xenstore/Makefile        | 1 +
 2 files changed, 3 insertions(+)

diff --git a/tools/ocaml/xenstored/Makefile b/tools/ocaml/xenstored/Makefile
index b18f190..7fa8f53 100644
--- a/tools/ocaml/xenstored/Makefile
+++ b/tools/ocaml/xenstored/Makefile
@@ -2,6 +2,8 @@ XEN_ROOT = $(CURDIR)/../../..
 OCAML_TOPLEVEL = $(CURDIR)/..
 include $(OCAML_TOPLEVEL)/common.make
 
+CFLAGS += -I$(XEN_ROOT)/tools/
+
 OCAMLINCLUDE += \
 	-I $(OCAML_TOPLEVEL)/libs/xb \
 	-I $(OCAML_TOPLEVEL)/libs/mmap \
diff --git a/tools/xenstore/Makefile b/tools/xenstore/Makefile
index 48b4e3d..08460c8 100644
--- a/tools/xenstore/Makefile
+++ b/tools/xenstore/Makefile
@@ -6,6 +6,7 @@ MINOR = 3
 
 CFLAGS += -Werror
 CFLAGS += -I.
+CFLAGS += -I$(XEN_ROOT)/tools/
 CFLAGS += $(CFLAGS_libxenctrl)
 
 CLIENTS := xenstore-exists xenstore-list xenstore-read xenstore-rm xenstore-chmod
-- 
2.0.1

[PATCH v7 2/8] cxenstored: add support for systemd active sockets

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

This adds systemd socket activation support for the C xenstored.
Active sockets enable xenstored to be loaded only if required by a system
onto which Xen is installed on. Socket activation is handled by
systemd, once a port for a service which claims a socket is used
systemd will start the required services for it, on demand. For more
details on socket activation refer to Lennart's socket-activation
post regarding this [0].

Right now this code adds a no-op for this functionality, leaving the
enablement to be done later once systemd is properly hooked into
the build system. The socket activation is ordered in aligment with
the socket activation order passed on to systemd.

[0] http://0pointer.de/blog/projects/socket-activation2.html

Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 tools/xenstore/xenstored_core.c | 104 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 103 insertions(+), 1 deletion(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 47f0722..7f72f68 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -40,6 +40,7 @@
 #include <signal.h>
 #include <assert.h>
 #include <setjmp.h>
+#include <config.h>
 
 #include "utils.h"
 #include "list.h"
@@ -54,6 +55,16 @@
 
 #include "hashtable.h"
 
+#ifndef NO_SOCKETS
+#if defined(HAVE_SYSTEMD)
+#define XEN_SYSTEMD_ENABLED 1
+#endif
+#endif
+
+#if defined(XEN_SYSTEMD_ENABLED)
+#include <systemd/sd-daemon.h>
+#endif
+
 extern xc_evtchn *xce_handle; /* in xenstored_domain.c */
 static int xce_pollfd_idx = -1;
 static struct pollfd *fds;
@@ -1714,6 +1725,75 @@ static int destroy_fd(void *_fd)
 	return 0;
 }
 
+#if defined(XEN_SYSTEMD_ENABLED)
+/* Will work regardless of the order systemd gives them to us */
+static int xs_get_sd_fd(const char *connect_to)
+{
+	int fd = SD_LISTEN_FDS_START;
+	int r;
+
+	while (fd <= SD_LISTEN_FDS_START + 1) {
+		r = sd_is_socket_unix(fd, SOCK_STREAM, 1, connect_to, 0);
+		if (r > 0)
+			return fd;
+		fd++;
+	}
+
+	return -EBADR;
+}
+
+static int xs_validate_active_socket(const char *connect_to)
+{
+	if ((strcmp("/var/run/xenstored/socket_ro", connect_to) != 0) &&
+	    (strcmp("/var/run/xenstored/socket", connect_to) != 0)) {
+		sd_notifyf(0, "STATUS=unexpected socket: %s\n"
+			   "ERRNO=%i",
+			   connect_to,
+			   EBADR);
+		return -EBADR;
+	}
+
+	return xs_get_sd_fd(connect_to);
+}
+
+static void xen_claim_active_sockets(int **psock, int **pro_sock)
+{
+	int *sock, *ro_sock;
+	const char *soc_str = xs_daemon_socket();
+	const char *soc_str_ro = xs_daemon_socket_ro();
+	int n;
+
+	n = sd_listen_fds(0);
+	if (n <= 0) {
+		sd_notifyf(0, "STATUS=Failed to get any active sockets: %s\n"
+			   "ERRNO=%i",
+			   strerror(errno),
+			   errno);
+		barf_perror("sd_listen_fds() failed\n");
+	} else if (n > 2) {
+		fprintf(stderr, SD_ERR "Expected 2 fds but given %d\n", n);
+		sd_notifyf(0, "STATUS=Mismatch on number (2): %s\n"
+			   "ERRNO=%d",
+			   strerror(EBADR),
+			   EBADR);
+		barf_perror("sd_listen_fds() gave too many fds\n");
+	}
+
+	*psock = sock = talloc(talloc_autofree_context(), int);
+	*sock = xs_validate_active_socket(soc_str);
+	if (*sock <= 0)
+		barf_perror("%s", soc_str);
+
+	*pro_sock = ro_sock = talloc(talloc_autofree_context(), int);
+	*ro_sock = xs_validate_active_socket(soc_str_ro);
+	if (*ro_sock <= 0)
+		barf_perror("%s", soc_str_ro);
+
+	talloc_set_destructor(sock, destroy_fd);
+	talloc_set_destructor(ro_sock, destroy_fd);
+}
+#endif
+
 static void init_sockets(int **psock, int **pro_sock)
 {
 	struct sockaddr_un addr;
@@ -1884,6 +1964,15 @@ int main(int argc, char *argv[])
 	if (optind != argc)
 		barf("%s: No arguments desired", argv[0]);
 
+#if defined(XEN_SYSTEMD_ENABLED)
+	if (sd_booted()) {
+		dofork = false;
+		if (pidfile)
+			barf("%s: PID file not needed on systemd", argv[0]);
+		pidfile = NULL;
+	}
+#endif
+
 	reopen_log();
 
 	/* make sure xenstored directories exist */
@@ -1905,7 +1994,13 @@ int main(int argc, char *argv[])
 	/* Don't kill us with SIGPIPE. */
 	signal(SIGPIPE, SIG_IGN);
 
-	init_sockets(&sock, &ro_sock);
+#if defined(XEN_SYSTEMD_ENABLED)
+	if (sd_booted())
+		xen_claim_active_sockets(&sock, &ro_sock);
+	else
+#endif
+		init_sockets(&sock, &ro_sock);
+
 	init_pipe(reopen_log_pipe);
 
 	/* Setup the database */
@@ -1936,6 +2031,13 @@ int main(int argc, char *argv[])
 	/* Tell the kernel we're up and running. */
 	xenbus_notify_running();
 
+#if defined(XEN_SYSTEMD_ENABLED)
+	if (sd_booted()) {
+		sd_notify(1, "READY=1");
+		fprintf(stderr, SD_NOTICE "xenstored is ready\n");
+	}
+#endif
+
 	/* Main loop. */
 	for (;;) {
 		struct connection *conn, *next;
-- 
2.0.1

[PATCH v7 3/8] oxenstored: add support for systemd active sockets

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

This adds systemd socket activation support for the Ocaml xenstored.
Ocaml lacks systemd library support so we provide our own C helpers
as is done with other functionality lacking on Ocaml.

Active sockets enables oxenstored to be loaded only if required by a system
onto which Xen is installed on. Socket activation is handled by
systemd, once a port for a service which claims a socket is used
systemd will start the required services for it, on demand. For more
details on socket activation refer to Lennart's socket-activation
post regarding this [0].

An important difference with socket activation is that systemd will set
FD_CLOEXEC for us on the socket before giving it to us, we'll sprinkly
the Unix.set_close_on_exec for LSB init next as a separate commit.

Right now this code adds a no-op for this functionality, leaving the
enablement to be done later once systemd is properly hooked into
the build system. The socket activation is ordered in aligment with
the socket activation order passed on to systemd.

[0] http://0pointer.de/blog/projects/socket-activation2.html

Cc: David Scott <dave.scott@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Vincent Hanquez <Vincent.Hanquez@eu.citrix.com>
Acked-by: Dave Scott <Dave.Scott@citrix.com>
Acked-by: Anil Madhavapeddy <anil@recoil.org>
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 tools/ocaml/xenstored/Makefile        |   8 +-
 tools/ocaml/xenstored/systemd.ml      |  17 ++++
 tools/ocaml/xenstored/systemd.mli     |  24 ++++++
 tools/ocaml/xenstored/systemd_stubs.c | 152 ++++++++++++++++++++++++++++++++++
 tools/ocaml/xenstored/utils.ml        |  20 +++--
 tools/ocaml/xenstored/xenstored.ml    |   2 +
 6 files changed, 215 insertions(+), 8 deletions(-)
 create mode 100644 tools/ocaml/xenstored/systemd.ml
 create mode 100644 tools/ocaml/xenstored/systemd.mli
 create mode 100644 tools/ocaml/xenstored/systemd_stubs.c

diff --git a/tools/ocaml/xenstored/Makefile b/tools/ocaml/xenstored/Makefile
index 7fa8f53..382a813 100644
--- a/tools/ocaml/xenstored/Makefile
+++ b/tools/ocaml/xenstored/Makefile
@@ -15,6 +15,11 @@ syslog_OBJS = syslog
 syslog_C_OBJS = syslog_stubs
 OCAML_LIBRARY = syslog
 
+LIBS += systemd.cma systemd.cmxa
+systemd_OBJS = systemd
+systemd_C_OBJS = systemd_stubs
+OCAML_LIBRARY += systemd
+
 OBJS = define \
 	stdext \
 	trie \
@@ -36,11 +41,12 @@ OBJS = define \
 	process \
 	xenstored
 
-INTF = symbol.cmi trie.cmi syslog.cmi
+INTF = symbol.cmi trie.cmi syslog.cmi systemd.cmi
 
 XENSTOREDLIBS = \
 	unix.cmxa \
 	-ccopt -L -ccopt . syslog.cmxa \
+	-ccopt -L -ccopt . systemd.cmxa \
 	-ccopt -L -ccopt $(OCAML_TOPLEVEL)/libs/mmap $(OCAML_TOPLEVEL)/libs/mmap/xenmmap.cmxa \
 	-ccopt -L -ccopt $(OCAML_TOPLEVEL)/libs/eventchn $(OCAML_TOPLEVEL)/libs/eventchn/xeneventchn.cmxa \
 	-ccopt -L -ccopt $(OCAML_TOPLEVEL)/libs/xc $(OCAML_TOPLEVEL)/libs/xc/xenctrl.cmxa \
diff --git a/tools/ocaml/xenstored/systemd.ml b/tools/ocaml/xenstored/systemd.ml
new file mode 100644
index 0000000..2aa39ea
--- /dev/null
+++ b/tools/ocaml/xenstored/systemd.ml
@@ -0,0 +1,17 @@
+(*
+ * Copyright (C) 2014 Luis R. Rodriguez <mcgrof@suse.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *)
+
+external sd_listen_fds: string -> Unix.file_descr = "ocaml_sd_listen_fds"
+external sd_booted: unit -> bool = "ocaml_sd_booted"
+external sd_notify_ready: unit -> unit = "ocaml_sd_notify_ready"
diff --git a/tools/ocaml/xenstored/systemd.mli b/tools/ocaml/xenstored/systemd.mli
new file mode 100644
index 0000000..85c9f2e
--- /dev/null
+++ b/tools/ocaml/xenstored/systemd.mli
@@ -0,0 +1,24 @@
+(*
+ * Copyright (C) 2014 Luis R. Rodriguez <mcgrof@suse.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *)
+
+(** Calls the C library sd_listen_fds() function for us. Although
+ *  the library doesn't accept argument we send one over to help
+ *  us do sanity checks on the expected sockets *)
+val sd_listen_fds: string -> Unix.file_descr
+
+(** Tells us whether or not systemd support was compiled in *)
+val sd_booted: unit -> bool
+
+(** Tells systemd we're ready *)
+external sd_notify_ready: unit -> unit = "ocaml_sd_notify_ready"
diff --git a/tools/ocaml/xenstored/systemd_stubs.c b/tools/ocaml/xenstored/systemd_stubs.c
new file mode 100644
index 0000000..a368ac1
--- /dev/null
+++ b/tools/ocaml/xenstored/systemd_stubs.c
@@ -0,0 +1,152 @@
+/*
+ * Copyright (C) 2014 Luis R. Rodriguez <mcgrof@suse.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ */
+
+#include <string.h>
+#include <stdio.h>
+#include <stdbool.h>
+#include <errno.h>
+#include <caml/mlvalues.h>
+#include <caml/memory.h>
+#include <caml/alloc.h>
+#include <caml/custom.h>
+#include <caml/signals.h>
+#include <caml/fail.h>
+#include <config.h>
+
+#if defined(HAVE_SYSTEMD)
+
+#include <sys/socket.h>
+#include <systemd/sd-daemon.h>
+
+/* Will work regardless of the order systemd gives them to us */
+static int oxen_get_sd_fd(const char *connect_to)
+{
+	int fd = SD_LISTEN_FDS_START;
+	int r;
+
+	while (fd <= SD_LISTEN_FDS_START + 1) {
+		r = sd_is_socket_unix(fd, SOCK_STREAM, 1, connect_to, 0);
+		if (r > 0)
+			return fd;
+		fd++;
+	}
+
+	return -EBADR;
+}
+
+static int oxen_verify_socket_socket(const char *connect_to)
+{
+	if ((strcmp("/var/run/xenstored/socket_ro", connect_to) != 0) &&
+	    (strcmp("/var/run/xenstored/socket", connect_to) != 0)) {
+		sd_notifyf(0, "STATUS=unexpected socket: %s\n"
+			   "ERRNO=%i",
+			   connect_to,
+			   EBADR);
+		return -EBADR;
+	}
+
+	return oxen_get_sd_fd(connect_to);
+}
+
+CAMLprim value ocaml_sd_listen_fds(value connect_to)
+{
+	CAMLparam1(connect_to);
+	CAMLlocal1(sock_ret);
+	int sock = -EBADR, n;
+
+	n = sd_listen_fds(0);
+	if (n <= 0) {
+		sd_notifyf(0, "STATUS=Failed to get any active sockets: %s\n"
+			   "ERRNO=%i",
+			   strerror(errno),
+			   errno);
+		caml_failwith("ocaml_sd_listen_fds() failed to get any sockets");
+	} else if (n > 2) {
+		fprintf(stderr, SD_ERR "Expected 2 fds but given %d\n", n);
+		sd_notifyf(0, "STATUS=Mismatch on number (2): %s\n"
+			   "ERRNO=%d",
+			   strerror(EBADR),
+			   EBADR);
+		caml_failwith("ocaml_sd_listen_fds() mismatch");
+	}
+
+	sock = oxen_verify_socket_socket(String_val(connect_to));
+	if (sock <= 0) {
+		fprintf(stderr, "failed to verify sock %s\n",
+			String_val(connect_to));
+		caml_failwith("ocaml_sd_listen_fds_init() invalid socket");
+	}
+
+	sock_ret = Val_int(sock);
+
+	CAMLreturn(sock_ret);
+}
+
+CAMLprim value ocaml_sd_booted(value ignore)
+{
+	CAMLparam1(ignore);
+	CAMLlocal1(ret);
+
+	ret = Val_false;
+
+	if (sd_booted())
+		ret = Val_true;
+
+	CAMLreturn(ret);
+}
+
+CAMLprim value ocaml_sd_notify_ready(value ignore)
+{
+	CAMLparam1(ignore);
+	CAMLlocal1(ret);
+
+	ret = Val_int(0);
+
+	sd_notify(1, "READY=1");
+
+	CAMLreturn(ret);
+}
+
+#else
+
+CAMLprim value ocaml_sd_listen_fds(value connect_to)
+{
+	CAMLparam1(connect_to);
+	CAMLlocal1(sock_ret);
+
+	sock_ret = Val_int(-1);
+
+	CAMLreturn(sock_ret);
+}
+
+CAMLprim value ocaml_sd_booted(value ignore)
+{
+	CAMLparam1(ignore);
+	CAMLlocal1(ret);
+
+	ret = Val_false;
+
+	CAMLreturn(ret);
+}
+
+CAMLprim value ocaml_sd_notify_ready(value ignore)
+{
+	CAMLparam1(ignore);
+	CAMLlocal1(ret);
+
+	ret = Val_int(-1);
+
+	CAMLreturn(ret);
+}
+#endif
diff --git a/tools/ocaml/xenstored/utils.ml b/tools/ocaml/xenstored/utils.ml
index 68b70c5..0cfeded 100644
--- a/tools/ocaml/xenstored/utils.ml
+++ b/tools/ocaml/xenstored/utils.ml
@@ -73,14 +73,20 @@ let trim_path path =
 let join_by_null ls = String.concat "\000" ls
 
 (* unix utils *)
+let create_regular_unix_socket name =
+        Unixext.unlink_safe name;
+        Unixext.mkdir_rec (Filename.dirname name) 0o700;
+        let sockaddr = Unix.ADDR_UNIX(name) in
+        let sock = Unix.socket Unix.PF_UNIX Unix.SOCK_STREAM 0 in
+        Unix.bind sock sockaddr;
+        Unix.listen sock 1;
+        sock
+
 let create_unix_socket name =
-	Unixext.unlink_safe name;
-	Unixext.mkdir_rec (Filename.dirname name) 0o700;
-	let sockaddr = Unix.ADDR_UNIX(name) in
-	let sock = Unix.socket Unix.PF_UNIX Unix.SOCK_STREAM 0 in
-	Unix.bind sock sockaddr;
-	Unix.listen sock 1;
-	sock
+        if Systemd.sd_booted() then
+                Systemd.sd_listen_fds name
+        else
+                create_regular_unix_socket name
 
 let read_file_single_integer filename =
 	let fd = Unix.openfile filename [ Unix.O_RDONLY ] 0o640 in
diff --git a/tools/ocaml/xenstored/xenstored.ml b/tools/ocaml/xenstored/xenstored.ml
index 438ecb9..1c02f2f 100644
--- a/tools/ocaml/xenstored/xenstored.ml
+++ b/tools/ocaml/xenstored/xenstored.ml
@@ -383,6 +383,8 @@ let _ =
 	while not !quit
 	do
 		try
+                        if Systemd.sd_booted() then
+                                Systemd.sd_notify_ready ();
 			main_loop ()
 		with exc ->
 			error "caught exception %s" (Printexc.to_string exc);
-- 
2.0.1

[PATCH v7 4/8] oxenstored: force FD_CLOEXEC with Unix.set_close_on_exec on LSB init

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

Lets match the systemd active socket activation implementation and
ensure that FD_CLOEXEC is set by usin Unix.set_close_on_exec. David
notes oxenstored likely does not exec but there is no harm in being
careful just in case things change in the future.

Cc: David Scott <dave.scott@eu.citrix.com>
Cc: Anil Madhavapeddy <anil@recoil.org>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Vincent Hanquez <Vincent.Hanquez@eu.citrix.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 tools/ocaml/xenstored/utils.ml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/ocaml/xenstored/utils.ml b/tools/ocaml/xenstored/utils.ml
index 0cfeded..61321c6 100644
--- a/tools/ocaml/xenstored/utils.ml
+++ b/tools/ocaml/xenstored/utils.ml
@@ -78,6 +78,7 @@ let create_regular_unix_socket name =
         Unixext.mkdir_rec (Filename.dirname name) 0o700;
         let sockaddr = Unix.ADDR_UNIX(name) in
         let sock = Unix.socket Unix.PF_UNIX Unix.SOCK_STREAM 0 in
+        Unix.set_close_on_exec sock;
         Unix.bind sock sockaddr;
         Unix.listen sock 1;
         sock
-- 
2.0.1

[PATCH v7 5/8] autoconf: xen: move standard path variables to config/Paths.mk.in

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

This moves all generic path variables to a new the config/Paths.mk.in
input source file to be processed at configure time, tons of files use
these so this just share them. This also paves the way to let us
easily dynamically configure these with autoconf, for now we leave the
same presets as was present before.

This work was prompted by looking for an autoconf way to do
replacements for the hotplug global file, while at it I realized
that a few other files use the same variables and have in places
around the tree the same constructs for generating their own
files. This makes use of the old buildmakevars2file() but generalizes
the definition of the paths at configure time and spreads the
new definitions out throughout the build system.

This has no impact on building the hypervisor and extras/mini-os,
you do not need to, and are not expected to, run configure to build
those targets.

While at it lets add some documentation on the for the two files on
the source file, we can expand further details on the wiki [0].

[0] http://wiki.xen.org/wiki/Category:Host_Configuration#System_wide_xen_configuration

Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Samuel Thibault <samuel.thibault@ens-lyon.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---

Please run autgen.sh after merging this.

 .gitignore           |  1 +
 config/Paths.mk.in   | 37 +++++++++++++++++++++++++++++++
 config/Stubdom.mk.in |  1 +
 configure.ac         |  8 ++++++-
 m4/paths.m4          | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 tools/Rules.mk       |  1 +
 6 files changed, 108 insertions(+), 1 deletion(-)
 create mode 100644 config/Paths.mk.in
 create mode 100644 m4/paths.m4

diff --git a/.gitignore b/.gitignore
index fefe13c..f1d1b9c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -37,6 +37,7 @@ config.log
 config.status
 config.cache
 config/Toplevel.mk
+config/Paths.mk
 
 build-*
 dist/*
diff --git a/config/Paths.mk.in b/config/Paths.mk.in
new file mode 100644
index 0000000..507b6d1
--- /dev/null
+++ b/config/Paths.mk.in
@@ -0,0 +1,37 @@
+# Xen system configuration
+# ========================
+#
+# Xen uses a set of variables for system configuration and at build time,
+# because of this these variables are defined on one master input source file
+# and is generated after running ./configure. The master source is located
+# on the xen source tree at under config/Paths.mk.in and it is used to
+# generate shell or header files by the build system upon demand through the
+# use of the helper makefile helper buildmakevars2file().
+#
+# For more documentation you can refer to the wiki:
+#
+# http://wiki.xen.org/wiki/Category:Host_Configuration#System_wide_xen_configuration
+
+SBINDIR                  := @SBINDIR@
+BINDIR                   := @BINDIR@
+LIBEXEC                  := @LIBEXEC@
+
+SHAREDIR                 := @SHAREDIR@
+LIBDIR                   := @LIBDIR@
+
+XEN_RUN_DIR              := @XEN_RUN_DIR@
+XEN_LOG_DIR              := @XEN_LOG_DIR@
+XEN_LIB_STORED           := @XEN_LIB_STORED@
+
+CONFIG_DIR               := @CONFIG_DIR@
+XEN_LOCK_DIR             := @XEN_LOCK_DIR@
+XEN_PAGING_DIR           := @XEN_PAGING_DIR@
+
+PRIVATE_PREFIX           := @PRIVATE_PREFIX@
+PRIVATE_PREFIX           := @PKG_XEN_PREFIX@
+PRIVATE_BINDIR           := @PRIVATE_BINDIR@
+
+XENFIRMWAREDIR           := @XENFIRMWAREDIR@
+
+XEN_CONFIG_DIR           := @XEN_CONFIG_DIR@
+XEN_SCRIPT_DIR           := @XEN_SCRIPT_DIR@
diff --git a/config/Stubdom.mk.in b/config/Stubdom.mk.in
index 302842e..6bce206 100644
--- a/config/Stubdom.mk.in
+++ b/config/Stubdom.mk.in
@@ -1,4 +1,5 @@
 # Prefix and install folder
+include $(XEN_ROOT)/config/Paths.mk
 prefix              := @prefix@
 PREFIX              := $(prefix)
 exec_prefix         := @exec_prefix@
diff --git a/configure.ac b/configure.ac
index 6c14524..f32f9af 100644
--- a/configure.ac
+++ b/configure.ac
@@ -5,12 +5,18 @@ AC_PREREQ([2.67])
 AC_INIT([Xen Hypervisor], m4_esyscmd([./version.sh ./xen/Makefile]),
     [xen-devel@lists.xen.org], [xen], [http://www.xen.org/])
 AC_CONFIG_SRCDIR([./xen/common/kernel.c])
-AC_CONFIG_FILES([./config/Toplevel.mk])
+AC_CONFIG_FILES([
+	config/Toplevel.mk
+	config/Paths.mk
+])
 
 AC_CANONICAL_HOST
 
 m4_include([m4/features.m4])
 m4_include([m4/subsystem.m4])
+m4_include([m4/paths.m4])
+
+AX_XEN_EXPAND_CONFIG()
 
 dnl mini-os is only ported to certain platforms
 case "$host_cpu" in
diff --git a/m4/paths.m4 b/m4/paths.m4
new file mode 100644
index 0000000..717fcd1
--- /dev/null
+++ b/m4/paths.m4
@@ -0,0 +1,61 @@
+AC_DEFUN([AX_XEN_EXPAND_CONFIG], [
+dnl expand these early so we can use this for substitutions
+test "x$prefix" = "xNONE" && prefix=$ac_default_prefix
+test "x$exec_prefix" = "xNONE" && exec_prefix=$ac_default_prefix
+
+BINDIR=$prefix/bin
+AC_SUBST(BINDIR)
+
+SBINDIR=$prefix/sbin
+AC_SUBST(SBINDIR)
+
+dnl XXX: this should be changed to use the passed $libexec
+dnl but can be done as a second step
+LIBEXEC=$prefix/lib/xen/bin
+AC_SUBST(LIBEXEC)
+
+LIBDIR=`eval echo $libdir`
+AC_SUBST(LIBDIR)
+
+XEN_RUN_DIR=/var/run/xen
+AC_SUBST(XEN_RUN_DIR)
+
+XEN_LOG_DIR=/var/log/xen
+AC_SUBST(XEN_LOG_DIR)
+
+XEN_LIB_STORED=/var/lib/xenstored
+AC_SUBST(XEN_LIB_STORED)
+
+SHAREDIR=$prefix/share
+AC_SUBST(SHAREDIR)
+
+PRIVATE_PREFIX=$LIBDIR/xen
+AC_SUBST(PRIVATE_PREFIX)
+
+PKG_XEN_PREFIX=$LIBDIR/xen
+AC_SUBST(PKG_XEN_PREFIX)
+
+PRIVATE_BINDIR=$PRIVATE_PREFIX/bin
+AC_SUBST(PRIVATE_BINDIR)
+
+XENFIRMWAREDIR=$prefix/lib/xen/boot
+AC_SUBST(XENFIRMWAREDIR)
+
+CONFIG_DIR=/etc
+AC_SUBST(CONFIG_DIR)
+
+XEN_CONFIG_DIR=$CONFIG_DIR/xen
+AC_SUBST(XEN_CONFIG_DIR)
+
+XEN_SCRIPT_DIR=$XEN_CONFIG_DIR/scripts
+AC_SUBST(XEN_SCRIPT_DIR)
+
+XEN_LOCK_DIR=/var/lock
+AC_SUBST(XEN_LOCK_DIR)
+
+XEN_RUN_DIR=/var/run/xen
+AC_SUBST(XEN_RUN_DIR)
+
+XEN_PAGING_DIR=/var/lib/xen/xenpaging
+AC_SUBST(XEN_PAGING_DIR)
+])
diff --git a/tools/Rules.mk b/tools/Rules.mk
index 9ac8541..0aa1e6b 100644
--- a/tools/Rules.mk
+++ b/tools/Rules.mk
@@ -5,6 +5,7 @@ all:
 
 -include $(XEN_ROOT)/config/Tools.mk
 include $(XEN_ROOT)/Config.mk
+include $(XEN_ROOT)/config/Paths.mk
 
 export _INSTALL := $(INSTALL)
 INSTALL = $(XEN_ROOT)/tools/cross-install
-- 
2.0.1

[PATCH v7 6/8] xencommons: move module list into a generic place

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

This will allows us to share the same module list with
sysemd, and lets us upkeep it in one place. Document this
while at it on the top level README and expand on the wiki:

http://wiki.xen.org/wiki/Category:Host_Configuration#Linux_kernel_modules

In order to upkeep parallelism builds be explicit about the
requirement to complete all actions before any installation
targets.

Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 README                                             | 12 ++++++++
 config/Linux.modules                               | 20 ++++++++++++
 tools/hotplug/Linux/Makefile                       | 36 +++++++++++++++++++---
 .../Linux/init.d/{xencommons => xencommons.in}     | 16 +---------
 4 files changed, 65 insertions(+), 19 deletions(-)
 create mode 100644 config/Linux.modules
 rename tools/hotplug/Linux/init.d/{xencommons => xencommons.in} (89%)

diff --git a/README b/README
index 9bbe734..c6cc09b 100644
--- a/README
+++ b/README
@@ -183,3 +183,15 @@ There are optional targets as part of Xen's top-level makefile that will
 download and build tboot: install-tboot, build-tboot, dist-tboot, clean-tboot.
 These will download the latest tar file from the SourceForge site using wget,
 then build/install/dist according to Xen's settings.
+
+Required Linux modules
+======================
+
+Xen has a set of Linux modules which the init scripts ensure to load before
+before starting Xen guests. The list of modules are maintained in one place:
+
+  * config/modules
+
+For more details refer to:
+
+http://wiki.xen.org/wiki/Category:Host_Configuration#Linux_kernel_modules
diff --git a/config/Linux.modules b/config/Linux.modules
new file mode 100644
index 0000000..8a764df
--- /dev/null
+++ b/config/Linux.modules
@@ -0,0 +1,20 @@
+# The file supports a simple language, comments are ignored, and if you there
+# are module replacements this can be listed by using a pipe to show preference
+# for the first module, followed by the older module.
+
+xen-evtchn
+xen-gntdev
+xen-gntalloc
+xen-blkback
+xen-netback
+xen-pciback
+evtchn
+gntdev
+netbk
+blkbk
+xen-scsibk
+usbbk
+pciback
+xen-acpi-processor
+# Prefer to load blktap2 if found, otherwise load blktap
+blktap2|blktap
diff --git a/tools/hotplug/Linux/Makefile b/tools/hotplug/Linux/Makefile
index d5de9e6..1805746 100644
--- a/tools/hotplug/Linux/Makefile
+++ b/tools/hotplug/Linux/Makefile
@@ -33,17 +33,45 @@ UDEV_RULES_DIR = $(CONFIG_DIR)/udev
 UDEV_RULES = xen-backend.rules $(UDEV_RULES-y)
 
 .PHONY: all
-all:
+all: $(XENCOMMONS_INITD)
+
+$(XENCOMMONS_INITD): $(XEN_ROOT)/config/$(XEN_OS).modules $(XENCOMMONS_INITD).in
+	@set -e ;							\
+	IFS=''								;\
+	cat  $(XEN_ROOT)/config/$(XEN_OS).modules	| (		\
+		while read l ; do					\
+			if echo $${l} | egrep -q "^#" ; then		\
+				continue				;\
+			fi						;\
+			if echo "$${l}" | egrep -q "\|" ; then		\
+				m1=$${l%%|*}				;\
+				m2=$${l#*|} 				;\
+				echo "        modprobe $$m1 2>/dev/null || modprobe $$m2 2>/dev/null" ;\
+			else						\
+				echo "        modprobe $$l 2>/dev/null"		;\
+			fi						;\
+		done							\
+	) > $(XENCOMMONS_INITD).modules					;\
+	cat  $(XENCOMMONS_INITD).in	| (				\
+		while read l ; do					\
+			if echo "$${l}" | egrep -q "@LOAD_MODULES@" ; then \
+				cat $(XENCOMMONS_INITD).modules 	;\
+			else						\
+				echo $$l				;\
+			fi						;\
+		done							\
+	) > $@
+	@rm -f $(XENCOMMONS_INITD).modules
 
 .PHONY: build
-build:
+build: all
 
 .PHONY: install
 install: all install-initd install-scripts install-udev
 
 # See docs/misc/distro_mapping.txt for INITD_DIR location
 .PHONY: install-initd
-install-initd:
+install-initd: all
 	[ -d $(DESTDIR)$(INITD_DIR) ] || $(INSTALL_DIR) $(DESTDIR)$(INITD_DIR)
 	[ -d $(DESTDIR)$(SYSCONFIG_DIR) ] || $(INSTALL_DIR) $(DESTDIR)$(SYSCONFIG_DIR)
 	[ -d $(DESTDIR)$(LIBEXEC) ] || $(INSTALL_DIR) $(DESTDIR)$(LIBEXEC)
@@ -55,7 +83,7 @@ install-initd:
 	$(INSTALL_PROG) init.d/xen-watchdog $(DESTDIR)$(INITD_DIR)
 
 .PHONY: install-scripts
-install-scripts:
+install-scripts: all
 	[ -d $(DESTDIR)$(XEN_SCRIPT_DIR) ] || \
 		$(INSTALL_DIR) $(DESTDIR)$(XEN_SCRIPT_DIR)
 	set -e; for i in $(XEN_SCRIPTS); \
diff --git a/tools/hotplug/Linux/init.d/xencommons b/tools/hotplug/Linux/init.d/xencommons.in
similarity index 89%
rename from tools/hotplug/Linux/init.d/xencommons
rename to tools/hotplug/Linux/init.d/xencommons.in
index 4ebd636..3939bcc 100644
--- a/tools/hotplug/Linux/init.d/xencommons
+++ b/tools/hotplug/Linux/init.d/xencommons.in
@@ -57,21 +57,7 @@ do_start () {
         local time=0
 	local timeout=30
 
-	modprobe xen-evtchn 2>/dev/null
-	modprobe xen-gntdev 2>/dev/null
-	modprobe xen-gntalloc 2>/dev/null
-	modprobe xen-blkback 2>/dev/null
-	modprobe xen-netback 2>/dev/null
-	modprobe xen-pciback 2>/dev/null
-	modprobe evtchn 2>/dev/null
-	modprobe gntdev 2>/dev/null
-	modprobe netbk 2>/dev/null
-	modprobe blkbk 2>/dev/null
-	modprobe xen-scsibk 2>/dev/null
-	modprobe usbbk 2>/dev/null
-	modprobe pciback 2>/dev/null
-	modprobe xen-acpi-processor 2>/dev/null
-	modprobe blktap2 2>/dev/null || modprobe blktap 2>/dev/null
+	@LOAD_MODULES@
 	mkdir -p /var/run/xen
 
 	if ! `${BINDIR}/xenstore-read -s / >/dev/null 2>&1`
-- 
2.0.1

[PATCH v7 7/8] autoconf: xen: enable explicit preference option for xenstored preference

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

As it stands oxenstored will be used by default if ocaml tools are
found, the init system will also try to use oxenstored first if its
found otherwise the cxenstored will be used. Lets simplify the init
script and let users be explicit about the preference through configure.

This adds support to let you be explicit about the xenstored preference,
you can only use one of these two options:

./configure --with-xenstored=xenstored
./configure --with-xenstored=oxenstored

We continue with the old behaviour and default oxenstored will be used
but only if you have ocaml dependencies. Since the xenstored preference
is explicit now and since we require configure substitutions for it we
make use of the AX_XEN_EXPAND_CONFIG() helpers as otherwise substitution
for SBINDIR is not propagated from the top level configuration.

All this allows us to simplify the init script to use the configured
xenstore from the start. We update the sysconfig/default xencommons file
with the paths for the different options though, this can be used by
users to override the default xenstored, this follows the old behaviour
but we now just explicitly provide the full configured paths for users.

As before, changing the xenstore requires a reboot.

In order to help with documentation we update the README with some
details on configure usage refer to the wiki [0] [1] [2] for more elaborate
details.

Since we are now parsing an entry within Paths.mk.in on tools we let
the move the parsing of the file to be the tool's configure.

[0] http://wiki.xen.org/wiki/Xenstored
[1] http://wiki.xen.org/wiki/XenStore
[2] http://wiki.xen.org/wiki/XenStoreReference

Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---

Please run autogen.sh after merging this patch.

 README                                             | 37 ++++++++++++++
 m4/xenstored.m4                                    | 56 ++++++++++++++++++++++
 tools/configure.ac                                 | 19 ++++++--
 ...ysconfig.xencommons => sysconfig.xencommons.in} | 13 ++++-
 .../init.d/{xencommons.in => xencommons.in.in}     |  8 +---
 5 files changed, 122 insertions(+), 11 deletions(-)
 create mode 100644 m4/xenstored.m4
 rename tools/hotplug/Linux/init.d/{sysconfig.xencommons => sysconfig.xencommons.in} (63%)
 rename tools/hotplug/Linux/init.d/{xencommons.in => xencommons.in.in} (92%)

diff --git a/README b/README
index c6cc09b..0a19eb6 100644
--- a/README
+++ b/README
@@ -129,6 +129,43 @@ performed with root privileges.]
    versions of those scripts, so that you can copy the dist directory
    to another machine and install from that distribution.
 
+xenstore: xenstored and oxenstored
+====================================
+
+Xen uses a configuration database called xenstore [0] to maintain configuration
+and status information shared between domains. A daemon is implemented as part
+of xenstore to act as an interface for access to the database for dom0 and
+guests. Two xenstored daemons are supported, one written in C which we refer
+to as the xenstored (sometimes referred to as cxenstored), and another written
+in Ocaml called oxenstored. Details for xenstore and the different
+implementations can be found on the wiki's xenstore reference guide [1] and
+the xenstored [2] page. You can choose which xenstore you want to enable as
+default on a system through configure:
+
+	./configure --with-xenstored=xenstored
+	./configure --with-xenstored=oxenstored
+
+By default oxenstored will be used if the ocaml development tools are found.
+If you enable oxenstored the xenstored will still be built and installed,
+the xenstored used can be changed through the configuration file:
+
+/etc/sysconfig/xencommons
+or
+/etc/default/xencommons
+
+This file has one relevant variables which is specific to the version of
+xenstored used:
+
+	* XENSTORED - specifies the full path of the xenstored binary
+
+You can change the preferred xenstored you want to use in the configuration
+but since we cannot stop the daemon a reboot will be required to make the
+change take effect.
+
+[0] http://wiki.xen.org/wiki/XenStore
+[1] http://wiki.xen.org/wiki/XenStoreReference
+[2] http://wiki.xen.org/wiki/Xenstored
+
 Python Runtime Libraries
 ========================
 
diff --git a/m4/xenstored.m4 b/m4/xenstored.m4
new file mode 100644
index 0000000..30b44c9
--- /dev/null
+++ b/m4/xenstored.m4
@@ -0,0 +1,56 @@
+AC_DEFUN([AX_XEN_OCAML_XENSTORE_CHECK], [
+	AS_IF([test "x$OCAMLC" = "xno" || test "x$OCAMLFIND" = "xno"], [
+		AC_MSG_ERROR([Missing ocaml dependencies for oxenstored, try installing ocaml ocaml-compiler-libs ocaml-runtime ocaml-findlib])
+	])
+])
+
+AC_DEFUN([AX_XEN_OCAML_XENSTORE_DEFAULTS], [
+	xenstore="oxenstored"
+	xenstored=$SBINDIR/oxenstored
+	AS_IF([test "x$OCAMLC" = "xno" || test "x$OCAMLFIND" = "xno"], [
+		xenstore="xenstored"
+		xenstored=$SBINDIR/xenstored
+	])
+])
+
+AC_DEFUN([AX_XENSTORE_OPTIONS], [
+AS_IF([test "x$XENSTORE" = "x"], [
+AC_ARG_WITH([xenstored],
+	AS_HELP_STRING([--with-xenstored@<:@=oxenstored|xenstored@:>@],
+		[This lets you choose which xenstore daemon you want, you have
+		two options: the original xenstored written in C (xenstored)
+		or the newer and robust one written in Ocaml (oxenstored).
+		The oxenstored daemon is the default but will but can only
+		be used if you have ocaml library / build dependencies solved,
+		if you have not specified a preference and do not have ocaml
+		dependencies resolved we'll enable the C xenstored for you. If
+		you ask for oxenstored we'll complain until you resolve those
+		dependencies]),
+	[
+		AS_IF([test "x$withval" = "xxenstored"], [
+			xenstore=$withval
+			xenstored=$SBINDIR/xenstored
+		])
+		AS_IF([test "x$withval" = "xoxenstored"], [
+			xenstore=$withval
+			xenstored=$SBINDIR/oxenstored
+			AX_XEN_OCAML_XENSTORE_CHECK()
+		])
+		AS_IF([test "x$withval" != "xoxenstored" && test "x$withval" != "xxenstored"], [
+			AC_MSG_ERROR([Unsupported xenstored specified, supported types: oxenstored xenstored])
+		])
+	],
+	[
+		AX_XEN_OCAML_XENSTORE_DEFAULTS()
+	])
+])
+])
+
+AC_DEFUN([AX_XENSTORE_SET], [
+	XENSTORE=$xenstore
+
+	AS_IF([test "x$XENSTORED" = "x"], [
+		XENSTORED=$xenstored
+	])
+	AC_SUBST(XENSTORED)
+])
diff --git a/tools/configure.ac b/tools/configure.ac
index 629d6a0..e74fe4b 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -5,7 +5,11 @@ AC_PREREQ([2.67])
 AC_INIT([Xen Hypervisor Tools], m4_esyscmd([../version.sh ../xen/Makefile]),
     [xen-devel@lists.xen.org], [xen], [http://www.xen.org/])
 AC_CONFIG_SRCDIR([libxl/libxl.c])
-AC_CONFIG_FILES([../config/Tools.mk])
+AC_CONFIG_FILES([
+../config/Tools.mk
+hotplug/Linux/init.d/xencommons.in
+hotplug/Linux/init.d/sysconfig.xencommons
+])
 AC_CONFIG_HEADERS([config.h])
 AC_CONFIG_AUX_DIR([../])
 
@@ -53,6 +57,10 @@ m4_include([../m4/ptyfuncs.m4])
 m4_include([../m4/extfs.m4])
 m4_include([../m4/fetcher.m4])
 m4_include([../m4/ax_compare_version.m4])
+m4_include([../m4/paths.m4])
+m4_include([../m4/xenstored.m4])
+
+AX_XEN_EXPAND_CONFIG()
 
 # Enable/disable options
 AX_ARG_DEFAULT_DISABLE([githttp], [Download GIT repositories via HTTP])
@@ -203,9 +211,14 @@ AC_PROG_INSTALL
 AC_PATH_PROG([BISON], [bison])
 AC_PATH_PROG([FLEX], [flex])
 AX_PATH_PROG_OR_FAIL([PERL], [perl])
+
+AC_PROG_OCAML
+AC_PROG_FINDLIB
+
+AX_XENSTORE_OPTIONS
+AX_XENSTORE_SET
+
 AS_IF([test "x$ocamltools" = "xy"], [
-    AC_PROG_OCAML
-    AC_PROG_FINDLIB
     AS_IF([test "x$OCAMLC" = "xno" || test "x$OCAMLFIND" = "xno"], [
         AS_IF([test "x$enable_ocamltools" = "xyes"], [
             AC_MSG_ERROR([Ocaml tools enabled, but unable to find Ocaml])])
diff --git a/tools/hotplug/Linux/init.d/sysconfig.xencommons b/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
similarity index 63%
rename from tools/hotplug/Linux/init.d/sysconfig.xencommons
rename to tools/hotplug/Linux/init.d/sysconfig.xencommons.in
index 25f7f00..d423ff8 100644
--- a/tools/hotplug/Linux/init.d/sysconfig.xencommons
+++ b/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
@@ -8,8 +8,17 @@
 ## Type: string
 ## Default: xenstored
 #
-# Select xenstored implementation
-#XENSTORED=[oxenstored|xenstored]
+# Select xenstore implementation, this can be either
+# of these below. If using systemd it's preferred that you
+# just edit the xenstored.service unit file and change
+# the XENSTORED variable there.
+#
+# This can be either of:
+#  * @SBINDIR@/oxenstored
+#  * @SBINDIR@/xenstored
+#
+# Changing this requires a reboot to take effect.
+#XENSTORED=@XENSTORED@
 
 ## Type: string
 ## Default: Not defined, tracing off
diff --git a/tools/hotplug/Linux/init.d/xencommons.in b/tools/hotplug/Linux/init.d/xencommons.in.in
similarity index 92%
rename from tools/hotplug/Linux/init.d/xencommons.in
rename to tools/hotplug/Linux/init.d/xencommons.in.in
index 3939bcc..b311bb8 100644
--- a/tools/hotplug/Linux/init.d/xencommons.in
+++ b/tools/hotplug/Linux/init.d/xencommons.in.in
@@ -18,6 +18,8 @@
 # Description:       Starts and stops the daemons neeeded for xl/xend
 ### END INIT INFO
 
+XENSTORED=@XENSTORED@
+
 . /etc/xen/scripts/hotplugpath.sh
 
 if [ -d /etc/sysconfig ]; then
@@ -69,12 +71,6 @@ do_start () {
 		if [ -n "$XENSTORED" ] ; then
 		    echo -n Starting $XENSTORED...
 		    $XENSTORED --pid-file /var/run/xenstored.pid $XENSTORED_ARGS
-		elif [ -x ${SBINDIR}/oxenstored ] ; then
-		    echo -n Starting oxenstored...
-		    ${SBINDIR}/oxenstored --pid-file /var/run/xenstored.pid $XENSTORED_ARGS
-		elif [ -x ${SBINDIR}/xenstored ] ; then
-		    echo -n Starting C xenstored...
-		    ${SBINDIR}/xenstored --pid-file /var/run/xenstored.pid $XENSTORED_ARGS
 		else
 		    echo "No xenstored found"
 		    exit 1
-- 
2.0.1

[PATCH v7 8/8] systemd: add xen systemd service and module files

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

This adds the systemd xen service / module files and integrates
support into the build system.

This goes in with AX_AVAILABLE_SYSTEMD() which will enable
systemd if development libraries have been found on your
build system. If you don't have systemd on target systems
for binaries built with systemd then the binary will not
work, you must explicitly disable systemd support if you
do not want to build systemd support.

When systemd libraries are present only systems that
have booted into systemd go through the systemd initialization,
otherwise the legacy init is used.

These are originally based on the Fedora systemd files.

Changes made from Fedora's systemd files:

  * split sockets into two files to claim different permissions
  * Use /bin/sh -c exec to for a simple launcher implementation
  * enables systemd socket activation for C xenstored and Ocaml
    oxenstored
  * use sd_notify(), so change the service to Type=notify, because of
    this we remove the PIDFile specification as we don't care for it, and let
    systemd do its magic for us, this also means we don't have to fork
    so we use --no-fork with systemd
  * defines a modules-load.d, its original source file will be shared
    between systemd and old init systems
  * simplify service files with ConditionVirtualization=xen which uses
    the built in systemd virtualization backend detection, these
    service files will not be available to start on systems that do not
    boot with xen as a hypervisor
  * use autoconf to replace @variable@ paths for us which piggy
    backs on top of the latest autoconf changes to xen
  * removes oxenstored service file in favor of a system variable which
    controls which which xentored to use at run time, we avoid multiple
    service files this way.
  * simplifies startup to not require polling on the sockets
    as initial socket management is handled by systemd, we just
    take on the socket later once anything pokes at it, a simple nc -U
    (as root) on any of the sockets files can activate the service for example.
    Anything queued up will be sent to us once we start. Socket activation
    should in theory also let us dynamically switch between xenstores but more
    importantly we could upgrade xenstored while keeping all active
    socket communication queued up, but in order to take advantage of
    this we eventually would need to remove the requirement of not being
    able to bring down the xenstored. Even though active sockets are
    supported since most libxl communication doesn't triggger a check
    on the unix socket first administrators are encouraged to enable
    the xenstored.service to triggger an initialization of the xenstored
    upon bring up. Some systems also never use unix sockets for
    communication with the xenstored and as such active sockets will
    not be used there.
  * allow for xenstored configuration through *either* of these
    configuration files:
	- /etc/sysconfig/xenstored
	- /etc/default/xenstored
    The /etc/default/xenstored will let debian based systems do
    the same, while SUSE/OpenSUSE/Fedora/RedHat can keep on chugging
    with sysconfig. We leave these files all commented out by default
    though given that for systemd we want to encourage not using them.
  * ensures we create the run directory as most systems will likely
    be using a tmpfs for run dirs for the pid files
  * Some systems define the selinux context in the systemd Option for the
    /var/lib/xenstored tmpfs:
	Options=mode=755,context="system_u:object_r:xenstored_var_lib_t:s0"
    For the upstream version we remove that and let systems specify the
    context on their system /etc/default/xenstored or /etc/sysconfig/xenstored
    $XENSTORED_MOUNT_CTX variable, with a default to none.
  * takes advantage of the shared xendomains helper for the xendomains
    service
  * Add the new dom0 that gets kicked off for disk backend access into
    its own systemd service associated to xen

We end up with these systemd files:

General requirements:

  * proc-xen.mount
  * var-lib-xenstored.mount

xenstored:

  * xenstored.service
  * xenstored.socket
  * xenstored_ro.socket
  * xenconsoled.service
  * xen-qemu-dom0-disk-backend.service.in

Optional:

  * xendomains.service
  * xen-watchdog.service

As for integration with xen, we house keep all the systemd files
under a new directory tools/hotplug/Linux/systemd/ and will be targeted
by default when building on Linux systems if systemd development
libraries are present at build time.

The systemd files will be sanitized for meta @VARIABLES@ upon
configuration and installed upon the install target. Systems that
do not use systemd can still get systemd service unit files installed
if the build system enabled systemd support, this however does not
mandate a requirement of having systemd libraries present. Old init
scripts are always installed.

If you don't specify a prefix you will end up with the services
files under /usr/local/lib/systemd/system/ by default, and systemd
modules-load.d conf files under /usr/local/lib/modules-load.d/ which
systemd does look for (although it seems this is not documented).

Distributions are expected to provide their /usr/ prefix to end up in
the more generic location upon distribution install at
/usr/lib/systemd/system/ and /usr/lib/modules-load.d/ respectively.

Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Jan Rękorajski <baggins@pld-linux.org>
Cc: M A Young <m.a.young@durham.ac.uk>
Cc: Jacek Konieczny <jajcus@jajcus.net>
Cc: xen-devel@lists.xenproject.org
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---

Please run autogen.sh after merging this patch.

 .gitignore                                         |   5 +
 Makefile                                           |   6 +-
 README                                             |  18 +++
 config/Tools.mk.in                                 |   6 +
 m4/README.source                                   |   8 ++
 m4/systemd.m4                                      | 123 +++++++++++++++++++++
 tools/configure.ac                                 |  11 ++
 tools/hotplug/Linux/Makefile                       |   8 +-
 tools/hotplug/Linux/systemd/Makefile               |  67 +++++++++++
 tools/hotplug/Linux/systemd/proc-xen.mount.in      |   9 ++
 .../Linux/systemd/var-lib-xenstored.mount.in       |  13 +++
 .../systemd/xen-qemu-dom0-disk-backend.service.in  |  22 ++++
 .../hotplug/Linux/systemd/xen-watchdog.service.in  |  13 +++
 tools/hotplug/Linux/systemd/xenconsoled.service.in |  20 ++++
 tools/hotplug/Linux/systemd/xendomains.service.in  |  16 +++
 tools/hotplug/Linux/systemd/xenstored.service.in   |  27 +++++
 tools/hotplug/Linux/systemd/xenstored.socket.in    |  11 ++
 tools/hotplug/Linux/systemd/xenstored_ro.socket.in |  11 ++
 tools/ocaml/xenstored/Makefile                     |   5 +
 tools/xenstore/Makefile                            |   6 +
 20 files changed, 401 insertions(+), 4 deletions(-)
 create mode 100644 m4/systemd.m4
 create mode 100644 tools/hotplug/Linux/systemd/Makefile
 create mode 100644 tools/hotplug/Linux/systemd/proc-xen.mount.in
 create mode 100644 tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in
 create mode 100644 tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in
 create mode 100644 tools/hotplug/Linux/systemd/xen-watchdog.service.in
 create mode 100644 tools/hotplug/Linux/systemd/xenconsoled.service.in
 create mode 100644 tools/hotplug/Linux/systemd/xendomains.service.in
 create mode 100644 tools/hotplug/Linux/systemd/xenstored.service.in
 create mode 100644 tools/hotplug/Linux/systemd/xenstored.socket.in
 create mode 100644 tools/hotplug/Linux/systemd/xenstored_ro.socket.in

diff --git a/.gitignore b/.gitignore
index f1d1b9c..6d725aa 100644
--- a/.gitignore
+++ b/.gitignore
@@ -353,3 +353,8 @@ tools/xenstore/xenstore-watch
 docs/txt/misc/*.txt
 docs/txt/man/*.txt
 docs/figs/*.png
+
+tools/hotplug/Linux/systemd/*.conf
+tools/hotplug/Linux/systemd/*.mount
+tools/hotplug/Linux/systemd/*.socket
+tools/hotplug/Linux/systemd/*.service
diff --git a/Makefile b/Makefile
index 41dabbf..104e39d 100644
--- a/Makefile
+++ b/Makefile
@@ -216,8 +216,12 @@ uninstall:
 	rm -f  $(D)$(CONFIG_DIR)/udev/rules.d/xen-backend.rules
 	rm -f  $(D)$(CONFIG_DIR)/udev/rules.d/xend.rules
 	rm -f  $(D)$(SYSCONFIG_DIR)/xendomains
+	rm -f  $(D)$(SBINDIR)/xendomains
 	rm -f  $(D)$(SYSCONFIG_DIR)/xencommons
-	rm -rf $(D)/var/run/xen* $(D)/var/lib/xen*
+	rm -f  $(D)$(XEN_SYSTEMD_DIR)/*.service
+	rm -f  $(D)$(XEN_SYSTEMD_DIR)/*.mount
+	rm -f  $(D)$(XEN_SYSTEMD_MODULES_LOAD)/*.conf
+	rm -rf $(D)${XEN_RUN_DIR}* $(D)/var/lib/xen*
 	make -C tools uninstall
 	rm -rf $(D)/boot/tboot*
 
diff --git a/README b/README
index 0a19eb6..45528b0 100644
--- a/README
+++ b/README
@@ -72,6 +72,7 @@ disabled at compile time:
     * cmake (if building vtpm stub domains)
     * markdown
     * figlet (for generating the traditional Xen start of day banner)
+    * systemd daemon development files
 
 Second, you need to acquire a suitable kernel for use in domain 0. If
 possible you should use a kernel provided by your OS distributor. If
@@ -166,6 +167,23 @@ change take effect.
 [1] http://wiki.xen.org/wiki/XenStoreReference
 [2] http://wiki.xen.org/wiki/Xenstored
 
+Systemd and legacy init support
+===============================
+
+If you have systemd development packages installed you can build binaries
+with systemd support. Systemd support is enabled by default if you have
+systemd development libraries present. If you want to force enable systemd to
+ensure you build binaries with systemd support you can use the --enable-systemd
+flag. Likewise if you want to force disable systemd you can use either of
+these two options:
+
+	./configure --disable-systemd
+	./configure --enable-systemd=no
+
+For more details refer to the xen xenstored systemd wiki page [3].
+
+[3] http://wiki.xen.org/wiki/Xenstored#xenstored_systemd_support
+
 Python Runtime Libraries
 ========================
 
diff --git a/config/Tools.mk.in b/config/Tools.mk.in
index 748cc69..974e28e 100644
--- a/config/Tools.mk.in
+++ b/config/Tools.mk.in
@@ -63,6 +63,12 @@ CONFIG_BLKTAP2      := @blktap2@
 CONFIG_VTPM         := @vtpm@
 CONFIG_QEMUU_EXTRA_ARGS:= @EXTRA_QEMUU_CONFIGURE_ARGS@
 
+CONFIG_SYSTEMD      := @systemd@
+SYSTEMD_CFLAGS      := @SYSTEMD_CFLAGS@
+SYSTEMD_LIBS        := @SYSTEMD_LIBS@
+XEN_SYSTEMD_DIR     := @SYSTEMD_DIR@
+XEN_SYSTEMD_MODULES_LOAD := @SYSTEMD_MODULES_LOAD@
+
 #System options
 ZLIB                := @zlib@
 CONFIG_LIBICONV     := @libiconv@
diff --git a/m4/README.source b/m4/README.source
index 8922ea0..21850a6 100644
--- a/m4/README.source
+++ b/m4/README.source
@@ -26,3 +26,11 @@ Date:   Mon Feb 3 15:59:18 2014 -0800
     With the newly added glib.mk, some of the noinst_* variables need to use
     += in the evaluation to avoid multiple definition warnings from
     automake.
+
+systemd.m4
+==========
+
+systemd.m4 was contributed to by Luis R. Rodriguez <mcgrof@do-not-panic.com>,
+its current home project can be found at:
+
+https://github.com/mcgrof/funk-systemd
diff --git a/m4/systemd.m4 b/m4/systemd.m4
new file mode 100644
index 0000000..760bbad
--- /dev/null
+++ b/m4/systemd.m4
@@ -0,0 +1,123 @@
+# systemd.m4 - Macros to check for and enable systemd          -*- Autoconf -*-
+#
+# Copyright (C) 2014 Luis R. Rodriguez <mcgrof@suse.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software Foundation, Inc.,
+# 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+
+dnl Some optional path options
+AC_DEFUN([AX_SYSTEMD_OPTIONS], [
+	AC_ARG_WITH(systemd, [  --with-systemd          set directory for systemd service files],
+		SYSTEMD_DIR="$withval", SYSTEMD_DIR="")
+	AC_SUBST(SYSTEMD_DIR)
+
+	AC_ARG_WITH(systemd, [  --with-systemd-modules-load          set directory for systemd modules load files],
+		SYSTEMD_MODULES_LOAD="$withval", SYSTEMD_MODULES_LOAD="")
+	AC_SUBST(SYSTEMD_MODULES_LOAD)
+])
+
+AC_DEFUN([AX_ENABLE_SYSTEMD_OPTS], [
+	AX_ARG_DEFAULT_ENABLE([systemd], [Disable systemd support])
+	AX_SYSTEMD_OPTIONS()
+])
+
+AC_DEFUN([AX_ALLOW_SYSTEMD_OPTS], [
+	AX_ARG_DEFAULT_DISABLE([systemd], [Enable systemd support])
+	AX_SYSTEMD_OPTIONS()
+])
+
+AC_DEFUN([AX_CHECK_SYSTEMD_LIBS], [
+	AC_CHECK_HEADER([systemd/sd-daemon.h], [
+	    AC_CHECK_LIB([systemd-daemon], [sd_listen_fds], [libsystemddaemon="y"])
+	])
+	AS_IF([test "x$libsystemddaemon" = x], [
+	    AC_MSG_ERROR([Unable to find a suitable libsystemd-daemon library])
+	])
+
+	PKG_CHECK_MODULES([SYSTEMD], [libsystemd-daemon])
+	dnl pkg-config older than 0.24 does not set these for
+	dnl PKG_CHECK_MODULES() worth also noting is that as of version 208
+	dnl of systemd pkg-config --cflags currently yields no extra flags yet.
+	AC_SUBST([SYSTEMD_CFLAGS])
+	AC_SUBST([SYSTEMD_LIBS])
+
+	AS_IF([test "x$SYSTEMD_DIR" = x], [
+	    dnl In order to use the line below we need to fix upstream systemd
+	    dnl to properly ${prefix} for child variables in
+	    dnl src/core/systemd.pc.in but this is a bit complex at the
+	    dnl moment as they depend on another rootprefix, which can vary
+	    dnl from prefix in practice. We provide our own definition as we
+	    dnl *know* where systemd will dump this to, but this does limit
+	    dnl us to stick to a non custom systemdsystemunitdir, to work
+	    dnl around this we provide the additional configure option
+	    dnl --with-systemd where you can specify the directory for the unit
+	    dnl files. It would also be best to just extend the upstream
+	    dnl pkg-config  pkg.m4 with an AC_DEFUN() to do this neatly.
+	    dnl SYSTEMD_DIR="`$PKG_CONFIG --define-variable=prefix=$PREFIX --variable=systemdsystemunitdir systemd`"
+	    SYSTEMD_DIR="\$(prefix)/lib/systemd/system/"
+	], [])
+
+	AS_IF([test "x$SYSTEMD_DIR" = x], [
+	    AC_MSG_ERROR([SYSTEMD_DIR is unset])
+	], [])
+
+	dnl There is no variable for this yet for some reason
+	AS_IF([test "x$SYSTEMD_MODULES_LOAD" = x], [
+	    SYSTEMD_MODULES_LOAD="\$(prefix)/lib/modules-load.d/"
+	], [])
+
+	AS_IF([test "x$SYSTEMD_MODULES_LOAD" = x], [
+	    AC_MSG_ERROR([SYSTEMD_MODULES_LOAD is unset])
+	], [])
+])
+
+AC_DEFUN([AX_CHECK_SYSTEMD], [
+	dnl Respect user override to disable
+	AS_IF([test "x$enable_systemd" != "xno"], [
+	     AS_IF([test "x$systemd" = "xy" ], [
+		AC_DEFINE([HAVE_SYSTEMD], [1], [Systemd available and enabled])
+			systemd=y
+			AX_CHECK_SYSTEMD_LIBS()
+	    ],[systemd=n])
+	],[systemd=n])
+])
+
+AC_DEFUN([AX_CHECK_SYSTEMD_ENABLE_AVAILABLE], [
+	AC_CHECK_HEADER([systemd/sd-daemon.h], [
+	    AC_CHECK_LIB([systemd-daemon], [sd_listen_fds], [systemd="y"])
+	])
+])
+
+dnl Enables systemd by default and requires a --disable-systemd option flag
+dnl to configure if you want to disable.
+AC_DEFUN([AX_ENABLE_SYSTEMD], [
+	AX_ENABLE_SYSTEMD_OPTS()
+	AX_CHECK_SYSTEMD()
+])
+
+dnl Systemd will be disabled by default and requires you to run configure with
+dnl --enable-systemd to look for and enable systemd.
+AC_DEFUN([AX_ALLOW_SYSTEMD], [
+	AX_ALLOW_SYSTEMD_OPTS()
+	AX_CHECK_SYSTEMD()
+])
+
+dnl Systemd will be disabled by default but if your build system is detected
+dnl to have systemd build libraries it will be enabled. You can always force
+dnl disable with --disable-systemd
+AC_DEFUN([AX_AVAILABLE_SYSTEMD], [
+	AX_ALLOW_SYSTEMD_OPTS()
+	AX_CHECK_SYSTEMD_ENABLE_AVAILABLE()
+	AX_CHECK_SYSTEMD()
+])
diff --git a/tools/configure.ac b/tools/configure.ac
index e74fe4b..f44b9f3 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -9,6 +9,15 @@ AC_CONFIG_FILES([
 ../config/Tools.mk
 hotplug/Linux/init.d/xencommons.in
 hotplug/Linux/init.d/sysconfig.xencommons
+hotplug/Linux/systemd/proc-xen.mount
+hotplug/Linux/systemd/var-lib-xenstored.mount
+hotplug/Linux/systemd/xenstored.socket
+hotplug/Linux/systemd/xenstored_ro.socket
+hotplug/Linux/systemd/xenstored.service
+hotplug/Linux/systemd/xenconsoled.service
+hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service
+hotplug/Linux/systemd/xendomains.service
+hotplug/Linux/systemd/xen-watchdog.service
 ])
 AC_CONFIG_HEADERS([config.h])
 AC_CONFIG_AUX_DIR([../])
@@ -59,6 +68,7 @@ m4_include([../m4/fetcher.m4])
 m4_include([../m4/ax_compare_version.m4])
 m4_include([../m4/paths.m4])
 m4_include([../m4/xenstored.m4])
+m4_include([../m4/systemd.m4])
 
 AX_XEN_EXPAND_CONFIG()
 
@@ -310,5 +320,6 @@ AC_CHECK_HEADERS([yajl/yajl_version.h sys/eventfd.h valgrind/memcheck.h utmp.h])
 
 fi # ! $rump
 
+AX_AVAILABLE_SYSTEMD()
 AC_OUTPUT()
 
diff --git a/tools/hotplug/Linux/Makefile b/tools/hotplug/Linux/Makefile
index 1805746..63ec914 100644
--- a/tools/hotplug/Linux/Makefile
+++ b/tools/hotplug/Linux/Makefile
@@ -25,6 +25,8 @@ XEN_SCRIPTS += vscsi
 XEN_SCRIPTS += block-iscsi
 XEN_SCRIPTS += $(XEN_SCRIPTS-y)
 
+SUBDIRS-$(CONFIG_SYSTEMD) += systemd
+
 XEN_SCRIPT_DATA = xen-script-common.sh locking.sh logging.sh
 XEN_SCRIPT_DATA += xen-hotplug-common.sh xen-network-common.sh vif-common.sh
 XEN_SCRIPT_DATA += block-common.sh
@@ -33,7 +35,7 @@ UDEV_RULES_DIR = $(CONFIG_DIR)/udev
 UDEV_RULES = xen-backend.rules $(UDEV_RULES-y)
 
 .PHONY: all
-all: $(XENCOMMONS_INITD)
+all: $(XENCOMMONS_INITD) subdirs-all
 
 $(XENCOMMONS_INITD): $(XEN_ROOT)/config/$(XEN_OS).modules $(XENCOMMONS_INITD).in
 	@set -e ;							\
@@ -67,7 +69,7 @@ $(XENCOMMONS_INITD): $(XEN_ROOT)/config/$(XEN_OS).modules $(XENCOMMONS_INITD).in
 build: all
 
 .PHONY: install
-install: all install-initd install-scripts install-udev
+install: all install-initd install-scripts install-udev subdirs-install
 
 # See docs/misc/distro_mapping.txt for INITD_DIR location
 .PHONY: install-initd
@@ -105,4 +107,4 @@ install-udev:
 	done
 
 .PHONY: clean
-clean:
+clean: subdirs-clean
diff --git a/tools/hotplug/Linux/systemd/Makefile b/tools/hotplug/Linux/systemd/Makefile
new file mode 100644
index 0000000..dc98b67
--- /dev/null
+++ b/tools/hotplug/Linux/systemd/Makefile
@@ -0,0 +1,67 @@
+XEN_ROOT = $(CURDIR)/../../../..
+include $(XEN_ROOT)/tools/Rules.mk
+
+XEN_SYSTEMD_MODULES = xen.conf
+
+XEN_SYSTEMD_MOUNT =  proc-xen.mount
+XEN_SYSTEMD_MOUNT += var-lib-xenstored.mount
+
+XEN_SYSTEMD_SOCKET  = xenstored.socket
+XEN_SYSTEMD_SOCKET += xenstored_ro.socket
+
+XEN_SYSTEMD_SERVICE  = xenstored.service
+XEN_SYSTEMD_SERVICE += xenconsoled.service
+XEN_SYSTEMD_SERVICE += xen-qemu-dom0-disk-backend.service
+XEN_SYSTEMD_SERVICE += xendomains.service
+XEN_SYSTEMD_SERVICE += xen-watchdog.service
+
+ALL_XEN_SYSTEMD =	$(XEN_SYSTEMD_MODULES)  \
+			$(XEN_SYSTEMD_MOUNT)	\
+			$(XEN_SYSTEMD_SOCKET)	\
+			$(XEN_SYSTEMD_SERVICE)
+
+.PHONY: all
+all:	$(ALL_XEN_SYSTEMD)
+
+.PHONY: clean
+clean:
+
+.PHONY: install
+install: $(ALL_XEN_SYSTEMD)
+	[ -d $(DESTDIR)$(XEN_SYSTEMD_DIR) ] || \
+		$(INSTALL_DIR) $(DESTDIR)$(XEN_SYSTEMD_DIR)
+	[ -d $(DESTDIR)$(XEN_SYSTEMD_MODULES_LOAD) ] || \
+		$(INSTALL_DIR) $(DESTDIR)$(XEN_SYSTEMD_MODULES_LOAD)
+	$(INSTALL_DATA) *.socket $(DESTDIR)$(XEN_SYSTEMD_DIR)
+	$(INSTALL_DATA) *.service $(DESTDIR)$(XEN_SYSTEMD_DIR)
+	$(INSTALL_DATA) *.mount $(DESTDIR)$(XEN_SYSTEMD_DIR)
+	$(INSTALL_DATA) *.conf $(DESTDIR)$(XEN_SYSTEMD_MODULES_LOAD)
+
+$(XEN_SYSTEMD_MODULES): $(XEN_ROOT)/config/$(XEN_OS).modules
+	@set -e ;							\
+	IFS=''								;\
+	cat  $(XEN_ROOT)/config/$(XEN_OS).modules	| (		\
+		while read l ; do					\
+			if echo $${l} | egrep -q "^#" ; then		\
+				continue				;\
+			fi						;\
+			if echo "$${l}" | egrep -q "\|" ; then		\
+				m1=$${l%%|*}				;\
+				m2=$${l#*|} 				;\
+				# Systemd modules-load.d lacks support	;\
+				# for module replacement options, we	;\
+				# need to add that support upstream but ;\
+				# its best instead to ensure this file	;\
+				# is no longer needed. Some folks	;\
+				# however have reported issues with	;\
+				# some modules automatically loading	;\
+				# so we just load all necessary xen	;\
+				# modules and for replacements we load	;\
+				# the latest module			;\
+				echo "$$m1" ;\
+				echo "$$m2" ;\
+			else						\
+				echo "$$l"				;\
+			fi						;\
+		done							\
+	) > $@
diff --git a/tools/hotplug/Linux/systemd/proc-xen.mount.in b/tools/hotplug/Linux/systemd/proc-xen.mount.in
new file mode 100644
index 0000000..f0c4f3a
--- /dev/null
+++ b/tools/hotplug/Linux/systemd/proc-xen.mount.in
@@ -0,0 +1,9 @@
+[Unit]
+Description=Mount /proc/xen files
+ConditionVirtualization=xen
+RefuseManualStop=true
+
+[Mount]
+What=xenfs
+Where=/proc/xen
+Type=xenfs
diff --git a/tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in b/tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in
new file mode 100644
index 0000000..44dfce8
--- /dev/null
+++ b/tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in
@@ -0,0 +1,13 @@
+[Unit]
+Description=mount xenstore file system
+ConditionVirtualization=xen
+RefuseManualStop=true
+
+[Mount]
+Environment=XENSTORED_MOUNT_CTX=none
+EnvironmentFile=-/etc/sysconfig/xenstored
+EnvironmentFile=-/etc/default/xenstored
+What=xenstore
+Where=@XEN_LIB_STORED@
+Type=tmpfs
+Options=mode=755,context="$XENSTORED_MOUNT_CTX"
diff --git a/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in b/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in
new file mode 100644
index 0000000..8dbd110
--- /dev/null
+++ b/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in
@@ -0,0 +1,22 @@
+[Unit]
+Description=qemu for xen dom0 disk backend
+Requires=proc-xen.mount var-lib-xenstored.mount xenstored.socket
+After=xenstored.service xenconsoled.service
+Before=xendomains.service libvirtd.service libvirt-guests.service
+RefuseManualStop=true
+ConditionVirtualization=xen
+
+[Service]
+Type=simple
+EnvironmentFile=-/etc/default/xenstored
+EnvironmentFile=-/etc/sysconfig/xenstored
+PIDFile=@XEN_RUN_DIR@/qemu-dom0.pid
+ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities
+ExecStartPre=/bin/mkdir -p /var/run/xen
+ExecStart=@LIBEXEC@/qemu-system-i386 -xen-domid 0 \
+	-xen-attach -name dom0 -nographic -M xenpv -daemonize \
+	-monitor /dev/null -serial /dev/null -parallel /dev/null \
+	-pidfile @XEN_RUN_DIR@/qemu-dom0.pid
+
+[Install]
+WantedBy=multi-user.target
diff --git a/tools/hotplug/Linux/systemd/xen-watchdog.service.in b/tools/hotplug/Linux/systemd/xen-watchdog.service.in
new file mode 100644
index 0000000..acb2b77
--- /dev/null
+++ b/tools/hotplug/Linux/systemd/xen-watchdog.service.in
@@ -0,0 +1,13 @@
+[Unit]
+Description=Xen-watchdog - run xen watchdog daemon
+Requires=proc-xen.mount
+After=proc-xen.mount xendomains.service
+ConditionVirtualization=xen
+
+[Service]
+Type=forking
+ExecStart=@SBINDIR@/xenwatchdogd 30 15
+KillSignal=USR1
+
+[Install]
+WantedBy=multi-user.target
diff --git a/tools/hotplug/Linux/systemd/xenconsoled.service.in b/tools/hotplug/Linux/systemd/xenconsoled.service.in
new file mode 100644
index 0000000..15fad35
--- /dev/null
+++ b/tools/hotplug/Linux/systemd/xenconsoled.service.in
@@ -0,0 +1,20 @@
+[Unit]
+Description=Xenconsoled - handles logging from guest consoles and hypervisor
+Requires=xenstored.socket
+After=xenstored.service
+ConditionVirtualization=xen
+
+[Service]
+Type=simple
+Environment=XENCONSOLED_ARGS=
+Environment=XENCONSOLED_LOG=none
+Environment=XENCONSOLED_LOG_DIR=@XEN_LOG_DIR@/console
+EnvironmentFile=-/etc/default/xenconsoled
+EnvironmentFile=-/etc/sysconfig/xenconsoled
+PIDFile=@XEN_RUN_DIR@/xenconsoled.pid
+ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities
+ExecStartPre=/bin/mkdir -p ${XENCONSOLED_LOG_DIR}
+ExecStart=@SBINDIR@/xenconsoled --pid-file @XEN_RUN_DIR@/xenconsoled.pid --log=${XENCONSOLED_LOG} --log-dir=${XENCONSOLED_LOG_DIR} $XENCONSOLED_ARGS
+
+[Install]
+WantedBy=multi-user.target
diff --git a/tools/hotplug/Linux/systemd/xendomains.service.in b/tools/hotplug/Linux/systemd/xendomains.service.in
new file mode 100644
index 0000000..70ce7c0
--- /dev/null
+++ b/tools/hotplug/Linux/systemd/xendomains.service.in
@@ -0,0 +1,16 @@
+[Unit]
+Description=Xendomains - start and stop guests on boot and shutdown
+Requires=xenstored.socket
+After=xenstored.service xenconsoled.service
+ConditionVirtualization=xen
+
+[Service]
+Type=oneshot
+RemainAfterExit=true
+ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities
+ExecStart=-@LIBEXEC@/xendomains start
+ExecStop=@LIBEXEC@/xendomains stop
+ExecReload=@LIBEXEC@/xendomains restart
+
+[Install]
+WantedBy=multi-user.target
diff --git a/tools/hotplug/Linux/systemd/xenstored.service.in b/tools/hotplug/Linux/systemd/xenstored.service.in
new file mode 100644
index 0000000..4a9fcee
--- /dev/null
+++ b/tools/hotplug/Linux/systemd/xenstored.service.in
@@ -0,0 +1,27 @@
+[Unit]
+Description=The Xen xenstore
+Requires=xenstored_ro.socket xenstored.socket proc-xen.mount var-lib-xenstored.mount
+After=proc-xen.mount var-lib-xenstored.mount
+Before=libvirtd.service libvirt-guests.service
+RefuseManualStop=true
+ConditionVirtualization=xen
+
+[Service]
+Type=notify
+Environment=XENSTORED_ARGS=
+Environment=XENSTORED_ROOTDIR=@XEN_LIB_STORED@
+Environment=XENSTORED=@XENSTORED@
+EnvironmentFile=-/etc/default/xencommons
+EnvironmentFile=-/etc/sysconfig/xencommons
+ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities
+ExecStartPre=-/bin/rm -f @XEN_LIB_STORED@/tdb*
+ExecStartPre=/bin/mkdir -p @XEN_RUN_DIR@
+ExecStart=/bin/sh -c "exec $XENSTORED --no-fork $XENSTORED_ARGS"
+ExecStartPost=-@BINDIR@/xenstore-write "/local/domain/0/name" "Domain-0"
+ExecStartPost=-@BINDIR@/xenstore-write "/local/domain/0/domid" 0
+
+[Install]
+WantedBy=multi-user.target
+Also=xenstored_ro.socket xenstored.socket
+Also=proc-xen.mount
+Also=var-lib-xenstored.mount
diff --git a/tools/hotplug/Linux/systemd/xenstored.socket.in b/tools/hotplug/Linux/systemd/xenstored.socket.in
new file mode 100644
index 0000000..461e4f4
--- /dev/null
+++ b/tools/hotplug/Linux/systemd/xenstored.socket.in
@@ -0,0 +1,11 @@
+[Unit]
+Description=xenstore socket
+ConditionVirtualization=xen
+
+[Socket]
+ListenStream=/var/run/xenstored/socket
+SocketMode=0600
+Service=xenstored.service
+
+[Install]
+WantedBy=sockets.target
diff --git a/tools/hotplug/Linux/systemd/xenstored_ro.socket.in b/tools/hotplug/Linux/systemd/xenstored_ro.socket.in
new file mode 100644
index 0000000..6ab5c28
--- /dev/null
+++ b/tools/hotplug/Linux/systemd/xenstored_ro.socket.in
@@ -0,0 +1,11 @@
+[Unit]
+Description=xenstore ro socket
+ConditionVirtualization=xen
+
+[Socket]
+ListenStream=/var/run/xenstored/socket_ro
+SocketMode=0660
+Service=xenstored.service
+
+[Install]
+WantedBy=sockets.target
diff --git a/tools/ocaml/xenstored/Makefile b/tools/ocaml/xenstored/Makefile
index 382a813..068e04a 100644
--- a/tools/ocaml/xenstored/Makefile
+++ b/tools/ocaml/xenstored/Makefile
@@ -3,6 +3,11 @@ OCAML_TOPLEVEL = $(CURDIR)/..
 include $(OCAML_TOPLEVEL)/common.make
 
 CFLAGS += -I$(XEN_ROOT)/tools/
+CFLAGS-$(CONFIG_SYSTEMD)  += $(SYSTEMD_CFLAGS)
+LDFLAGS-$(CONFIG_SYSTEMD) += $(SYSTEMD_LIBS)
+
+CFLAGS  += $(CFLAGS-y)
+LDFLAGS += $(LDFLAGS-y)
 
 OCAMLINCLUDE += \
 	-I $(OCAML_TOPLEVEL)/libs/xb \
diff --git a/tools/xenstore/Makefile b/tools/xenstore/Makefile
index 08460c8..48f1e96 100644
--- a/tools/xenstore/Makefile
+++ b/tools/xenstore/Makefile
@@ -9,6 +9,12 @@ CFLAGS += -I.
 CFLAGS += -I$(XEN_ROOT)/tools/
 CFLAGS += $(CFLAGS_libxenctrl)
 
+CFLAGS-$(CONFIG_SYSTEMD)  += $(SYSTEMD_CFLAGS)
+LDFLAGS-$(CONFIG_SYSTEMD) += $(SYSTEMD_LIBS)
+
+CFLAGS  += $(CFLAGS-y)
+LDFLAGS += $(LDFLAGS-y)
+
 CLIENTS := xenstore-exists xenstore-list xenstore-read xenstore-rm xenstore-chmod
 CLIENTS += xenstore-write xenstore-ls xenstore-watch
 
-- 
2.0.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v7 4/8] oxenstored: force FD_CLOEXEC with Unix.set_close_on_exec on LSB init

[Ian Campbell]

On Thu, 2014-07-17 at 16:28 -0700, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
> 
> Lets match the systemd active socket activation implementation and
> ensure that FD_CLOEXEC is set by usin Unix.set_close_on_exec. David
> notes oxenstored likely does not exec but there is no harm in being
> careful just in case things change in the future.
> 
> Cc: David Scott <dave.scott@eu.citrix.com>
> Cc: Anil Madhavapeddy <anil@recoil.org>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Vincent Hanquez <Vincent.Hanquez@eu.citrix.com>
> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>

I seem to have acked v6, so again:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH v7 2/8] cxenstored: add support for systemd active sockets

[Ian Campbell]

On Thu, 2014-07-17 at 16:28 -0700, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
> 
> This adds systemd socket activation support for the C xenstored.
> Active sockets enable xenstored to be loaded only if required by a system
> onto which Xen is installed on. Socket activation is handled by
> systemd, once a port for a service which claims a socket is used
> systemd will start the required services for it, on demand. For more
> details on socket activation refer to Lennart's socket-activation
> post regarding this [0].
> 
> Right now this code adds a no-op for this functionality, leaving the
> enablement to be done later once systemd is properly hooked into
> the build system. The socket activation is ordered in aligment with
> the socket activation order passed on to systemd.
> 
> [0] http://0pointer.de/blog/projects/socket-activation2.html
> 
> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> ---
>  tools/xenstore/xenstored_core.c | 104 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 103 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index 47f0722..7f72f68 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -40,6 +40,7 @@
>  #include <signal.h>
>  #include <assert.h>
>  #include <setjmp.h>
> +#include <config.h>
>  
>  #include "utils.h"
>  #include "list.h"
> @@ -54,6 +55,16 @@
>  
>  #include "hashtable.h"
>  
> +#ifndef NO_SOCKETS
> +#if defined(HAVE_SYSTEMD)
> +#define XEN_SYSTEMD_ENABLED 1
> +#endif
> +#endif
> +
> +#if defined(XEN_SYSTEMD_ENABLED)
> +#include <systemd/sd-daemon.h>
> +#endif
> +
>  extern xc_evtchn *xce_handle; /* in xenstored_domain.c */
>  static int xce_pollfd_idx = -1;
>  static struct pollfd *fds;
> @@ -1714,6 +1725,75 @@ static int destroy_fd(void *_fd)
>  	return 0;
>  }
>  
> +#if defined(XEN_SYSTEMD_ENABLED)
> +/* Will work regardless of the order systemd gives them to us */
> +static int xs_get_sd_fd(const char *connect_to)
> +{
> +	int fd = SD_LISTEN_FDS_START;
> +	int r;
> +
> +	while (fd <= SD_LISTEN_FDS_START + 1) {
> +		r = sd_is_socket_unix(fd, SOCK_STREAM, 1, connect_to, 0);
> +		if (r > 0)
> +			return fd;
> +		fd++;
> +	}
> +
> +	return -EBADR;
> +}
> +
> +static int xs_validate_active_socket(const char *connect_to)
> +{
> +	if ((strcmp("/var/run/xenstored/socket_ro", connect_to) != 0) &&
> +	    (strcmp("/var/run/xenstored/socket", connect_to) != 0)) {
> +		sd_notifyf(0, "STATUS=unexpected socket: %s\n"
> +			   "ERRNO=%i",
> +			   connect_to,
> +			   EBADR);
> +		return -EBADR;
> +	}
> +
> +	return xs_get_sd_fd(connect_to);
> +}
> +
> +static void xen_claim_active_sockets(int **psock, int **pro_sock)
> +{
> +	int *sock, *ro_sock;
> +	const char *soc_str = xs_daemon_socket();
> +	const char *soc_str_ro = xs_daemon_socket_ro();
> +	int n;
> +
> +	n = sd_listen_fds(0);
> +	if (n <= 0) {
> +		sd_notifyf(0, "STATUS=Failed to get any active sockets: %s\n"
> +			   "ERRNO=%i",
> +			   strerror(errno),
> +			   errno);
> +		barf_perror("sd_listen_fds() failed\n");
> +	} else if (n > 2) {
> +		fprintf(stderr, SD_ERR "Expected 2 fds but given %d\n", n);
> +		sd_notifyf(0, "STATUS=Mismatch on number (2): %s\n"
> +			   "ERRNO=%d",

You've used %u everywhere else...

Also, you aren't handling the n == 1 case, is that supposed to be an
error or not? Either "Expected 2" is wrong or the conditions need to
differ.

Perhaps you wanted to send n in the STATUS?

Anyway, none of that seems super critical so perhaps you'd prefer to
make whichever changes are appropriate in a followup patch?

(I notice that the same comment seems to apply to the ocaml case in the
too...)

Re: [PATCH v7 5/8] autoconf: xen: move standard path variables to config/Paths.mk.in

[Ian Campbell]

On Thu, 2014-07-17 at 16:28 -0700, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
> 
> This moves all generic path variables to a new the config/Paths.mk.in
> input source file to be processed at configure time, tons of files use
> these so this just share them. This also paves the way to let us
> easily dynamically configure these with autoconf, for now we leave the
> same presets as was present before.
> 
> This work was prompted by looking for an autoconf way to do
> replacements for the hotplug global file, while at it I realized
> that a few other files use the same variables and have in places
> around the tree the same constructs for generating their own
> files. This makes use of the old buildmakevars2file() but generalizes
> the definition of the paths at configure time and spreads the
> new definitions out throughout the build system.
> 
> This has no impact on building the hypervisor and extras/mini-os,
> you do not need to, and are not expected to, run configure to build
> those targets.
> 
> While at it lets add some documentation on the for the two files on
> the source file, we can expand further details on the wiki [0].
> 
> [0] http://wiki.xen.org/wiki/Category:Host_Configuration#System_wide_xen_configuration
> 
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Samuel Thibault <samuel.thibault@ens-lyon.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Tim Deegan <tim@xen.org>
> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>

I think this breaks the case of "cd tools && ./configure" without
configuring at the toplevel. But a) I'm not sure that even works today
and b) I'm not sure we care about supporting that case anyway so:

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

Re: [PATCH v7 6/8] xencommons: move module list into a generic place

[Ian Campbell]

On Thu, 2014-07-17 at 16:28 -0700, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
> 
> This will allows us to share the same module list with
> sysemd, and lets us upkeep it in one place. Document this

"systemd"

> while at it on the top level README and expand on the wiki:
> 
> http://wiki.xen.org/wiki/Category:Host_Configuration#Linux_kernel_modules
> 
> In order to upkeep parallelism builds be explicit about the
> requirement to complete all actions before any installation
> targets.
> 
> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> ---
>  README                                             | 12 ++++++++
>  config/Linux.modules                               | 20 ++++++++++++
>  tools/hotplug/Linux/Makefile                       | 36 +++++++++++++++++++---
>  .../Linux/init.d/{xencommons => xencommons.in}     | 16 +---------
>  4 files changed, 65 insertions(+), 19 deletions(-)
>  create mode 100644 config/Linux.modules
>  rename tools/hotplug/Linux/init.d/{xencommons => xencommons.in} (89%)
> 
> diff --git a/README b/README
> index 9bbe734..c6cc09b 100644
> --- a/README
> +++ b/README
> @@ -183,3 +183,15 @@ There are optional targets as part of Xen's top-level makefile that will
>  download and build tboot: install-tboot, build-tboot, dist-tboot, clean-tboot.
>  These will download the latest tar file from the SourceForge site using wget,
>  then build/install/dist according to Xen's settings.
> +
> +Required Linux modules
> +======================
> +
> +Xen has a set of Linux modules which the init scripts ensure to load before
> +before starting Xen guests. The list of modules are maintained in one place:
> +
> +  * config/modules

It's Linux.modules now.

We try to avoid being to Linux specific, since we do support other OSes
as dom0 and are generally pretty agnostic.

Perhaps we could reword this slightly with s/Linux/kernel/ and
referencing config/$(XEN_OS).modules?

> +
> +For more details refer to:
> +
> +http://wiki.xen.org/wiki/Category:Host_Configuration#Linux_kernel_modules

The text in this is also out of date wrt the filename now.

Also, like previous iterations of the README, the text in that wiki page
also suffers from being a bit dev focused in what is supposed to be an
end user focused page.

> diff --git a/tools/hotplug/Linux/Makefile b/tools/hotplug/Linux/Makefile
> index d5de9e6..1805746 100644
> --- a/tools/hotplug/Linux/Makefile
> +++ b/tools/hotplug/Linux/Makefile
> @@ -33,17 +33,45 @@ UDEV_RULES_DIR = $(CONFIG_DIR)/udev
>  UDEV_RULES = xen-backend.rules $(UDEV_RULES-y)
>  
>  .PHONY: all
> -all:
> +all: $(XENCOMMONS_INITD)
> +
> +$(XENCOMMONS_INITD): $(XEN_ROOT)/config/$(XEN_OS).modules $(XENCOMMONS_INITD).in
> +	@set -e ;							\
> +	IFS=''								;\
> +	cat  $(XEN_ROOT)/config/$(XEN_OS).modules	| (		\
> +		while read l ; do					\
> +			if echo $${l} | egrep -q "^#" ; then		\
> +				continue				;\
> +			fi						;\
> +			if echo "$${l}" | egrep -q "\|" ; then		\
> +				m1=$${l%%|*}				;\
> +				m2=$${l#*|} 				;\
> +				echo "        modprobe $$m1 2>/dev/null || modprobe $$m2 2>/dev/null" ;\
> +			else						\
> +				echo "        modprobe $$l 2>/dev/null"		;\
> +			fi						;\
> +		done							\
> +	) > $(XENCOMMONS_INITD).modules					;\
> +	cat  $(XENCOMMONS_INITD).in	| (				\
> +		while read l ; do					\
> +			if echo "$${l}" | egrep -q "@LOAD_MODULES@" ; then \
> +				cat $(XENCOMMONS_INITD).modules 	;\
> +			else						\
> +				echo $$l				;\
> +			fi						;\
> +		done							\
> +	) > $@
> +	@rm -f $(XENCOMMONS_INITD).modules

Perhaps consider putting this into a mkinitscript shell script and
calling it instead?

Ian.

Re: [PATCH v7 7/8] autoconf: xen: enable explicit preference option for xenstored preference

[Ian Campbell]

On Thu, 2014-07-17 at 16:28 -0700, Luis R. Rodriguez wrote:
> +This file has one relevant variables

just "variable"

> which is specific to the version of xenstored used:

I'm not sure what you mean by this. The variable isn't specific to
either version, it's the path of whichever one. I think you can drop
that entire clause.

> -# Select xenstored implementation
> -#XENSTORED=[oxenstored|xenstored]
> +# Select xenstore implementation, this can be either
> +# of these below. If using systemd it's preferred that you
> +# just edit the xenstored.service unit file and change
> +# the XENSTORED variable there.
> +#
> +# This can be either of:
> +#  * @SBINDIR@/oxenstored
> +#  * @SBINDIR@/xenstored

Does it need to be the full path or is $PATH searched?

> +# Changing this requires a reboot to take effect.
> +#XENSTORED=@XENSTORED@

Re: [PATCH v7 8/8] systemd: add xen systemd service and module files

[Ian Campbell]

On Thu, 2014-07-17 at 16:28 -0700, Luis R. Rodriguez wrote:
> diff --git a/Makefile b/Makefile
> index 41dabbf..104e39d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -216,8 +216,12 @@ uninstall:
>  	rm -f  $(D)$(CONFIG_DIR)/udev/rules.d/xen-backend.rules
>  	rm -f  $(D)$(CONFIG_DIR)/udev/rules.d/xend.rules
>  	rm -f  $(D)$(SYSCONFIG_DIR)/xendomains
> +	rm -f  $(D)$(SBINDIR)/xendomains
>  	rm -f  $(D)$(SYSCONFIG_DIR)/xencommons
> -	rm -rf $(D)/var/run/xen* $(D)/var/lib/xen*
> +	rm -f  $(D)$(XEN_SYSTEMD_DIR)/*.service
> +	rm -f  $(D)$(XEN_SYSTEMD_DIR)/*.mount
> +	rm -f  $(D)$(XEN_SYSTEMD_MODULES_LOAD)/*.conf
> +	rm -rf $(D)${XEN_RUN_DIR}* $(D)/var/lib/xen*

${} rather than $()? I'm a bit concerned that might expand to nothing
and nuke $(D)...
 
> +Systemd and legacy init support

s/legacy/SysVinit/ please, no need to be rude about our old friend.

TBH, you don't mention sysvinit in this para, so the section could
equally be called "Systemd support".

> +===============================
> +
> +If you have systemd development packages installed you can build binaries
> +with systemd support. Systemd support is enabled by default if you have
> +systemd development libraries present. If you want to force enable systemd to
> +ensure you build binaries with systemd support you can use the --enable-systemd
> +flag. Likewise if you want to force disable systemd you can use either of
> +these two options:
> +
> +	./configure --disable-systemd
> +	./configure --enable-systemd=no

It's nice that both work but users only need to know about one of them,
else we are just bamboozling them with needlessly multiple ways to do
things.


> diff --git a/tools/hotplug/Linux/systemd/Makefile b/tools/hotplug/Linux/systemd/Makefile
> new file mode 100644
> index 0000000..dc98b67
> --- /dev/null
> +++ b/tools/hotplug/Linux/systemd/Makefile

I've not reviewed these files since I don't really know much about
systemd specifics. I assume that people who do know them have looked at
some point and/or they are derived from existing ones which work and/or
they will get fixed eventually if they happen to be not quite right.

Re: [PATCH v7 2/8] cxenstored: add support for systemd active sockets

[Ian Campbell]

On Thu, 2014-07-24 at 16:10 +0100, Ian Campbell wrote:
[...]
> Anyway, none of that seems super critical so perhaps you'd prefer to
> make whichever changes are appropriate in a followup patch?
> 
> (I notice that the same comment seems to apply to the ocaml case in the
> too...)

I ended up applying this along with the first four patches (the
*xenstored ones), so please can you fix this (if necessary) in a
followup.

BTW the only reason I didn't take the rest too is that I'm close to
running out of time for review/commit today so I kicked off the my
precommit tests with just this bit while I read the rest (I've now
replied to all I think).

Ian.

Re: [PATCH v7 2/8] cxenstored: add support for systemd active sockets

[Luis R. Rodriguez]

On Thu, Jul 24, 2014 at 04:10:13PM +0100, Ian Campbell wrote:
> 
> On Thu, 2014-07-17 at 16:28 -0700, Luis R. Rodriguez wrote:
> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> > 
> > This adds systemd socket activation support for the C xenstored.
> > Active sockets enable xenstored to be loaded only if required by a system
> > onto which Xen is installed on. Socket activation is handled by
> > systemd, once a port for a service which claims a socket is used
> > systemd will start the required services for it, on demand. For more
> > details on socket activation refer to Lennart's socket-activation
> > post regarding this [0].
> > 
> > Right now this code adds a no-op for this functionality, leaving the
> > enablement to be done later once systemd is properly hooked into
> > the build system. The socket activation is ordered in aligment with
> > the socket activation order passed on to systemd.
> > 
> > [0] http://0pointer.de/blog/projects/socket-activation2.html
> > 
> > Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> > ---
> >  tools/xenstore/xenstored_core.c | 104 +++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 103 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> > index 47f0722..7f72f68 100644
> > --- a/tools/xenstore/xenstored_core.c
> > +++ b/tools/xenstore/xenstored_core.c
> > @@ -40,6 +40,7 @@
> >  #include <signal.h>
> >  #include <assert.h>
> >  #include <setjmp.h>
> > +#include <config.h>
> >  
> >  #include "utils.h"
> >  #include "list.h"
> > @@ -54,6 +55,16 @@
> >  
> >  #include "hashtable.h"
> >  
> > +#ifndef NO_SOCKETS
> > +#if defined(HAVE_SYSTEMD)
> > +#define XEN_SYSTEMD_ENABLED 1
> > +#endif
> > +#endif
> > +
> > +#if defined(XEN_SYSTEMD_ENABLED)
> > +#include <systemd/sd-daemon.h>
> > +#endif
> > +
> >  extern xc_evtchn *xce_handle; /* in xenstored_domain.c */
> >  static int xce_pollfd_idx = -1;
> >  static struct pollfd *fds;
> > @@ -1714,6 +1725,75 @@ static int destroy_fd(void *_fd)
> >  	return 0;
> >  }
> >  
> > +#if defined(XEN_SYSTEMD_ENABLED)
> > +/* Will work regardless of the order systemd gives them to us */
> > +static int xs_get_sd_fd(const char *connect_to)
> > +{
> > +	int fd = SD_LISTEN_FDS_START;
> > +	int r;
> > +
> > +	while (fd <= SD_LISTEN_FDS_START + 1) {
> > +		r = sd_is_socket_unix(fd, SOCK_STREAM, 1, connect_to, 0);
> > +		if (r > 0)
> > +			return fd;
> > +		fd++;
> > +	}
> > +
> > +	return -EBADR;
> > +}
> > +
> > +static int xs_validate_active_socket(const char *connect_to)
> > +{
> > +	if ((strcmp("/var/run/xenstored/socket_ro", connect_to) != 0) &&
> > +	    (strcmp("/var/run/xenstored/socket", connect_to) != 0)) {
> > +		sd_notifyf(0, "STATUS=unexpected socket: %s\n"
> > +			   "ERRNO=%i",
> > +			   connect_to,
> > +			   EBADR);
> > +		return -EBADR;
> > +	}
> > +
> > +	return xs_get_sd_fd(connect_to);
> > +}
> > +
> > +static void xen_claim_active_sockets(int **psock, int **pro_sock)
> > +{
> > +	int *sock, *ro_sock;
> > +	const char *soc_str = xs_daemon_socket();
> > +	const char *soc_str_ro = xs_daemon_socket_ro();
> > +	int n;
> > +
> > +	n = sd_listen_fds(0);
> > +	if (n <= 0) {
> > +		sd_notifyf(0, "STATUS=Failed to get any active sockets: %s\n"
> > +			   "ERRNO=%i",
> > +			   strerror(errno),
> > +			   errno);
> > +		barf_perror("sd_listen_fds() failed\n");
> > +	} else if (n > 2) {
> > +		fprintf(stderr, SD_ERR "Expected 2 fds but given %d\n", n);
> > +		sd_notifyf(0, "STATUS=Mismatch on number (2): %s\n"
> > +			   "ERRNO=%d",
> 
> You've used %u everywhere else...

Sorry this was not clear, I don't see usage of %u in my patches.

> Also, you aren't handling the n == 1 case, is that supposed to be an
> error or not? Either "Expected 2" is wrong or the conditions need to
> differ.

Good catch, yeah that should be an error as well.

> Perhaps you wanted to send n in the STATUS?

There's an fprintf(stderr) right above above that provides the n variables,
when you use fprintf(stderr) that goes the systemd journal as well.

> Anyway, none of that seems super critical so perhaps you'd prefer to
> make whichever changes are appropriate in a followup patch?

Sure.

> (I notice that the same comment seems to apply to the ocaml case in the
> too...)

OK I'll send a follow up.

  Luis

Re: [PATCH v7 6/8] xencommons: move module list into a generic place

[Luis R. Rodriguez]

On Thu, Jul 24, 2014 at 04:35:31PM +0100, Ian Campbell wrote:
> On Thu, 2014-07-17 at 16:28 -0700, Luis R. Rodriguez wrote:
> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> > 
> > This will allows us to share the same module list with
> > sysemd, and lets us upkeep it in one place. Document this
> 
> "systemd"

Fixed.

> > while at it on the top level README and expand on the wiki:
> > 
> > http://wiki.xen.org/wiki/Category:Host_Configuration#Linux_kernel_modules
> > 
> > In order to upkeep parallelism builds be explicit about the
> > requirement to complete all actions before any installation
> > targets.
> > 
> > Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> > ---
> >  README                                             | 12 ++++++++
> >  config/Linux.modules                               | 20 ++++++++++++
> >  tools/hotplug/Linux/Makefile                       | 36 +++++++++++++++++++---
> >  .../Linux/init.d/{xencommons => xencommons.in}     | 16 +---------
> >  4 files changed, 65 insertions(+), 19 deletions(-)
> >  create mode 100644 config/Linux.modules
> >  rename tools/hotplug/Linux/init.d/{xencommons => xencommons.in} (89%)
> > 
> > diff --git a/README b/README
> > index 9bbe734..c6cc09b 100644
> > --- a/README
> > +++ b/README
> > @@ -183,3 +183,15 @@ There are optional targets as part of Xen's top-level makefile that will
> >  download and build tboot: install-tboot, build-tboot, dist-tboot, clean-tboot.
> >  These will download the latest tar file from the SourceForge site using wget,
> >  then build/install/dist according to Xen's settings.
> > +
> > +Required Linux modules
> > +======================
> > +
> > +Xen has a set of Linux modules which the init scripts ensure to load before
> > +before starting Xen guests. The list of modules are maintained in one place:
> > +
> > +  * config/modules
> 
> It's Linux.modules now.

Fixed.

> We try to avoid being to Linux specific, since we do support other OSes
> as dom0 and are generally pretty agnostic.
> 
> Perhaps we could reword this slightly with s/Linux/kernel/ and
> referencing config/$(XEN_OS).modules?

Sure.

> > +
> > +For more details refer to:
> > +
> > +http://wiki.xen.org/wiki/Category:Host_Configuration#Linux_kernel_modules
> 
> The text in this is also out of date wrt the filename now.

Updated.

> Also, like previous iterations of the README, the text in that wiki page
> also suffers from being a bit dev focused in what is supposed to be an
> end user focused page.

Its a wiki so help with editorial stuff is welcomed. I just figured its best
to add more information than less. People tend to complain about documentation.

> > diff --git a/tools/hotplug/Linux/Makefile b/tools/hotplug/Linux/Makefile
> > index d5de9e6..1805746 100644
> > --- a/tools/hotplug/Linux/Makefile
> > +++ b/tools/hotplug/Linux/Makefile
> > @@ -33,17 +33,45 @@ UDEV_RULES_DIR = $(CONFIG_DIR)/udev
> >  UDEV_RULES = xen-backend.rules $(UDEV_RULES-y)
> >  
> >  .PHONY: all
> > -all:
> > +all: $(XENCOMMONS_INITD)
> > +
> > +$(XENCOMMONS_INITD): $(XEN_ROOT)/config/$(XEN_OS).modules $(XENCOMMONS_INITD).in
> > +	@set -e ;							\
> > +	IFS=''								;\
> > +	cat  $(XEN_ROOT)/config/$(XEN_OS).modules	| (		\
> > +		while read l ; do					\
> > +			if echo $${l} | egrep -q "^#" ; then		\
> > +				continue				;\
> > +			fi						;\
> > +			if echo "$${l}" | egrep -q "\|" ; then		\
> > +				m1=$${l%%|*}				;\
> > +				m2=$${l#*|} 				;\
> > +				echo "        modprobe $$m1 2>/dev/null || modprobe $$m2 2>/dev/null" ;\
> > +			else						\
> > +				echo "        modprobe $$l 2>/dev/null"		;\
> > +			fi						;\
> > +		done							\
> > +	) > $(XENCOMMONS_INITD).modules					;\
> > +	cat  $(XENCOMMONS_INITD).in	| (				\
> > +		while read l ; do					\
> > +			if echo "$${l}" | egrep -q "@LOAD_MODULES@" ; then \
> > +				cat $(XENCOMMONS_INITD).modules 	;\
> > +			else						\
> > +				echo $$l				;\
> > +			fi						;\
> > +		done							\
> > +	) > $@
> > +	@rm -f $(XENCOMMONS_INITD).modules
> 
> Perhaps consider putting this into a mkinitscript shell script and
> calling it instead?

OK.

 Luis

Re: [PATCH v7 7/8] autoconf: xen: enable explicit preference option for xenstored preference

[Luis R. Rodriguez]

On Thu, Jul 24, 2014 at 04:40:46PM +0100, Ian Campbell wrote:
> On Thu, 2014-07-17 at 16:28 -0700, Luis R. Rodriguez wrote:
> > +This file has one relevant variables
> 
> just "variable"
> 
> > which is specific to the version of xenstored used:
> 
> I'm not sure what you mean by this. The variable isn't specific to
> either version, it's the path of whichever one. I think you can drop
> that entire clause.

OK. Dropped.

> > -# Select xenstored implementation
> > -#XENSTORED=[oxenstored|xenstored]
> > +# Select xenstore implementation, this can be either
> > +# of these below. If using systemd it's preferred that you
> > +# just edit the xenstored.service unit file and change
> > +# the XENSTORED variable there.
> > +#
> > +# This can be either of:
> > +#  * @SBINDIR@/oxenstored
> > +#  * @SBINDIR@/xenstored
> 
> Does it need to be the full path or is $PATH searched?

The full path is preferred, on legacy init the variable is used
to execute alone, but of course the path will be searched. For
systemd exec $XENSTORED is used, that looks on the path as well.
I think its best to specifcy the full path to be safe and explicit.

  Luis

Re: [PATCH v7 8/8] systemd: add xen systemd service and module files

[Luis R. Rodriguez]

On Thu, Jul 24, 2014 at 04:47:49PM +0100, Ian Campbell wrote:
> On Thu, 2014-07-17 at 16:28 -0700, Luis R. Rodriguez wrote:
> > diff --git a/Makefile b/Makefile
> > index 41dabbf..104e39d 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -216,8 +216,12 @@ uninstall:
> >  	rm -f  $(D)$(CONFIG_DIR)/udev/rules.d/xen-backend.rules
> >  	rm -f  $(D)$(CONFIG_DIR)/udev/rules.d/xend.rules
> >  	rm -f  $(D)$(SYSCONFIG_DIR)/xendomains
> > +	rm -f  $(D)$(SBINDIR)/xendomains
> >  	rm -f  $(D)$(SYSCONFIG_DIR)/xencommons
> > -	rm -rf $(D)/var/run/xen* $(D)/var/lib/xen*
> > +	rm -f  $(D)$(XEN_SYSTEMD_DIR)/*.service
> > +	rm -f  $(D)$(XEN_SYSTEMD_DIR)/*.mount
> > +	rm -f  $(D)$(XEN_SYSTEMD_MODULES_LOAD)/*.conf
> > +	rm -rf $(D)${XEN_RUN_DIR}* $(D)/var/lib/xen*
> 
> ${} rather than $()? I'm a bit concerned that might expand to nothing
> and nuke $(D)...

Adjusted.

> > +Systemd and legacy init support
> 
> s/legacy/SysVinit/ please, no need to be rude about our old friend.

Asdf.

> TBH, you don't mention sysvinit in this para, so the section could
> equally be called "Systemd support".

OK.

> > +===============================
> > +
> > +If you have systemd development packages installed you can build binaries
> > +with systemd support. Systemd support is enabled by default if you have
> > +systemd development libraries present. If you want to force enable systemd to
> > +ensure you build binaries with systemd support you can use the --enable-systemd
> > +flag. Likewise if you want to force disable systemd you can use either of
> > +these two options:
> > +
> > +	./configure --disable-systemd
> > +	./configure --enable-systemd=no
> 
> It's nice that both work but users only need to know about one of them,
> else we are just bamboozling them with needlessly multiple ways to do
> things.

Removed the second one.

> > diff --git a/tools/hotplug/Linux/systemd/Makefile b/tools/hotplug/Linux/systemd/Makefile
> > new file mode 100644
> > index 0000000..dc98b67
> > --- /dev/null
> > +++ b/tools/hotplug/Linux/systemd/Makefile
> 
> I've not reviewed these files since I don't really know much about
> systemd specifics. I assume that people who do know them have looked at
> some point and/or they are derived from existing ones which work and/or
> they will get fixed eventually if they happen to be not quite right.

These were derived from Fedora's systemd files, I've modified them
quite a bit and annotated all changes made on the commit log. All folks
who should review this are on the Cc and have been since v1. Some of
their own pointers have helped to evolve these as well.

  Luis

Re: [PATCH v7 2/8] cxenstored: add support for systemd active sockets

[Ian Campbell]

On Sat, 2014-07-26 at 00:45 +0200, Luis R. Rodriguez wrote:
> > > +	n = sd_listen_fds(0);
> > > +	if (n <= 0) {
> > > +		sd_notifyf(0, "STATUS=Failed to get any active sockets: %s\n"
> > > +			   "ERRNO=%i",
> > > +			   strerror(errno),
> > > +			   errno);
> > > +		barf_perror("sd_listen_fds() failed\n");
> > > +	} else if (n > 2) {
> > > +		fprintf(stderr, SD_ERR "Expected 2 fds but given %d\n", n);
> > > +		sd_notifyf(0, "STATUS=Mismatch on number (2): %s\n"
> > > +			   "ERRNO=%d",
> > 
> > You've used %u everywhere else...
> 
> Sorry this was not clear, I don't see usage of %u in my patches.

Sorry, looks like I meant %i not %u. %i and %d are equivalent, so I
suppose it's just a code consistency nit.

> > Perhaps you wanted to send n in the STATUS?
> 
> There's an fprintf(stderr) right above above that provides the n variables,
> when you use fprintf(stderr) that goes the systemd journal as well.

Hrm, so either the sd_notify or the fprintf is a bit redundant then?

Ian.

Re: [PATCH v7 2/8] cxenstored: add support for systemd active sockets

[Luis R. Rodriguez]

On Mon, Jul 28, 2014 at 10:48:12AM +0100, Ian Campbell wrote:
> On Sat, 2014-07-26 at 00:45 +0200, Luis R. Rodriguez wrote:
> > > > +	n = sd_listen_fds(0);
> > > > +	if (n <= 0) {
> > > > +		sd_notifyf(0, "STATUS=Failed to get any active sockets: %s\n"
> > > > +			   "ERRNO=%i",
> > > > +			   strerror(errno),
> > > > +			   errno);
> > > > +		barf_perror("sd_listen_fds() failed\n");
> > > > +	} else if (n > 2) {
> > > > +		fprintf(stderr, SD_ERR "Expected 2 fds but given %d\n", n);
> > > > +		sd_notifyf(0, "STATUS=Mismatch on number (2): %s\n"
> > > > +			   "ERRNO=%d",
> > > 
> > > You've used %u everywhere else...
> > 
> > Sorry this was not clear, I don't see usage of %u in my patches.
> 
> Sorry, looks like I meant %i not %u. %i and %d are equivalent, so I
> suppose it's just a code consistency nit.

OK.

> > > Perhaps you wanted to send n in the STATUS?
> > 
> > There's an fprintf(stderr) right above above that provides the n variables,
> > when you use fprintf(stderr) that goes the systemd journal as well.
> 
> Hrm, so either the sd_notify or the fprintf is a bit redundant then?

fprintf() is just easier for separate calls you can call one sd_notify() with
the status though so I printed earlier segments with fpritnf().

  Luis

Re: [PATCH v7 2/8] cxenstored: add support for systemd active sockets

[George Dunlap]

On Fri, Jul 18, 2014 at 12:28 AM, Luis R. Rodriguez
<mcgrof@do-not-panic.com> wrote:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
>
> This adds systemd socket activation support for the C xenstored.
> Active sockets enable xenstored to be loaded only if required by a system
> onto which Xen is installed on. Socket activation is handled by
> systemd, once a port for a service which claims a socket is used
> systemd will start the required services for it, on demand. For more
> details on socket activation refer to Lennart's socket-activation
> post regarding this [0].
>
> Right now this code adds a no-op for this functionality, leaving the
> enablement to be done later once systemd is properly hooked into
> the build system. The socket activation is ordered in aligment with
> the socket activation order passed on to systemd.
>
> [0] http://0pointer.de/blog/projects/socket-activation2.html

So with this patch in place, xenstored will not start on a system that
has systemd, *even if it wasn't started from systemd*.

Lots of systems (e.g., CentOS 7) have legacy systems in place to allow
you to do things like "chkconfig --add xencommons" even on a systemd
system.  I think we still want to work with those, right?

 -George

Re: [PATCH v7 2/8] cxenstored: add support for systemd active sockets

[Ian Campbell]

On Wed, 2015-08-05 at 11:06 +0100, George Dunlap wrote:
> On Fri, Jul 18, 2014 at 12:28 AM, Luis R. Rodriguez
> <mcgrof@do-not-panic.com> wrote:
> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> > 
> > This adds systemd socket activation support for the C xenstored.
> > Active sockets enable xenstored to be loaded only if required by a 
> > system
> > onto which Xen is installed on. Socket activation is handled by
> > systemd, once a port for a service which claims a socket is used
> > systemd will start the required services for it, on demand. For more
> > details on socket activation refer to Lennart's socket-activation
> > post regarding this [0].
> > 
> > Right now this code adds a no-op for this functionality, leaving the
> > enablement to be done later once systemd is properly hooked into
> > the build system. The socket activation is ordered in aligment with
> > the socket activation order passed on to systemd.
> > 
> > [0] http://0pointer.de/blog/projects/socket-activation2.html
> 
> So with this patch in place, xenstored will not start on a system that
> has systemd, *even if it wasn't started from systemd*.

But where systemd is /sbin/init, right?

The case where xenstored was compiled with systemd support but systemd is
not /sbin/init should still be expected to work, and isn't what I think you
are complaining about here.

> Lots of systems (e.g., CentOS 7) have legacy systems in place to allow
> you to do things like "chkconfig --add xencommons" even on a systemd
> system.  I think we still want to work with those, right?

Isn't chkconfig --add still arranging for the thing to be started by
systemd under the hood? If not systemd on a host where /sbin/init==systemd
then what does else would start it?

If you are asking "should the sysvinit initscripts still be us(ed|able)
even though systemd is being used as /sbin/init on the host and a unit file
is present" then AIUI the systemd answer is "no". (We may choose to
disagree with systemd on this I suppose)

On the other hand, does this mean I can no longer start xenstored by hand
from the CLI? _That_ would seem to be worth preserving, for debugging etc
if nothing else.

Ian.

Re: [PATCH v7 2/8] cxenstored: add support for systemd active sockets

[George Dunlap]

On Wed, Aug 5, 2015 at 11:17 AM, Ian Campbell <ian.campbell@citrix.com> wrote:
> On Wed, 2015-08-05 at 11:06 +0100, George Dunlap wrote:
>> On Fri, Jul 18, 2014 at 12:28 AM, Luis R. Rodriguez
>> <mcgrof@do-not-panic.com> wrote:
>> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
>> >
>> > This adds systemd socket activation support for the C xenstored.
>> > Active sockets enable xenstored to be loaded only if required by a
>> > system
>> > onto which Xen is installed on. Socket activation is handled by
>> > systemd, once a port for a service which claims a socket is used
>> > systemd will start the required services for it, on demand. For more
>> > details on socket activation refer to Lennart's socket-activation
>> > post regarding this [0].
>> >
>> > Right now this code adds a no-op for this functionality, leaving the
>> > enablement to be done later once systemd is properly hooked into
>> > the build system. The socket activation is ordered in aligment with
>> > the socket activation order passed on to systemd.
>> >
>> > [0] http://0pointer.de/blog/projects/socket-activation2.html
>>
>> So with this patch in place, xenstored will not start on a system that
>> has systemd, *even if it wasn't started from systemd*.
>
> But where systemd is /sbin/init, right?
>
> The case where xenstored was compiled with systemd support but systemd is
> not /sbin/init should still be expected to work, and isn't what I think you
> are complaining about here.
>
>> Lots of systems (e.g., CentOS 7) have legacy systems in place to allow
>> you to do things like "chkconfig --add xencommons" even on a systemd
>> system.  I think we still want to work with those, right?
>
> Isn't chkconfig --add still arranging for the thing to be started by
> systemd under the hood? If not systemd on a host where /sbin/init==systemd
> then what does else would start it?
>
> If you are asking "should the sysvinit initscripts still be us(ed|able)
> even though systemd is being used as /sbin/init on the host and a unit file
> is present" then AIUI the systemd answer is "no". (We may choose to
> disagree with systemd on this I suppose)

Well that's not (apparently) the RHEL answer; doing "chkconfig --add
[foo]" Just Works on CentOS 7 for all the sysvinit scripts I've used
(including the Xen 4.4 Xen4CentOS packages).

I think we want to still *enable* people to use that mode if they want
to.  But I won't argue if people feel strongly otherwise.

> On the other hand, does this mean I can no longer start xenstored by hand
> from the CLI? _That_ would seem to be worth preserving, for debugging etc
> if nothing else.

So what happens at the moment is that xenstored, run either from the
command-line says, "Oh, look!  I'm running on a systemd system.  I'll
check for my systemd sockets.  Oh no, no sockets!  *dies*".

If run from xencommons, it doesn't even get that far: it says, "Oh,
look! I'm running on a systemd system.  But wait! You asked me to use
a pidfile! BAD USER! NO PIDFILE ON SYSTEMD! *dies*".

Modifying xenstored to try to open the systemd sockets, and fall back
to normal sockets if it doesn't find any, works when started from the
command-line.  But for some reason, if you take out the aforementioned
check preventing --pid-file, it still segfaults (!) at some point.  I
haven't tracked down what the problem is there yet; but I don't want
to bother trying if that's not what we're going for. :-)

 -George

Re: [PATCH v7 2/8] cxenstored: add support for systemd active sockets

[Ian Campbell]

On Wed, 2015-08-05 at 11:56 +0100, George Dunlap wrote:
> On Wed, Aug 5, 2015 at 11:17 AM, Ian Campbell <ian.campbell@citrix.com> 
> wrote:
> > On Wed, 2015-08-05 at 11:06 +0100, George Dunlap wrote:
> > > On Fri, Jul 18, 2014 at 12:28 AM, Luis R. Rodriguez
> > > <mcgrof@do-not-panic.com> wrote:
> > > > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> > > > 
> > > > This adds systemd socket activation support for the C xenstored.
> > > > Active sockets enable xenstored to be loaded only if required by a
> > > > system
> > > > onto which Xen is installed on. Socket activation is handled by
> > > > systemd, once a port for a service which claims a socket is used
> > > > systemd will start the required services for it, on demand. For 
> > > > more
> > > > details on socket activation refer to Lennart's socket-activation
> > > > post regarding this [0].
> > > > 
> > > > Right now this code adds a no-op for this functionality, leaving 
> > > > the
> > > > enablement to be done later once systemd is properly hooked into
> > > > the build system. The socket activation is ordered in aligment with
> > > > the socket activation order passed on to systemd.
> > > > 
> > > > [0] http://0pointer.de/blog/projects/socket-activation2.html
> > > 
> > > So with this patch in place, xenstored will not start on a system 
> > > that
> > > has systemd, *even if it wasn't started from systemd*.
> > 
> > But where systemd is /sbin/init, right?
> > 
> > The case where xenstored was compiled with systemd support but systemd 
> > is
> > not /sbin/init should still be expected to work, and isn't what I think 
> > you
> > are complaining about here.
> > 
> > > Lots of systems (e.g., CentOS 7) have legacy systems in place to 
> > > allow
> > > you to do things like "chkconfig --add xencommons" even on a systemd
> > > system.  I think we still want to work with those, right?
> > 
> > Isn't chkconfig --add still arranging for the thing to be started by
> > systemd under the hood? If not systemd on a host where 
> > /sbin/init==systemd
> > then what does else would start it?
> > 
> > If you are asking "should the sysvinit initscripts still be us(ed|able)
> > even though systemd is being used as /sbin/init on the host and a unit 
> > file
> > is present" then AIUI the systemd answer is "no". (We may choose to
> > disagree with systemd on this I suppose)
> 
> Well that's not (apparently) the RHEL answer; doing "chkconfig --add
> [foo]" Just Works on CentOS 7 for all the sysvinit scripts I've used
> (including the Xen 4.4 Xen4CentOS packages).

I would expect that the CentOS 7 packaging guidelines would
require/encourage you to use the systemd unit files (via whatever command
that is) in preference to the sysvinit initscripts when they are available.

AUIU the compatibility works the other way round, which is if you use the
new systemd commands and there is no unit file with that name but there is
a sysv initscript with that name then systemd will invoke the initscript in
a compatibility mode.

I'm a bit surprised that chkconfig doesn't just to the right thing. It's
possible that the fact that our initscript and our systemd unitfiles do not
share the same names has defeated its heuristics.

> I think we want to still *enable* people to use that mode if they want
> to.  But I won't argue if people feel strongly otherwise.
> 
> > On the other hand, does this mean I can no longer start xenstored by 
> > hand
> > from the CLI? _That_ would seem to be worth preserving, for debugging 
> > etc
> > if nothing else.
> 
> So what happens at the moment is that xenstored, run either from the
> command-line says, "Oh, look!  I'm running on a systemd system.  I'll
> check for my systemd sockets.  Oh no, no sockets!  *dies*".

This shouldn't happen...

> If run from xencommons, it doesn't even get that far: it says, "Oh,
> look! I'm running on a systemd system.  But wait! You asked me to use
> a pidfile! BAD USER! NO PIDFILE ON SYSTEMD! *dies*".

This isn't strictly speaking what we want people to be doing (they should
use the systemd units), but I think by properly fixing the first issue this
will make this start working too.

> Modifying xenstored to try to open the systemd sockets, and fall back
> to normal sockets if it doesn't find any, works when started from the
> command-line.  But for some reason, if you take out the aforementioned
> check preventing --pid-file, it still segfaults (!) at some point.  I
> haven't tracked down what the problem is there yet; but I don't want
> to bother trying if that's not what we're going for. :-)

I think the check for socket activation should be gated on a new command
line option as well as the presence of systemd, and the systemd unit file
should pass that option. Then invoking from the CLI works.

Ian.

Re: [PATCH v7 2/8] cxenstored: add support for systemd active sockets

[Ian Campbell]

On Wed, 2015-08-05 at 12:11 +0100, Ian Campbell wrote:
> 
> I'm a bit surprised that chkconfig doesn't just to the right thing. It's
> possible that the fact that our initscript and our systemd unitfiles do not
> share the same names has defeated its heuristics.

Perhaps as well as fixing xenstored we should also add a xencommons meta
-unit which depends on the others, solely for the purpose of having
chkconfig spot that it should use those and not try the initscript?

Re: [PATCH v7 2/8] cxenstored: add support for systemd active sockets

[George Dunlap]

On Wed, Aug 5, 2015 at 12:11 PM, Ian Campbell <ian.campbell@citrix.com> wrote:
> On Wed, 2015-08-05 at 11:56 +0100, George Dunlap wrote:
>> On Wed, Aug 5, 2015 at 11:17 AM, Ian Campbell <ian.campbell@citrix.com>
>> wrote:
>> > On Wed, 2015-08-05 at 11:06 +0100, George Dunlap wrote:
>> > > On Fri, Jul 18, 2014 at 12:28 AM, Luis R. Rodriguez
>> > > <mcgrof@do-not-panic.com> wrote:
>> > > > From: "Luis R. Rodriguez" <mcgrof@suse.com>
>> > > >
>> > > > This adds systemd socket activation support for the C xenstored.
>> > > > Active sockets enable xenstored to be loaded only if required by a
>> > > > system
>> > > > onto which Xen is installed on. Socket activation is handled by
>> > > > systemd, once a port for a service which claims a socket is used
>> > > > systemd will start the required services for it, on demand. For
>> > > > more
>> > > > details on socket activation refer to Lennart's socket-activation
>> > > > post regarding this [0].
>> > > >
>> > > > Right now this code adds a no-op for this functionality, leaving
>> > > > the
>> > > > enablement to be done later once systemd is properly hooked into
>> > > > the build system. The socket activation is ordered in aligment with
>> > > > the socket activation order passed on to systemd.
>> > > >
>> > > > [0] http://0pointer.de/blog/projects/socket-activation2.html
>> > >
>> > > So with this patch in place, xenstored will not start on a system
>> > > that
>> > > has systemd, *even if it wasn't started from systemd*.
>> >
>> > But where systemd is /sbin/init, right?
>> >
>> > The case where xenstored was compiled with systemd support but systemd
>> > is
>> > not /sbin/init should still be expected to work, and isn't what I think
>> > you
>> > are complaining about here.
>> >
>> > > Lots of systems (e.g., CentOS 7) have legacy systems in place to
>> > > allow
>> > > you to do things like "chkconfig --add xencommons" even on a systemd
>> > > system.  I think we still want to work with those, right?
>> >
>> > Isn't chkconfig --add still arranging for the thing to be started by
>> > systemd under the hood? If not systemd on a host where
>> > /sbin/init==systemd
>> > then what does else would start it?
>> >
>> > If you are asking "should the sysvinit initscripts still be us(ed|able)
>> > even though systemd is being used as /sbin/init on the host and a unit
>> > file
>> > is present" then AIUI the systemd answer is "no". (We may choose to
>> > disagree with systemd on this I suppose)
>>
>> Well that's not (apparently) the RHEL answer; doing "chkconfig --add
>> [foo]" Just Works on CentOS 7 for all the sysvinit scripts I've used
>> (including the Xen 4.4 Xen4CentOS packages).
>
> I would expect that the CentOS 7 packaging guidelines would
> require/encourage you to use the systemd unit files (via whatever command
> that is) in preference to the sysvinit initscripts when they are available.
>
> AUIU the compatibility works the other way round, which is if you use the
> new systemd commands and there is no unit file with that name but there is
> a sysv initscript with that name then systemd will invoke the initscript in
> a compatibility mode.
>
> I'm a bit surprised that chkconfig doesn't just to the right thing. It's
> possible that the fact that our initscript and our systemd unitfiles do not
> share the same names has defeated its heuristics.

It seems to me that "the right thing" for chkconfig to do is to run
the script you've asked it to run, not do some other thing you haven't
asked it to do. :-)  If you think about how different systemd is than
sysvinit, the chance of a script with a similar name to a systemd rule
being *actually* interchangeable is pretty low.  If they really want
to push people into using systemd they should remove chkconfig
altogether, or make it print a warning, not do something completely
different.

>> I think we want to still *enable* people to use that mode if they want
>> to.  But I won't argue if people feel strongly otherwise.
>>
>> > On the other hand, does this mean I can no longer start xenstored by
>> > hand
>> > from the CLI? _That_ would seem to be worth preserving, for debugging
>> > etc
>> > if nothing else.
>>
>> So what happens at the moment is that xenstored, run either from the
>> command-line says, "Oh, look!  I'm running on a systemd system.  I'll
>> check for my systemd sockets.  Oh no, no sockets!  *dies*".
>
> This shouldn't happen...
>
>> If run from xencommons, it doesn't even get that far: it says, "Oh,
>> look! I'm running on a systemd system.  But wait! You asked me to use
>> a pidfile! BAD USER! NO PIDFILE ON SYSTEMD! *dies*".
>
> This isn't strictly speaking what we want people to be doing (they should
> use the systemd units), but I think by properly fixing the first issue this
> will make this start working too.
>
>> Modifying xenstored to try to open the systemd sockets, and fall back
>> to normal sockets if it doesn't find any, works when started from the
>> command-line.  But for some reason, if you take out the aforementioned
>> check preventing --pid-file, it still segfaults (!) at some point.  I
>> haven't tracked down what the problem is there yet; but I don't want
>> to bother trying if that's not what we're going for. :-)
>
> I think the check for socket activation should be gated on a new command
> line option as well as the presence of systemd, and the systemd unit file
> should pass that option. Then invoking from the CLI works.

FWIW I now suspect that the crash I was seeing is selinux related.
But this is a lower priority bug for me; I'll come back to it later if
I get a chance.

 -George

Re: [PATCH v7 2/8] cxenstored: add support for systemd active sockets

[Ian Campbell]

On Wed, 2015-08-05 at 12:21 +0100, George Dunlap wrote:
> On Wed, Aug 5, 2015 at 12:11 PM, Ian Campbell <ian.campbell@citrix.com> 
> wrote:
> > On Wed, 2015-08-05 at 11:56 +0100, George Dunlap wrote:
> > > On Wed, Aug 5, 2015 at 11:17 AM, Ian Campbell <
> > > ian.campbell@citrix.com>
> > > wrote:
> > > > On Wed, 2015-08-05 at 11:06 +0100, George Dunlap wrote:
> > > > > On Fri, Jul 18, 2014 at 12:28 AM, Luis R. Rodriguez
> > > > > <mcgrof@do-not-panic.com> wrote:
> > > > > > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> > > > > > 
> > > > > > This adds systemd socket activation support for the C 
> > > > > > xenstored.
> > > > > > Active sockets enable xenstored to be loaded only if required 
> > > > > > by a
> > > > > > system
> > > > > > onto which Xen is installed on. Socket activation is handled by
> > > > > > systemd, once a port for a service which claims a socket is 
> > > > > > used
> > > > > > systemd will start the required services for it, on demand. For
> > > > > > more
> > > > > > details on socket activation refer to Lennart's socket
> > > > > > -activation
> > > > > > post regarding this [0].
> > > > > > 
> > > > > > Right now this code adds a no-op for this functionality, 
> > > > > > leaving
> > > > > > the
> > > > > > enablement to be done later once systemd is properly hooked 
> > > > > > into
> > > > > > the build system. The socket activation is ordered in aligment 
> > > > > > with
> > > > > > the socket activation order passed on to systemd.
> > > > > > 
> > > > > > [0] http://0pointer.de/blog/projects/socket-activation2.html
> > > > > 
> > > > > So with this patch in place, xenstored will not start on a system
> > > > > that
> > > > > has systemd, *even if it wasn't started from systemd*.
> > > > 
> > > > But where systemd is /sbin/init, right?
> > > > 
> > > > The case where xenstored was compiled with systemd support but 
> > > > systemd
> > > > is
> > > > not /sbin/init should still be expected to work, and isn't what I 
> > > > think
> > > > you
> > > > are complaining about here.
> > > > 
> > > > > Lots of systems (e.g., CentOS 7) have legacy systems in place to
> > > > > allow
> > > > > you to do things like "chkconfig --add xencommons" even on a 
> > > > > systemd
> > > > > system.  I think we still want to work with those, right?
> > > > 
> > > > Isn't chkconfig --add still arranging for the thing to be started 
> > > > by
> > > > systemd under the hood? If not systemd on a host where
> > > > /sbin/init==systemd
> > > > then what does else would start it?
> > > > 
> > > > If you are asking "should the sysvinit initscripts still be 
> > > > us(ed|able)
> > > > even though systemd is being used as /sbin/init on the host and a 
> > > > unit
> > > > file
> > > > is present" then AIUI the systemd answer is "no". (We may choose to
> > > > disagree with systemd on this I suppose)
> > > 
> > > Well that's not (apparently) the RHEL answer; doing "chkconfig --add
> > > [foo]" Just Works on CentOS 7 for all the sysvinit scripts I've used
> > > (including the Xen 4.4 Xen4CentOS packages).
> > 
> > I would expect that the CentOS 7 packaging guidelines would
> > require/encourage you to use the systemd unit files (via whatever 
> > command
> > that is) in preference to the sysvinit initscripts when they are 
> > available.
> > 
> > AUIU the compatibility works the other way round, which is if you use the
> > new systemd commands and there is no unit file with that name but there is
> > a sysv initscript with that name then systemd will invoke the initscript in
> > a compatibility mode.
> > 
> > I'm a bit surprised that chkconfig doesn't just to the right thing. It's
> > possible that the fact that our initscript and our systemd unitfiles do not
> > share the same names has defeated its heuristics.
> 
> It seems to me that "the right thing" for chkconfig to do is to run
> the script you've asked it to run, not do some other thing you haven't
> asked it to do. :-)  If you think about how different systemd is than
> sysvinit, the chance of a script with a similar name to a systemd rule
> being *actually* interchangeable is pretty low.  If they really want
> to push people into using systemd they should remove chkconfig
> altogether, or make it print a warning, not do something completely
> different.

There's no point arguing about that here or with me.

Re: [PATCH v7 2/8] cxenstored: add support for systemd active sockets

[Wei Liu]

On Wed, Aug 05, 2015 at 11:56:44AM +0100, George Dunlap wrote:
> On Wed, Aug 5, 2015 at 11:17 AM, Ian Campbell <ian.campbell@citrix.com> wrote:
> > On Wed, 2015-08-05 at 11:06 +0100, George Dunlap wrote:
> >> On Fri, Jul 18, 2014 at 12:28 AM, Luis R. Rodriguez
> >> <mcgrof@do-not-panic.com> wrote:
> >> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> >> >
> >> > This adds systemd socket activation support for the C xenstored.
> >> > Active sockets enable xenstored to be loaded only if required by a
> >> > system
> >> > onto which Xen is installed on. Socket activation is handled by
> >> > systemd, once a port for a service which claims a socket is used
> >> > systemd will start the required services for it, on demand. For more
> >> > details on socket activation refer to Lennart's socket-activation
> >> > post regarding this [0].
> >> >
> >> > Right now this code adds a no-op for this functionality, leaving the
> >> > enablement to be done later once systemd is properly hooked into
> >> > the build system. The socket activation is ordered in aligment with
> >> > the socket activation order passed on to systemd.
> >> >
> >> > [0] http://0pointer.de/blog/projects/socket-activation2.html
> >>
> >> So with this patch in place, xenstored will not start on a system that
> >> has systemd, *even if it wasn't started from systemd*.
> >
> > But where systemd is /sbin/init, right?
> >
> > The case where xenstored was compiled with systemd support but systemd is
> > not /sbin/init should still be expected to work, and isn't what I think you
> > are complaining about here.
> >
> >> Lots of systems (e.g., CentOS 7) have legacy systems in place to allow
> >> you to do things like "chkconfig --add xencommons" even on a systemd
> >> system.  I think we still want to work with those, right?
> >
> > Isn't chkconfig --add still arranging for the thing to be started by
> > systemd under the hood? If not systemd on a host where /sbin/init==systemd
> > then what does else would start it?
> >
> > If you are asking "should the sysvinit initscripts still be us(ed|able)
> > even though systemd is being used as /sbin/init on the host and a unit file
> > is present" then AIUI the systemd answer is "no". (We may choose to
> > disagree with systemd on this I suppose)
> 
> Well that's not (apparently) the RHEL answer; doing "chkconfig --add
> [foo]" Just Works on CentOS 7 for all the sysvinit scripts I've used
> (including the Xen 4.4 Xen4CentOS packages).
> 
> I think we want to still *enable* people to use that mode if they want
> to.  But I won't argue if people feel strongly otherwise.
> 
> > On the other hand, does this mean I can no longer start xenstored by hand
> > from the CLI? _That_ would seem to be worth preserving, for debugging etc
> > if nothing else.
> 
> So what happens at the moment is that xenstored, run either from the
> command-line says, "Oh, look!  I'm running on a systemd system.  I'll
> check for my systemd sockets.  Oh no, no sockets!  *dies*".
> 
> If run from xencommons, it doesn't even get that far: it says, "Oh,
> look! I'm running on a systemd system.  But wait! You asked me to use
> a pidfile! BAD USER! NO PIDFILE ON SYSTEMD! *dies*".
> 
> Modifying xenstored to try to open the systemd sockets, and fall back
> to normal sockets if it doesn't find any, works when started from the
> command-line. 

I have always thought this is the expected behaviour. Just that the code
has a bug.

Here is a patch that is not even compile test. :-)

---8<---
>From 6f050ca085014fc121e2bc2c0ff66feded0cd210 Mon Sep 17 00:00:00 2001
From: Wei Liu <wei.liu2@citrix.com>
Date: Wed, 5 Aug 2015 14:15:27 +0100
Subject: [PATCH] cxenstored: fix systemd activation

Function sd_booted() returns positive number when systemd is running.

Don't use barf when we don't intent to exit the program.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/xenstore/xenstored_core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index b7e4936..3a8e2fe 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1990,10 +1990,10 @@ int main(int argc, char *argv[])
 		barf("%s: No arguments desired", argv[0]);
 
 #if defined(XEN_SYSTEMD_ENABLED)
-	if (sd_booted()) {
+	if (sd_booted() > 0) {
 		dofork = false;
 		if (pidfile)
-			barf("%s: PID file not needed on systemd", argv[0]);
+			xprintf("%s: PID file not needed on systemd", argv[0]);
 		pidfile = NULL;
 	}
 #endif
@@ -2020,7 +2020,7 @@ int main(int argc, char *argv[])
 	signal(SIGPIPE, SIG_IGN);
 
 #if defined(XEN_SYSTEMD_ENABLED)
-	if (sd_booted())
+	if (sd_booted() > 0)
 		xen_claim_active_sockets(&sock, &ro_sock);
 	else
 #endif
@@ -2057,7 +2057,7 @@ int main(int argc, char *argv[])
 	xenbus_notify_running();
 
 #if defined(XEN_SYSTEMD_ENABLED)
-	if (sd_booted()) {
+	if (sd_booted() > 0) {
 		sd_notify(1, "READY=1");
 		fprintf(stderr, SD_NOTICE "xenstored is ready\n");
 	}
-- 
2.4.6


>  -George

Re: [PATCH v7 2/8] cxenstored: add support for systemd active sockets

[George Dunlap]

[-- Attachment #1: Type: text/plain, Size: 4171 bytes --]

On Wed, Aug 5, 2015 at 2:17 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> On Wed, Aug 05, 2015 at 11:56:44AM +0100, George Dunlap wrote:
>> On Wed, Aug 5, 2015 at 11:17 AM, Ian Campbell <ian.campbell@citrix.com> wrote:
>> > On Wed, 2015-08-05 at 11:06 +0100, George Dunlap wrote:
>> >> On Fri, Jul 18, 2014 at 12:28 AM, Luis R. Rodriguez
>> >> <mcgrof@do-not-panic.com> wrote:
>> >> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
>> >> >
>> >> > This adds systemd socket activation support for the C xenstored.
>> >> > Active sockets enable xenstored to be loaded only if required by a
>> >> > system
>> >> > onto which Xen is installed on. Socket activation is handled by
>> >> > systemd, once a port for a service which claims a socket is used
>> >> > systemd will start the required services for it, on demand. For more
>> >> > details on socket activation refer to Lennart's socket-activation
>> >> > post regarding this [0].
>> >> >
>> >> > Right now this code adds a no-op for this functionality, leaving the
>> >> > enablement to be done later once systemd is properly hooked into
>> >> > the build system. The socket activation is ordered in aligment with
>> >> > the socket activation order passed on to systemd.
>> >> >
>> >> > [0] http://0pointer.de/blog/projects/socket-activation2.html
>> >>
>> >> So with this patch in place, xenstored will not start on a system that
>> >> has systemd, *even if it wasn't started from systemd*.
>> >
>> > But where systemd is /sbin/init, right?
>> >
>> > The case where xenstored was compiled with systemd support but systemd is
>> > not /sbin/init should still be expected to work, and isn't what I think you
>> > are complaining about here.
>> >
>> >> Lots of systems (e.g., CentOS 7) have legacy systems in place to allow
>> >> you to do things like "chkconfig --add xencommons" even on a systemd
>> >> system.  I think we still want to work with those, right?
>> >
>> > Isn't chkconfig --add still arranging for the thing to be started by
>> > systemd under the hood? If not systemd on a host where /sbin/init==systemd
>> > then what does else would start it?
>> >
>> > If you are asking "should the sysvinit initscripts still be us(ed|able)
>> > even though systemd is being used as /sbin/init on the host and a unit file
>> > is present" then AIUI the systemd answer is "no". (We may choose to
>> > disagree with systemd on this I suppose)
>>
>> Well that's not (apparently) the RHEL answer; doing "chkconfig --add
>> [foo]" Just Works on CentOS 7 for all the sysvinit scripts I've used
>> (including the Xen 4.4 Xen4CentOS packages).
>>
>> I think we want to still *enable* people to use that mode if they want
>> to.  But I won't argue if people feel strongly otherwise.
>>
>> > On the other hand, does this mean I can no longer start xenstored by hand
>> > from the CLI? _That_ would seem to be worth preserving, for debugging etc
>> > if nothing else.
>>
>> So what happens at the moment is that xenstored, run either from the
>> command-line says, "Oh, look!  I'm running on a systemd system.  I'll
>> check for my systemd sockets.  Oh no, no sockets!  *dies*".
>>
>> If run from xencommons, it doesn't even get that far: it says, "Oh,
>> look! I'm running on a systemd system.  But wait! You asked me to use
>> a pidfile! BAD USER! NO PIDFILE ON SYSTEMD! *dies*".
>>
>> Modifying xenstored to try to open the systemd sockets, and fall back
>> to normal sockets if it doesn't find any, works when started from the
>> command-line.
>
> I have always thought this is the expected behaviour. Just that the code
> has a bug.
>
> Here is a patch that is not even compile test. :-)

sd_booted() checks to see *if systemd is running on the system*; it
doesn't actually tell you *if the program was started from systemd*
(and thus has the appropriate sockets &c).

So although checking for non-negative rather than non-zero is more
correct, this will have the same basic behavior -- if xenstored is
started *on a system with systemd running*, even if it wasn't started
*by* systemd, it will try to use the systemd sockets and fail (and
also ignore the pidfile).

Something like the attached (compile-tested only).

 -George

[-- Attachment #2: 0001-tools-cxenstored-Fall-back-to-normal-sockets-if-syst.patch --]
[-- Type: application/octet-stream, Size: 5017 bytes --]

From 3a914fa8e7db998efd386947ab8df80c3f5ddf36 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@eu.citrix.com>
Date: Tue, 4 Aug 2015 18:51:12 +0100
Subject: [PATCH] tools/cxenstored: Fall back to normal sockets if systemd
 sockets are not available

cxenstored tries to determine if it should use systemd "active
sockets" by calling sd_booted().  However, this tells you whether
systemd is running on the system, not whether you've actually been
called from systemd.  If xenstored is run directly from the
command-line, or from the legacy xencommons script, it will try to use
the (non-existent) systemd sockets, fail and exit.

Have cxenstore try to use the systemd sockets if run on a systemd system, but
fall back to the old method if it doesn't work.  Specifically:

- Modify xen_claim_active_sockets() to return 0 on success, -1 on failure.
- Reorganize set-up to make common things happen before the check
- If sd_booted() returns >0, try to use the extra sockets; if it doesn't work,
  fall back to the old method.
- Ignore --pid-file if systemd sockets work rather than exiting

While we're here, change the sd_booted() check to interpret positive
number as a success, rather than any non-zero value.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
 tools/xenstore/xenstored_core.c | 65 ++++++++++++++++++++++-------------------
 1 file changed, 35 insertions(+), 30 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index b7e4936..6f4be78 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1781,7 +1781,7 @@ static int xs_validate_active_socket(const char *connect_to)
 	return xs_get_sd_fd(connect_to);
 }
 
-static void xen_claim_active_sockets(int **psock, int **pro_sock)
+static int xen_claim_active_sockets(int **psock, int **pro_sock)
 {
 	int *sock, *ro_sock;
 	const char *soc_str = xs_daemon_socket();
@@ -1790,11 +1790,9 @@ static void xen_claim_active_sockets(int **psock, int **pro_sock)
 
 	n = sd_listen_fds(0);
 	if (n <= 0) {
-		sd_notifyf(0, "STATUS=Failed to get any active sockets: %s\n"
-			   "ERRNO=%i",
-			   strerror(errno),
-			   errno);
-		barf_perror("sd_listen_fds() failed\n");
+		perror("sd_listen_fds failed() failed");
+		fprintf(stderr, "Falling back to normal sockets\n");
+		return -1;
 	} else if (n != 2) {
 		fprintf(stderr, SD_ERR "Expected 2 fds but given %d\n", n);
 		sd_notifyf(0, "STATUS=Mismatch on number (2): %s\n"
@@ -1802,6 +1800,7 @@ static void xen_claim_active_sockets(int **psock, int **pro_sock)
 			   strerror(EBADR),
 			   EBADR);
 		barf_perror("sd_listen_fds() gave too many fds\n");
+		/* This should never return */
 	}
 
 	*psock = sock = talloc(talloc_autofree_context(), int);
@@ -1816,6 +1815,8 @@ static void xen_claim_active_sockets(int **psock, int **pro_sock)
 
 	talloc_set_destructor(sock, destroy_fd);
 	talloc_set_destructor(ro_sock, destroy_fd);
+
+	return 0;
 }
 #endif
 
@@ -1927,6 +1928,7 @@ int main(int argc, char *argv[])
 	bool dofork = true;
 	bool outputpid = false;
 	bool no_domain_init = false;
+	bool systemd_sockets = false;
 	const char *pidfile = NULL;
 	int timeout;
 
@@ -1989,15 +1991,6 @@ int main(int argc, char *argv[])
 	if (optind != argc)
 		barf("%s: No arguments desired", argv[0]);
 
-#if defined(XEN_SYSTEMD_ENABLED)
-	if (sd_booted()) {
-		dofork = false;
-		if (pidfile)
-			barf("%s: PID file not needed on systemd", argv[0]);
-		pidfile = NULL;
-	}
-#endif
-
 	reopen_log();
 
 	/* make sure xenstored directories exist */
@@ -2005,26 +1998,38 @@ int main(int argc, char *argv[])
 	mkdir(xs_daemon_rundir(), 0755);
 	mkdir(xs_daemon_rootdir(), 0755);
 
-	if (dofork) {
-		openlog("xenstored", 0, LOG_DAEMON);
-		daemonize();
-	}
-	if (pidfile)
-		write_pidfile(pidfile);
-
-	/* Talloc leak reports go to stderr, which is closed if we fork. */
-	if (!dofork)
-		talloc_enable_leak_report_full();
-
 	/* Don't kill us with SIGPIPE. */
 	signal(SIGPIPE, SIG_IGN);
 
 #if defined(XEN_SYSTEMD_ENABLED)
-	if (sd_booted())
-		xen_claim_active_sockets(&sock, &ro_sock);
-	else
+	/* 
+	 * A failure in xen_claim_active_sockets one will return
+	 * non-zero; in that case we want to fall back to the default.
+	 */
+	if (sd_booted()>0 &&
+	    xen_claim_active_sockets(&sock, &ro_sock)) {
+		systemd_sockets=true;
+		dofork = false;
+		pidfile = NULL;
+	} else
 #endif
+	{ 
+		systemd_sockets=false;
+
+		if (dofork) {
+		    openlog("xenstored", 0, LOG_DAEMON);
+		    daemonize();
+	        }
+		
+		if (pidfile)
+			write_pidfile(pidfile);
+
 		init_sockets(&sock, &ro_sock);
+	}
+
+	/* Talloc leak reports go to stderr, which is closed if we fork. */
+	if (!dofork)
+		talloc_enable_leak_report_full();
 
 	init_pipe(reopen_log_pipe);
 
@@ -2057,7 +2062,7 @@ int main(int argc, char *argv[])
 	xenbus_notify_running();
 
 #if defined(XEN_SYSTEMD_ENABLED)
-	if (sd_booted()) {
+	if (systemd_sockets) {
 		sd_notify(1, "READY=1");
 		fprintf(stderr, SD_NOTICE "xenstored is ready\n");
 	}
-- 
1.9.1


[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v7 2/8] cxenstored: add support for systemd active sockets

[Wei Liu]

On Wed, Aug 05, 2015 at 05:30:34PM +0100, George Dunlap wrote:
> On Wed, Aug 5, 2015 at 2:17 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> > On Wed, Aug 05, 2015 at 11:56:44AM +0100, George Dunlap wrote:
> >> On Wed, Aug 5, 2015 at 11:17 AM, Ian Campbell <ian.campbell@citrix.com> wrote:
> >> > On Wed, 2015-08-05 at 11:06 +0100, George Dunlap wrote:
> >> >> On Fri, Jul 18, 2014 at 12:28 AM, Luis R. Rodriguez
> >> >> <mcgrof@do-not-panic.com> wrote:
> >> >> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> >> >> >
> >> >> > This adds systemd socket activation support for the C xenstored.
> >> >> > Active sockets enable xenstored to be loaded only if required by a
> >> >> > system
> >> >> > onto which Xen is installed on. Socket activation is handled by
> >> >> > systemd, once a port for a service which claims a socket is used
> >> >> > systemd will start the required services for it, on demand. For more
> >> >> > details on socket activation refer to Lennart's socket-activation
> >> >> > post regarding this [0].
> >> >> >
> >> >> > Right now this code adds a no-op for this functionality, leaving the
> >> >> > enablement to be done later once systemd is properly hooked into
> >> >> > the build system. The socket activation is ordered in aligment with
> >> >> > the socket activation order passed on to systemd.
> >> >> >
> >> >> > [0] http://0pointer.de/blog/projects/socket-activation2.html
> >> >>
> >> >> So with this patch in place, xenstored will not start on a system that
> >> >> has systemd, *even if it wasn't started from systemd*.
> >> >
> >> > But where systemd is /sbin/init, right?
> >> >
> >> > The case where xenstored was compiled with systemd support but systemd is
> >> > not /sbin/init should still be expected to work, and isn't what I think you
> >> > are complaining about here.
> >> >
> >> >> Lots of systems (e.g., CentOS 7) have legacy systems in place to allow
> >> >> you to do things like "chkconfig --add xencommons" even on a systemd
> >> >> system.  I think we still want to work with those, right?
> >> >
> >> > Isn't chkconfig --add still arranging for the thing to be started by
> >> > systemd under the hood? If not systemd on a host where /sbin/init==systemd
> >> > then what does else would start it?
> >> >
> >> > If you are asking "should the sysvinit initscripts still be us(ed|able)
> >> > even though systemd is being used as /sbin/init on the host and a unit file
> >> > is present" then AIUI the systemd answer is "no". (We may choose to
> >> > disagree with systemd on this I suppose)
> >>
> >> Well that's not (apparently) the RHEL answer; doing "chkconfig --add
> >> [foo]" Just Works on CentOS 7 for all the sysvinit scripts I've used
> >> (including the Xen 4.4 Xen4CentOS packages).
> >>
> >> I think we want to still *enable* people to use that mode if they want
> >> to.  But I won't argue if people feel strongly otherwise.
> >>
> >> > On the other hand, does this mean I can no longer start xenstored by hand
> >> > from the CLI? _That_ would seem to be worth preserving, for debugging etc
> >> > if nothing else.
> >>
> >> So what happens at the moment is that xenstored, run either from the
> >> command-line says, "Oh, look!  I'm running on a systemd system.  I'll
> >> check for my systemd sockets.  Oh no, no sockets!  *dies*".
> >>
> >> If run from xencommons, it doesn't even get that far: it says, "Oh,
> >> look! I'm running on a systemd system.  But wait! You asked me to use
> >> a pidfile! BAD USER! NO PIDFILE ON SYSTEMD! *dies*".
> >>
> >> Modifying xenstored to try to open the systemd sockets, and fall back
> >> to normal sockets if it doesn't find any, works when started from the
> >> command-line.
> >
> > I have always thought this is the expected behaviour. Just that the code
> > has a bug.
> >
> > Here is a patch that is not even compile test. :-)
> 
> sd_booted() checks to see *if systemd is running on the system*; it
> doesn't actually tell you *if the program was started from systemd*
> (and thus has the appropriate sockets &c).
> 
> So although checking for non-negative rather than non-zero is more
> correct, this will have the same basic behavior -- if xenstored is
> started *on a system with systemd running*, even if it wasn't started
> *by* systemd, it will try to use the systemd sockets and fail (and
> also ignore the pidfile).
> 
> Something like the attached (compile-tested only).
> 

Right. I misinterpreted sd_boot.

You patch, however, has the undesirable effect that it fails to report
error if xenstored is started by systemd but couldn't claim the
socket. I don't think this is the correct behaviour.

After consulting with systemd manual [0], I think we should check
sd_listen_fds return value to determine if it is started by systemd.
Currently it only checks for <= 0, which covers 1) not started by
systemd 2) an error occurs.

Hopefully I interpret the doc correctly this time. I will prepare a
patch shortly.

Wei.

[0] http://www.freedesktop.org/software/systemd/man/sd_listen_fds.html

>  -George

Re: [PATCH v7 2/8] cxenstored: add support for systemd active sockets

[Wei Liu]

On Wed, Aug 05, 2015 at 06:24:37PM +0100, Wei Liu wrote:
[...]
> > 
> 
> Right. I misinterpreted sd_boot.
> 
> You patch, however, has the undesirable effect that it fails to report
> error if xenstored is started by systemd but couldn't claim the
> socket. I don't think this is the correct behaviour.
> 
> After consulting with systemd manual [0], I think we should check
> sd_listen_fds return value to determine if it is started by systemd.
> Currently it only checks for <= 0, which covers 1) not started by
> systemd 2) an error occurs.
> 
> Hopefully I interpret the doc correctly this time. I will prepare a
> patch shortly.
> 
> Wei.
> 
> [0] http://www.freedesktop.org/software/systemd/man/sd_listen_fds.html

Patch attached. I start cxenstored by hand and it seems to works fine --
now it fails with other errors. Can you test this patch to see if it
works?

Apparently oxenstored has similar issues. If this patch works I will
need to fix oxenstored, too. Time to test my ocaml-fu! :-/

---8<---
>From a03eba6e258d8097b974366abb50b39af9e9abbf Mon Sep 17 00:00:00 2001
From: Wei Liu <wei.liu2@citrix.com>
Date: Wed, 5 Aug 2015 19:02:34 +0100
Subject: [PATCH] cxenstored: fix systemd socket activation

There were two problems with original code:

1. sd_booted() was used to determined if the process was started by
   systemd, which was wrong.
2. Exit with error if pidfile was specified, which was too harsh.

These two combined made cxenstored unable to start by hand if it ran
on a system which had systemd.

Fix issues with following changes:

1. Use sd_listen_fds to determine if the process is started by systemd.
2. Don't exit if pidfile is specified.

Rename function and restructure code to make things clearer.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/xenstore/xenstored_core.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index b7e4936..57581e0 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1781,7 +1781,10 @@ static int xs_validate_active_socket(const char *connect_to)
 	return xs_get_sd_fd(connect_to);
 }
 
-static void xen_claim_active_sockets(int **psock, int **pro_sock)
+/* Return true if started by systemd and false if not. Exit with
+ * error if things go wrong.
+ */
+static bool systemd_checkin(int **psock, int **pro_sock)
 {
 	int *sock, *ro_sock;
 	const char *soc_str = xs_daemon_socket();
@@ -1789,7 +1792,11 @@ static void xen_claim_active_sockets(int **psock, int **pro_sock)
 	int n;
 
 	n = sd_listen_fds(0);
-	if (n <= 0) {
+
+	if (n == 0)
+		return false;
+
+	 if (n < 0) {
 		sd_notifyf(0, "STATUS=Failed to get any active sockets: %s\n"
 			   "ERRNO=%i",
 			   strerror(errno),
@@ -1816,6 +1823,8 @@ static void xen_claim_active_sockets(int **psock, int **pro_sock)
 
 	talloc_set_destructor(sock, destroy_fd);
 	talloc_set_destructor(ro_sock, destroy_fd);
+
+	return true;
 }
 #endif
 
@@ -1929,6 +1938,9 @@ int main(int argc, char *argv[])
 	bool no_domain_init = false;
 	const char *pidfile = NULL;
 	int timeout;
+#if defined(XEN_SYSTEMD_ENABLED)
+	bool systemd;
+#endif
 
 	while ((opt = getopt_long(argc, argv, "DE:F:HNPS:t:T:RLVW:", options,
 				  NULL)) != -1) {
@@ -1990,10 +2002,11 @@ int main(int argc, char *argv[])
 		barf("%s: No arguments desired", argv[0]);
 
 #if defined(XEN_SYSTEMD_ENABLED)
-	if (sd_booted()) {
+	systemd = systemd_checkin(&sock, &ro_sock);
+	if (systemd) {
 		dofork = false;
 		if (pidfile)
-			barf("%s: PID file not needed on systemd", argv[0]);
+			xprintf("%s: PID file not needed on systemd", argv[0]);
 		pidfile = NULL;
 	}
 #endif
@@ -2020,9 +2033,7 @@ int main(int argc, char *argv[])
 	signal(SIGPIPE, SIG_IGN);
 
 #if defined(XEN_SYSTEMD_ENABLED)
-	if (sd_booted())
-		xen_claim_active_sockets(&sock, &ro_sock);
-	else
+	if (!systemd)
 #endif
 		init_sockets(&sock, &ro_sock);
 
@@ -2057,7 +2068,7 @@ int main(int argc, char *argv[])
 	xenbus_notify_running();
 
 #if defined(XEN_SYSTEMD_ENABLED)
-	if (sd_booted()) {
+	if (systemd) {
 		sd_notify(1, "READY=1");
 		fprintf(stderr, SD_NOTICE "xenstored is ready\n");
 	}
-- 
2.1.4

Re: [PATCH v7 2/8] cxenstored: add support for systemd active sockets

[Ian Campbell]

On Wed, 2015-08-05 at 19:19 +0100, Wei Liu wrote:
> On Wed, Aug 05, 2015 at 06:24:37PM +0100, Wei Liu wrote:
> [...]
> > > 
> > 
> > Right. I misinterpreted sd_boot.
> > 
> > You patch, however, has the undesirable effect that it fails to report
> > error if xenstored is started by systemd but couldn't claim the
> > socket. I don't think this is the correct behaviour.
> > 
> > After consulting with systemd manual [0], I think we should check
> > sd_listen_fds return value to determine if it is started by systemd.
> > Currently it only checks for <= 0, which covers 1) not started by
> > systemd 2) an error occurs.
> > 
> > Hopefully I interpret the doc correctly this time. I will prepare a
> > patch shortly.
> > 
> > Wei.
> > 
> > [0] http://www.freedesktop.org/software/systemd/man/sd_listen_fds.html
> 
> Patch attached. I start cxenstored by hand and it seems to works fine --
> now it fails with other errors.

What other errors? Are they blockers for accepting this patch?

>  Can you test this patch to see if it
> works?
> 
> Apparently oxenstored has similar issues. If this patch works I will
> need to fix oxenstored, too. Time to test my ocaml-fu! :-/

Luckily it looks like most changes required will be in systemd_stubs.c!
(Probably not all through, sorry!)

> ---8<---
> From a03eba6e258d8097b974366abb50b39af9e9abbf Mon Sep 17 00:00:00 2001
> From: Wei Liu <wei.liu2@citrix.com>
> Date: Wed, 5 Aug 2015 19:02:34 +0100
> Subject: [PATCH] cxenstored: fix systemd socket activation
> 
> There were two problems with original code:
> 
> 1. sd_booted() was used to determined if the process was started by
>    systemd, which was wrong.
> 2. Exit with error if pidfile was specified, which was too harsh.
> 
> These two combined made cxenstored unable to start by hand if it ran
> on a system which had systemd.
> 
> Fix issues with following changes:
> 
> 1. Use sd_listen_fds to determine if the process is started by systemd.
> 2. Don't exit if pidfile is specified.
> 
> Rename function and restructure code to make things clearer.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/xenstore/xenstored_core.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/xenstore/xenstored_core.c 
> b/tools/xenstore/xenstored_core.c
> index b7e4936..57581e0 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -1781,7 +1781,10 @@ static int xs_validate_active_socket(const char 
> *connect_to)
>  	return xs_get_sd_fd(connect_to);
>  }
>  
> -static void xen_claim_active_sockets(int **psock, int **pro_sock)
> +/* Return true if started by systemd and false if not. Exit with
> + * error if things go wrong.
> + */
> +static bool systemd_checkin(int **psock, int **pro_sock)
>  {
>  	int *sock, *ro_sock;
>  	const char *soc_str = xs_daemon_socket();
> @@ -1789,7 +1792,11 @@ static void xen_claim_active_sockets(int **psock, 
> int **pro_sock)
>  	int n;
>  
>  	n = sd_listen_fds(0);

Do we need/want a !sd_booted() => false before doing this? What is the
expected behaviour of sd_listen_fds if we aren't running under systemd at
all?

Maybe that would be good from a belt-and-braced PoV if nothing else?

> -	if (n <= 0) {
> +
> +	if (n == 0)
> +		return false;
> +
> +	 if (n < 0) {
>  > 	> 	> sd_notifyf(0, "STATUS=Failed to get any active sockets: 
> %s\n"

If we were running under something other than systemd which happens to do
socket activation (e.g. upstart), perhaps we wouldn't want this here.
Probably the sd_booted() check mentioned above would protect against this
case too.

Ian.

Re: [PATCH v7 2/8] cxenstored: add support for systemd active sockets

[Wei Liu]

On Thu, Aug 06, 2015 at 10:13:16AM +0100, Ian Campbell wrote:
> On Wed, 2015-08-05 at 19:19 +0100, Wei Liu wrote:
> > On Wed, Aug 05, 2015 at 06:24:37PM +0100, Wei Liu wrote:
> > [...]
> > > > 
> > > 
> > > Right. I misinterpreted sd_boot.
> > > 
> > > You patch, however, has the undesirable effect that it fails to report
> > > error if xenstored is started by systemd but couldn't claim the
> > > socket. I don't think this is the correct behaviour.
> > > 
> > > After consulting with systemd manual [0], I think we should check
> > > sd_listen_fds return value to determine if it is started by systemd.
> > > Currently it only checks for <= 0, which covers 1) not started by
> > > systemd 2) an error occurs.
> > > 
> > > Hopefully I interpret the doc correctly this time. I will prepare a
> > > patch shortly.
> > > 
> > > Wei.
> > > 
> > > [0] http://www.freedesktop.org/software/systemd/man/sd_listen_fds.html
> > 
> > Patch attached. I start cxenstored by hand and it seems to works fine --
> > now it fails with other errors.
> 
> What other errors? Are they blockers for accepting this patch?
> 

 "FATAL: Failed to initialize dom0 state: Invalid argument"

Xenstored tries to initialise domain 0 state once again. That can be
dealt with with a simple -D (--no-domain-init) option to stop it from
doing that.

With -D the restarted xenstored worked fine. Strangely the short options
are not documented in help text at all. :-)

> >  Can you test this patch to see if it
> > works?
> > 
> > Apparently oxenstored has similar issues. If this patch works I will
> > need to fix oxenstored, too. Time to test my ocaml-fu! :-/
> 
> Luckily it looks like most changes required will be in systemd_stubs.c!
> (Probably not all through, sorry!)
> 

More than that, I will also need to change the callsites etc. I'm
looking into that at the moment.

Wei.

Re: [PATCH v7 2/8] cxenstored: add support for systemd active sockets

[Ian Campbell]

On Thu, 2015-08-06 at 10:20 +0100, Wei Liu wrote:
> On Thu, Aug 06, 2015 at 10:13:16AM +0100, Ian Campbell wrote:
> > On Wed, 2015-08-05 at 19:19 +0100, Wei Liu wrote:
> > > On Wed, Aug 05, 2015 at 06:24:37PM +0100, Wei Liu wrote:
> > > [...]
> > > > > 
> > > > 
> > > > Right. I misinterpreted sd_boot.
> > > > 
> > > > You patch, however, has the undesirable effect that it fails to 
> > > > report
> > > > error if xenstored is started by systemd but couldn't claim the
> > > > socket. I don't think this is the correct behaviour.
> > > > 
> > > > After consulting with systemd manual [0], I think we should check
> > > > sd_listen_fds return value to determine if it is started by 
> > > > systemd.
> > > > Currently it only checks for <= 0, which covers 1) not started by
> > > > systemd 2) an error occurs.
> > > > 
> > > > Hopefully I interpret the doc correctly this time. I will prepare a
> > > > patch shortly.
> > > > 
> > > > Wei.
> > > > 
> > > > [0] 
> > > > http://www.freedesktop.org/software/systemd/man/sd_listen_fds.html
> > > 
> > > Patch attached. I start cxenstored by hand and it seems to works fine 
> > > --
> > > now it fails with other errors.
> > 
> > What other errors? Are they blockers for accepting this patch?
> > 
> 
>  "FATAL: Failed to initialize dom0 state: Invalid argument"
> 
> Xenstored tries to initialise domain 0 state once again. That can be
> dealt with with a simple -D (--no-domain-init) option to stop it from
> doing that.

OK, is this just a symptom of having already started xenstored once from
the initscript and then trying to do it again?

If you disable the initscript/unit file then you should be able to start
xenstored manually (perhaps after having done some setup. e.g. mount
/proc/xen) without error I think.

Re: [PATCH v7 2/8] cxenstored: add support for systemd active sockets

[Wei Liu]

On Thu, Aug 06, 2015 at 10:29:59AM +0100, Ian Campbell wrote:
> On Thu, 2015-08-06 at 10:20 +0100, Wei Liu wrote:
> > On Thu, Aug 06, 2015 at 10:13:16AM +0100, Ian Campbell wrote:
> > > On Wed, 2015-08-05 at 19:19 +0100, Wei Liu wrote:
> > > > On Wed, Aug 05, 2015 at 06:24:37PM +0100, Wei Liu wrote:
> > > > [...]
> > > > > > 
> > > > > 
> > > > > Right. I misinterpreted sd_boot.
> > > > > 
> > > > > You patch, however, has the undesirable effect that it fails to 
> > > > > report
> > > > > error if xenstored is started by systemd but couldn't claim the
> > > > > socket. I don't think this is the correct behaviour.
> > > > > 
> > > > > After consulting with systemd manual [0], I think we should check
> > > > > sd_listen_fds return value to determine if it is started by 
> > > > > systemd.
> > > > > Currently it only checks for <= 0, which covers 1) not started by
> > > > > systemd 2) an error occurs.
> > > > > 
> > > > > Hopefully I interpret the doc correctly this time. I will prepare a
> > > > > patch shortly.
> > > > > 
> > > > > Wei.
> > > > > 
> > > > > [0] 
> > > > > http://www.freedesktop.org/software/systemd/man/sd_listen_fds.html
> > > > 
> > > > Patch attached. I start cxenstored by hand and it seems to works fine 
> > > > --
> > > > now it fails with other errors.
> > > 
> > > What other errors? Are they blockers for accepting this patch?
> > > 
> > 
> >  "FATAL: Failed to initialize dom0 state: Invalid argument"
> > 
> > Xenstored tries to initialise domain 0 state once again. That can be
> > dealt with with a simple -D (--no-domain-init) option to stop it from
> > doing that.
> 
> OK, is this just a symptom of having already started xenstored once from
> the initscript and then trying to do it again?
> 

Yes.

> If you disable the initscript/unit file then you should be able to start
> xenstored manually (perhaps after having done some setup. e.g. mount
> /proc/xen) without error I think.

I tried that and xenstored works as expected.

Wei.

Re: [PATCH v7 2/8] cxenstored: add support for systemd active sockets

[Wei Liu]

Sorry I missed some inline comments.

On Thu, Aug 06, 2015 at 10:13:16AM +0100, Ian Campbell wrote:
> > ---8<---
> > From a03eba6e258d8097b974366abb50b39af9e9abbf Mon Sep 17 00:00:00 2001
> > From: Wei Liu <wei.liu2@citrix.com>
> > Date: Wed, 5 Aug 2015 19:02:34 +0100
> > Subject: [PATCH] cxenstored: fix systemd socket activation
> > 
> > There were two problems with original code:
> > 
> > 1. sd_booted() was used to determined if the process was started by
> >    systemd, which was wrong.
> > 2. Exit with error if pidfile was specified, which was too harsh.
> > 
> > These two combined made cxenstored unable to start by hand if it ran
> > on a system which had systemd.
> > 
> > Fix issues with following changes:
> > 
> > 1. Use sd_listen_fds to determine if the process is started by systemd.
> > 2. Don't exit if pidfile is specified.
> > 
> > Rename function and restructure code to make things clearer.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> >  tools/xenstore/xenstored_core.c | 27 +++++++++++++++++++--------
> >  1 file changed, 19 insertions(+), 8 deletions(-)
> > 
> > diff --git a/tools/xenstore/xenstored_core.c 
> > b/tools/xenstore/xenstored_core.c
> > index b7e4936..57581e0 100644
> > --- a/tools/xenstore/xenstored_core.c
> > +++ b/tools/xenstore/xenstored_core.c
> > @@ -1781,7 +1781,10 @@ static int xs_validate_active_socket(const char 
> > *connect_to)
> >  	return xs_get_sd_fd(connect_to);
> >  }
> >  
> > -static void xen_claim_active_sockets(int **psock, int **pro_sock)
> > +/* Return true if started by systemd and false if not. Exit with
> > + * error if things go wrong.
> > + */
> > +static bool systemd_checkin(int **psock, int **pro_sock)
> >  {
> >  	int *sock, *ro_sock;
> >  	const char *soc_str = xs_daemon_socket();
> > @@ -1789,7 +1792,11 @@ static void xen_claim_active_sockets(int **psock, 
> > int **pro_sock)
> >  	int n;
> >  
> >  	n = sd_listen_fds(0);
> 
> Do we need/want a !sd_booted() => false before doing this? What is the
> expected behaviour of sd_listen_fds if we aren't running under systemd at
> all?
> 

I don't think so. Though the doc is not clear on how we should use those
APIs, I got my idea from 

http://0pointer.de/public/cups-patch-core.txt

which doesn't call sd_booted.

> Maybe that would be good from a belt-and-braced PoV if nothing else?
> 
> > -	if (n <= 0) {
> > +
> > +	if (n == 0)
> > +		return false;
> > +
> > +	 if (n < 0) {
> >  > 	> 	> sd_notifyf(0, "STATUS=Failed to get any active sockets: 
> > %s\n"
> 
> If we were running under something other than systemd which happens to do
> socket activation (e.g. upstart), perhaps we wouldn't want this here.
> Probably the sd_booted() check mentioned above would protect against this
> case too.
> 

In that case I *think* sd_listen_fds should return 0 which makes this
function exits before this line.

Wei.

> Ian.

Re: [PATCH v7 2/8] cxenstored: add support for systemd active sockets

[Ian Campbell]

On Thu, 2015-08-06 at 11:17 +0100, Wei Liu wrote:
> 
> I don't think so. Though the doc is not clear on how we should use those
> APIs, I got my idea from 
> 
> http://0pointer.de/public/cups-patch-core.txt
> 
> which doesn't call sd_booted.

OK, FM then: Acked-by: Ian Campbell <ian.campbell@citrix.com>

As a future cleanup I think some of the ifdeferry could be removed by
having systemd = false ifndef SYSTEMD_ENABLED, what do you think?

Re: [PATCH v7 2/8] cxenstored: add support for systemd active sockets

[Wei Liu]

On Thu, Aug 06, 2015 at 11:48:15AM +0100, Ian Campbell wrote:
> On Thu, 2015-08-06 at 11:17 +0100, Wei Liu wrote:
> > 
> > I don't think so. Though the doc is not clear on how we should use those
> > APIs, I got my idea from 
> > 
> > http://0pointer.de/public/cups-patch-core.txt
> > 
> > which doesn't call sd_booted.
> 
> OK, FM then: Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> As a future cleanup I think some of the ifdeferry could be removed by
> having systemd = false ifndef SYSTEMD_ENABLED, what do you think?

Maybe. In the end there has to be some ifdeferry some where to gate the
implementation. As a future cleanup I think I can provide a stub
systemd_checkin() which always returns false and remove all ifdef in
main().

Wei.

Re: [PATCH v7 2/8] cxenstored: add support for systemd active sockets

[Ian Campbell]

On Thu, 2015-08-06 at 11:56 +0100, Wei Liu wrote:
> On Thu, Aug 06, 2015 at 11:48:15AM +0100, Ian Campbell wrote:
> > On Thu, 2015-08-06 at 11:17 +0100, Wei Liu wrote:
> > > 
> > > I don't think so. Though the doc is not clear on how we should use 
> > > those
> > > APIs, I got my idea from 
> > > 
> > > http://0pointer.de/public/cups-patch-core.txt
> > > 
> > > which doesn't call sd_booted.
> > 
> > OK, FM then: Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > As a future cleanup I think some of the ifdeferry could be removed by
> > having systemd = false ifndef SYSTEMD_ENABLED, what do you think?
> 
> Maybe. In the end there has to be some ifdeferry some where to gate the
> implementation. As a future cleanup I think I can provide a stub
> systemd_checkin() which always returns false and remove all ifdef in
> main().

Right, that's what I meant, i.e. just reduce the amount of ifdef.

> 
> Wei.

Re: [PATCH v7 2/8] cxenstored: add support for systemd active sockets

[George Dunlap]

On 08/05/2015 07:19 PM, Wei Liu wrote:
> On Wed, Aug 05, 2015 at 06:24:37PM +0100, Wei Liu wrote:
> [...]
>>>
>>
>> Right. I misinterpreted sd_boot.
>>
>> You patch, however, has the undesirable effect that it fails to report
>> error if xenstored is started by systemd but couldn't claim the
>> socket. I don't think this is the correct behaviour.
>>
>> After consulting with systemd manual [0], I think we should check
>> sd_listen_fds return value to determine if it is started by systemd.
>> Currently it only checks for <= 0, which covers 1) not started by
>> systemd 2) an error occurs.
>>
>> Hopefully I interpret the doc correctly this time. I will prepare a
>> patch shortly.
>>
>> Wei.
>>
>> [0] http://www.freedesktop.org/software/systemd/man/sd_listen_fds.html
> 
> Patch attached. I start cxenstored by hand and it seems to works fine --
> now it fails with other errors. Can you test this patch to see if it
> works?
> 
> Apparently oxenstored has similar issues. If this patch works I will
> need to fix oxenstored, too. Time to test my ocaml-fu! :-/
> 
> ---8<---
> From a03eba6e258d8097b974366abb50b39af9e9abbf Mon Sep 17 00:00:00 2001
> From: Wei Liu <wei.liu2@citrix.com>
> Date: Wed, 5 Aug 2015 19:02:34 +0100
> Subject: [PATCH] cxenstored: fix systemd socket activation
> 
> There were two problems with original code:
> 
> 1. sd_booted() was used to determined if the process was started by
>    systemd, which was wrong.
> 2. Exit with error if pidfile was specified, which was too harsh.
> 
> These two combined made cxenstored unable to start by hand if it ran
> on a system which had systemd.
> 
> Fix issues with following changes:
> 
> 1. Use sd_listen_fds to determine if the process is started by systemd.
> 2. Don't exit if pidfile is specified.
> 
> Rename function and restructure code to make things clearer.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

I've booted with this patch using both the chkconfig+xencommons sysvinit
script, and the systemd method, and both work for me:

Tested-by: George Dunlap <george.dunlap@eu.citrix.com>

Thanks,
 -George

> ---
>  tools/xenstore/xenstored_core.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index b7e4936..57581e0 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -1781,7 +1781,10 @@ static int xs_validate_active_socket(const char *connect_to)
>  	return xs_get_sd_fd(connect_to);
>  }
>  
> -static void xen_claim_active_sockets(int **psock, int **pro_sock)
> +/* Return true if started by systemd and false if not. Exit with
> + * error if things go wrong.
> + */
> +static bool systemd_checkin(int **psock, int **pro_sock)
>  {
>  	int *sock, *ro_sock;
>  	const char *soc_str = xs_daemon_socket();
> @@ -1789,7 +1792,11 @@ static void xen_claim_active_sockets(int **psock, int **pro_sock)
>  	int n;
>  
>  	n = sd_listen_fds(0);
> -	if (n <= 0) {
> +
> +	if (n == 0)
> +		return false;
> +
> +	 if (n < 0) {
>  		sd_notifyf(0, "STATUS=Failed to get any active sockets: %s\n"
>  			   "ERRNO=%i",
>  			   strerror(errno),
> @@ -1816,6 +1823,8 @@ static void xen_claim_active_sockets(int **psock, int **pro_sock)
>  
>  	talloc_set_destructor(sock, destroy_fd);
>  	talloc_set_destructor(ro_sock, destroy_fd);
> +
> +	return true;
>  }
>  #endif
>  
> @@ -1929,6 +1938,9 @@ int main(int argc, char *argv[])
>  	bool no_domain_init = false;
>  	const char *pidfile = NULL;
>  	int timeout;
> +#if defined(XEN_SYSTEMD_ENABLED)
> +	bool systemd;
> +#endif
>  
>  	while ((opt = getopt_long(argc, argv, "DE:F:HNPS:t:T:RLVW:", options,
>  				  NULL)) != -1) {
> @@ -1990,10 +2002,11 @@ int main(int argc, char *argv[])
>  		barf("%s: No arguments desired", argv[0]);
>  
>  #if defined(XEN_SYSTEMD_ENABLED)
> -	if (sd_booted()) {
> +	systemd = systemd_checkin(&sock, &ro_sock);
> +	if (systemd) {
>  		dofork = false;
>  		if (pidfile)
> -			barf("%s: PID file not needed on systemd", argv[0]);
> +			xprintf("%s: PID file not needed on systemd", argv[0]);
>  		pidfile = NULL;
>  	}
>  #endif
> @@ -2020,9 +2033,7 @@ int main(int argc, char *argv[])
>  	signal(SIGPIPE, SIG_IGN);
>  
>  #if defined(XEN_SYSTEMD_ENABLED)
> -	if (sd_booted())
> -		xen_claim_active_sockets(&sock, &ro_sock);
> -	else
> +	if (!systemd)
>  #endif
>  		init_sockets(&sock, &ro_sock);
>  
> @@ -2057,7 +2068,7 @@ int main(int argc, char *argv[])
>  	xenbus_notify_running();
>  
>  #if defined(XEN_SYSTEMD_ENABLED)
> -	if (sd_booted()) {
> +	if (systemd) {
>  		sd_notify(1, "READY=1");
>  		fprintf(stderr, SD_NOTICE "xenstored is ready\n");
>  	}
>