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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

Sorry I missed some inline comments.

On Thu, Aug 06, 2015 at 10:13:16AM +0100, Ian Campbell wrote:
> > ---8<---
> > From a03eba6e258d8097b974366abb50b39af9e9abbf Mon Sep 17 00:00:00 2001
> > From: Wei Liu <wei.liu2@xxxxxxxxxx>
> > 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@xxxxxxxxxx>
> > ---
> >  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?
> 

I don't think so. Though the doc is not clear on how we should use those
APIs, I got my idea from 

http://0pointer.de/public/cups-patch-core.txt

which doesn't call sd_booted.

> 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.
> 

In that case I *think* sd_listen_fds should return 0 which makes this
function exits before this line.

Wei.

> Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel