[PATCH 0/2] tools: make xenstore domain/daemon configurable

[PATCH 0/2] tools: make xenstore domain/daemon configurable

[Juergen Gross]

Add a configuration option to /etc/sysconfig/xencommons to let the
user configure whether he wants to start xenstore service as a daemon
or as a stubdom.

Juergen Gross (2):
  tools: remove systemd xenstore socket definitions
  tools: make xenstore domain easy configurable

 tools/configure                                    |   4 +-
 tools/hotplug/Linux/Makefile                       |   1 +
 tools/hotplug/Linux/init.d/sysconfig.xencommons.in |  42 +++++-
 tools/hotplug/Linux/init.d/xencommons.in           |  35 +----
 tools/hotplug/Linux/launch-xenstore.in             |  84 +++++++++++
 tools/hotplug/Linux/systemd/Makefile               |   5 -
 tools/hotplug/Linux/systemd/xenstored.service.in   |  14 +-
 tools/hotplug/Linux/systemd/xenstored.socket.in    |  13 --
 tools/hotplug/Linux/systemd/xenstored_ro.socket.in |  13 --
 tools/ocaml/xenstored/Makefile                     |  10 +-
 tools/ocaml/xenstored/systemd.ml                   |  17 ---
 tools/ocaml/xenstored/systemd.mli                  |  24 ----
 tools/ocaml/xenstored/systemd_stubs.c              | 153 ---------------------
 tools/ocaml/xenstored/utils.ml                     |  21 +--
 tools/ocaml/xenstored/xenstored.ml                 |   2 -
 tools/xenstore/Makefile                            |   3 -
 tools/xenstore/xenstored_core.c                    | 113 +--------------
 17 files changed, 143 insertions(+), 411 deletions(-)
 create mode 100644 tools/hotplug/Linux/launch-xenstore.in
 delete mode 100644 tools/hotplug/Linux/systemd/xenstored.socket.in
 delete mode 100644 tools/hotplug/Linux/systemd/xenstored_ro.socket.in
 delete mode 100644 tools/ocaml/xenstored/systemd.ml
 delete mode 100644 tools/ocaml/xenstored/systemd.mli
 delete mode 100644 tools/ocaml/xenstored/systemd_stubs.c

-- 
2.6.6


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

[PATCH 1/2] tools: remove systemd xenstore socket definitions

[Juergen Gross]

On a system with systemd the xenstore sockets are created via systemd.
Remove the related configuration files in order to be able to decide
at runtime whether the sockets should be created or not. This will
enable Xen to start xenstore either via a daemon or via a stub domain.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/configure                                    |   2 +-
 tools/hotplug/Linux/systemd/Makefile               |   5 -
 tools/hotplug/Linux/systemd/xenstored.service.in   |   3 +-
 tools/hotplug/Linux/systemd/xenstored.socket.in    |  13 --
 tools/hotplug/Linux/systemd/xenstored_ro.socket.in |  13 --
 tools/ocaml/xenstored/Makefile                     |  10 +-
 tools/ocaml/xenstored/systemd.ml                   |  17 ---
 tools/ocaml/xenstored/systemd.mli                  |  24 ----
 tools/ocaml/xenstored/systemd_stubs.c              | 153 ---------------------
 tools/ocaml/xenstored/utils.ml                     |  21 +--
 tools/ocaml/xenstored/xenstored.ml                 |   2 -
 tools/xenstore/Makefile                            |   3 -
 tools/xenstore/xenstored_core.c                    | 113 +--------------
 13 files changed, 11 insertions(+), 368 deletions(-)
 delete mode 100644 tools/hotplug/Linux/systemd/xenstored.socket.in
 delete mode 100644 tools/hotplug/Linux/systemd/xenstored_ro.socket.in
 delete mode 100644 tools/ocaml/xenstored/systemd.ml
 delete mode 100644 tools/ocaml/xenstored/systemd.mli
 delete mode 100644 tools/ocaml/xenstored/systemd_stubs.c

diff --git a/tools/configure b/tools/configure
index 4c92fa2..0854271 100755
--- a/tools/configure
+++ b/tools/configure
@@ -9670,7 +9670,7 @@ fi
 
 if test "x$systemd" = "xy"; then :
 
-    ac_config_files="$ac_config_files hotplug/Linux/systemd/proc-xen.mount hotplug/Linux/systemd/var-lib-xenstored.mount hotplug/Linux/systemd/xen-init-dom0.service hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service hotplug/Linux/systemd/xen-watchdog.service hotplug/Linux/systemd/xenconsoled.service hotplug/Linux/systemd/xendomains.service hotplug/Linux/systemd/xenstored.service hotplug/Linux/systemd/xenstored.socket hotplug/Linux/systemd/xenstored_ro.socket"
+    ac_config_files="$ac_config_files hotplug/Linux/systemd/proc-xen.mount hotplug/Linux/systemd/var-lib-xenstored.mount hotplug/Linux/systemd/xen-init-dom0.service hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service hotplug/Linux/systemd/xen-watchdog.service hotplug/Linux/systemd/xenconsoled.service hotplug/Linux/systemd/xendomains.service hotplug/Linux/systemd/xenstored.service"
 
 
 fi
diff --git a/tools/hotplug/Linux/systemd/Makefile b/tools/hotplug/Linux/systemd/Makefile
index 83e3b32..e5144f1 100644
--- a/tools/hotplug/Linux/systemd/Makefile
+++ b/tools/hotplug/Linux/systemd/Makefile
@@ -6,9 +6,6 @@ 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
@@ -18,7 +15,6 @@ XEN_SYSTEMD_SERVICE += xen-init-dom0.service
 
 ALL_XEN_SYSTEMD =	$(XEN_SYSTEMD_MODULES)  \
 			$(XEN_SYSTEMD_MOUNT)	\
-			$(XEN_SYSTEMD_SOCKET)	\
 			$(XEN_SYSTEMD_SERVICE)
 
 .PHONY: all
@@ -37,7 +33,6 @@ install: $(ALL_XEN_SYSTEMD)
 		$(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)
diff --git a/tools/hotplug/Linux/systemd/xenstored.service.in b/tools/hotplug/Linux/systemd/xenstored.service.in
index a5f836b..a1e1db1 100644
--- a/tools/hotplug/Linux/systemd/xenstored.service.in
+++ b/tools/hotplug/Linux/systemd/xenstored.service.in
@@ -1,6 +1,6 @@
 [Unit]
 Description=The Xen xenstore
-Requires=xenstored_ro.socket xenstored.socket proc-xen.mount var-lib-xenstored.mount
+Requires=proc-xen.mount var-lib-xenstored.mount
 After=proc-xen.mount var-lib-xenstored.mount
 Before=libvirtd.service libvirt-guests.service
 RefuseManualStop=true
