From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH v7 2/8] cxenstored: add support for systemd active sockets Date: Thu, 6 Aug 2015 14:56:31 +0100 Message-ID: <55C3678F.4060008@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 1ZNLf1-0008UI-ME for xen-devel@lists.xenproject.org; Thu, 06 Aug 2015 13:56:43 +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" , Ian Campbell , "Luis R. Rodriguez" List-Id: xen-devel@lists.xenproject.org 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 > 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 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 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"); > } >