From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v7 2/8] cxenstored: add support for systemd active sockets Date: Thu, 6 Aug 2015 10:13:16 +0100 Message-ID: <1438852396.9747.123.camel@citrix.com> References: <1405639699-13494-1-git-send-email-mcgrof@do-not-panic.com> <1405639699-13494-3-git-send-email-mcgrof@do-not-panic.com> <1438769878.9747.47.camel@citrix.com> <20150805131758.GM26074@zion.uk.xensource.com> <20150805172437.GA15719@zion.uk.xensource.com> <20150805181920.GB15719@zion.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZNHEo-0002i9-46 for xen-devel@lists.xenproject.org; Thu, 06 Aug 2015 09:13:22 +0000 In-Reply-To: <20150805181920.GB15719@zion.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Wei Liu , George Dunlap Cc: xen-devel , "Luis R. Rodriguez" , "Luis R. Rodriguez" List-Id: xen-devel@lists.xenproject.org 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 > 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 > --- > 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.