@@ -19,6 +19,5 @@ ExecStart=/bin/sh -c "exec $XENSTORED --no-fork $XENSTORED_ARGS"
 
 [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
deleted file mode 100644
index 375c4b7..0000000
--- a/tools/hotplug/Linux/systemd/xenstored.socket.in
+++ /dev/null
@@ -1,13 +0,0 @@
-[Unit]
-Description=xenstore socket
-Requires=proc-xen.mount var-lib-xenstored.mount
-After=proc-xen.mount var-lib-xenstored.mount
-ConditionPathExists=/proc/xen/capabilities
-
-[Socket]
-ListenStream=@XEN_RUN_STORED@/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
deleted file mode 100644
index 82fe377..0000000
--- a/tools/hotplug/Linux/systemd/xenstored_ro.socket.in
+++ /dev/null
@@ -1,13 +0,0 @@
-[Unit]
-Description=xenstore ro socket
-Requires=proc-xen.mount var-lib-xenstored.mount
-After=proc-xen.mount var-lib-xenstored.mount
-ConditionPathExists=/proc/xen/capabilities
-
-[Socket]
-ListenStream=@XEN_RUN_STORED@/socket_ro
-SocketMode=0660
-Service=xenstored.service
-
-[Install]
-WantedBy=sockets.target
diff --git a/tools/ocaml/xenstored/Makefile b/tools/ocaml/xenstored/Makefile
index 1769e55..a7008b1 100644
--- a/tools/ocaml/xenstored/Makefile
+++ b/tools/ocaml/xenstored/Makefile
@@ -4,8 +4,6 @@ include $(OCAML_TOPLEVEL)/common.make
 
 # Include configure output (config.h)
 CFLAGS += -include $(XEN_ROOT)/tools/config.h
-CFLAGS-$(CONFIG_SYSTEMD)  += $(SYSTEMD_CFLAGS)
-LDFLAGS-$(CONFIG_SYSTEMD) += $(SYSTEMD_LIBS)
 
 CFLAGS  += $(CFLAGS-y)
 CFLAGS  += $(APPEND_CFLAGS)
@@ -25,11 +23,6 @@ select_OBJS = select
 select_C_OBJS = select_stubs
 OCAML_LIBRARY = syslog select
 
-LIBS += systemd.cma systemd.cmxa
-systemd_OBJS = systemd
-systemd_C_OBJS = systemd_stubs
-OCAML_LIBRARY += systemd
-
 $(foreach obj,$(systemd_C_OBJS),$(obj).o): _paths.h
 
 LIBS_systemd += $(LDFLAGS-y)
@@ -57,12 +50,11 @@ OBJS = paths \
 	process \
 	xenstored
 
-INTF = symbol.cmi trie.cmi syslog.cmi systemd.cmi select.cmi
+INTF = symbol.cmi trie.cmi syslog.cmi select.cmi
 
 XENSTOREDLIBS = \
 	unix.cmxa \
 	-ccopt -L -ccopt . syslog.cmxa \
-	-ccopt -L -ccopt . systemd.cmxa \
 	-ccopt -L -ccopt . select.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 \
diff --git a/tools/ocaml/xenstored/systemd.ml b/tools/ocaml/xenstored/systemd.ml
deleted file mode 100644
index 732446d..0000000
--- a/tools/ocaml/xenstored/systemd.ml
+++ /dev/null
@@ -1,17 +0,0 @@
-(*
- * 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 launched_by_systemd: unit -> bool = "ocaml_launched_by_systemd"
-external sd_notify_ready: unit -> unit = "ocaml_sd_notify_ready"
diff --git a/tools/ocaml/xenstored/systemd.mli b/tools/ocaml/xenstored/systemd.mli
deleted file mode 100644
index 538fc5e..0000000
--- a/tools/ocaml/xenstored/systemd.mli
+++ /dev/null
@@ -1,24 +0,0 @@
-(*
- * 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 the process is launched by systemd *)
-val launched_by_systemd: 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
deleted file mode 100644
index 322f1e0..0000000
--- a/tools/ocaml/xenstored/systemd_stubs.c
+++ /dev/null
@@ -1,153 +0,0 @@
-/*
- * 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>
-
-#if defined(HAVE_SYSTEMD)
-
-#include <sys/socket.h>
-#include <systemd/sd-daemon.h>
-
-#include "_paths.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(XEN_RUN_STORED "/socket_ro", connect_to) != 0) &&
-	    (strcmp(XEN_RUN_STORED "/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_launched_by_systemd(value ignore)
-{
-	CAMLparam1(ignore);
-	CAMLlocal1(ret);
-
-	ret = Val_false;
-
-	if (sd_listen_fds(0) > 0)
-		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(-1U);
-
-	CAMLreturn(sock_ret);
-}
-
-CAMLprim value ocaml_launched_by_systemd(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(-1U);
-
-	CAMLreturn(ret);
-}
-#endif
diff --git a/tools/ocaml/xenstored/utils.ml b/tools/ocaml/xenstored/utils.ml
index 9f82c1c..68b70c5 100644
--- a/tools/ocaml/xenstored/utils.ml
+++ b/tools/ocaml/xenstored/utils.ml
@@ -73,21 +73,14 @@ 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.set_close_on_exec sock;
-        Unix.bind sock sockaddr;
-        Unix.listen sock 1;
-        sock
-
 let create_unix_socket name =
-        if Systemd.launched_by_systemd() then
-                Systemd.sd_listen_fds name
-        else
-                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 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 30570ed..8a61dfe 100644
--- a/tools/ocaml/xenstored/xenstored.ml
+++ b/tools/ocaml/xenstored/xenstored.ml
@@ -428,8 +428,6 @@ let _ =
 		process_domains store cons domains
 		in
 
-	if Systemd.launched_by_systemd () then
-		Systemd.sd_notify_ready ();
 	while not !quit
 	do
 		try
diff --git a/tools/xenstore/Makefile b/tools/xenstore/Makefile
index 36b6fd4..47d7fe0 100644
--- a/tools/xenstore/Makefile
+++ b/tools/xenstore/Makefile
@@ -14,9 +14,6 @@ CFLAGS += $(CFLAGS_libxenctrl)
 CFLAGS += -DXEN_LIB_STORED="\"$(XEN_LIB_STORED)\""
 CFLAGS += -DXEN_RUN_STORED="\"$(XEN_RUN_STORED)\""
 
-CFLAGS-$(CONFIG_SYSTEMD)  += $(SYSTEMD_CFLAGS)
-LDFLAGS-$(CONFIG_SYSTEMD) += $(SYSTEMD_LIBS)
-
 CFLAGS  += $(CFLAGS-y)
 LDFLAGS += $(LDFLAGS-y)
 
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 51fb0b3..098865f 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -54,16 +54,6 @@
 
 #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 xenevtchn_handle *xce_handle; /* in xenstored_domain.c */
 static int xce_pollfd_idx = -1;
 static struct pollfd *fds;
@@ -1751,84 +1741,6 @@ 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(xs_daemon_socket_ro(), connect_to) != 0) &&
-	    (strcmp(xs_daemon_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);
-}
-
-/* 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();
-	const char *soc_str_ro = xs_daemon_socket_ro();
-	int n;
-
-	n = sd_listen_fds(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),
-			   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);
-
-	return true;
-}
-#endif
-
 static void init_sockets(int **psock, int **pro_sock)
 {
 	struct sockaddr_un addr;
@@ -1939,9 +1851,6 @@ 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) {
@@ -2002,16 +1911,6 @@ int main(int argc, char *argv[])
 	if (optind != argc)
 		barf("%s: No arguments desired", argv[0]);
 
-#if defined(XEN_SYSTEMD_ENABLED)
-	systemd = systemd_checkin(&sock, &ro_sock);
-	if (systemd) {
-		dofork = false;
-		if (pidfile)
-			xprintf("%s: PID file not needed on systemd", argv[0]);
-		pidfile = NULL;
-	}
-#endif
-
 	reopen_log();
 
 	/* make sure xenstored directories exist */
@@ -2033,10 +1932,7 @@ int main(int argc, char *argv[])
 	/* Don't kill us with SIGPIPE. */
 	signal(SIGPIPE, SIG_IGN);
 
-#if defined(XEN_SYSTEMD_ENABLED)
-	if (!systemd)
-#endif
-		init_sockets(&sock, &ro_sock);
+	init_sockets(&sock, &ro_sock);
 
 	init_pipe(reopen_log_pipe);
 
@@ -2068,13 +1964,6 @@ int main(int argc, char *argv[])
 	/* Tell the kernel we're up and running. */
 	xenbus_notify_running();
 
-#if defined(XEN_SYSTEMD_ENABLED)
-	if (systemd) {
-		sd_notify(1, "READY=1");
-		fprintf(stderr, SD_NOTICE "xenstored is ready\n");
-	}
-#endif
-
 	/* Main loop. */
 	for (;;) {
 		struct connection *conn, *next;
-- 
2.6.6


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

[PATCH 2/2] tools: make xenstore domain easy configurable

[Juergen Gross]

Add configuration entries to sysconfig.xencommons for selection of the
xenstore type (domain or daemon) and start the selected xenstore
service via a script called from sysvinit or systemd.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/configure                                    |  2 +-
 tools/hotplug/Linux/Makefile                       |  1 +
 tools/hotplug/Linux/init.d/sysconfig.xencommons.in | 42 ++++++++++-
 tools/hotplug/Linux/init.d/xencommons.in           | 35 ++-------
 tools/hotplug/Linux/launch-xenstore.in             | 84 ++++++++++++++++++++++
 tools/hotplug/Linux/systemd/xenstored.service.in   | 11 +--
 6 files changed, 132 insertions(+), 43 deletions(-)
 create mode 100644 tools/hotplug/Linux/launch-xenstore.in

diff --git a/tools/configure b/tools/configure
index 0854271..b9b9f27 100755
--- a/tools/configure
+++ b/tools/configure
@@ -2410,7 +2410,7 @@ ac_compiler_gnu=$ac_cv_c_compiler_gnu
 
 
 
-ac_config_files="$ac_config_files ../config/Tools.mk hotplug/FreeBSD/rc.d/xencommons hotplug/FreeBSD/rc.d/xendriverdomain hotplug/Linux/init.d/sysconfig.xencommons hotplug/Linux/init.d/sysconfig.xendomains hotplug/Linux/init.d/xen-watchdog hotplug/Linux/init.d/xencommons hotplug/Linux/init.d/xendomains hotplug/Linux/init.d/xendriverdomain hotplug/Linux/vif-setup hotplug/Linux/xen-hotplug-common.sh hotplug/Linux/xendomains hotplug/NetBSD/rc.d/xencommons hotplug/NetBSD/rc.d/xendriverdomain libxl/xenlight.pc.in libxl/xlutil.pc.in ocaml/xenstored/oxenstored.conf"
+ac_config_files="$ac_config_files ../config/Tools.mk hotplug/FreeBSD/rc.d/xencommons hotplug/FreeBSD/rc.d/xendriverdomain hotplug/Linux/init.d/sysconfig.xencommons hotplug/Linux/init.d/sysconfig.xendomains hotplug/Linux/init.d/xen-watchdog hotplug/Linux/init.d/xencommons hotplug/Linux/init.d/xendomains hotplug/Linux/init.d/xendriverdomain hotplug/Linux/launch-xenstore hotplug/Linux/vif-setup hotplug/Linux/xen-hotplug-common.sh hotplug/Linux/xendomains hotplug/NetBSD/rc.d/xencommons hotplug/NetBSD/rc.d/xendriverdomain libxl/xenlight.pc.in libxl/xlutil.pc.in ocaml/xenstored/oxenstored.conf"
 
 ac_config_headers="$ac_config_headers config.h"
 
diff --git a/tools/hotplug/Linux/Makefile b/tools/hotplug/Linux/Makefile
index 6d6ccee..29280cb 100644
--- a/tools/hotplug/Linux/Makefile
+++ b/tools/hotplug/Linux/Makefile
@@ -30,6 +30,7 @@ XEN_SCRIPTS += block-drbd-probe
 XEN_SCRIPTS += block-dummy
 XEN_SCRIPTS += $(XEN_SCRIPTS-y)
 XEN_SCRIPTS += colo-proxy-setup
+XEN_SCRIPTS += launch-xenstore
 
 SUBDIRS-$(CONFIG_SYSTEMD) += systemd
 
diff --git a/tools/hotplug/Linux/init.d/sysconfig.xencommons.in b/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
index c27a476..cc8185c 100644
--- a/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
+++ b/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
@@ -6,12 +6,24 @@
 #XENCONSOLED_TRACE=[none|guest|hv|all]
 
 ## Type: string
+## Default: daemon
+#
+# Select type of xentore service.
+#
+# This can be either of:
+#  * daemon
+#  * domain
+#
+# Changing this requires a reboot to take effect.
+#
+#XENSTORETYPE=daemon
+
+## Type: string
 ## Default: 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.
+# of these below.
+# Only evaluated if XENSTORETYPE is "daemon".
 #
 # This can be either of:
 #  * @sbindir@/oxenstored
@@ -26,21 +38,45 @@
 # Additional commandline arguments to start xenstored,
 # like "--trace-file @XEN_LOG_DIR@/xenstored-trace.log"
 # See "@sbindir@/xenstored --help" for possible options.
+# Only evaluated if XENSTORETYPE is "daemon".
 XENSTORED_ARGS=
 
 ## Type: string
 ## Default: Not defined, tracing off
 #
 # Log xenstored messages
+# Only evaluated if XENSTORETYPE is "daemon".
 #XENSTORED_TRACE=[yes|on|1]
 
 ## Type: string
 ## Default: "@XEN_LIB_STORED@"
 #
 # Running xenstored on XENSTORED_ROOTDIR
+# Only evaluated if XENSTORETYPE is "daemon".
 #XENSTORED_ROOTDIR=@XEN_LIB_STORED@
 
 ## Type: string
+## Default: @LIBEXEC@/boot/xenstore-stubdom.gz
+#
+# xenstore domain kernel.
+# Only evaluated if XENSTORETYPE is "domain".
+#XENSTORE_DOMAIN_KERNEL=@LIBEXEC@/boot/xenstore-stubdom.gz
+
+## Type: integer
+## Default: 8
+#
+# xenstore domain memory size in MiB.
+# Only evaluated if XENSTORETYPE is "domain".
+#XENSTORE_DOMAIN_SIZE=8
+
+## Type: string
+## Default: ""
+#
+# Additional arguments for starting the xenstore domain.
+# Only evaluated if XENSTORETYPE is "domain".
+XENSTORE_DOMAIN_ARGS=
+
+## Type: string
 ## Default: Not defined, xenbackendd debug mode off
 #
 # Running xenbackendd in debug mode
diff --git a/tools/hotplug/Linux/init.d/xencommons.in b/tools/hotplug/Linux/init.d/xencommons.in
index 7b69fc2..1f4f198 100644
--- a/tools/hotplug/Linux/init.d/xencommons.in
+++ b/tools/hotplug/Linux/init.d/xencommons.in
@@ -62,37 +62,10 @@ do_start () {
 	mkdir -p ${XEN_RUN_DIR}
 	mkdir -p ${XEN_LOCK_DIR}
 
-	if ! `${bindir}/xenstore-read -s / >/dev/null 2>&1`
-	then
-		test -z "$XENSTORED_ROOTDIR" && XENSTORED_ROOTDIR="@XEN_LIB_STORED@"
-		rm -f "$XENSTORED_ROOTDIR"/tdb* &>/dev/null
-		test -z "$XENSTORED_TRACE" || XENSTORED_ARGS=" -T @XEN_LOG_DIR@/xenstored-trace.log"
+	@XEN_SCRIPT_DIR@/launch-xenstore || exit 1
 
-		if [ -n "$XENSTORED" ] ; then
-		    echo -n Starting $XENSTORED...
-		    $XENSTORED --pid-file /var/run/xenstored.pid $XENSTORED_ARGS
-		else
-		    echo "No xenstored found"
-		    exit 1
-		fi
-
-		# Wait for xenstored to actually come up, timing out after 30 seconds
-                while [ $time -lt $timeout ] && ! `${bindir}/xenstore-read -s / >/dev/null 2>&1` ; do
-                    echo -n .
-		    time=$(($time+1))
-                    sleep 1
-                done
-		echo
-
-		# Exit if we timed out
-		if ! [ $time -lt $timeout ] ; then
-		    echo Could not start xenstored
-		    exit 1
-		fi
-
-		echo Setting domain 0 name, domid and JSON config...
-		${LIBEXEC_BIN}/xen-init-dom0
-	fi
+	echo Setting domain 0 name, domid and JSON config...
+	${LIBEXEC_BIN}/xen-init-dom0
 
 	echo Starting xenconsoled...
 	test -z "$XENCONSOLED_TRACE" || XENCONSOLED_ARGS=" --log=$XENCONSOLED_TRACE"
@@ -126,7 +99,7 @@ case "$1" in
 	do_start
 	;;
   status)
-        ${bindir}/xenstore-read -s /
+        test -f /var/run/xenstored.pid
 	;;
   stop)
 	do_stop
diff --git a/tools/hotplug/Linux/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in
new file mode 100644
index 0000000..ca082b1
--- /dev/null
+++ b/tools/hotplug/Linux/launch-xenstore.in
@@ -0,0 +1,84 @@
+#
+# Copyright (c) 2016 SUSE Linux GmbH
+#
+# This library is free software; you can redistribute it and/or
+# modify it under the terms of version 2.1 of the GNU Lesser General Public
+# License as published by the Free Software Foundation.
+#
+# This library 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.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this library; If not, see <http://www.gnu.org/licenses/>.
+#
+
+. @XEN_SCRIPT_DIR@/hotplugpath.sh
+
+test_xenstore () {
+	test -f /var/run/xenstored.pid
+	return $?
+}
+
+timeout_xenstore () {
+	local time=0
+	local timeout=30
+
+	while [ $time -lt $timeout ] && ! test_xenstore ; do
+		echo -n .
+		time=$(($time+1))
+		sleep 1
+	done
+	echo
+
+	# Exit if we timed out
+	if ! [ $time -lt $timeout ] ; then
+		echo "Could not start $@"
+		return 1
+	fi
+
+	return 0
+}
+
+test_xenstore && exit 0
+
+test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons
+
+[ "$XENSTORETYPE" = "" ] && XENSTORETYPE=daemon
+
+[ "$XENSTORETYPE" = "daemon" ] && {
+	[ -z "$XENSTORED_ROOTDIR" ] && XENSTORED_ROOTDIR="@XEN_LIB_STORED@"
+	/bin/rm -f $XENSTORED_ROOTDIR/tdb* &>/dev/null
+	/bin/mkdir -p @XEN_RUN_DIR@
+	[ -z "$XENSTORED_TRACE" ] || XENSTORED_ARGS="$XENSTORED_ARGS -T @XEN_LOG_DIR@/xenstored-trace.log"
+	[ -z "$XENSTORED" ] && XENSTORED=@XENSTORED@
+	[ -x "$XENSTORED" ] || {
+		echo "No xenstored found"
+		exit 1
+	}
+
+	echo -n Starting $XENSTORED...
+	$XENSTORED --pid-file /var/run/xenstored.pid $XENSTORED_ARGS
+
+	timeout_xenstore xenstored || exit 1
+
+	exit 0
+}
+
+[ "$XENSTORETYPE" = "domain" ] && {
+	[ -z "$XENSTORE_DOMAIN_KERNEL" ] && XENSTORE_DOMAIN_KERNEL=@LIBEXEC@/boot/xenstore-stubdom.gz
+	XENSTORE_DOMAIN_ARGS="$XENSTORE_DOMAIN_ARGS --kernel $XENSTORE_DOMAIN_KERNEL"
+	[ -z "$XENSTORE_DOMAIN_SIZE" ] && XENSTORE_DOMAIN_SIZE=8
+	XENSTORE_DOMAIN_ARGS="$XENSTORE_DOMAIN_ARGS --memory $XENSTORE_DOMAIN_SIZE"
+
+	echo -n Starting $XENSTORE_DOMAIN_KERNEL...
+	${LIBEXEC_BIN}/init-xenstore-domain $XENSTORE_DOMAIN_ARGS
+
+	timeout_xenstore xenstore domain || exit 1
+
+	exit 0
+}
+
+echo "illegal value $XENSTORETYPE for XENSTORETYPE"
+exit 1
diff --git a/tools/hotplug/Linux/systemd/xenstored.service.in b/tools/hotplug/Linux/systemd/xenstored.service.in
index a1e1db1..6467b77 100644
--- a/tools/hotplug/Linux/systemd/xenstored.service.in
+++ b/tools/hotplug/Linux/systemd/xenstored.service.in
@@ -7,15 +7,10 @@ RefuseManualStop=true
 ConditionPathExists=/proc/xen/capabilities
 
 [Service]
-Type=notify
-KillMode=none
-Environment=XENSTORED_ARGS=
-Environment=XENSTORED=@XENSTORED@
-EnvironmentFile=-@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons
+Type=oneshot
+RemainAfterExit=true
 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"
+ExecStart=@XEN_SCRIPT_DIR@/launch-xenstore
 
 [Install]
 WantedBy=multi-user.target
-- 
2.6.6


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

Re: [PATCH 1/2] tools: remove systemd xenstore socket definitions

[Andrew Cooper]

On 29/06/16 13:44, Juergen Gross wrote:
> @@ -2068,13 +1964,6 @@ int main(int argc, char *argv[])
>  	/* Tell the kernel we're up and running. */
>  	xenbus_notify_running();
>  
> -#if defined(XEN_SYSTEMD_ENABLED)
> -	if (systemd) {
> -		sd_notify(1, "READY=1");
> -		fprintf(stderr, SD_NOTICE "xenstored is ready\n");
> -	}
> -#endif

Getting rid of the socket configuration for systemd is ok, but we should
keep the sd_notify() calls for when the daemon is started by systemd.

Socket activiation and sd_notify() are orthogonal, and sd_notify() is
still required if we don't want systemd to treat xenstored as a legacy
unix daemon.

~Andrew

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

Re: [PATCH 1/2] tools: remove systemd xenstore socket definitions

[Juergen Gross]

On 29/06/16 14:52, Andrew Cooper wrote:
> On 29/06/16 13:44, Juergen Gross wrote:
>> @@ -2068,13 +1964,6 @@ int main(int argc, char *argv[])
>>  	/* Tell the kernel we're up and running. */
>>  	xenbus_notify_running();
>>  
>> -#if defined(XEN_SYSTEMD_ENABLED)
>> -	if (systemd) {
>> -		sd_notify(1, "READY=1");
>> -		fprintf(stderr, SD_NOTICE "xenstored is ready\n");
>> -	}
>> -#endif
> 
> Getting rid of the socket configuration for systemd is ok, but we should
> keep the sd_notify() calls for when the daemon is started by systemd.
> 
> Socket activiation and sd_notify() are orthogonal, and sd_notify() is
> still required if we don't want systemd to treat xenstored as a legacy
> unix daemon.

So what is the downside of xenstored being treated as a legacy daemon?
This question is especially interesting for the case of patch 2 being
considered: xenstored is no longer started by systemd, but by a wrapper
script which might decide to start the xenstore domain instead.

Another problem: today xenstored decides whether to call sd_notify()
by testing the xenstore sockets being specified via systemd. This will
no longer work. So how to do it now?


Juergen


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

Re: [PATCH 1/2] tools: remove systemd xenstore socket definitions

[Ross Lagerwall]

On 06/29/2016 02:00 PM, Juergen Gross wrote:
> On 29/06/16 14:52, Andrew Cooper wrote:
>> On 29/06/16 13:44, Juergen Gross wrote:
>>> @@ -2068,13 +1964,6 @@ int main(int argc, char *argv[])
>>>  	/* Tell the kernel we're up and running. */
>>>  	xenbus_notify_running();
>>>
>>> -#if defined(XEN_SYSTEMD_ENABLED)
>>> -	if (systemd) {
>>> -		sd_notify(1, "READY=1");
>>> -		fprintf(stderr, SD_NOTICE "xenstored is ready\n");
>>> -	}
>>> -#endif
>>
>> Getting rid of the socket configuration for systemd is ok, but we should
>> keep the sd_notify() calls for when the daemon is started by systemd.
>>
>> Socket activiation and sd_notify() are orthogonal, and sd_notify() is
>> still required if we don't want systemd to treat xenstored as a legacy
>> unix daemon.
>
> So what is the downside of xenstored being treated as a legacy daemon?
> This question is especially interesting for the case of patch 2 being
> considered: xenstored is no longer started by systemd, but by a wrapper
> script which might decide to start the xenstore domain instead.
>
> Another problem: today xenstored decides whether to call sd_notify()
> by testing the xenstore sockets being specified via systemd. This will
> no longer work. So how to do it now?
>
>

One problem with the patch as it currently is implemented is that the 
service type is not correct for when xenstored is a daemon. This makes 
it difficult to manage with systemd and difficult for other services to 
depend on it in a sensible fashion. The end result is subtle races and 
occasional failures.

How about:
* Maintain the existing service for xenstored
* Have a separate service (with different a service type) for starting 
the xenstore domain
* Switch between the two services

Personally I think it is better and more uniform for the administrator 
to enable and disable services in the normal fashion, but if you want to 
make it configurable with /etc/sysconfig/xencommons, then you can add 
something like this to xenstored.service:

ExecStartPre=/bin/grep -q XENSTORETYPE=daemon /etc/sysconfig/xencommons

and to xenstore-domain.service:

ExecStartPre=/bin/grep -q XENSTORETYPE=domain /etc/sysconfig/xencommons

Regards,
-- 
Ross Lagerwall

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

Re: [PATCH 1/2] tools: remove systemd xenstore socket definitions

[Juergen Gross]

On 29/06/16 15:31, Ross Lagerwall wrote:
> On 06/29/2016 02:00 PM, Juergen Gross wrote:
>> On 29/06/16 14:52, Andrew Cooper wrote:
>>> On 29/06/16 13:44, Juergen Gross wrote:
>>>> @@ -2068,13 +1964,6 @@ int main(int argc, char *argv[])
>>>>      /* Tell the kernel we're up and running. */
>>>>      xenbus_notify_running();
>>>>
>>>> -#if defined(XEN_SYSTEMD_ENABLED)
>>>> -    if (systemd) {
>>>> -        sd_notify(1, "READY=1");
>>>> -        fprintf(stderr, SD_NOTICE "xenstored is ready\n");
>>>> -    }
>>>> -#endif
>>>
>>> Getting rid of the socket configuration for systemd is ok, but we should
>>> keep the sd_notify() calls for when the daemon is started by systemd.
>>>
>>> Socket activiation and sd_notify() are orthogonal, and sd_notify() is
>>> still required if we don't want systemd to treat xenstored as a legacy
>>> unix daemon.
>>
>> So what is the downside of xenstored being treated as a legacy daemon?
>> This question is especially interesting for the case of patch 2 being
>> considered: xenstored is no longer started by systemd, but by a wrapper
>> script which might decide to start the xenstore domain instead.
>>
>> Another problem: today xenstored decides whether to call sd_notify()
>> by testing the xenstore sockets being specified via systemd. This will
>> no longer work. So how to do it now?
>>
>>
> 
> One problem with the patch as it currently is implemented is that the
> service type is not correct for when xenstored is a daemon. This makes
> it difficult to manage with systemd and difficult for other services to
> depend on it in a sensible fashion. The end result is subtle races and
> occasional failures.

Could you please educate me what's wrong? I'm no systemd expert.

> How about:
> * Maintain the existing service for xenstored
> * Have a separate service (with different a service type) for starting
> the xenstore domain
> * Switch between the two services

How could I specify e.g. in xendomains.service the dependency to
xenstore?

> Personally I think it is better and more uniform for the administrator
> to enable and disable services in the normal fashion, but if you want to

The two services would be mutually exclusive. Can I tell systemd this
is the case?

> make it configurable with /etc/sysconfig/xencommons, then you can add
> something like this to xenstored.service:
> 
> ExecStartPre=/bin/grep -q XENSTORETYPE=daemon /etc/sysconfig/xencommons
> 
> and to xenstore-domain.service:
> 
> ExecStartPre=/bin/grep -q XENSTORETYPE=domain /etc/sysconfig/xencommons

That's no good idea. Someone commenting out the old line and adding the
other option would end in both variants to be tried. This would have to
be a little bit more sophisticated. :-)


Juergen

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

Re: [PATCH 1/2] tools: remove systemd xenstore socket definitions

[Wei Liu]

On Wed, Jun 29, 2016 at 03:00:41PM +0200, Juergen Gross wrote:
> On 29/06/16 14:52, Andrew Cooper wrote:
> > On 29/06/16 13:44, Juergen Gross wrote:
> >> @@ -2068,13 +1964,6 @@ int main(int argc, char *argv[])
> >>  	/* Tell the kernel we're up and running. */
> >>  	xenbus_notify_running();
> >>  
> >> -#if defined(XEN_SYSTEMD_ENABLED)
> >> -	if (systemd) {
> >> -		sd_notify(1, "READY=1");
> >> -		fprintf(stderr, SD_NOTICE "xenstored is ready\n");
> >> -	}
> >> -#endif
> > 
> > Getting rid of the socket configuration for systemd is ok, but we should
> > keep the sd_notify() calls for when the daemon is started by systemd.
> > 
> > Socket activiation and sd_notify() are orthogonal, and sd_notify() is
> > still required if we don't want systemd to treat xenstored as a legacy
> > unix daemon.
> 
> So what is the downside of xenstored being treated as a legacy daemon?
> This question is especially interesting for the case of patch 2 being
> considered: xenstored is no longer started by systemd, but by a wrapper
> script which might decide to start the xenstore domain instead.
> 
> Another problem: today xenstored decides whether to call sd_notify()
> by testing the xenstore sockets being specified via systemd. This will
> no longer work. So how to do it now?
> 

