[PATCH for-4.6 v2 0/4] Patches for c/oxenstored

[PATCH for-4.6 v2 0/4] Patches for c/oxenstored

[Wei Liu]

Wei Liu (4):
  cxenstored: fix systemd socket activation
  cxenstored: document a bunch of short options in help string
  oxenstored: fix systemd socket activation
  oxenstored: move sd_notify_ready out of main loop

 tools/ocaml/xenstored/systemd.ml      |  2 +-
 tools/ocaml/xenstored/systemd.mli     |  4 +--
 tools/ocaml/xenstored/systemd_stubs.c |  6 ++--
 tools/ocaml/xenstored/utils.ml        |  2 +-
 tools/ocaml/xenstored/xenstored.ml    |  4 +--
 tools/xenstore/xenstored_core.c       | 59 +++++++++++++++++++++--------------
 6 files changed, 44 insertions(+), 33 deletions(-)

-- 
2.1.4

[PATCH for-4.6 v2 1/4] cxenstored: fix systemd socket activation

[Wei Liu]

There were two problems with original code:

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

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

Fix issues with following changes:

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

Rename function and restructure code to make things clearer.

A side effect of this patch is that gcc 4.8 with -Wmaybe-uninitialized
in non-debug build spits out spurious warning about sock and ro_sock
might be uninitialized. Since CentOS 7 ships gcc 4.8, we need to work
around that by setting sock and ro_sock to NULL at the beginning of
main.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Tested-by: George Dunlap <george.dunlap@eu.citrix.com>
---
For 4.6: without this cxenstored is broken when running on a system with
systemd but not started by systemd.

v2: keep tested-by because there is no functional change from v1, drop
acked-by because a workaround for gcc 4.8 is introduced.
---
 tools/xenstore/xenstored_core.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

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

Re: [PATCH for-4.6 v2 1/4] cxenstored: fix systemd socket activation

[Ian Campbell]

On Mon, 2015-08-10 at 09:00 +0100, Wei Liu wrote:
> There were two problems with original code:
> 
> 1. sd_booted() was used to determined if the process was started by
>    systemd, which was wrong.
> 2. Exit with error if pidfile was specified, which was too harsh.
> 
> These two combined made cxenstored unable to start by hand if it ran
> on a system which had systemd.
> 
> Fix issues with following changes:
> 
> 1. Use sd_listen_fds to determine if the process is started by systemd.
> 2. Don't exit if pidfile is specified.
> 
> Rename function and restructure code to make things clearer.
> 
> A side effect of this patch is that gcc 4.8 with -Wmaybe-uninitialized
> in non-debug build spits out spurious warning about sock and ro_sock
> might be uninitialized. Since CentOS 7 ships gcc 4.8, we need to work
> around that by setting sock and ro_sock to NULL at the beginning of
> main.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Tested-by: George Dunlap <george.dunlap@eu.citrix.com>

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

[PATCH for-4.6 v2 2/4] cxenstored: document a bunch of short options in help string

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
For 4.6: pure doc changes, risk free.
---
 tools/xenstore/xenstored_core.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 87cb715..25a548d 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1886,21 +1886,21 @@ static void usage(void)
 "\n"
 "where options may include:\n"
 "\n"
-"  --no-domain-init    to state that xenstored should not initialise dom0,\n"
-"  --pid-file <file>   giving a file for the daemon's pid to be written,\n"
-"  --help              to output this message,\n"
-"  --no-fork           to request that the daemon does not fork,\n"
-"  --output-pid        to request that the pid of the daemon is output,\n"
-"  --trace-file <file> giving the file for logging, and\n"
-"  --entry-nb <nb>     limit the number of entries per domain,\n"
-"  --entry-size <size> limit the size of entry per domain, and\n"
-"  --watch-nb <nb>     limit the number of watches per domain,\n"
-"  --transaction <nb>  limit the number of transaction allowed per domain,\n"
-"  --no-recovery       to request that no recovery should be attempted when\n"
-"                      the store is corrupted (debug only),\n"
-"  --internal-db       store database in memory, not on disk\n"
-"  --preserve-local    to request that /local is preserved on start-up,\n"
-"  --verbose           to request verbose execution.\n");
+"  -D, --no-domain-init    to state that xenstored should not initialise dom0,\n"
+"  -F, --pid-file <file>   giving a file for the daemon's pid to be written,\n"
+"  -H, --help              to output this message,\n"
+"  -N, --no-fork           to request that the daemon does not fork,\n"
+"  -P, --output-pid        to request that the pid of the daemon is output,\n"
+"  -T, --trace-file <file> giving the file for logging, and\n"
+"  -E, --entry-nb <nb>     limit the number of entries per domain,\n"
+"  -S, --entry-size <size> limit the size of entry per domain, and\n"
+"  -W, --watch-nb <nb>     limit the number of watches per domain,\n"
+"  -t, --transaction <nb>  limit the number of transaction allowed per domain,\n"
+"  -R, --no-recovery       to request that no recovery should be attempted when\n"
+"                          the store is corrupted (debug only),\n"
+"  -I, --internal-db       store database in memory, not on disk\n"
+"  -L, --preserve-local    to request that /local is preserved on start-up,\n"
+"  -V, --verbose           to request verbose execution.\n");
 }
 
 
-- 
2.1.4

[PATCH for-4.6 v2 3/4] oxenstored: fix systemd socket activation

[Wei Liu]

Use the correct API sd_listen_fds to determine whether the process is
started by systemd.

Change sd_booted to launched_by_systemd to avoid confusion with
systemd's API.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Dave Scott <dave.scott@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---

For 4.6: without this oxenstored is broken when running on a system with
systemd but not started by systemd.

v2: booted -> lanuched, no functional change so I keep Dave's ack.
---
 tools/ocaml/xenstored/systemd.ml      | 2 +-
 tools/ocaml/xenstored/systemd.mli     | 4 ++--
 tools/ocaml/xenstored/systemd_stubs.c | 6 +++---
 tools/ocaml/xenstored/utils.ml        | 2 +-
 tools/ocaml/xenstored/xenstored.ml    | 2 +-
 5 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/tools/ocaml/xenstored/systemd.ml b/tools/ocaml/xenstored/systemd.ml
index 2aa39ea..732446d 100644
--- a/tools/ocaml/xenstored/systemd.ml
+++ b/tools/ocaml/xenstored/systemd.ml
@@ -13,5 +13,5 @@
  *)
 
 external sd_listen_fds: string -> Unix.file_descr = "ocaml_sd_listen_fds"
-external sd_booted: unit -> bool = "ocaml_sd_booted"
+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 85c9f2e..538fc5e 100644
--- a/tools/ocaml/xenstored/systemd.mli
+++ b/tools/ocaml/xenstored/systemd.mli
@@ -17,8 +17,8 @@
  *  us do sanity checks on the expected sockets *)
 val sd_listen_fds: string -> Unix.file_descr
 
-(** Tells us whether or not systemd support was compiled in *)
-val sd_booted: unit -> bool
+(** Tells 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
index d924ff1..1bd5dea 100644
--- a/tools/ocaml/xenstored/systemd_stubs.c
+++ b/tools/ocaml/xenstored/systemd_stubs.c
@@ -92,14 +92,14 @@ CAMLprim value ocaml_sd_listen_fds(value connect_to)
 	CAMLreturn(sock_ret);
 }
 
-CAMLprim value ocaml_sd_booted(value ignore)
+CAMLprim value ocaml_launched_by_systemd(value ignore)
 {
 	CAMLparam1(ignore);
 	CAMLlocal1(ret);
 
 	ret = Val_false;
 
-	if (sd_booted())
+	if (sd_listen_fds(0) > 0)
 		ret = Val_true;
 
 	CAMLreturn(ret);
@@ -129,7 +129,7 @@ CAMLprim value ocaml_sd_listen_fds(value connect_to)
 	CAMLreturn(sock_ret);
 }
 
