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

[Ian Campbell <ian.campbell@citrix.com>]

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

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

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

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

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

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

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

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

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

Ian.