Not sure I follow.

See 81d758afca7c3c1e3ccbd78154b33d64fd7757fb. I expect systemd_checkin
to be able to tell if cxenstored is started by systemd or not.
sd_listen_fds doesn't seem to involve testing xenstore sockets. Do I
miss anything?

Wei.

> 
> Juergen
> 

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

Re: [PATCH 1/2] tools: remove systemd xenstore socket definitions

[Juergen Gross]

On 08/07/16 14:15, Wei Liu wrote:
> On Wed, Jun 29, 2016 at 03:00:41PM +0200, Juergen Gross wrote:
>> On 29/06/16 14:52, Andrew Cooper wrote:
>>> On 29/06/16 13:44, Juergen Gross wrote:
>>>> @@ -2068,13 +1964,6 @@ int main(int argc, char *argv[])
>>>>  	/* Tell the kernel we're up and running. */
>>>>  	xenbus_notify_running();
>>>>  
>>>> -#if defined(XEN_SYSTEMD_ENABLED)
>>>> -	if (systemd) {
>>>> -		sd_notify(1, "READY=1");
>>>> -		fprintf(stderr, SD_NOTICE "xenstored is ready\n");
>>>> -	}
>>>> -#endif
>>>
>>> Getting rid of the socket configuration for systemd is ok, but we should
>>> keep the sd_notify() calls for when the daemon is started by systemd.
>>>
>>> Socket activiation and sd_notify() are orthogonal, and sd_notify() is
>>> still required if we don't want systemd to treat xenstored as a legacy
>>> unix daemon.
>>
>> So what is the downside of xenstored being treated as a legacy daemon?
>> This question is especially interesting for the case of patch 2 being
>> considered: xenstored is no longer started by systemd, but by a wrapper
>> script which might decide to start the xenstore domain instead.
>>
>> Another problem: today xenstored decides whether to call sd_notify()
>> by testing the xenstore sockets being specified via systemd. This will
>> no longer work. So how to do it now?
>>
> 
> Not sure I follow.
> 
> See 81d758afca7c3c1e3ccbd78154b33d64fd7757fb. I expect systemd_checkin
> to be able to tell if cxenstored is started by systemd or not.
> sd_listen_fds doesn't seem to involve testing xenstore sockets. Do I
> miss anything?

Did you have a look at systemd_checkin()? It expects systemd does know
exactly the number of sockets xenstored needs. Only if systemd knows
about those sockets xenstored is regarded to have been started via
systemd.


Juergen


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

Re: [PATCH 1/2] tools: remove systemd xenstore socket definitions

[Wei Liu]

On Fri, Jul 08, 2016 at 02:32:31PM +0200, Juergen Gross wrote:
> On 08/07/16 14:15, Wei Liu wrote:
> > On Wed, Jun 29, 2016 at 03:00:41PM +0200, Juergen Gross wrote:
> >> On 29/06/16 14:52, Andrew Cooper wrote:
> >>> On 29/06/16 13:44, Juergen Gross wrote:
> >>>> @@ -2068,13 +1964,6 @@ int main(int argc, char *argv[])
> >>>>  	/* Tell the kernel we're up and running. */
> >>>>  	xenbus_notify_running();
> >>>>  
> >>>> -#if defined(XEN_SYSTEMD_ENABLED)
> >>>> -	if (systemd) {
> >>>> -		sd_notify(1, "READY=1");
> >>>> -		fprintf(stderr, SD_NOTICE "xenstored is ready\n");
> >>>> -	}
> >>>> -#endif
> >>>
> >>> Getting rid of the socket configuration for systemd is ok, but we should
> >>> keep the sd_notify() calls for when the daemon is started by systemd.
> >>>
> >>> Socket activiation and sd_notify() are orthogonal, and sd_notify() is
> >>> still required if we don't want systemd to treat xenstored as a legacy
> >>> unix daemon.
> >>
> >> So what is the downside of xenstored being treated as a legacy daemon?
> >> This question is especially interesting for the case of patch 2 being
> >> considered: xenstored is no longer started by systemd, but by a wrapper
> >> script which might decide to start the xenstore domain instead.
> >>
> >> Another problem: today xenstored decides whether to call sd_notify()
> >> by testing the xenstore sockets being specified via systemd. This will
> >> no longer work. So how to do it now?
> >>
> > 
> > Not sure I follow.
> > 
> > See 81d758afca7c3c1e3ccbd78154b33d64fd7757fb. I expect systemd_checkin
> > to be able to tell if cxenstored is started by systemd or not.
> > sd_listen_fds doesn't seem to involve testing xenstore sockets. Do I
> > miss anything?
> 
> Did you have a look at systemd_checkin()? It expects systemd does know
> exactly the number of sockets xenstored needs. Only if systemd knows
> about those sockets xenstored is regarded to have been started via
> systemd.
> 
> 

Ah, I see what you meant. I will let someone who is more familiar with
systemd to comment.

Wei.

> Juergen
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel

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

Re: [PATCH 1/2] tools: remove systemd xenstore socket definitions

[Juergen Gross]

On 29/06/16 15:44, Juergen Gross wrote:
> On 29/06/16 15:31, Ross Lagerwall wrote:
>> On 06/29/2016 02:00 PM, Juergen Gross wrote:
>>> On 29/06/16 14:52, Andrew Cooper wrote:
>>>> On 29/06/16 13:44, Juergen Gross wrote:
>>>>> @@ -2068,13 +1964,6 @@ int main(int argc, char *argv[])
>>>>>      /* Tell the kernel we're up and running. */
>>>>>      xenbus_notify_running();
>>>>>
>>>>> -#if defined(XEN_SYSTEMD_ENABLED)
>>>>> -    if (systemd) {
>>>>> -        sd_notify(1, "READY=1");
>>>>> -        fprintf(stderr, SD_NOTICE "xenstored is ready\n");
>>>>> -    }
>>>>> -#endif
>>>>
>>>> Getting rid of the socket configuration for systemd is ok, but we should
>>>> keep the sd_notify() calls for when the daemon is started by systemd.
>>>>
>>>> Socket activiation and sd_notify() are orthogonal, and sd_notify() is
>>>> still required if we don't want systemd to treat xenstored as a legacy
>>>> unix daemon.
>>>
>>> So what is the downside of xenstored being treated as a legacy daemon?
>>> This question is especially interesting for the case of patch 2 being
>>> considered: xenstored is no longer started by systemd, but by a wrapper
>>> script which might decide to start the xenstore domain instead.
>>>
>>> Another problem: today xenstored decides whether to call sd_notify()
>>> by testing the xenstore sockets being specified via systemd. This will
>>> no longer work. So how to do it now?
>>>
>>>
>>
>> One problem with the patch as it currently is implemented is that the
>> service type is not correct for when xenstored is a daemon. This makes
>> it difficult to manage with systemd and difficult for other services to
>> depend on it in a sensible fashion. The end result is subtle races and
>> occasional failures.
> 
> Could you please educate me what's wrong? I'm no systemd expert.

Ross, you had some concerns I'm still not sure I understand.

Could you please answer my questions?

>> How about:
>> * Maintain the existing service for xenstored
>> * Have a separate service (with different a service type) for starting
>> the xenstore domain
>> * Switch between the two services
> 
> How could I specify e.g. in xendomains.service the dependency to
> xenstore?
> 
>> Personally I think it is better and more uniform for the administrator
>> to enable and disable services in the normal fashion, but if you want to
> 
> The two services would be mutually exclusive. Can I tell systemd this
> is the case?
> 
>> make it configurable with /etc/sysconfig/xencommons, then you can add
>> something like this to xenstored.service:
>>
>> ExecStartPre=/bin/grep -q XENSTORETYPE=daemon /etc/sysconfig/xencommons
>>
>> and to xenstore-domain.service:
>>
>> ExecStartPre=/bin/grep -q XENSTORETYPE=domain /etc/sysconfig/xencommons
> 
> That's no good idea. Someone commenting out the old line and adding the
> other option would end in both variants to be tried. This would have to
> be a little bit more sophisticated. :-)