-CAMLprim value ocaml_sd_booted(value ignore)
+CAMLprim value ocaml_launched_by_systemd(value ignore)
 {
 	CAMLparam1(ignore);
 	CAMLlocal1(ret);
diff --git a/tools/ocaml/xenstored/utils.ml b/tools/ocaml/xenstored/utils.ml
index 61321c6..9f82c1c 100644
--- a/tools/ocaml/xenstored/utils.ml
+++ b/tools/ocaml/xenstored/utils.ml
@@ -84,7 +84,7 @@ let create_regular_unix_socket name =
         sock
 
 let create_unix_socket name =
-        if Systemd.sd_booted() then
+        if Systemd.launched_by_systemd() then
                 Systemd.sd_listen_fds name
         else
                 create_regular_unix_socket name
diff --git a/tools/ocaml/xenstored/xenstored.ml b/tools/ocaml/xenstored/xenstored.ml
index bfe689b..f484024 100644
--- a/tools/ocaml/xenstored/xenstored.ml
+++ b/tools/ocaml/xenstored/xenstored.ml
@@ -431,7 +431,7 @@ let _ =
 	while not !quit
 	do
 		try
-                        if Systemd.sd_booted() then
+                        if Systemd.launched_by_systemd() then
                                 Systemd.sd_notify_ready ();
 			main_loop ()
 		with exc ->
-- 
2.1.4

[PATCH for-4.6 v2 4/4] oxenstored: move sd_notify_ready out of main loop

[Wei Liu]

Oxenstored only needs to notify systemd its readiness state once. Move
sd_notify_ready out of main loop.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Dave Scott <dave.scott@eu.citrix.com>
---
For 4.6: avoid wasting CPU cycles, easy to reason its correctness.

There is a small risk that either I wrote the wrong code or I
misunderstand the usage of systemd API. However I've tested the modified
oxenstored it worked fine.
---
 tools/ocaml/xenstored/xenstored.ml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/ocaml/xenstored/xenstored.ml b/tools/ocaml/xenstored/xenstored.ml
index f484024..42b8183 100644
--- a/tools/ocaml/xenstored/xenstored.ml
+++ b/tools/ocaml/xenstored/xenstored.ml
@@ -428,11 +428,11 @@ let _ =
 		process_domains store cons domains
 		in
 
+	if Systemd.launched_by_systemd () then
+		Systemd.sd_notify_ready ();
 	while not !quit
 	do
 		try
-                        if Systemd.launched_by_systemd() then
-                                Systemd.sd_notify_ready ();
 			main_loop ()
 		with exc ->
 			error "caught exception %s" (Printexc.to_string exc);
-- 
2.1.4

Re: [PATCH for-4.6 v2 4/4] oxenstored: move sd_notify_ready out of main loop

[Andrew Cooper]

On 10/08/2015 09:00, Wei Liu wrote:
> Oxenstored only needs to notify systemd its readiness state once. Move
> sd_notify_ready out of main loop.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Acked-by: Dave Scott <dave.scott@eu.citrix.com>
> ---
> For 4.6: avoid wasting CPU cycles, easy to reason its correctness.
>
> There is a small risk that either I wrote the wrong code or I
> misunderstand the usage of systemd API. However I've tested the modified
> oxenstored it worked fine.
> ---
>  tools/ocaml/xenstored/xenstored.ml | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/ocaml/xenstored/xenstored.ml b/tools/ocaml/xenstored/xenstored.ml
> index f484024..42b8183 100644
> --- a/tools/ocaml/xenstored/xenstored.ml
> +++ b/tools/ocaml/xenstored/xenstored.ml
> @@ -428,11 +428,11 @@ let _ =
>  		process_domains store cons domains
>  		in
>  
> +	if Systemd.launched_by_systemd () then
> +		Systemd.sd_notify_ready ();
>  	while not !quit
>  	do
>  		try
> -                        if Systemd.launched_by_systemd() then
> -                                Systemd.sd_notify_ready ();

You have tabs/spaces issues here.

However, the two oxenstored patches are Tested-by: Andrew Cooper
<andrew.cooper3@citrix.com>.  XenServer testing over the weekend has
shown no regressions.

~Andrew

Re: [PATCH for-4.6 v2 4/4] oxenstored: move sd_notify_ready out of main loop

[Wei Liu]

On Mon, Aug 10, 2015 at 09:04:20AM +0100, Andrew Cooper wrote:
> On 10/08/2015 09:00, Wei Liu wrote:
> > Oxenstored only needs to notify systemd its readiness state once. Move
> > sd_notify_ready out of main loop.
> >
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > Acked-by: Dave Scott <dave.scott@eu.citrix.com>
> > ---
> > For 4.6: avoid wasting CPU cycles, easy to reason its correctness.
> >
> > There is a small risk that either I wrote the wrong code or I
> > misunderstand the usage of systemd API. However I've tested the modified
> > oxenstored it worked fine.
> > ---
> >  tools/ocaml/xenstored/xenstored.ml | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/ocaml/xenstored/xenstored.ml b/tools/ocaml/xenstored/xenstored.ml
> > index f484024..42b8183 100644
> > --- a/tools/ocaml/xenstored/xenstored.ml
> > +++ b/tools/ocaml/xenstored/xenstored.ml
> > @@ -428,11 +428,11 @@ let _ =
> >  		process_domains store cons domains
> >  		in
> >  
> > +	if Systemd.launched_by_systemd () then
> > +		Systemd.sd_notify_ready ();
> >  	while not !quit
> >  	do
> >  		try
> > -                        if Systemd.launched_by_systemd() then
> > -                                Systemd.sd_notify_ready ();
> 
> You have tabs/spaces issues here.
> 

Yeah. I know that. But that's what it used to be, not introduced by me.
Furthermore, it's removal, not addition, so I didn't bother sending out
another patch to adjust that.

> However, the two oxenstored patches are Tested-by: Andrew Cooper
> <andrew.cooper3@citrix.com>.  XenServer testing over the weekend has
> shown no regressions.
> 

Thanks.

Wei.

> ~Andrew

Re: [PATCH for-4.6 v2 0/4] Patches for c/oxenstored

[Ian Campbell]

On Mon, 2015-08-10 at 09:00 +0100, Wei Liu wrote:
> Wei Liu (4):
>   cxenstored: fix systemd socket activation
>   cxenstored: document a bunch of short options in help string
>   oxenstored: fix systemd socket activation
>   oxenstored: move sd_notify_ready out of main loop

Applied.

> 
>  tools/ocaml/xenstored/systemd.ml      |  2 +-
>  tools/ocaml/xenstored/systemd.mli     |  4 +--
>  tools/ocaml/xenstored/systemd_stubs.c |  6 ++--
>  tools/ocaml/xenstored/utils.ml        |  2 +-
>  tools/ocaml/xenstored/xenstored.ml    |  4 +--
>  tools/xenstore/xenstored_core.c       | 59 +++++++++++++++++++++--------
> ------
>  6 files changed, 44 insertions(+), 33 deletions(-)
>