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

[George Dunlap <george.dunlap@citrix.com>]

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

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

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

Thanks,
 -George

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