Juergen


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

Re: [PATCH 1/2] tools: remove systemd xenstore socket definitions

[Ross Lagerwall]

On 06/29/2016 02:44 PM, Juergen Gross wrote:
> On 29/06/16 15:31, Ross Lagerwall wrote:
>> On 06/29/2016 02:00 PM, Juergen Gross wrote:
>>> On 29/06/16 14:52, Andrew Cooper wrote:
>>>> On 29/06/16 13:44, Juergen Gross wrote:
>>>>> @@ -2068,13 +1964,6 @@ int main(int argc, char *argv[])
>>>>>      /* Tell the kernel we're up and running. */
>>>>>      xenbus_notify_running();
>>>>>
>>>>> -#if defined(XEN_SYSTEMD_ENABLED)
>>>>> -    if (systemd) {
>>>>> -        sd_notify(1, "READY=1");
>>>>> -        fprintf(stderr, SD_NOTICE "xenstored is ready\n");
>>>>> -    }
>>>>> -#endif
>>>>
>>>> Getting rid of the socket configuration for systemd is ok, but we should
>>>> keep the sd_notify() calls for when the daemon is started by systemd.
>>>>
>>>> Socket activiation and sd_notify() are orthogonal, and sd_notify() is
>>>> still required if we don't want systemd to treat xenstored as a legacy
>>>> unix daemon.
>>>
>>> So what is the downside of xenstored being treated as a legacy daemon?
>>> This question is especially interesting for the case of patch 2 being
>>> considered: xenstored is no longer started by systemd, but by a wrapper
>>> script which might decide to start the xenstore domain instead.
>>>
>>> Another problem: today xenstored decides whether to call sd_notify()
>>> by testing the xenstore sockets being specified via systemd. This will
>>> no longer work. So how to do it now?
>>>
>>>
>>
>> One problem with the patch as it currently is implemented is that the
>> service type is not correct for when xenstored is a daemon. This makes
>> it difficult to manage with systemd and difficult for other services to
>> depend on it in a sensible fashion. The end result is subtle races and
>> occasional failures.
>
> Could you please educate me what's wrong? I'm no systemd expert.

Sorry for the late response.

With Type=oneshot, systemd considers the service to be "started" as soon 
as the ExecStart command completes. After re-reading the patches, I 
realize that timeout_xenstore() should ensure that xenstored is actually 
ready before systemd starts dependent services. This should prevent the 
races I was mentioned previously.

Nevertheless, I feel that this patch series makes the implementation 
worse for users of xenstored as a daemon:

- Because Type=oneshot is not intended to be used for daemon processes, 
systemctl status does not show the service as "running", instead it 
shows "exited". This is surprising, at the very least. I haven't tested, 
but I believe it would also prevent us someday using the systemd service 
management features like Restart=on-failure

- Removes socket activation. Services would have to explicitly depend on 
xenstored.service rather than having an implicit dependency on simply by 
using xenstored.socket. This means configuring explicit ordering, etc., 
which is easier to get wrong. Socket activation is also generally the 
most efficient way of starting a service.

- Uses a poll loop to determine whether the daemon is ready or not 
rather than explicit notification from the daemon itself.

- Uses a shell script wrapper...

>
>> How about:
>> * Maintain the existing service for xenstored
>> * Have a separate service (with different a service type) for starting
>> the xenstore domain
>> * Switch between the two services
>
> How could I specify e.g. in xendomains.service the dependency to
> xenstore?

Hmm. I'm not sure of the best way, but it should be possible to define 
something like xenstored.target:
[Unit]
Description=...
Wants=xenstored.service
Wants=xenstore-domain.service

and then have services depend on xenstored.target. With the ExecStartPre 
lines I mentioned previously, only one of the xenstore*.service will 
actually start.

>
>> Personally I think it is better and more uniform for the administrator
>> to enable and disable services in the normal fashion, but if you want to
>
> The two services would be mutually exclusive. Can I tell systemd this
> is the case?

It is possible to set Conflicts=... on a service. I'm not sure if that 
would suit your needs.

>
>> make it configurable with /etc/sysconfig/xencommons, then you can add
>> something like this to xenstored.service:
>>
>> ExecStartPre=/bin/grep -q XENSTORETYPE=daemon /etc/sysconfig/xencommons
>>
>> and to xenstore-domain.service:
>>
>> ExecStartPre=/bin/grep -q XENSTORETYPE=domain /etc/sysconfig/xencommons
>
> That's no good idea. Someone commenting out the old line and adding the
> other option would end in both variants to be tried. This would have to
> be a little bit more sophisticated. :-)
>

Yeah, indeed...


I would suggest asking on the systemd-devel mailing list or the #systemd 
IRC channel on freenode. They should be able to suggest the best way of 
solving this.

Thanks,
-- 
Ross Lagerwall

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

Re: [PATCH 1/2] tools: remove systemd xenstore socket definitions

[Juergen Gross]

On 20/07/16 11:02, Ross Lagerwall wrote:
> On 06/29/2016 02:44 PM, Juergen Gross wrote:
>> On 29/06/16 15:31, Ross Lagerwall wrote:
>>> On 06/29/2016 02:00 PM, Juergen Gross wrote:
>>>> On 29/06/16 14:52, Andrew Cooper wrote:
>>>>> On 29/06/16 13:44, Juergen Gross wrote:
>>>>>> @@ -2068,13 +1964,6 @@ int main(int argc, char *argv[])
>>>>>>      /* Tell the kernel we're up and running. */
>>>>>>      xenbus_notify_running();
>>>>>>
>>>>>> -#if defined(XEN_SYSTEMD_ENABLED)
>>>>>> -    if (systemd) {
>>>>>> -        sd_notify(1, "READY=1");
>>>>>> -        fprintf(stderr, SD_NOTICE "xenstored is ready\n");
>>>>>> -    }
>>>>>> -#endif
>>>>>
>>>>> Getting rid of the socket configuration for systemd is ok, but we
>>>>> should
>>>>> keep the sd_notify() calls for when the daemon is started by systemd.
>>>>>
>>>>> Socket activiation and sd_notify() are orthogonal, and sd_notify() is
>>>>> still required if we don't want systemd to treat xenstored as a legacy
>>>>> unix daemon.
>>>>
>>>> So what is the downside of xenstored being treated as a legacy daemon?
>>>> This question is especially interesting for the case of patch 2 being
>>>> considered: xenstored is no longer started by systemd, but by a wrapper
>>>> script which might decide to start the xenstore domain instead.
>>>>
>>>> Another problem: today xenstored decides whether to call sd_notify()
>>>> by testing the xenstore sockets being specified via systemd. This will
>>>> no longer work. So how to do it now?
>>>>
>>>>
>>>
>>> One problem with the patch as it currently is implemented is that the
>>> service type is not correct for when xenstored is a daemon. This makes
>>> it difficult to manage with systemd and difficult for other services to
>>> depend on it in a sensible fashion. The end result is subtle races and
>>> occasional failures.
>>
>> Could you please educate me what's wrong? I'm no systemd expert.
> 
> Sorry for the late response.
> 
> With Type=oneshot, systemd considers the service to be "started" as soon
> as the ExecStart command completes. After re-reading the patches, I
> realize that timeout_xenstore() should ensure that xenstored is actually
> ready before systemd starts dependent services. This should prevent the
> races I was mentioned previously.
> 
> Nevertheless, I feel that this patch series makes the implementation
> worse for users of xenstored as a daemon:
> 
> - Because Type=oneshot is not intended to be used for daemon processes,
> systemctl status does not show the service as "running", instead it
> shows "exited". This is surprising, at the very least. I haven't tested,
> but I believe it would also prevent us someday using the systemd service
> management features like Restart=on-failure
> 
> - Removes socket activation. Services would have to explicitly depend on
> xenstored.service rather than having an implicit dependency on simply by
> using xenstored.socket. This means configuring explicit ordering, etc.,
> which is easier to get wrong. Socket activation is also generally the
> most efficient way of starting a service.

So you are not taking xenstore domain into account at all.

There is no socket for xenstore domain so wanting to rely on socket
activation is the wrong way.

> - Uses a poll loop to determine whether the daemon is ready or not
> rather than explicit notification from the daemon itself.

How to do this in the domain case?

> - Uses a shell script wrapper...

That's on purpose: this enables me to switch between both setups
(daemon or domain) by modifying only _one_ configuration file.

>>> How about:
>>> * Maintain the existing service for xenstored
>>> * Have a separate service (with different a service type) for starting
>>> the xenstore domain
>>> * Switch between the two services
>>
>> How could I specify e.g. in xendomains.service the dependency to
>> xenstore?
> 
> Hmm. I'm not sure of the best way, but it should be possible to define
> something like xenstored.target:
> [Unit]
> Description=...
> Wants=xenstored.service
> Wants=xenstore-domain.service
> 
> and then have services depend on xenstored.target. With the ExecStartPre
> lines I mentioned previously, only one of the xenstore*.service will
> actually start.

And the other will be "failed". How is this better than the current
"exited" state?

>>> Personally I think it is better and more uniform for the administrator
>>> to enable and disable services in the normal fashion, but if you want to
>>
>> The two services would be mutually exclusive. Can I tell systemd this
>> is the case?
> 
> It is possible to set Conflicts=... on a service. I'm not sure if that
> would suit your needs.

On a first glance I don't think so. This would be interesting in case I
could switch between both setups during runtime, but it is no easy way
to select one of two mutual exclusive services at boot time.

>>> make it configurable with /etc/sysconfig/xencommons, then you can add
>>> something like this to xenstored.service:
>>>
>>> ExecStartPre=/bin/grep -q XENSTORETYPE=daemon /etc/sysconfig/xencommons
>>>
>>> and to xenstore-domain.service:
>>>
>>> ExecStartPre=/bin/grep -q XENSTORETYPE=domain /etc/sysconfig/xencommons
>>
>> That's no good idea. Someone commenting out the old line and adding the
>> other option would end in both variants to be tried. This would have to
>> be a little bit more sophisticated. :-)
>>
> 
> Yeah, indeed...
> 
> 
> I would suggest asking on the systemd-devel mailing list or the #systemd
> IRC channel on freenode. They should be able to suggest the best way of
> solving this.

I'm sure I could tweak the whole setup more in direction of systemd.
I'm not sure the final picture would be the preferred one.

All of your reasoning seems to boil down to rejecting the "exited" state
of the xenstore service. The only way to avoid this (but only for the
daemon case, the domain would still suffer from it) would be to accept
a "failed" state for the complementary service. In case you want to
avoid removing the systemd socket handling you'd add those to be
"failed" in the domain case, too.

I think the maintainers should decide. Ian, Wei?


Juergen

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

Re: [PATCH 1/2] tools: remove systemd xenstore socket definitions

[George Dunlap]

