* [PATCH v3 0/4] tools: make xenstore domain/daemon configurable @ 2016-07-22 15:09 Juergen Gross 2016-07-22 15:09 ` [PATCH v3 1/4] tools: remove systemd xenstore socket definitions Juergen Gross ` (5 more replies) 0 siblings, 6 replies; 23+ messages in thread From: Juergen Gross @ 2016-07-22 15:09 UTC (permalink / raw) To: xen-devel Cc: Juergen Gross, wei.liu2, andrew.cooper3, ian.jackson, ross.lagerwall, dave 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. Changes in V3: - patch 1: re-add sd_notify() call - split up former patch 2 into 3 patches as requested by Ian Jackson - patch 4 (was 2): remove check for running xenstore domain, as this is done in init-xenstore-domain already - patch 4 (was 2): if booted with systemd send a systemd-notify message in the xenstore domain case - patch 4 (was 2): if booted with systemd don't wait until xenstore daemon is running, as the daemon will have sent a notify message by its own Changes in V2: - move service type modification form patch 2 to patch 1 as implied by Ross Lagerwall (at least I guess so) - add .gitignore entry for launch-xenstore Juergen Gross (4): tools: remove systemd xenstore socket definitions tools: split out xenstored starting form xencommons tools: use pidfile for test if xenstored is running tools: make xenstore domain easy configurable .gitignore | 1 + tools/configure | 7 +- tools/configure.ac | 3 +- tools/hotplug/Linux/Makefile | 1 + tools/hotplug/Linux/init.d/sysconfig.xencommons.in | 42 +++++++- tools/hotplug/Linux/init.d/xencommons.in | 38 +------ tools/hotplug/Linux/launch-xenstore.in | 87 ++++++++++++++++ tools/hotplug/Linux/systemd/Makefile | 5 - tools/hotplug/Linux/systemd/xenstored.service.in | 13 +-- tools/hotplug/Linux/systemd/xenstored.socket.in | 13 --- tools/hotplug/Linux/systemd/xenstored_ro.socket.in | 13 --- tools/ocaml/xenstored/systemd.ml | 1 - tools/ocaml/xenstored/systemd.mli | 5 - tools/ocaml/xenstored/systemd_stubs.c | 113 ++++----------------- tools/ocaml/xenstored/utils.ml | 21 ++-- tools/xenstore/xenstored_core.c | 92 +---------------- 16 files changed, 169 insertions(+), 286 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 -- 2.6.6 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 1/4] tools: remove systemd xenstore socket definitions 2016-07-22 15:09 [PATCH v3 0/4] tools: make xenstore domain/daemon configurable Juergen Gross @ 2016-07-22 15:09 ` Juergen Gross 2016-07-22 16:31 ` Wei Liu 2016-07-22 15:09 ` [PATCH v3 2/4] tools: split out xenstored starting form xencommons Juergen Gross ` (4 subsequent siblings) 5 siblings, 1 reply; 23+ messages in thread From: Juergen Gross @ 2016-07-22 15:09 UTC (permalink / raw) To: xen-devel Cc: Juergen Gross, wei.liu2, andrew.cooper3, ian.jackson, ross.lagerwall, dave 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. As the xenstore domain start program will exit after it has done its job prepare the same behaviour to be tolerated by systemd for the xenstore daemon by specifying the appropriate flags in the service file. A rerun of autogen.sh is required. Signed-off-by: Juergen Gross <jgross@suse.com> --- V3: re-add sd_notify() call --- tools/configure | 4 +- tools/configure.ac | 2 - tools/hotplug/Linux/systemd/Makefile | 5 - tools/hotplug/Linux/systemd/xenstored.service.in | 5 +- tools/hotplug/Linux/systemd/xenstored.socket.in | 13 --- tools/hotplug/Linux/systemd/xenstored_ro.socket.in | 13 --- tools/ocaml/xenstored/systemd.ml | 1 - tools/ocaml/xenstored/systemd.mli | 5 - tools/ocaml/xenstored/systemd_stubs.c | 113 ++++----------------- tools/ocaml/xenstored/utils.ml | 21 ++-- tools/xenstore/xenstored_core.c | 92 +---------------- 11 files changed, 33 insertions(+), 241 deletions(-) delete mode 100644 tools/hotplug/Linux/systemd/xenstored.socket.in delete mode 100644 tools/hotplug/Linux/systemd/xenstored_ro.socket.in diff --git a/tools/configure b/tools/configure index 5b5dcce..1c84c6c 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/xendriverdomain.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/xendriverdomain.service hotplug/Linux/systemd/xenstored.service" fi @@ -10394,8 +10394,6 @@ do "hotplug/Linux/systemd/xendomains.service") CONFIG_FILES="$CONFIG_FILES hotplug/Linux/systemd/xendomains.service" ;; "hotplug/Linux/systemd/xendriverdomain.service") CONFIG_FILES="$CONFIG_FILES hotplug/Linux/systemd/xendriverdomain.service" ;; "hotplug/Linux/systemd/xenstored.service") CONFIG_FILES="$CONFIG_FILES hotplug/Linux/systemd/xenstored.service" ;; - "hotplug/Linux/systemd/xenstored.socket") CONFIG_FILES="$CONFIG_FILES hotplug/Linux/systemd/xenstored.socket" ;; - "hotplug/Linux/systemd/xenstored_ro.socket") CONFIG_FILES="$CONFIG_FILES hotplug/Linux/systemd/xenstored_ro.socket" ;; *) as_fn_error $? "invalid argument: \`$ac_config_target'" "$LINENO" 5;; esac diff --git a/tools/configure.ac b/tools/configure.ac index 87e14a8..f991ab3 100644 --- a/tools/configure.ac +++ b/tools/configure.ac @@ -438,8 +438,6 @@ AS_IF([test "x$systemd" = "xy"], [ hotplug/Linux/systemd/xendomains.service hotplug/Linux/systemd/xendriverdomain.service hotplug/Linux/systemd/xenstored.service - hotplug/Linux/systemd/xenstored.socket - hotplug/Linux/systemd/xenstored_ro.socket ]) ]) diff --git a/tools/hotplug/Linux/systemd/Makefile b/tools/hotplug/Linux/systemd/Makefile index 558e459..7d24bbe 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 @@ -19,7 +16,6 @@ XEN_SYSTEMD_SERVICE += xendriverdomain.service ALL_XEN_SYSTEMD = $(XEN_SYSTEMD_MODULES) \ $(XEN_SYSTEMD_MOUNT) \ - $(XEN_SYSTEMD_SOCKET) \ $(XEN_SYSTEMD_SERVICE) .PHONY: all @@ -38,7 +34,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..d520d70 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 @@ -8,6 +8,8 @@ ConditionPathExists=/proc/xen/capabilities [Service] Type=notify +NotifyAccess=all +RemainAfterExit=true KillMode=none Environment=XENSTORED_ARGS= Environment=XENSTORED=@XENSTORED@ @@ -19,6 +21,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/systemd.ml b/tools/ocaml/xenstored/systemd.ml index 732446d..b3090e5 100644 --- a/tools/ocaml/xenstored/systemd.ml +++ b/tools/ocaml/xenstored/systemd.ml @@ -12,6 +12,5 @@ * 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 index 538fc5e..9871137 100644 --- a/tools/ocaml/xenstored/systemd.mli +++ b/tools/ocaml/xenstored/systemd.mli @@ -12,11 +12,6 @@ * 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 diff --git a/tools/ocaml/xenstored/systemd_stubs.c b/tools/ocaml/xenstored/systemd_stubs.c index 322f1e0..c9af95e 100644 --- a/tools/ocaml/xenstored/systemd_stubs.c +++ b/tools/ocaml/xenstored/systemd_stubs.c @@ -25,129 +25,54 @@ #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); + CAMLparam1(ignore); + CAMLlocal1(ret); - ret = Val_false; + ret = Val_false; - if (sd_listen_fds(0) > 0) - ret = Val_true; + if (sd_booted() > 0) + ret = Val_true; - CAMLreturn(ret); + CAMLreturn(ret); } CAMLprim value ocaml_sd_notify_ready(value ignore) { - CAMLparam1(ignore); - CAMLlocal1(ret); + CAMLparam1(ignore); + CAMLlocal1(ret); - ret = Val_int(0); + ret = Val_int(0); - sd_notify(1, "READY=1"); + sd_notify(1, "READY=1"); - CAMLreturn(ret); + 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); + CAMLparam1(ignore); + CAMLlocal1(ret); - ret = Val_false; + ret = Val_false; - CAMLreturn(ret); + CAMLreturn(ret); } CAMLprim value ocaml_sd_notify_ready(value ignore) { - CAMLparam1(ignore); - CAMLlocal1(ret); + CAMLparam1(ignore); + CAMLlocal1(ret); - ret = Val_int(-1U); + ret = Val_int(-1U); - CAMLreturn(ret); + 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/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c index 693d47d..5ac2970 100644 --- a/tools/xenstore/xenstored_core.c +++ b/tools/xenstore/xenstored_core.c @@ -1788,84 +1788,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; @@ -1988,6 +1910,7 @@ int main(int argc, char *argv[]) bool systemd; #endif + while ((opt = getopt_long(argc, argv, "DE:F:HNPS:t:T:RLVW:M:", options, NULL)) != -1) { switch (opt) { @@ -2051,13 +1974,7 @@ int main(int argc, char *argv[]) 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; - } + systemd = (sd_booted() > 0); #endif reopen_log(); @@ -2086,10 +2003,7 @@ int main(int argc, char *argv[]) signal(SIGUSR1, do_talloc_report); } -#if defined(XEN_SYSTEMD_ENABLED) - if (!systemd) -#endif - init_sockets(&sock, &ro_sock); + init_sockets(&sock, &ro_sock); init_pipe(reopen_log_pipe); -- 2.6.6 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v3 1/4] tools: remove systemd xenstore socket definitions 2016-07-22 15:09 ` [PATCH v3 1/4] tools: remove systemd xenstore socket definitions Juergen Gross @ 2016-07-22 16:31 ` Wei Liu 2016-07-22 18:49 ` Juergen Gross 0 siblings, 1 reply; 23+ messages in thread From: Wei Liu @ 2016-07-22 16:31 UTC (permalink / raw) To: Juergen Gross Cc: wei.liu2, andrew.cooper3, ian.jackson, xen-devel, ross.lagerwall, dave Only skim-read this patch, will do proper review later. On Fri, Jul 22, 2016 at 05:09:28PM +0200, Juergen Gross wrote: [...] > CAMLprim value ocaml_launched_by_systemd(value ignore) > { > - CAMLparam1(ignore); > - CAMLlocal1(ret); > + CAMLparam1(ignore); > + CAMLlocal1(ret); > > - ret = Val_false; > + ret = Val_false; > > - if (sd_listen_fds(0) > 0) > - ret = Val_true; > + if (sd_booted() > 0) > + ret = Val_true; I think this may be problematic. sd_booted returns true if system is booted with systemd, but it has no bearing whether this particular process is launched by systemd. IIRC using sd_booted would cause oxenstored thinks it is launched by systemd even if the user launches it by hand in a shell. That caused it's initialisation to fail. 81d758afca7c3c1e3ccbd78154b33d64fd7757fb was written to address that issue. So, what would happen if you start oxenstored by hand with your patch apply? Maybe we can just remove this launched_by_systemd check all together -- i.e. we always call sd_notify? > > - CAMLreturn(ret); > + CAMLreturn(ret); > } > > CAMLprim value ocaml_sd_notify_ready(value ignore) > { > - CAMLparam1(ignore); > - CAMLlocal1(ret); > + CAMLparam1(ignore); > + CAMLlocal1(ret); > > - ret = Val_int(0); > + ret = Val_int(0); > > - sd_notify(1, "READY=1"); > + sd_notify(1, "READY=1"); > > - CAMLreturn(ret); > + CAMLreturn(ret); It seems that you have introduced quite a few white space changes. If you really want to change tabs to spaces, please do that in a separate patch. Wei. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 1/4] tools: remove systemd xenstore socket definitions 2016-07-22 16:31 ` Wei Liu @ 2016-07-22 18:49 ` Juergen Gross 2016-07-22 18:51 ` Wei Liu 0 siblings, 1 reply; 23+ messages in thread From: Juergen Gross @ 2016-07-22 18:49 UTC (permalink / raw) To: Wei Liu; +Cc: ross.lagerwall, dave, ian.jackson, andrew.cooper3, xen-devel On 22/07/16 18:31, Wei Liu wrote: > Only skim-read this patch, will do proper review later. > > On Fri, Jul 22, 2016 at 05:09:28PM +0200, Juergen Gross wrote: > [...] >> CAMLprim value ocaml_launched_by_systemd(value ignore) >> { >> - CAMLparam1(ignore); >> - CAMLlocal1(ret); >> + CAMLparam1(ignore); >> + CAMLlocal1(ret); >> >> - ret = Val_false; >> + ret = Val_false; >> >> - if (sd_listen_fds(0) > 0) >> - ret = Val_true; >> + if (sd_booted() > 0) >> + ret = Val_true; > > I think this may be problematic. > > sd_booted returns true if system is booted with systemd, but it has no > bearing whether this particular process is launched by systemd. > > IIRC using sd_booted would cause oxenstored thinks it is launched by > systemd even if the user launches it by hand in a shell. That caused > it's initialisation to fail. 81d758afca7c3c1e3ccbd78154b33d64fd7757fb > was written to address that issue. > > So, what would happen if you start oxenstored by hand with your patch > apply? Maybe we can just remove this launched_by_systemd check all > together -- i.e. we always call sd_notify? So you are concerned sd_notify() will be called too often, but you are suggesting to call it always? I don't understand your concerns then. > >> >> - CAMLreturn(ret); >> + CAMLreturn(ret); >> } >> >> CAMLprim value ocaml_sd_notify_ready(value ignore) >> { >> - CAMLparam1(ignore); >> - CAMLlocal1(ret); >> + CAMLparam1(ignore); >> + CAMLlocal1(ret); >> >> - ret = Val_int(0); >> + ret = Val_int(0); >> >> - sd_notify(1, "READY=1"); >> + sd_notify(1, "READY=1"); >> >> - CAMLreturn(ret); >> + CAMLreturn(ret); > > It seems that you have introduced quite a few white space changes. > If you really want to change tabs to spaces, please do that in a > separate patch. Oops, didn't mean to do this. Sorry. Juergen _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 1/4] tools: remove systemd xenstore socket definitions 2016-07-22 18:49 ` Juergen Gross @ 2016-07-22 18:51 ` Wei Liu 2016-07-25 4:33 ` Juergen Gross 0 siblings, 1 reply; 23+ messages in thread From: Wei Liu @ 2016-07-22 18:51 UTC (permalink / raw) To: Juergen Gross Cc: Wei Liu, andrew.cooper3, ian.jackson, xen-devel, ross.lagerwall, dave On Fri, Jul 22, 2016 at 08:49:17PM +0200, Juergen Gross wrote: > On 22/07/16 18:31, Wei Liu wrote: > > Only skim-read this patch, will do proper review later. > > > > On Fri, Jul 22, 2016 at 05:09:28PM +0200, Juergen Gross wrote: > > [...] > >> CAMLprim value ocaml_launched_by_systemd(value ignore) > >> { > >> - CAMLparam1(ignore); > >> - CAMLlocal1(ret); > >> + CAMLparam1(ignore); > >> + CAMLlocal1(ret); > >> > >> - ret = Val_false; > >> + ret = Val_false; > >> > >> - if (sd_listen_fds(0) > 0) > >> - ret = Val_true; > >> + if (sd_booted() > 0) > >> + ret = Val_true; > > > > I think this may be problematic. > > > > sd_booted returns true if system is booted with systemd, but it has no > > bearing whether this particular process is launched by systemd. > > > > IIRC using sd_booted would cause oxenstored thinks it is launched by > > systemd even if the user launches it by hand in a shell. That caused > > it's initialisation to fail. 81d758afca7c3c1e3ccbd78154b33d64fd7757fb > > was written to address that issue. > > > > So, what would happen if you start oxenstored by hand with your patch > > apply? Maybe we can just remove this launched_by_systemd check all > > together -- i.e. we always call sd_notify? > > So you are concerned sd_notify() will be called too often, but you are > suggesting to call it always? I don't understand your concerns then. > No, my concern is that you won't be able to start oxenstored from command line manually if you boot with systemd. At least that was the bug that caused me to write that patch. Wei. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 1/4] tools: remove systemd xenstore socket definitions 2016-07-22 18:51 ` Wei Liu @ 2016-07-25 4:33 ` Juergen Gross 2016-07-25 11:05 ` Wei Liu 0 siblings, 1 reply; 23+ messages in thread From: Juergen Gross @ 2016-07-25 4:33 UTC (permalink / raw) To: Wei Liu; +Cc: ross.lagerwall, dave, ian.jackson, andrew.cooper3, xen-devel On 22/07/16 20:51, Wei Liu wrote: > On Fri, Jul 22, 2016 at 08:49:17PM +0200, Juergen Gross wrote: >> On 22/07/16 18:31, Wei Liu wrote: >>> Only skim-read this patch, will do proper review later. >>> >>> On Fri, Jul 22, 2016 at 05:09:28PM +0200, Juergen Gross wrote: >>> [...] >>>> CAMLprim value ocaml_launched_by_systemd(value ignore) >>>> { >>>> - CAMLparam1(ignore); >>>> - CAMLlocal1(ret); >>>> + CAMLparam1(ignore); >>>> + CAMLlocal1(ret); >>>> >>>> - ret = Val_false; >>>> + ret = Val_false; >>>> >>>> - if (sd_listen_fds(0) > 0) >>>> - ret = Val_true; >>>> + if (sd_booted() > 0) >>>> + ret = Val_true; >>> >>> I think this may be problematic. >>> >>> sd_booted returns true if system is booted with systemd, but it has no >>> bearing whether this particular process is launched by systemd. >>> >>> IIRC using sd_booted would cause oxenstored thinks it is launched by >>> systemd even if the user launches it by hand in a shell. That caused >>> it's initialisation to fail. 81d758afca7c3c1e3ccbd78154b33d64fd7757fb >>> was written to address that issue. >>> >>> So, what would happen if you start oxenstored by hand with your patch >>> apply? Maybe we can just remove this launched_by_systemd check all >>> together -- i.e. we always call sd_notify? Sure we could. I'll remove the checks in both xenstored variants if nobody objects. >> >> So you are concerned sd_notify() will be called too often, but you are >> suggesting to call it always? I don't understand your concerns then. >> > > No, my concern is that you won't be able to start oxenstored from > command line manually if you boot with systemd. At least that was the > bug that caused me to write that patch. I believe the main problem was xenstored not calling daemonize() in that case, right? This problem is being remove with my patch as daemonize() will be called always. The systemd service file is modified to reflect this change in behavior. Juergen _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 1/4] tools: remove systemd xenstore socket definitions 2016-07-25 4:33 ` Juergen Gross @ 2016-07-25 11:05 ` Wei Liu 2016-07-25 12:21 ` Juergen Gross 0 siblings, 1 reply; 23+ messages in thread From: Wei Liu @ 2016-07-25 11:05 UTC (permalink / raw) To: Juergen Gross Cc: Wei Liu, andrew.cooper3, ian.jackson, xen-devel, ross.lagerwall, dave On Mon, Jul 25, 2016 at 06:33:35AM +0200, Juergen Gross wrote: > On 22/07/16 20:51, Wei Liu wrote: > > On Fri, Jul 22, 2016 at 08:49:17PM +0200, Juergen Gross wrote: > >> On 22/07/16 18:31, Wei Liu wrote: > >>> Only skim-read this patch, will do proper review later. > >>> > >>> On Fri, Jul 22, 2016 at 05:09:28PM +0200, Juergen Gross wrote: > >>> [...] > >>>> CAMLprim value ocaml_launched_by_systemd(value ignore) > >>>> { > >>>> - CAMLparam1(ignore); > >>>> - CAMLlocal1(ret); > >>>> + CAMLparam1(ignore); > >>>> + CAMLlocal1(ret); > >>>> > >>>> - ret = Val_false; > >>>> + ret = Val_false; > >>>> > >>>> - if (sd_listen_fds(0) > 0) > >>>> - ret = Val_true; > >>>> + if (sd_booted() > 0) > >>>> + ret = Val_true; > >>> > >>> I think this may be problematic. > >>> > >>> sd_booted returns true if system is booted with systemd, but it has no > >>> bearing whether this particular process is launched by systemd. > >>> > >>> IIRC using sd_booted would cause oxenstored thinks it is launched by > >>> systemd even if the user launches it by hand in a shell. That caused > >>> it's initialisation to fail. 81d758afca7c3c1e3ccbd78154b33d64fd7757fb > >>> was written to address that issue. > >>> > >>> So, what would happen if you start oxenstored by hand with your patch > >>> apply? Maybe we can just remove this launched_by_systemd check all > >>> together -- i.e. we always call sd_notify? > > Sure we could. I'll remove the checks in both xenstored variants if > nobody objects. > > >> > >> So you are concerned sd_notify() will be called too often, but you are > >> suggesting to call it always? I don't understand your concerns then. > >> > > > > No, my concern is that you won't be able to start oxenstored from > > command line manually if you boot with systemd. At least that was the > > bug that caused me to write that patch. > > I believe the main problem was xenstored not calling daemonize() in that > case, right? This problem is being remove with my patch as daemonize() > will be called always. The systemd service file is modified to reflect > this change in behavior. > I'm afraid I can't remember all the details. But as long as you can launch [o/c]xenstored by hand I think we're fine. Wei. > > Juergen _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 1/4] tools: remove systemd xenstore socket definitions 2016-07-25 11:05 ` Wei Liu @ 2016-07-25 12:21 ` Juergen Gross 0 siblings, 0 replies; 23+ messages in thread From: Juergen Gross @ 2016-07-25 12:21 UTC (permalink / raw) To: Wei Liu; +Cc: ross.lagerwall, dave, ian.jackson, andrew.cooper3, xen-devel On 25/07/16 13:05, Wei Liu wrote: > On Mon, Jul 25, 2016 at 06:33:35AM +0200, Juergen Gross wrote: >> On 22/07/16 20:51, Wei Liu wrote: >>> On Fri, Jul 22, 2016 at 08:49:17PM +0200, Juergen Gross wrote: >>>> On 22/07/16 18:31, Wei Liu wrote: >>>>> Only skim-read this patch, will do proper review later. >>>>> >>>>> On Fri, Jul 22, 2016 at 05:09:28PM +0200, Juergen Gross wrote: >>>>> [...] >>>>>> CAMLprim value ocaml_launched_by_systemd(value ignore) >>>>>> { >>>>>> - CAMLparam1(ignore); >>>>>> - CAMLlocal1(ret); >>>>>> + CAMLparam1(ignore); >>>>>> + CAMLlocal1(ret); >>>>>> >>>>>> - ret = Val_false; >>>>>> + ret = Val_false; >>>>>> >>>>>> - if (sd_listen_fds(0) > 0) >>>>>> - ret = Val_true; >>>>>> + if (sd_booted() > 0) >>>>>> + ret = Val_true; >>>>> >>>>> I think this may be problematic. >>>>> >>>>> sd_booted returns true if system is booted with systemd, but it has no >>>>> bearing whether this particular process is launched by systemd. >>>>> >>>>> IIRC using sd_booted would cause oxenstored thinks it is launched by >>>>> systemd even if the user launches it by hand in a shell. That caused >>>>> it's initialisation to fail. 81d758afca7c3c1e3ccbd78154b33d64fd7757fb >>>>> was written to address that issue. >>>>> >>>>> So, what would happen if you start oxenstored by hand with your patch >>>>> apply? Maybe we can just remove this launched_by_systemd check all >>>>> together -- i.e. we always call sd_notify? >> >> Sure we could. I'll remove the checks in both xenstored variants if >> nobody objects. >> >>>> >>>> So you are concerned sd_notify() will be called too often, but you are >>>> suggesting to call it always? I don't understand your concerns then. >>>> >>> >>> No, my concern is that you won't be able to start oxenstored from >>> command line manually if you boot with systemd. At least that was the >>> bug that caused me to write that patch. >> >> I believe the main problem was xenstored not calling daemonize() in that >> case, right? This problem is being remove with my patch as daemonize() >> will be called always. The systemd service file is modified to reflect >> this change in behavior. >> > > I'm afraid I can't remember all the details. But as long as you can > launch [o/c]xenstored by hand I think we're fine. Verified for both xenstored variants. Juergen _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 2/4] tools: split out xenstored starting form xencommons 2016-07-22 15:09 [PATCH v3 0/4] tools: make xenstore domain/daemon configurable Juergen Gross 2016-07-22 15:09 ` [PATCH v3 1/4] tools: remove systemd xenstore socket definitions Juergen Gross @ 2016-07-22 15:09 ` Juergen Gross 2016-07-25 13:43 ` Wei Liu 2016-07-22 15:09 ` [PATCH v3 3/4] tools: use pidfile for test if xenstored is running Juergen Gross ` (3 subsequent siblings) 5 siblings, 1 reply; 23+ messages in thread From: Juergen Gross @ 2016-07-22 15:09 UTC (permalink / raw) To: xen-devel Cc: Juergen Gross, wei.liu2, andrew.cooper3, ian.jackson, ross.lagerwall, dave In order to prepare starting a xenstore domain split out the starting of the xenstore daemon from the xencommons script into a dedicated launch-xenstore script. Correct one error: don't remove old tdb files in background, as this could lead to very subtle races. A rerun of autogen.sh is required. Signed-off-by: Juergen Gross <jgross@suse.com> --- .gitignore | 1 + tools/configure | 3 +- tools/configure.ac | 1 + tools/hotplug/Linux/Makefile | 1 + tools/hotplug/Linux/init.d/xencommons.in | 36 ++------------------- tools/hotplug/Linux/launch-xenstore.in | 55 ++++++++++++++++++++++++++++++++ 6 files changed, 63 insertions(+), 34 deletions(-) create mode 100644 tools/hotplug/Linux/launch-xenstore.in diff --git a/.gitignore b/.gitignore index 9b8dece..d193820 100644 --- a/.gitignore +++ b/.gitignore @@ -157,6 +157,7 @@ tools/hotplug/Linux/init.d/xen-watchdog tools/hotplug/Linux/init.d/xencommons tools/hotplug/Linux/init.d/xendomains tools/hotplug/Linux/init.d/xendriverdomain +tools/hotplug/Linux/launch-xenstore tools/hotplug/Linux/systemd/*.conf tools/hotplug/Linux/systemd/*.mount tools/hotplug/Linux/systemd/*.socket diff --git a/tools/configure b/tools/configure index 1c84c6c..51f16c5 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" @@ -10376,6 +10376,7 @@ do "hotplug/Linux/init.d/xencommons") CONFIG_FILES="$CONFIG_FILES hotplug/Linux/init.d/xencommons" ;; "hotplug/Linux/init.d/xendomains") CONFIG_FILES="$CONFIG_FILES hotplug/Linux/init.d/xendomains" ;; "hotplug/Linux/init.d/xendriverdomain") CONFIG_FILES="$CONFIG_FILES hotplug/Linux/init.d/xendriverdomain" ;; + "hotplug/Linux/launch-xenstore") CONFIG_FILES="$CONFIG_FILES hotplug/Linux/launch-xenstore" ;; "hotplug/Linux/vif-setup") CONFIG_FILES="$CONFIG_FILES hotplug/Linux/vif-setup" ;; "hotplug/Linux/xen-hotplug-common.sh") CONFIG_FILES="$CONFIG_FILES hotplug/Linux/xen-hotplug-common.sh" ;; "hotplug/Linux/xendomains") CONFIG_FILES="$CONFIG_FILES hotplug/Linux/xendomains" ;; diff --git a/tools/configure.ac b/tools/configure.ac index f991ab3..3a4abb5 100644 --- a/tools/configure.ac +++ b/tools/configure.ac @@ -15,6 +15,7 @@ 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 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/xencommons.in b/tools/hotplug/Linux/init.d/xencommons.in index 2d8f30b..a32608c 100644 --- a/tools/hotplug/Linux/init.d/xencommons.in +++ b/tools/hotplug/Linux/init.d/xencommons.in @@ -18,7 +18,6 @@ # Description: Starts and stops the daemons neeeded for xl/xend ### END INIT INFO -XENSTORED=@XENSTORED@ BACKEND_MODULES="@LINUX_BACKEND_MODULES@" . @XEN_SCRIPT_DIR@/hotplugpath.sh @@ -53,8 +52,6 @@ if test -f /proc/xen/capabilities && \ fi do_start () { - local time=0 - local timeout=30 local mod for mod in $BACKEND_MODULES ; do modprobe "$mod" &>/dev/null ; done @@ -62,37 +59,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 @XEN_RUN_DIR@/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" diff --git a/tools/hotplug/Linux/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in new file mode 100644 index 0000000..a0cbfd3 --- /dev/null +++ b/tools/hotplug/Linux/launch-xenstore.in @@ -0,0 +1,55 @@ +#!/bin/bash +# +# 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/>. +# + +XENSTORED=@XENSTORED@ + +. @XEN_SCRIPT_DIR@/hotplugpath.sh +test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons + +time=0 +timeout=30 + +if ! `${bindir}/xenstore-read -s / >/dev/null 2>&1` +then + test -z "$XENSTORED_ROOTDIR" && XENSTORED_ROOTDIR="@XEN_LIB_STORED@" + rm -f "$XENSTORED_ROOTDIR"/tdb* 2>/dev/null + test -z "$XENSTORED_TRACE" || XENSTORED_ARGS=" -T @XEN_LOG_DIR@/xenstored-trace.log" + + if [ -n "$XENSTORED" ] ; then + echo -n Starting $XENSTORED... + $XENSTORED --pid-file @XEN_RUN_DIR@/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 +fi + +exit 0 -- 2.6.6 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/4] tools: split out xenstored starting form xencommons 2016-07-22 15:09 ` [PATCH v3 2/4] tools: split out xenstored starting form xencommons Juergen Gross @ 2016-07-25 13:43 ` Wei Liu 0 siblings, 0 replies; 23+ messages in thread From: Wei Liu @ 2016-07-25 13:43 UTC (permalink / raw) To: Juergen Gross Cc: wei.liu2, andrew.cooper3, ian.jackson, xen-devel, ross.lagerwall, dave On Fri, Jul 22, 2016 at 05:09:29PM +0200, Juergen Gross wrote: > In order to prepare starting a xenstore domain split out the starting > of the xenstore daemon from the xencommons script into a dedicated > launch-xenstore script. > > Correct one error: don't remove old tdb files in background, as this > could lead to very subtle races. > > A rerun of autogen.sh is required. > > Signed-off-by: Juergen Gross <jgross@suse.com> Acked-by: Wei Liu <wei.liu2@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 3/4] tools: use pidfile for test if xenstored is running 2016-07-22 15:09 [PATCH v3 0/4] tools: make xenstore domain/daemon configurable Juergen Gross 2016-07-22 15:09 ` [PATCH v3 1/4] tools: remove systemd xenstore socket definitions Juergen Gross 2016-07-22 15:09 ` [PATCH v3 2/4] tools: split out xenstored starting form xencommons Juergen Gross @ 2016-07-22 15:09 ` Juergen Gross 2016-08-02 10:23 ` Wei Liu 2016-07-22 15:09 ` [PATCH v3 4/4] tools: make xenstore domain easy configurable Juergen Gross ` (2 subsequent siblings) 5 siblings, 1 reply; 23+ messages in thread From: Juergen Gross @ 2016-07-22 15:09 UTC (permalink / raw) To: xen-devel Cc: Juergen Gross, wei.liu2, andrew.cooper3, ian.jackson, ross.lagerwall, dave Instead of trying to read xenstore via xenstore-read use the pidfile of xenstored for the test whether xenstored is running. This prepares support of xenstore domain, as trying to read xenstore will block for ever in case xenstore domain is started after trying to read. Signed-off-by: Juergen Gross <jgross@suse.com> --- tools/hotplug/Linux/init.d/xencommons.in | 2 +- tools/hotplug/Linux/launch-xenstore.in | 58 +++++++++++++++++++------------- 2 files changed, 35 insertions(+), 25 deletions(-) diff --git a/tools/hotplug/Linux/init.d/xencommons.in b/tools/hotplug/Linux/init.d/xencommons.in index a32608c..a6a40d6 100644 --- a/tools/hotplug/Linux/init.d/xencommons.in +++ b/tools/hotplug/Linux/init.d/xencommons.in @@ -96,7 +96,7 @@ case "$1" in do_start ;; status) - ${bindir}/xenstore-read -s / + test -f @XEN_RUN_DIR@/xenstored.pid ;; stop) do_stop diff --git a/tools/hotplug/Linux/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in index a0cbfd3..2bd9f64 100644 --- a/tools/hotplug/Linux/launch-xenstore.in +++ b/tools/hotplug/Linux/launch-xenstore.in @@ -18,38 +18,48 @@ XENSTORED=@XENSTORED@ . @XEN_SCRIPT_DIR@/hotplugpath.sh -test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons -time=0 -timeout=30 +test_xenstore () { + test -f /var/run/xenstored.pid + return $? +} -if ! `${bindir}/xenstore-read -s / >/dev/null 2>&1` -then - test -z "$XENSTORED_ROOTDIR" && XENSTORED_ROOTDIR="@XEN_LIB_STORED@" - rm -f "$XENSTORED_ROOTDIR"/tdb* 2>/dev/null - test -z "$XENSTORED_TRACE" || XENSTORED_ARGS=" -T @XEN_LOG_DIR@/xenstored-trace.log" +timeout_xenstore () { + local time=0 + local timeout=30 - if [ -n "$XENSTORED" ] ; then - echo -n Starting $XENSTORED... - $XENSTORED --pid-file @XEN_RUN_DIR@/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 + 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 xenstored - exit 1 + 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 + +test -z "$XENSTORED_ROOTDIR" && XENSTORED_ROOTDIR="@XEN_LIB_STORED@" +rm -f "$XENSTORED_ROOTDIR"/tdb* 2>/dev/null +test -z "$XENSTORED_TRACE" || XENSTORED_ARGS=" -T @XEN_LOG_DIR@/xenstored-trace.log" + +if [ -n "$XENSTORED" ] ; then + echo -n Starting $XENSTORED... + $XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS +else + echo "No xenstored found" + exit 1 fi +timeout_xenstore $XENSTORED || exit 1 + exit 0 -- 2.6.6 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v3 3/4] tools: use pidfile for test if xenstored is running 2016-07-22 15:09 ` [PATCH v3 3/4] tools: use pidfile for test if xenstored is running Juergen Gross @ 2016-08-02 10:23 ` Wei Liu 2016-08-02 10:26 ` Juergen Gross 0 siblings, 1 reply; 23+ messages in thread From: Wei Liu @ 2016-08-02 10:23 UTC (permalink / raw) To: Juergen Gross Cc: wei.liu2, andrew.cooper3, ian.jackson, xen-devel, ross.lagerwall, dave On Fri, Jul 22, 2016 at 05:09:30PM +0200, Juergen Gross wrote: > Instead of trying to read xenstore via xenstore-read use the pidfile > of xenstored for the test whether xenstored is running. This prepares > support of xenstore domain, as trying to read xenstore will block > for ever in case xenstore domain is started after trying to read. > > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > tools/hotplug/Linux/init.d/xencommons.in | 2 +- > tools/hotplug/Linux/launch-xenstore.in | 58 +++++++++++++++++++------------- > 2 files changed, 35 insertions(+), 25 deletions(-) > > diff --git a/tools/hotplug/Linux/init.d/xencommons.in b/tools/hotplug/Linux/init.d/xencommons.in > index a32608c..a6a40d6 100644 > --- a/tools/hotplug/Linux/init.d/xencommons.in > +++ b/tools/hotplug/Linux/init.d/xencommons.in > @@ -96,7 +96,7 @@ case "$1" in > do_start > ;; > status) > - ${bindir}/xenstore-read -s / > + test -f @XEN_RUN_DIR@/xenstored.pid > ;; > stop) > do_stop > diff --git a/tools/hotplug/Linux/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in > index a0cbfd3..2bd9f64 100644 > --- a/tools/hotplug/Linux/launch-xenstore.in > +++ b/tools/hotplug/Linux/launch-xenstore.in > @@ -18,38 +18,48 @@ > XENSTORED=@XENSTORED@ > > . @XEN_SCRIPT_DIR@/hotplugpath.sh > -test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons > > -time=0 > -timeout=30 > +test_xenstore () { > + test -f /var/run/xenstored.pid You need to change this to @XEN_RUN_DIR@ as well. Other than this, this patch looks good to me. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 3/4] tools: use pidfile for test if xenstored is running 2016-08-02 10:23 ` Wei Liu @ 2016-08-02 10:26 ` Juergen Gross 0 siblings, 0 replies; 23+ messages in thread From: Juergen Gross @ 2016-08-02 10:26 UTC (permalink / raw) To: Wei Liu; +Cc: ross.lagerwall, dave, ian.jackson, andrew.cooper3, xen-devel On 02/08/16 12:23, Wei Liu wrote: > On Fri, Jul 22, 2016 at 05:09:30PM +0200, Juergen Gross wrote: >> Instead of trying to read xenstore via xenstore-read use the pidfile >> of xenstored for the test whether xenstored is running. This prepares >> support of xenstore domain, as trying to read xenstore will block >> for ever in case xenstore domain is started after trying to read. >> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> tools/hotplug/Linux/init.d/xencommons.in | 2 +- >> tools/hotplug/Linux/launch-xenstore.in | 58 +++++++++++++++++++------------- >> 2 files changed, 35 insertions(+), 25 deletions(-) >> >> diff --git a/tools/hotplug/Linux/init.d/xencommons.in b/tools/hotplug/Linux/init.d/xencommons.in >> index a32608c..a6a40d6 100644 >> --- a/tools/hotplug/Linux/init.d/xencommons.in >> +++ b/tools/hotplug/Linux/init.d/xencommons.in >> @@ -96,7 +96,7 @@ case "$1" in >> do_start >> ;; >> status) >> - ${bindir}/xenstore-read -s / >> + test -f @XEN_RUN_DIR@/xenstored.pid >> ;; >> stop) >> do_stop >> diff --git a/tools/hotplug/Linux/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in >> index a0cbfd3..2bd9f64 100644 >> --- a/tools/hotplug/Linux/launch-xenstore.in >> +++ b/tools/hotplug/Linux/launch-xenstore.in >> @@ -18,38 +18,48 @@ >> XENSTORED=@XENSTORED@ >> >> . @XEN_SCRIPT_DIR@/hotplugpath.sh >> -test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons >> >> -time=0 >> -timeout=30 >> +test_xenstore () { >> + test -f /var/run/xenstored.pid > > You need to change this to @XEN_RUN_DIR@ as well. Indeed. Thanks for noticing. Juergen _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 4/4] tools: make xenstore domain easy configurable 2016-07-22 15:09 [PATCH v3 0/4] tools: make xenstore domain/daemon configurable Juergen Gross ` (2 preceding siblings ...) 2016-07-22 15:09 ` [PATCH v3 3/4] tools: use pidfile for test if xenstored is running Juergen Gross @ 2016-07-22 15:09 ` Juergen Gross 2016-07-25 13:43 ` Wei Liu 2016-07-25 13:43 ` [PATCH v3 0/4] tools: make xenstore domain/daemon configurable Wei Liu 2016-08-01 8:02 ` Juergen Gross 5 siblings, 1 reply; 23+ messages in thread From: Juergen Gross @ 2016-07-22 15:09 UTC (permalink / raw) To: xen-devel Cc: Juergen Gross, wei.liu2, andrew.cooper3, ian.jackson, ross.lagerwall, dave 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> --- V3: - remove check for running xenstore domain, as this is done in init-xenstore-domain already - if booted with systemd send a systemd-notify message in the xenstore domain case - if booted with systemd don't wait until xenstore daemon is running, as the daemon will have sent a notify message by its own - split up patch as requested by Ian Jackson V2: - add .gitignore entry for launch-xenstore --- tools/hotplug/Linux/init.d/sysconfig.xencommons.in | 42 ++++++++++++++++++++-- tools/hotplug/Linux/launch-xenstore.in | 42 ++++++++++++++++------ tools/hotplug/Linux/systemd/xenstored.service.in | 8 +---- 3 files changed, 72 insertions(+), 20 deletions(-) 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/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in index 2bd9f64..fdfa33a 100644 --- a/tools/hotplug/Linux/launch-xenstore.in +++ b/tools/hotplug/Linux/launch-xenstore.in @@ -48,18 +48,40 @@ test_xenstore && exit 0 test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons -test -z "$XENSTORED_ROOTDIR" && XENSTORED_ROOTDIR="@XEN_LIB_STORED@" -rm -f "$XENSTORED_ROOTDIR"/tdb* 2>/dev/null -test -z "$XENSTORED_TRACE" || XENSTORED_ARGS=" -T @XEN_LOG_DIR@/xenstored-trace.log" +[ "$XENSTORETYPE" = "" ] && XENSTORETYPE=daemon + +/bin/mkdir -p @XEN_RUN_DIR@ + +[ "$XENSTORETYPE" = "daemon" ] && { + [ -z "$XENSTORED_ROOTDIR" ] && XENSTORED_ROOTDIR="@XEN_LIB_STORED@" + /bin/rm -f $XENSTORED_ROOTDIR/tdb* 2>/dev/null + [ -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 + } -if [ -n "$XENSTORED" ] ; then echo -n Starting $XENSTORED... $XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS -else - echo "No xenstored found" - exit 1 -fi -timeout_xenstore $XENSTORED || exit 1 + systemd-notify --booted 2>/dev/null || timeout_xenstore $XENSTORED || exit 1 -exit 0 + 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 || exit 1 + systemd-notify --ready 2>/dev/null + + 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 d520d70..80c1d40 100644 --- a/tools/hotplug/Linux/systemd/xenstored.service.in +++ b/tools/hotplug/Linux/systemd/xenstored.service.in @@ -10,14 +10,8 @@ ConditionPathExists=/proc/xen/capabilities Type=notify NotifyAccess=all RemainAfterExit=true -KillMode=none -Environment=XENSTORED_ARGS= -Environment=XENSTORED=@XENSTORED@ -EnvironmentFile=-@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities -ExecStartPre=-/bin/rm -f @XEN_LIB_STORED@/tdb* -ExecStartPre=/bin/mkdir -p @XEN_RUN_DIR@ -ExecStart=/bin/sh -c "exec $XENSTORED --no-fork $XENSTORED_ARGS" +ExecStart=@XEN_SCRIPT_DIR@/launch-xenstore [Install] WantedBy=multi-user.target -- 2.6.6 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v3 4/4] tools: make xenstore domain easy configurable 2016-07-22 15:09 ` [PATCH v3 4/4] tools: make xenstore domain easy configurable Juergen Gross @ 2016-07-25 13:43 ` Wei Liu 2016-07-25 13:56 ` Juergen Gross 0 siblings, 1 reply; 23+ messages in thread From: Wei Liu @ 2016-07-25 13:43 UTC (permalink / raw) To: Juergen Gross Cc: wei.liu2, andrew.cooper3, ian.jackson, xen-devel, ross.lagerwall, dave On Fri, Jul 22, 2016 at 05:09:31PM +0200, Juergen Gross wrote: [...] > diff --git a/tools/hotplug/Linux/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in > index 2bd9f64..fdfa33a 100644 > --- a/tools/hotplug/Linux/launch-xenstore.in > +++ b/tools/hotplug/Linux/launch-xenstore.in > @@ -48,18 +48,40 @@ test_xenstore && exit 0 > > test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons > > -test -z "$XENSTORED_ROOTDIR" && XENSTORED_ROOTDIR="@XEN_LIB_STORED@" > -rm -f "$XENSTORED_ROOTDIR"/tdb* 2>/dev/null > -test -z "$XENSTORED_TRACE" || XENSTORED_ARGS=" -T @XEN_LOG_DIR@/xenstored-trace.log" > +[ "$XENSTORETYPE" = "" ] && XENSTORETYPE=daemon > + > +/bin/mkdir -p @XEN_RUN_DIR@ > + > +[ "$XENSTORETYPE" = "daemon" ] && { > + [ -z "$XENSTORED_ROOTDIR" ] && XENSTORED_ROOTDIR="@XEN_LIB_STORED@" > + /bin/rm -f $XENSTORED_ROOTDIR/tdb* 2>/dev/null > + [ -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 > + } > > -if [ -n "$XENSTORED" ] ; then > echo -n Starting $XENSTORED... > $XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS > -else > - echo "No xenstored found" > - exit 1 > -fi > > -timeout_xenstore $XENSTORED || exit 1 > + systemd-notify --booted 2>/dev/null || timeout_xenstore $XENSTORED || exit 1 > > -exit 0 > + 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 || exit 1 > + systemd-notify --ready 2>/dev/null Please test if there is systemd-notify before trying to invoke it and then you can properly log the failure of the invocation. Wei. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 4/4] tools: make xenstore domain easy configurable 2016-07-25 13:43 ` Wei Liu @ 2016-07-25 13:56 ` Juergen Gross 2016-07-25 14:01 ` Wei Liu 0 siblings, 1 reply; 23+ messages in thread From: Juergen Gross @ 2016-07-25 13:56 UTC (permalink / raw) To: Wei Liu; +Cc: ross.lagerwall, dave, ian.jackson, andrew.cooper3, xen-devel On 25/07/16 15:43, Wei Liu wrote: > On Fri, Jul 22, 2016 at 05:09:31PM +0200, Juergen Gross wrote: > [...] >> diff --git a/tools/hotplug/Linux/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in >> index 2bd9f64..fdfa33a 100644 >> --- a/tools/hotplug/Linux/launch-xenstore.in >> +++ b/tools/hotplug/Linux/launch-xenstore.in >> @@ -48,18 +48,40 @@ test_xenstore && exit 0 >> >> test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons >> >> -test -z "$XENSTORED_ROOTDIR" && XENSTORED_ROOTDIR="@XEN_LIB_STORED@" >> -rm -f "$XENSTORED_ROOTDIR"/tdb* 2>/dev/null >> -test -z "$XENSTORED_TRACE" || XENSTORED_ARGS=" -T @XEN_LOG_DIR@/xenstored-trace.log" >> +[ "$XENSTORETYPE" = "" ] && XENSTORETYPE=daemon >> + >> +/bin/mkdir -p @XEN_RUN_DIR@ >> + >> +[ "$XENSTORETYPE" = "daemon" ] && { >> + [ -z "$XENSTORED_ROOTDIR" ] && XENSTORED_ROOTDIR="@XEN_LIB_STORED@" >> + /bin/rm -f $XENSTORED_ROOTDIR/tdb* 2>/dev/null >> + [ -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 >> + } >> >> -if [ -n "$XENSTORED" ] ; then >> echo -n Starting $XENSTORED... >> $XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS >> -else >> - echo "No xenstored found" >> - exit 1 >> -fi >> >> -timeout_xenstore $XENSTORED || exit 1 >> + systemd-notify --booted 2>/dev/null || timeout_xenstore $XENSTORED || exit 1 >> >> -exit 0 >> + 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 || exit 1 >> + systemd-notify --ready 2>/dev/null > > Please test if there is systemd-notify before trying to invoke it and > then you can properly log the failure of the invocation. I thought about that. What would be the purpose doing so? Following cases are possible: - system has no systemd installed: systemd-notify will fail, but calling it was not necessary -> no harm done - system is with systemd, but not booted under control of it, and systemd-notify is not found: same as above -> no harm done - system is with systemd, but not booted under control of it, and systemd-notify is found: calling systemd-notify isn't really needed, but it won't harm - system is booted under control of systemd, systemd-notify is not found: I could log it, but I can't know that I'm under control of systemd (standard way to tell from a script is calling "systemd-notify --booted" which is kind of chicken and egg problem here) so I can't know whether not finding systemd-notify is an error or not - system is booted under control of systemd, systemd-notify is found: everything is nice, systemd receives the notification it is waiting for Juergen _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 4/4] tools: make xenstore domain easy configurable 2016-07-25 13:56 ` Juergen Gross @ 2016-07-25 14:01 ` Wei Liu 2016-07-25 14:06 ` Juergen Gross 0 siblings, 1 reply; 23+ messages in thread From: Wei Liu @ 2016-07-25 14:01 UTC (permalink / raw) To: Juergen Gross Cc: Wei Liu, andrew.cooper3, ian.jackson, xen-devel, ross.lagerwall, dave On Mon, Jul 25, 2016 at 03:56:17PM +0200, Juergen Gross wrote: > On 25/07/16 15:43, Wei Liu wrote: > > On Fri, Jul 22, 2016 at 05:09:31PM +0200, Juergen Gross wrote: > > [...] > >> diff --git a/tools/hotplug/Linux/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in > >> index 2bd9f64..fdfa33a 100644 > >> --- a/tools/hotplug/Linux/launch-xenstore.in > >> +++ b/tools/hotplug/Linux/launch-xenstore.in > >> @@ -48,18 +48,40 @@ test_xenstore && exit 0 > >> > >> test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons > >> > >> -test -z "$XENSTORED_ROOTDIR" && XENSTORED_ROOTDIR="@XEN_LIB_STORED@" > >> -rm -f "$XENSTORED_ROOTDIR"/tdb* 2>/dev/null > >> -test -z "$XENSTORED_TRACE" || XENSTORED_ARGS=" -T @XEN_LOG_DIR@/xenstored-trace.log" > >> +[ "$XENSTORETYPE" = "" ] && XENSTORETYPE=daemon > >> + > >> +/bin/mkdir -p @XEN_RUN_DIR@ > >> + > >> +[ "$XENSTORETYPE" = "daemon" ] && { > >> + [ -z "$XENSTORED_ROOTDIR" ] && XENSTORED_ROOTDIR="@XEN_LIB_STORED@" > >> + /bin/rm -f $XENSTORED_ROOTDIR/tdb* 2>/dev/null > >> + [ -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 > >> + } > >> > >> -if [ -n "$XENSTORED" ] ; then > >> echo -n Starting $XENSTORED... > >> $XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS > >> -else > >> - echo "No xenstored found" > >> - exit 1 > >> -fi > >> > >> -timeout_xenstore $XENSTORED || exit 1 > >> + systemd-notify --booted 2>/dev/null || timeout_xenstore $XENSTORED || exit 1 > >> > >> -exit 0 > >> + 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 || exit 1 > >> + systemd-notify --ready 2>/dev/null > > > > Please test if there is systemd-notify before trying to invoke it and > > then you can properly log the failure of the invocation. > > I thought about that. What would be the purpose doing so? Following > cases are possible: > This > - system is booted under control of systemd, systemd-notify is found: > everything is nice, systemd receives the notification it is waiting > for > Here you assume systemd-notify always succeed. It can fail due to some reasons. That's what its manpage suggests. We need to handle this. Wei. > > Juergen _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 4/4] tools: make xenstore domain easy configurable 2016-07-25 14:01 ` Wei Liu @ 2016-07-25 14:06 ` Juergen Gross 2016-07-25 14:11 ` Wei Liu 0 siblings, 1 reply; 23+ messages in thread From: Juergen Gross @ 2016-07-25 14:06 UTC (permalink / raw) To: Wei Liu; +Cc: ross.lagerwall, dave, ian.jackson, andrew.cooper3, xen-devel On 25/07/16 16:01, Wei Liu wrote: > On Mon, Jul 25, 2016 at 03:56:17PM +0200, Juergen Gross wrote: >> On 25/07/16 15:43, Wei Liu wrote: >>> On Fri, Jul 22, 2016 at 05:09:31PM +0200, Juergen Gross wrote: >>> [...] >>>> diff --git a/tools/hotplug/Linux/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in >>>> index 2bd9f64..fdfa33a 100644 >>>> --- a/tools/hotplug/Linux/launch-xenstore.in >>>> +++ b/tools/hotplug/Linux/launch-xenstore.in >>>> @@ -48,18 +48,40 @@ test_xenstore && exit 0 >>>> >>>> test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons >>>> >>>> -test -z "$XENSTORED_ROOTDIR" && XENSTORED_ROOTDIR="@XEN_LIB_STORED@" >>>> -rm -f "$XENSTORED_ROOTDIR"/tdb* 2>/dev/null >>>> -test -z "$XENSTORED_TRACE" || XENSTORED_ARGS=" -T @XEN_LOG_DIR@/xenstored-trace.log" >>>> +[ "$XENSTORETYPE" = "" ] && XENSTORETYPE=daemon >>>> + >>>> +/bin/mkdir -p @XEN_RUN_DIR@ >>>> + >>>> +[ "$XENSTORETYPE" = "daemon" ] && { >>>> + [ -z "$XENSTORED_ROOTDIR" ] && XENSTORED_ROOTDIR="@XEN_LIB_STORED@" >>>> + /bin/rm -f $XENSTORED_ROOTDIR/tdb* 2>/dev/null >>>> + [ -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 >>>> + } >>>> >>>> -if [ -n "$XENSTORED" ] ; then >>>> echo -n Starting $XENSTORED... >>>> $XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS >>>> -else >>>> - echo "No xenstored found" >>>> - exit 1 >>>> -fi >>>> >>>> -timeout_xenstore $XENSTORED || exit 1 >>>> + systemd-notify --booted 2>/dev/null || timeout_xenstore $XENSTORED || exit 1 >>>> >>>> -exit 0 >>>> + 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 || exit 1 >>>> + systemd-notify --ready 2>/dev/null >>> >>> Please test if there is systemd-notify before trying to invoke it and >>> then you can properly log the failure of the invocation. >> >> I thought about that. What would be the purpose doing so? Following >> cases are possible: >> > > This > >> - system is booted under control of systemd, systemd-notify is found: >> everything is nice, systemd receives the notification it is waiting >> for >> > > Here you assume systemd-notify always succeed. It can fail due to some > reasons. That's what its manpage suggests. > > We need to handle this. May I repeat the sd_notify() man page here? "In order to support both, init systems that implement this scheme and those which do not, it is generally recommended to ignore the return value of this call." Juergen _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 4/4] tools: make xenstore domain easy configurable 2016-07-25 14:06 ` Juergen Gross @ 2016-07-25 14:11 ` Wei Liu 2016-07-25 14:16 ` Juergen Gross 0 siblings, 1 reply; 23+ messages in thread From: Wei Liu @ 2016-07-25 14:11 UTC (permalink / raw) To: Juergen Gross Cc: Wei Liu, andrew.cooper3, ian.jackson, xen-devel, ross.lagerwall, dave On Mon, Jul 25, 2016 at 04:06:11PM +0200, Juergen Gross wrote: > On 25/07/16 16:01, Wei Liu wrote: > > On Mon, Jul 25, 2016 at 03:56:17PM +0200, Juergen Gross wrote: > >> On 25/07/16 15:43, Wei Liu wrote: > >>> On Fri, Jul 22, 2016 at 05:09:31PM +0200, Juergen Gross wrote: > >>> [...] > >>>> diff --git a/tools/hotplug/Linux/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in > >>>> index 2bd9f64..fdfa33a 100644 > >>>> --- a/tools/hotplug/Linux/launch-xenstore.in > >>>> +++ b/tools/hotplug/Linux/launch-xenstore.in > >>>> @@ -48,18 +48,40 @@ test_xenstore && exit 0 > >>>> > >>>> test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons > >>>> > >>>> -test -z "$XENSTORED_ROOTDIR" && XENSTORED_ROOTDIR="@XEN_LIB_STORED@" > >>>> -rm -f "$XENSTORED_ROOTDIR"/tdb* 2>/dev/null > >>>> -test -z "$XENSTORED_TRACE" || XENSTORED_ARGS=" -T @XEN_LOG_DIR@/xenstored-trace.log" > >>>> +[ "$XENSTORETYPE" = "" ] && XENSTORETYPE=daemon > >>>> + > >>>> +/bin/mkdir -p @XEN_RUN_DIR@ > >>>> + > >>>> +[ "$XENSTORETYPE" = "daemon" ] && { > >>>> + [ -z "$XENSTORED_ROOTDIR" ] && XENSTORED_ROOTDIR="@XEN_LIB_STORED@" > >>>> + /bin/rm -f $XENSTORED_ROOTDIR/tdb* 2>/dev/null > >>>> + [ -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 > >>>> + } > >>>> > >>>> -if [ -n "$XENSTORED" ] ; then > >>>> echo -n Starting $XENSTORED... > >>>> $XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS > >>>> -else > >>>> - echo "No xenstored found" > >>>> - exit 1 > >>>> -fi > >>>> > >>>> -timeout_xenstore $XENSTORED || exit 1 > >>>> + systemd-notify --booted 2>/dev/null || timeout_xenstore $XENSTORED || exit 1 > >>>> > >>>> -exit 0 > >>>> + 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 || exit 1 > >>>> + systemd-notify --ready 2>/dev/null > >>> > >>> Please test if there is systemd-notify before trying to invoke it and > >>> then you can properly log the failure of the invocation. > >> > >> I thought about that. What would be the purpose doing so? Following > >> cases are possible: > >> > > > > This > > > >> - system is booted under control of systemd, systemd-notify is found: > >> everything is nice, systemd receives the notification it is waiting > >> for > >> > > > > Here you assume systemd-notify always succeed. It can fail due to some > > reasons. That's what its manpage suggests. > > > > We need to handle this. > > May I repeat the sd_notify() man page here? > > "In order to support both, init systems that implement this scheme and > those which do not, it is generally recommended to ignore the return > value of this call." That's a different thing from the systemd-notify utility, isn't it? I read man systemd-notify in this case: EXIT STATUS On success, 0 is returned, a non-zero failure code otherwise. I'm not too sure about the relationship between systemd-notify and sd_notify, but I bet it is more than just a call to sd_notify. Wei. > > Juergen _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 4/4] tools: make xenstore domain easy configurable 2016-07-25 14:11 ` Wei Liu @ 2016-07-25 14:16 ` Juergen Gross 2016-07-25 14:27 ` Wei Liu 0 siblings, 1 reply; 23+ messages in thread From: Juergen Gross @ 2016-07-25 14:16 UTC (permalink / raw) To: Wei Liu; +Cc: ross.lagerwall, dave, ian.jackson, andrew.cooper3, xen-devel On 25/07/16 16:11, Wei Liu wrote: > On Mon, Jul 25, 2016 at 04:06:11PM +0200, Juergen Gross wrote: >> On 25/07/16 16:01, Wei Liu wrote: >>> On Mon, Jul 25, 2016 at 03:56:17PM +0200, Juergen Gross wrote: >>>> On 25/07/16 15:43, Wei Liu wrote: >>>>> On Fri, Jul 22, 2016 at 05:09:31PM +0200, Juergen Gross wrote: >>>>> [...] >>>>>> diff --git a/tools/hotplug/Linux/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in >>>>>> index 2bd9f64..fdfa33a 100644 >>>>>> --- a/tools/hotplug/Linux/launch-xenstore.in >>>>>> +++ b/tools/hotplug/Linux/launch-xenstore.in >>>>>> @@ -48,18 +48,40 @@ test_xenstore && exit 0 >>>>>> >>>>>> test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons >>>>>> >>>>>> -test -z "$XENSTORED_ROOTDIR" && XENSTORED_ROOTDIR="@XEN_LIB_STORED@" >>>>>> -rm -f "$XENSTORED_ROOTDIR"/tdb* 2>/dev/null >>>>>> -test -z "$XENSTORED_TRACE" || XENSTORED_ARGS=" -T @XEN_LOG_DIR@/xenstored-trace.log" >>>>>> +[ "$XENSTORETYPE" = "" ] && XENSTORETYPE=daemon >>>>>> + >>>>>> +/bin/mkdir -p @XEN_RUN_DIR@ >>>>>> + >>>>>> +[ "$XENSTORETYPE" = "daemon" ] && { >>>>>> + [ -z "$XENSTORED_ROOTDIR" ] && XENSTORED_ROOTDIR="@XEN_LIB_STORED@" >>>>>> + /bin/rm -f $XENSTORED_ROOTDIR/tdb* 2>/dev/null >>>>>> + [ -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 >>>>>> + } >>>>>> >>>>>> -if [ -n "$XENSTORED" ] ; then >>>>>> echo -n Starting $XENSTORED... >>>>>> $XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS >>>>>> -else >>>>>> - echo "No xenstored found" >>>>>> - exit 1 >>>>>> -fi >>>>>> >>>>>> -timeout_xenstore $XENSTORED || exit 1 >>>>>> + systemd-notify --booted 2>/dev/null || timeout_xenstore $XENSTORED || exit 1 >>>>>> >>>>>> -exit 0 >>>>>> + 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 || exit 1 >>>>>> + systemd-notify --ready 2>/dev/null >>>>> >>>>> Please test if there is systemd-notify before trying to invoke it and >>>>> then you can properly log the failure of the invocation. >>>> >>>> I thought about that. What would be the purpose doing so? Following >>>> cases are possible: >>>> >>> >>> This >>> >>>> - system is booted under control of systemd, systemd-notify is found: >>>> everything is nice, systemd receives the notification it is waiting >>>> for >>>> >>> >>> Here you assume systemd-notify always succeed. It can fail due to some >>> reasons. That's what its manpage suggests. >>> >>> We need to handle this. >> >> May I repeat the sd_notify() man page here? >> >> "In order to support both, init systems that implement this scheme and >> those which do not, it is generally recommended to ignore the return >> value of this call." > > That's a different thing from the systemd-notify utility, isn't it? > > I read man systemd-notify in this case: > > EXIT STATUS > On success, 0 is returned, a non-zero failure code otherwise. > > I'm not too sure about the relationship between systemd-notify and > sd_notify, but I bet it is more than just a call to sd_notify. The systemd-notify man-page tells us: "This is mostly just a wrapper around sd_notify() and makes this functionality available to shell scripts. For details see sd_notify(3)." Juergen _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 4/4] tools: make xenstore domain easy configurable 2016-07-25 14:16 ` Juergen Gross @ 2016-07-25 14:27 ` Wei Liu 0 siblings, 0 replies; 23+ messages in thread From: Wei Liu @ 2016-07-25 14:27 UTC (permalink / raw) To: Juergen Gross Cc: Wei Liu, andrew.cooper3, ian.jackson, xen-devel, ross.lagerwall, dave On Mon, Jul 25, 2016 at 04:16:51PM +0200, Juergen Gross wrote: > On 25/07/16 16:11, Wei Liu wrote: > > On Mon, Jul 25, 2016 at 04:06:11PM +0200, Juergen Gross wrote: > >> On 25/07/16 16:01, Wei Liu wrote: > >>> On Mon, Jul 25, 2016 at 03:56:17PM +0200, Juergen Gross wrote: > >>>> On 25/07/16 15:43, Wei Liu wrote: > >>>>> On Fri, Jul 22, 2016 at 05:09:31PM +0200, Juergen Gross wrote: > >>>>> [...] > >>>>>> diff --git a/tools/hotplug/Linux/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in > >>>>>> index 2bd9f64..fdfa33a 100644 > >>>>>> --- a/tools/hotplug/Linux/launch-xenstore.in > >>>>>> +++ b/tools/hotplug/Linux/launch-xenstore.in > >>>>>> @@ -48,18 +48,40 @@ test_xenstore && exit 0 > >>>>>> > >>>>>> test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons > >>>>>> > >>>>>> -test -z "$XENSTORED_ROOTDIR" && XENSTORED_ROOTDIR="@XEN_LIB_STORED@" > >>>>>> -rm -f "$XENSTORED_ROOTDIR"/tdb* 2>/dev/null > >>>>>> -test -z "$XENSTORED_TRACE" || XENSTORED_ARGS=" -T @XEN_LOG_DIR@/xenstored-trace.log" > >>>>>> +[ "$XENSTORETYPE" = "" ] && XENSTORETYPE=daemon > >>>>>> + > >>>>>> +/bin/mkdir -p @XEN_RUN_DIR@ > >>>>>> + > >>>>>> +[ "$XENSTORETYPE" = "daemon" ] && { > >>>>>> + [ -z "$XENSTORED_ROOTDIR" ] && XENSTORED_ROOTDIR="@XEN_LIB_STORED@" > >>>>>> + /bin/rm -f $XENSTORED_ROOTDIR/tdb* 2>/dev/null > >>>>>> + [ -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 > >>>>>> + } > >>>>>> > >>>>>> -if [ -n "$XENSTORED" ] ; then > >>>>>> echo -n Starting $XENSTORED... > >>>>>> $XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS > >>>>>> -else > >>>>>> - echo "No xenstored found" > >>>>>> - exit 1 > >>>>>> -fi > >>>>>> > >>>>>> -timeout_xenstore $XENSTORED || exit 1 > >>>>>> + systemd-notify --booted 2>/dev/null || timeout_xenstore $XENSTORED || exit 1 > >>>>>> > >>>>>> -exit 0 > >>>>>> + 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 || exit 1 > >>>>>> + systemd-notify --ready 2>/dev/null > >>>>> > >>>>> Please test if there is systemd-notify before trying to invoke it and > >>>>> then you can properly log the failure of the invocation. > >>>> > >>>> I thought about that. What would be the purpose doing so? Following > >>>> cases are possible: > >>>> > >>> > >>> This > >>> > >>>> - system is booted under control of systemd, systemd-notify is found: > >>>> everything is nice, systemd receives the notification it is waiting > >>>> for > >>>> > >>> > >>> Here you assume systemd-notify always succeed. It can fail due to some > >>> reasons. That's what its manpage suggests. > >>> > >>> We need to handle this. > >> > >> May I repeat the sd_notify() man page here? > >> > >> "In order to support both, init systems that implement this scheme and > >> those which do not, it is generally recommended to ignore the return > >> value of this call." > > > > That's a different thing from the systemd-notify utility, isn't it? > > > > I read man systemd-notify in this case: > > > > EXIT STATUS > > On success, 0 is returned, a non-zero failure code otherwise. > > > > I'm not too sure about the relationship between systemd-notify and > > sd_notify, but I bet it is more than just a call to sd_notify. > > The systemd-notify man-page tells us: > > "This is mostly just a wrapper around sd_notify() and makes this > functionality available to shell scripts. For details see sd_notify(3)." > OK. I'm convinced now. Wei. > > Juergen > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 0/4] tools: make xenstore domain/daemon configurable 2016-07-22 15:09 [PATCH v3 0/4] tools: make xenstore domain/daemon configurable Juergen Gross ` (3 preceding siblings ...) 2016-07-22 15:09 ` [PATCH v3 4/4] tools: make xenstore domain easy configurable Juergen Gross @ 2016-07-25 13:43 ` Wei Liu 2016-08-01 8:02 ` Juergen Gross 5 siblings, 0 replies; 23+ messages in thread From: Wei Liu @ 2016-07-25 13:43 UTC (permalink / raw) To: Juergen Gross Cc: wei.liu2, andrew.cooper3, ian.jackson, xen-devel, ross.lagerwall, dave I went back to the long thread in v1. I don't think I can figure out if all the comments are addressed. Ian asked for something to be rectified, but I'm not sure if this series satisfies him. I will let him comment on that. Wei. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 0/4] tools: make xenstore domain/daemon configurable 2016-07-22 15:09 [PATCH v3 0/4] tools: make xenstore domain/daemon configurable Juergen Gross ` (4 preceding siblings ...) 2016-07-25 13:43 ` [PATCH v3 0/4] tools: make xenstore domain/daemon configurable Wei Liu @ 2016-08-01 8:02 ` Juergen Gross 5 siblings, 0 replies; 23+ messages in thread From: Juergen Gross @ 2016-08-01 8:02 UTC (permalink / raw) To: xen-devel; +Cc: andrew.cooper3, dave, wei.liu2, ian.jackson, ross.lagerwall On 22/07/16 17:09, Juergen Gross wrote: > 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. > > Changes in V3: > - patch 1: re-add sd_notify() call > - split up former patch 2 into 3 patches as requested by Ian Jackson > - patch 4 (was 2): remove check for running xenstore domain, as this > is done in init-xenstore-domain already > - patch 4 (was 2): if booted with systemd send a systemd-notify message > in the xenstore domain case > - patch 4 (was 2): if booted with systemd don't wait until xenstore > daemon is running, as the daemon will have sent a notify message by > its own > > Changes in V2: > - move service type modification form patch 2 to patch 1 as implied by > Ross Lagerwall (at least I guess so) > - add .gitignore entry for launch-xenstore I think I have addressed all concerns raised on version 1 and 2 of this series. Did I miss anything? Juergen > > Juergen Gross (4): > tools: remove systemd xenstore socket definitions > tools: split out xenstored starting form xencommons > tools: use pidfile for test if xenstored is running > tools: make xenstore domain easy configurable > > .gitignore | 1 + > tools/configure | 7 +- > tools/configure.ac | 3 +- > tools/hotplug/Linux/Makefile | 1 + > tools/hotplug/Linux/init.d/sysconfig.xencommons.in | 42 +++++++- > tools/hotplug/Linux/init.d/xencommons.in | 38 +------ > tools/hotplug/Linux/launch-xenstore.in | 87 ++++++++++++++++ > tools/hotplug/Linux/systemd/Makefile | 5 - > tools/hotplug/Linux/systemd/xenstored.service.in | 13 +-- > tools/hotplug/Linux/systemd/xenstored.socket.in | 13 --- > tools/hotplug/Linux/systemd/xenstored_ro.socket.in | 13 --- > tools/ocaml/xenstored/systemd.ml | 1 - > tools/ocaml/xenstored/systemd.mli | 5 - > tools/ocaml/xenstored/systemd_stubs.c | 113 ++++----------------- > tools/ocaml/xenstored/utils.ml | 21 ++-- > tools/xenstore/xenstored_core.c | 92 +---------------- > 16 files changed, 169 insertions(+), 286 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 > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2016-08-02 10:26 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-07-22 15:09 [PATCH v3 0/4] tools: make xenstore domain/daemon configurable Juergen Gross 2016-07-22 15:09 ` [PATCH v3 1/4] tools: remove systemd xenstore socket definitions Juergen Gross 2016-07-22 16:31 ` Wei Liu 2016-07-22 18:49 ` Juergen Gross 2016-07-22 18:51 ` Wei Liu 2016-07-25 4:33 ` Juergen Gross 2016-07-25 11:05 ` Wei Liu 2016-07-25 12:21 ` Juergen Gross 2016-07-22 15:09 ` [PATCH v3 2/4] tools: split out xenstored starting form xencommons Juergen Gross 2016-07-25 13:43 ` Wei Liu 2016-07-22 15:09 ` [PATCH v3 3/4] tools: use pidfile for test if xenstored is running Juergen Gross 2016-08-02 10:23 ` Wei Liu 2016-08-02 10:26 ` Juergen Gross 2016-07-22 15:09 ` [PATCH v3 4/4] tools: make xenstore domain easy configurable Juergen Gross 2016-07-25 13:43 ` Wei Liu 2016-07-25 13:56 ` Juergen Gross 2016-07-25 14:01 ` Wei Liu 2016-07-25 14:06 ` Juergen Gross 2016-07-25 14:11 ` Wei Liu 2016-07-25 14:16 ` Juergen Gross 2016-07-25 14:27 ` Wei Liu 2016-07-25 13:43 ` [PATCH v3 0/4] tools: make xenstore domain/daemon configurable Wei Liu 2016-08-01 8:02 ` Juergen Gross
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.