On Wed, Jul 20, 2016 at 10:58 AM, Juergen Gross <jgross@suse.com> wrote:
> On 20/07/16 11:02, Ross Lagerwall wrote:
>> On 06/29/2016 02:44 PM, Juergen Gross wrote:
>>> On 29/06/16 15:31, Ross Lagerwall wrote:
>>>> On 06/29/2016 02:00 PM, Juergen Gross wrote:
>>>>> On 29/06/16 14:52, Andrew Cooper wrote:
>>>>>> On 29/06/16 13:44, Juergen Gross wrote:
>>>>>>> @@ -2068,13 +1964,6 @@ int main(int argc, char *argv[])
>>>>>>>      /* Tell the kernel we're up and running. */
>>>>>>>      xenbus_notify_running();
>>>>>>>
>>>>>>> -#if defined(XEN_SYSTEMD_ENABLED)
>>>>>>> -    if (systemd) {
>>>>>>> -        sd_notify(1, "READY=1");
>>>>>>> -        fprintf(stderr, SD_NOTICE "xenstored is ready\n");
>>>>>>> -    }
>>>>>>> -#endif
>>>>>>
>>>>>> Getting rid of the socket configuration for systemd is ok, but we
>>>>>> should
>>>>>> keep the sd_notify() calls for when the daemon is started by systemd.
>>>>>>
>>>>>> Socket activiation and sd_notify() are orthogonal, and sd_notify() is
>>>>>> still required if we don't want systemd to treat xenstored as a legacy
>>>>>> unix daemon.
>>>>>
>>>>> So what is the downside of xenstored being treated as a legacy daemon?
>>>>> This question is especially interesting for the case of patch 2 being
>>>>> considered: xenstored is no longer started by systemd, but by a wrapper
>>>>> script which might decide to start the xenstore domain instead.
>>>>>
>>>>> Another problem: today xenstored decides whether to call sd_notify()
>>>>> by testing the xenstore sockets being specified via systemd. This will
>>>>> no longer work. So how to do it now?
>>>>>
>>>>>
>>>>
>>>> One problem with the patch as it currently is implemented is that the
>>>> service type is not correct for when xenstored is a daemon. This makes
>>>> it difficult to manage with systemd and difficult for other services to
>>>> depend on it in a sensible fashion. The end result is subtle races and
>>>> occasional failures.
>>>
>>> Could you please educate me what's wrong? I'm no systemd expert.
>>
>> Sorry for the late response.
>>
>> With Type=oneshot, systemd considers the service to be "started" as soon
>> as the ExecStart command completes. After re-reading the patches, I
>> realize that timeout_xenstore() should ensure that xenstored is actually
>> ready before systemd starts dependent services. This should prevent the
>> races I was mentioned previously.
>>
>> Nevertheless, I feel that this patch series makes the implementation
>> worse for users of xenstored as a daemon:
>>
>> - Because Type=oneshot is not intended to be used for daemon processes,
>> systemctl status does not show the service as "running", instead it
>> shows "exited". This is surprising, at the very least. I haven't tested,
>> but I believe it would also prevent us someday using the systemd service
>> management features like Restart=on-failure
>>
>> - Removes socket activation. Services would have to explicitly depend on
>> xenstored.service rather than having an implicit dependency on simply by
>> using xenstored.socket. This means configuring explicit ordering, etc.,
>> which is easier to get wrong. Socket activation is also generally the
>> most efficient way of starting a service.
>
> So you are not taking xenstore domain into account at all.
>
> There is no socket for xenstore domain so wanting to rely on socket
> activation is the wrong way.
>
>> - Uses a poll loop to determine whether the daemon is ready or not
>> rather than explicit notification from the daemon itself.
>
> How to do this in the domain case?
>
>> - Uses a shell script wrapper...
>
> That's on purpose: this enables me to switch between both setups
> (daemon or domain) by modifying only _one_ configuration file.

FWIW I'm pretty sure using a shell script wrapper also clobbers the
SELinux context.  (I forget whether this means xenstore doesn't run or
xenstore runs completely unrestricted.)

(Ross, does XenServer use SELinux?  Any ideas here?)

>> I would suggest asking on the systemd-devel mailing list or the #systemd
>> IRC channel on freenode. They should be able to suggest the best way of
>> solving this.
>
> I'm sure I could tweak the whole setup more in direction of systemd.
> I'm not sure the final picture would be the preferred one.

But as Ross said, there are advantages to using the systemd-specific
functionality; and it's likely that the majority of our users will be
running xenstored in dom0.  Doesn't it make sense to at least see if
there are sensible options for how we can take advantage of those
before we just drop it?

 -George

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

Re: [PATCH 1/2] tools: remove systemd xenstore socket definitions

[Juergen Gross]

On 20/07/16 12:52, George Dunlap wrote:
> On Wed, Jul 20, 2016 at 10:58 AM, Juergen Gross <jgross@suse.com> wrote:
>> On 20/07/16 11:02, Ross Lagerwall wrote:
>>> On 06/29/2016 02:44 PM, Juergen Gross wrote:
>>>> On 29/06/16 15:31, Ross Lagerwall wrote:
>>>>> On 06/29/2016 02:00 PM, Juergen Gross wrote:
>>>>>> On 29/06/16 14:52, Andrew Cooper wrote:
>>>>>>> On 29/06/16 13:44, Juergen Gross wrote:
>>>>>>>> @@ -2068,13 +1964,6 @@ int main(int argc, char *argv[])
>>>>>>>>      /* Tell the kernel we're up and running. */
>>>>>>>>      xenbus_notify_running();
>>>>>>>>
>>>>>>>> -#if defined(XEN_SYSTEMD_ENABLED)
>>>>>>>> -    if (systemd) {
>>>>>>>> -        sd_notify(1, "READY=1");
>>>>>>>> -        fprintf(stderr, SD_NOTICE "xenstored is ready\n");
>>>>>>>> -    }
>>>>>>>> -#endif
>>>>>>>
>>>>>>> Getting rid of the socket configuration for systemd is ok, but we
>>>>>>> should
>>>>>>> keep the sd_notify() calls for when the daemon is started by systemd.
>>>>>>>
>>>>>>> Socket activiation and sd_notify() are orthogonal, and sd_notify() is
>>>>>>> still required if we don't want systemd to treat xenstored as a legacy
>>>>>>> unix daemon.
>>>>>>
>>>>>> So what is the downside of xenstored being treated as a legacy daemon?
>>>>>> This question is especially interesting for the case of patch 2 being
>>>>>> considered: xenstored is no longer started by systemd, but by a wrapper
>>>>>> script which might decide to start the xenstore domain instead.
>>>>>>
>>>>>> Another problem: today xenstored decides whether to call sd_notify()
>>>>>> by testing the xenstore sockets being specified via systemd. This will
>>>>>> no longer work. So how to do it now?
>>>>>>
>>>>>>
>>>>>
>>>>> One problem with the patch as it currently is implemented is that the
>>>>> service type is not correct for when xenstored is a daemon. This makes
>>>>> it difficult to manage with systemd and difficult for other services to
>>>>> depend on it in a sensible fashion. The end result is subtle races and
>>>>> occasional failures.
>>>>
>>>> Could you please educate me what's wrong? I'm no systemd expert.
>>>
>>> Sorry for the late response.
>>>
>>> With Type=oneshot, systemd considers the service to be "started" as soon
>>> as the ExecStart command completes. After re-reading the patches, I
>>> realize that timeout_xenstore() should ensure that xenstored is actually
>>> ready before systemd starts dependent services. This should prevent the
>>> races I was mentioned previously.
>>>
>>> Nevertheless, I feel that this patch series makes the implementation
>>> worse for users of xenstored as a daemon:
>>>
>>> - Because Type=oneshot is not intended to be used for daemon processes,
>>> systemctl status does not show the service as "running", instead it
>>> shows "exited". This is surprising, at the very least. I haven't tested,
>>> but I believe it would also prevent us someday using the systemd service
>>> management features like Restart=on-failure
>>>
>>> - Removes socket activation. Services would have to explicitly depend on
>>> xenstored.service rather than having an implicit dependency on simply by
>>> using xenstored.socket. This means configuring explicit ordering, etc.,
>>> which is easier to get wrong. Socket activation is also generally the
>>> most efficient way of starting a service.
>>
>> So you are not taking xenstore domain into account at all.
>>
>> There is no socket for xenstore domain so wanting to rely on socket
>> activation is the wrong way.
>>
>>> - Uses a poll loop to determine whether the daemon is ready or not
>>> rather than explicit notification from the daemon itself.
>>
>> How to do this in the domain case?
>>
>>> - Uses a shell script wrapper...
>>
>> That's on purpose: this enables me to switch between both setups
>> (daemon or domain) by modifying only _one_ configuration file.
> 
> FWIW I'm pretty sure using a shell script wrapper also clobbers the
> SELinux context.  (I forget whether this means xenstore doesn't run or
> xenstore runs completely unrestricted.)

Okay, this would need to be corrected. I assume it is possible to
address such an issue in a script?

> (Ross, does XenServer use SELinux?  Any ideas here?)
> 
>>> I would suggest asking on the systemd-devel mailing list or the #systemd
>>> IRC channel on freenode. They should be able to suggest the best way of
>>> solving this.
>>
>> I'm sure I could tweak the whole setup more in direction of systemd.
>> I'm not sure the final picture would be the preferred one.
> 
> But as Ross said, there are advantages to using the systemd-specific
> functionality; and it's likely that the majority of our users will be
> running xenstored in dom0.  Doesn't it make sense to at least see if
> there are sensible options for how we can take advantage of those
> before we just drop it?

I still don't see the advantages, as relying on socket activation of
the xenstore service is no option: as soon as you do this you drop the
possibility to ever use the xenstore domain on such a system.

To be clear: I don't want to avoid systemd by any means. I just don't
want to have a complex and ugly solution with no gain just because
doing it the systemd way.


Juergen

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

Re: [PATCH 1/2] tools: remove systemd xenstore socket definitions

[Andrew Cooper]

On 20/07/16 12:12, Juergen Gross wrote:
> On 20/07/16 12:52, George Dunlap wrote:
>> On Wed, Jul 20, 2016 at 10:58 AM, Juergen Gross <jgross@suse.com> wrote:
>>> On 20/07/16 11:02, Ross Lagerwall wrote:
>>>> On 06/29/2016 02:44 PM, Juergen Gross wrote:
>>>>> On 29/06/16 15:31, Ross Lagerwall wrote:
>>>>>> On 06/29/2016 02:00 PM, Juergen Gross wrote:
>>>>>>> On 29/06/16 14:52, Andrew Cooper wrote:
>>>>>>>> On 29/06/16 13:44, Juergen Gross wrote:
>>>>>>>>> @@ -2068,13 +1964,6 @@ int main(int argc, char *argv[])
>>>>>>>>>      /* Tell the kernel we're up and running. */
>>>>>>>>>      xenbus_notify_running();
>>>>>>>>>
>>>>>>>>> -#if defined(XEN_SYSTEMD_ENABLED)
>>>>>>>>> -    if (systemd) {
>>>>>>>>> -        sd_notify(1, "READY=1");
>>>>>>>>> -        fprintf(stderr, SD_NOTICE "xenstored is ready\n");
>>>>>>>>> -    }
>>>>>>>>> -#endif
>>>>>>>> Getting rid of the socket configuration for systemd is ok, but we
>>>>>>>> should
>>>>>>>> keep the sd_notify() calls for when the daemon is started by systemd.
>>>>>>>>
>>>>>>>> Socket activiation and sd_notify() are orthogonal, and sd_notify() is
>>>>>>>> still required if we don't want systemd to treat xenstored as a legacy
>>>>>>>> unix daemon.
>>>>>>> So what is the downside of xenstored being treated as a legacy daemon?
>>>>>>> This question is especially interesting for the case of patch 2 being
>>>>>>> considered: xenstored is no longer started by systemd, but by a wrapper
>>>>>>> script which might decide to start the xenstore domain instead.
>>>>>>>
>>>>>>> Another problem: today xenstored decides whether to call sd_notify()
>>>>>>> by testing the xenstore sockets being specified via systemd. This will
>>>>>>> no longer work. So how to do it now?
>>>>>>>
>>>>>>>
>>>>>> One problem with the patch as it currently is implemented is that the
>>>>>> service type is not correct for when xenstored is a daemon. This makes
>>>>>> it difficult to manage with systemd and difficult for other services to
>>>>>> depend on it in a sensible fashion. The end result is subtle races and
>>>>>> occasional failures.
>>>>> Could you please educate me what's wrong? I'm no systemd expert.
>>>> Sorry for the late response.
>>>>
>>>> With Type=oneshot, systemd considers the service to be "started" as soon
>>>> as the ExecStart command completes. After re-reading the patches, I
>>>> realize that timeout_xenstore() should ensure that xenstored is actually
>>>> ready before systemd starts dependent services. This should prevent the
>>>> races I was mentioned previously.
>>>>
>>>> Nevertheless, I feel that this patch series makes the implementation
>>>> worse for users of xenstored as a daemon:
>>>>
>>>> - Because Type=oneshot is not intended to be used for daemon processes,
>>>> systemctl status does not show the service as "running", instead it
>>>> shows "exited". This is surprising, at the very least. I haven't tested,
>>>> but I believe it would also prevent us someday using the systemd service
>>>> management features like Restart=on-failure
>>>>
>>>> - Removes socket activation. Services would have to explicitly depend on
>>>> xenstored.service rather than having an implicit dependency on simply by
>>>> using xenstored.socket. This means configuring explicit ordering, etc.,
>>>> which is easier to get wrong. Socket activation is also generally the
>>>> most efficient way of starting a service.
>>> So you are not taking xenstore domain into account at all.
>>>
>>> There is no socket for xenstore domain so wanting to rely on socket
>>> activation is the wrong way.
>>>
>>>> - Uses a poll loop to determine whether the daemon is ready or not
>>>> rather than explicit notification from the daemon itself.
>>> How to do this in the domain case?
>>>
>>>> - Uses a shell script wrapper...
>>> That's on purpose: this enables me to switch between both setups
>>> (daemon or domain) by modifying only _one_ configuration file.
>> FWIW I'm pretty sure using a shell script wrapper also clobbers the
>> SELinux context.  (I forget whether this means xenstore doesn't run or
>> xenstore runs completely unrestricted.)
> Okay, this would need to be corrected. I assume it is possible to
> address such an issue in a script?
>
>> (Ross, does XenServer use SELinux?  Any ideas here?)

XenServer does not use any SELinux.


>>
>>>> I would suggest asking on the systemd-devel mailing list or the #systemd
>>>> IRC channel on freenode. They should be able to suggest the best way of
>>>> solving this.
>>> I'm sure I could tweak the whole setup more in direction of systemd.
>>> I'm not sure the final picture would be the preferred one.
>> But as Ross said, there are advantages to using the systemd-specific
>> functionality; and it's likely that the majority of our users will be
>> running xenstored in dom0.  Doesn't it make sense to at least see if
>> there are sensible options for how we can take advantage of those
>> before we just drop it?
> I still don't see the advantages, as relying on socket activation of
> the xenstore service is no option: as soon as you do this you drop the
> possibility to ever use the xenstore domain on such a system.
>
> To be clear: I don't want to avoid systemd by any means. I just don't
> want to have a complex and ugly solution with no gain just because
> doing it the systemd way.

Given the introduction of this new choice, I agree that socket
activation isn't sensible.  In the grand scheme of things it doesn't buy
you much, as xenstored does not match the intended use for socket
activation (on-demand launch of services when something tries to use its
socket), as it is a start of day service that runs forever.

However, socket activation and sd_notify() are entirely orthogonal, and
the removal of socket activation should not remove sd_notify().

~Andrew

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

Re: [PATCH 1/2] tools: remove systemd xenstore socket definitions

[Juergen Gross]

On 20/07/16 13:21, Andrew Cooper wrote:
> On 20/07/16 12:12, Juergen Gross wrote:
>> On 20/07/16 12:52, George Dunlap wrote:
>>> On Wed, Jul 20, 2016 at 10:58 AM, Juergen Gross <jgross@suse.com> wrote:
>>>> On 20/07/16 11:02, Ross Lagerwall wrote:
>>>>> On 06/29/2016 02:44 PM, Juergen Gross wrote:
>>>>>> On 29/06/16 15:31, Ross Lagerwall wrote:
>>>>>>> On 06/29/2016 02:00 PM, Juergen Gross wrote:
>>>>>>>> On 29/06/16 14:52, Andrew Cooper wrote:
>>>>>>>>> On 29/06/16 13:44, Juergen Gross wrote:
>>>>>>>>>> @@ -2068,13 +1964,6 @@ int main(int argc, char *argv[])
>>>>>>>>>>      /* Tell the kernel we're up and running. */
>>>>>>>>>>      xenbus_notify_running();
>>>>>>>>>>
>>>>>>>>>> -#if defined(XEN_SYSTEMD_ENABLED)
>>>>>>>>>> -    if (systemd) {
>>>>>>>>>> -        sd_notify(1, "READY=1");
>>>>>>>>>> -        fprintf(stderr, SD_NOTICE "xenstored is ready\n");
>>>>>>>>>> -    }
>>>>>>>>>> -#endif
>>>>>>>>> Getting rid of the socket configuration for systemd is ok, but we
>>>>>>>>> should
>>>>>>>>> keep the sd_notify() calls for when the daemon is started by systemd.
>>>>>>>>>
>>>>>>>>> Socket activiation and sd_notify() are orthogonal, and sd_notify() is
>>>>>>>>> still required if we don't want systemd to treat xenstored as a legacy
>>>>>>>>> unix daemon.
>>>>>>>> So what is the downside of xenstored being treated as a legacy daemon?
>>>>>>>> This question is especially interesting for the case of patch 2 being
>>>>>>>> considered: xenstored is no longer started by systemd, but by a wrapper
>>>>>>>> script which might decide to start the xenstore domain instead.
>>>>>>>>
>>>>>>>> Another problem: today xenstored decides whether to call sd_notify()
>>>>>>>> by testing the xenstore sockets being specified via systemd. This will
>>>>>>>> no longer work. So how to do it now?
>>>>>>>>
>>>>>>>>
>>>>>>> One problem with the patch as it currently is implemented is that the
>>>>>>> service type is not correct for when xenstored is a daemon. This makes
>>>>>>> it difficult to manage with systemd and difficult for other services to
>>>>>>> depend on it in a sensible fashion. The end result is subtle races and
>>>>>>> occasional failures.
>>>>>> Could you please educate me what's wrong? I'm no systemd expert.
>>>>> Sorry for the late response.
>>>>>
>>>>> With Type=oneshot, systemd considers the service to be "started" as soon
>>>>> as the ExecStart command completes. After re-reading the patches, I
>>>>> realize that timeout_xenstore() should ensure that xenstored is actually
>>>>> ready before systemd starts dependent services. This should prevent the
>>>>> races I was mentioned previously.
>>>>>
>>>>> Nevertheless, I feel that this patch series makes the implementation
>>>>> worse for users of xenstored as a daemon:
>>>>>
>>>>> - Because Type=oneshot is not intended to be used for daemon processes,
>>>>> systemctl status does not show the service as "running", instead it
>>>>> shows "exited". This is surprising, at the very least. I haven't tested,
>>>>> but I believe it would also prevent us someday using the systemd service
>>>>> management features like Restart=on-failure
>>>>>
>>>>> - Removes socket activation. Services would have to explicitly depend on
>>>>> xenstored.service rather than having an implicit dependency on simply by
>>>>> using xenstored.socket. This means configuring explicit ordering, etc.,
>>>>> which is easier to get wrong. Socket activation is also generally the
>>>>> most efficient way of starting a service.
>>>> So you are not taking xenstore domain into account at all.
>>>>
>>>> There is no socket for xenstore domain so wanting to rely on socket
>>>> activation is the wrong way.
>>>>
>>>>> - Uses a poll loop to determine whether the daemon is ready or not
>>>>> rather than explicit notification from the daemon itself.
>>>> How to do this in the domain case?
>>>>
>>>>> - Uses a shell script wrapper...
>>>> That's on purpose: this enables me to switch between both setups
>>>> (daemon or domain) by modifying only _one_ configuration file.
>>> FWIW I'm pretty sure using a shell script wrapper also clobbers the
>>> SELinux context.  (I forget whether this means xenstore doesn't run or
>>> xenstore runs completely unrestricted.)
>> Okay, this would need to be corrected. I assume it is possible to
>> address such an issue in a script?
>>
>>> (Ross, does XenServer use SELinux?  Any ideas here?)
> 
> XenServer does not use any SELinux.
> 
> 
>>>
>>>>> I would suggest asking on the systemd-devel mailing list or the #systemd
>>>>> IRC channel on freenode. They should be able to suggest the best way of
>>>>> solving this.
>>>> I'm sure I could tweak the whole setup more in direction of systemd.
>>>> I'm not sure the final picture would be the preferred one.
>>> But as Ross said, there are advantages to using the systemd-specific
>>> functionality; and it's likely that the majority of our users will be
>>> running xenstored in dom0.  Doesn't it make sense to at least see if
>>> there are sensible options for how we can take advantage of those
>>> before we just drop it?
>> I still don't see the advantages, as relying on socket activation of
>> the xenstore service is no option: as soon as you do this you drop the
>> possibility to ever use the xenstore domain on such a system.
>>
>> To be clear: I don't want to avoid systemd by any means. I just don't
>> want to have a complex and ugly solution with no gain just because
>> doing it the systemd way.
> 
> Given the introduction of this new choice, I agree that socket
> activation isn't sensible.  In the grand scheme of things it doesn't buy
> you much, as xenstored does not match the intended use for socket
> activation (on-demand launch of services when something tries to use its
> socket), as it is a start of day service that runs forever.

Okay, thanks for supporting my POV.

> However, socket activation and sd_notify() are entirely orthogonal, and
> the removal of socket activation should not remove sd_notify().

Hmm, given that you'd probably want something like this for the domain
case, too, wouldn't it make sense to just use systemd-notify in the
wrapper script launching either xenstored or the xenstore domain?


Juergen

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

Re: [PATCH 1/2] tools: remove systemd xenstore socket definitions

[Ian Jackson]

Andrew Cooper writes ("Re: [Xen-devel] [PATCH 1/2] tools: remove systemd xenstore socket definitions"):
> On 20/07/16 12:12, Juergen Gross wrote:
> > To be clear: I don't want to avoid systemd by any means. I just don't
> > want to have a complex and ugly solution with no gain just because
> > doing it the systemd way.
> 
> Given the introduction of this new choice, I agree that socket
> activation isn't sensible.  In the grand scheme of things it doesn't buy
> you much, as xenstored does not match the intended use for socket
> activation (on-demand launch of services when something tries to use its
> socket), as it is a start of day service that runs forever.

xenstore in its own domain is not a `new choice' which is being
`introduced'.  It has been supported by Xen upstream for a long time.
AFAICT from what Juergen is saying it seems that it was broken on
systemd systems by systemd-specific configuration.

> However, socket activation and sd_notify() are entirely orthogonal, and
> the removal of socket activation should not remove sd_notify().

I don't have a clear opinion opinion about this but it seems likely to
me that retaining some kind of systemd `ready now' call is desirable
or even necessary.

Ian.

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

Re: [PATCH 1/2] tools: remove systemd xenstore socket definitions

[Ian Jackson]

Ross Lagerwall writes ("Re: [Xen-devel] [PATCH 1/2] tools: remove systemd xenstore socket definitions"):
> Nevertheless, I feel that this patch series makes the implementation 
> worse for users of xenstored as a daemon:
> 
> - Because Type=oneshot is not intended to be used for daemon processes, 
> systemctl status does not show the service as "running", instead it 
> shows "exited". This is surprising, at the very least. I haven't tested, 
> but I believe it would also prevent us someday using the systemd service 
> management features like Restart=on-failure

This sounds undesirable.  I would like to see this rectified.

> - Removes socket activation. Services would have to explicitly depend on 
> xenstored.service rather than having an implicit dependency on simply by 
> using xenstored.socket. This means configuring explicit ordering, etc., 
> which is easier to get wrong. Socket activation is also generally the 
> most efficient way of starting a service.

Socket activation is a means to and end, not an objective in itself.

> - Uses a poll loop to determine whether the daemon is ready or not 
> rather than explicit notification from the daemon itself.

I agree that polling is rather poor.

> - Uses a shell script wrapper...

I'm afraid that ISTM likeloy that however we do this a shell script
wrapper is going to be needed.

One reason is that there should be _one_ place in the source tree
which reads the relevant config snippets etc. about how the whole Xen
system should be started.

Ian.

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

Re: [PATCH 1/2] tools: remove systemd xenstore socket definitions

[Juergen Gross]

On 20/07/16 14:08, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [Xen-devel] [PATCH 1/2] tools: remove systemd xenstore socket definitions"):
>> On 20/07/16 12:12, Juergen Gross wrote:
>>> To be clear: I don't want to avoid systemd by any means. I just don't
>>> want to have a complex and ugly solution with no gain just because
>>> doing it the systemd way.
>>
>> Given the introduction of this new choice, I agree that socket
>> activation isn't sensible.  In the grand scheme of things it doesn't buy
>> you much, as xenstored does not match the intended use for socket
>> activation (on-demand launch of services when something tries to use its
>> socket), as it is a start of day service that runs forever.
> 
> xenstore in its own domain is not a `new choice' which is being
> `introduced'.  It has been supported by Xen upstream for a long time.
> AFAICT from what Juergen is saying it seems that it was broken on
> systemd systems by systemd-specific configuration.
> 
>> However, socket activation and sd_notify() are entirely orthogonal, and
>> the removal of socket activation should not remove sd_notify().
> 
> I don't have a clear opinion opinion about this but it seems likely to
> me that retaining some kind of systemd `ready now' call is desirable
> or even necessary.

To be precise: the call might be desirable, but it is not necessary.
With a systemd service type "onehot" systemd will start follow-up units
only after the process started by systemd has exited. So the 'ready now'
indication would probably speed up the boot process by a few msecs.


Juergen


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

Re: [PATCH 1/2] tools: remove systemd xenstore socket definitions

[Andrew Cooper]

On 20/07/16 13:08, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [Xen-devel] [PATCH 1/2] tools: remove systemd xenstore socket definitions"):
>> On 20/07/16 12:12, Juergen Gross wrote:
>>> To be clear: I don't want to avoid systemd by any means. I just don't
>>> want to have a complex and ugly solution with no gain just because
>>> doing it the systemd way.
>> Given the introduction of this new choice, I agree that socket
>> activation isn't sensible.  In the grand scheme of things it doesn't buy
>> you much, as xenstored does not match the intended use for socket
>> activation (on-demand launch of services when something tries to use its
>> socket), as it is a start of day service that runs forever.
> xenstore in its own domain is not a `new choice' which is being
> `introduced'.  It has been supported by Xen upstream for a long time.
> AFAICT from what Juergen is saying it seems that it was broken on
> systemd systems by systemd-specific configuration.

I know that the concept of a xenstored stubdomain isn't new, but
sensible configuration and integration into the $INITSYSTEM_OF_CHOICE
definitely is new.

For the record, I fully support the general direction being taken.
Juergen is doing a stellar job improving the status quo, and this will
be great for the Xen community moving forwards.


However, calling what previously exists WRT xenstored stubdomains as
"supported" is laughable.  The lack of integration meant that anyone
trying to use it had to make intrusive modifications to make it start
properly, and the lack of MiniOS ballooning shows that inadequate
consideration was given to production usecases.  It is disingenuous
pretend that stub-xenstored is anywhere beyond "demo" in terms of
real-world usage at the moment.

~Andrew

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

Re: [PATCH 1/2] tools: remove systemd xenstore socket definitions

[Juergen Gross]

On 20/07/16 14:11, Ian Jackson wrote:
> Ross Lagerwall writes ("Re: [Xen-devel] [PATCH 1/2] tools: remove systemd xenstore socket definitions"):
>> Nevertheless, I feel that this patch series makes the implementation 
>> worse for users of xenstored as a daemon:
>>
>> - Because Type=oneshot is not intended to be used for daemon processes, 
>> systemctl status does not show the service as "running", instead it 
>> shows "exited". This is surprising, at the very least. I haven't tested, 

With "RemainAfterExit" specified it is still "active (running)" on my
system.

>> but I believe it would also prevent us someday using the systemd service 
>> management features like Restart=on-failure
> 
> This sounds undesirable.  I would like to see this rectified.

What are you speaking about: a failure during service start or in the
running system?

And how would you want to handle the failure of a xenstore domain?

I think introducing restart capability to xenstored (d being daemon or
domain) will need some more work than just rely on systemd. It should
not be completely daemon specific!

>> - Removes socket activation. Services would have to explicitly depend on 
>> xenstored.service rather than having an implicit dependency on simply by 
>> using xenstored.socket. This means configuring explicit ordering, etc., 
>> which is easier to get wrong. Socket activation is also generally the 
>> most efficient way of starting a service.
> 
> Socket activation is a means to and end, not an objective in itself.
> 
>> - Uses a poll loop to determine whether the daemon is ready or not 
>> rather than explicit notification from the daemon itself.
> 
> I agree that polling is rather poor.

I can remove the polling by adding appropriate code to xenstore daemon:
the non-daemon process should exit only after the daemon is ready to
work.

For the xenstore domain init-xenstore-domain will exit only after the
xenstore is up as the program itself is writing some xenstore entries.

>> - Uses a shell script wrapper...
> 
> I'm afraid that ISTM likeloy that however we do this a shell script
> wrapper is going to be needed.
> 
> One reason is that there should be _one_ place in the source tree
> which reads the relevant config snippets etc. about how the whole Xen
> system should be started.

+1


Juergen

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

Re: [PATCH 1/2] tools: remove systemd xenstore socket definitions

[Juergen Gross]

On 20/07/16 14:32, Andrew Cooper wrote:
> On 20/07/16 13:08, Ian Jackson wrote:
>> Andrew Cooper writes ("Re: [Xen-devel] [PATCH 1/2] tools: remove systemd xenstore socket definitions"):
>>> On 20/07/16 12:12, Juergen Gross wrote:
>>>> To be clear: I don't want to avoid systemd by any means. I just don't
>>>> want to have a complex and ugly solution with no gain just because
>>>> doing it the systemd way.
>>> Given the introduction of this new choice, I agree that socket
>>> activation isn't sensible.  In the grand scheme of things it doesn't buy
>>> you much, as xenstored does not match the intended use for socket
>>> activation (on-demand launch of services when something tries to use its
>>> socket), as it is a start of day service that runs forever.
>> xenstore in its own domain is not a `new choice' which is being
>> `introduced'.  It has been supported by Xen upstream for a long time.
>> AFAICT from what Juergen is saying it seems that it was broken on
>> systemd systems by systemd-specific configuration.
> 
> I know that the concept of a xenstored stubdomain isn't new, but
> sensible configuration and integration into the $INITSYSTEM_OF_CHOICE
> definitely is new.

Sure. And I appreciate all the discussions taking place.

> For the record, I fully support the general direction being taken.
> Juergen is doing a stellar job improving the status quo, and this will
> be great for the Xen community moving forwards.

Thanks.

> However, calling what previously exists WRT xenstored stubdomains as
> "supported" is laughable.  The lack of integration meant that anyone
> trying to use it had to make intrusive modifications to make it start
> properly, and the lack of MiniOS ballooning shows that inadequate
> consideration was given to production usecases.  It is disingenuous
> pretend that stub-xenstored is anywhere beyond "demo" in terms of
> real-world usage at the moment.

Just trying to change this. :-D


Juergen

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

Re: [PATCH 1/2] tools: remove systemd xenstore socket definitions

[Ian Jackson]

Andrew Cooper writes ("Re: [Xen-devel] [PATCH 1/2] tools: remove systemd xenstore socket definitions"):
> However, calling what previously exists WRT xenstored stubdomains as
> "supported" is laughable.  The lack of integration meant that anyone
> trying to use it had to make intrusive modifications to make it start
> properly, and the lack of MiniOS ballooning shows that inadequate
> consideration was given to production usecases.  It is disingenuous
> pretend that stub-xenstored is anywhere beyond "demo" in terms of
> real-world usage at the moment.

Well, I may well be wrong about how well it was supported before.  I
don't remember these problems but it's been a long time since I have
actually seen reports of stub xenstored in use.

I'm glad to see that you support improvements.

But I think you need to be more careful with your use of language.
`Disingenuous' implies dishonesty.

thanks,
Ian.

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

Re: [PATCH 1/2] tools: remove systemd xenstore socket definitions

[George Dunlap]

On 20/07/16 14:23, Ian Jackson wrote:
> But I think you need to be more careful with your use of language.
> `Disingenuous' implies dishonesty.

The way I've heard it used implies a level of pretense, not necessarily
dishonesty -- i.e., pretending that stubdom xenstored was a usable
option when it wasn't actually usable in practice.

I don't think I would disagree with Andy's choice of words here or his
main point (although one should certainly be aware that there may be
other usages that may give offence).

 -George

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

Re: [PATCH 1/2] tools: remove systemd xenstore socket definitions

[Andrew Cooper]

On 20/07/16 14:29, George Dunlap wrote:
> On 20/07/16 14:23, Ian Jackson wrote:
>> But I think you need to be more careful with your use of language.
>> `Disingenuous' implies dishonesty.
> The way I've heard it used implies a level of pretense, not necessarily
> dishonesty -- i.e., pretending that stubdom xenstored was a usable
> option when it wasn't actually usable in practice.

This was my intended meaning.  My apologies if it came across anyhow else.

~Andrew

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