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

["Luis R. Rodriguez" <mcgrof@suse.com>]

On Thu, Jul 24, 2014 at 04:10:13PM +0100, Ian Campbell wrote:
> 
> On Thu, 2014-07-17 at 16:28 -0700, Luis R. Rodriguez wrote:
> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> > 
> > This adds systemd socket activation support for the C xenstored.
> > Active sockets enable xenstored to be loaded only if required by a system
> > onto which Xen is installed on. Socket activation is handled by
> > systemd, once a port for a service which claims a socket is used
> > systemd will start the required services for it, on demand. For more
> > details on socket activation refer to Lennart's socket-activation
> > post regarding this [0].
> > 
> > Right now this code adds a no-op for this functionality, leaving the
> > enablement to be done later once systemd is properly hooked into
> > the build system. The socket activation is ordered in aligment with
> > the socket activation order passed on to systemd.
> > 
> > [0] http://0pointer.de/blog/projects/socket-activation2.html
> > 
> > Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> > ---
> >  tools/xenstore/xenstored_core.c | 104 +++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 103 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> > index 47f0722..7f72f68 100644
> > --- a/tools/xenstore/xenstored_core.c
> > +++ b/tools/xenstore/xenstored_core.c
> > @@ -40,6 +40,7 @@
> >  #include <signal.h>
> >  #include <assert.h>
> >  #include <setjmp.h>
> > +#include <config.h>
> >  
> >  #include "utils.h"
> >  #include "list.h"
> > @@ -54,6 +55,16 @@
> >  
> >  #include "hashtable.h"
> >  
> > +#ifndef NO_SOCKETS
> > +#if defined(HAVE_SYSTEMD)
> > +#define XEN_SYSTEMD_ENABLED 1
> > +#endif
> > +#endif
> > +
> > +#if defined(XEN_SYSTEMD_ENABLED)
> > +#include <systemd/sd-daemon.h>
> > +#endif
> > +
> >  extern xc_evtchn *xce_handle; /* in xenstored_domain.c */
> >  static int xce_pollfd_idx = -1;
> >  static struct pollfd *fds;
> > @@ -1714,6 +1725,75 @@ static int destroy_fd(void *_fd)
> >  	return 0;
> >  }
> >  
> > +#if defined(XEN_SYSTEMD_ENABLED)
> > +/* Will work regardless of the order systemd gives them to us */
> > +static int xs_get_sd_fd(const char *connect_to)
> > +{
> > +	int fd = SD_LISTEN_FDS_START;
> > +	int r;
> > +
> > +	while (fd <= SD_LISTEN_FDS_START + 1) {
> > +		r = sd_is_socket_unix(fd, SOCK_STREAM, 1, connect_to, 0);
> > +		if (r > 0)
> > +			return fd;
> > +		fd++;
> > +	}
> > +
> > +	return -EBADR;
> > +}
> > +
> > +static int xs_validate_active_socket(const char *connect_to)
> > +{
> > +	if ((strcmp("/var/run/xenstored/socket_ro", connect_to) != 0) &&
> > +	    (strcmp("/var/run/xenstored/socket", connect_to) != 0)) {
> > +		sd_notifyf(0, "STATUS=unexpected socket: %s\n"
> > +			   "ERRNO=%i",
> > +			   connect_to,
> > +			   EBADR);
> > +		return -EBADR;
> > +	}
> > +
> > +	return xs_get_sd_fd(connect_to);
> > +}
> > +
> > +static void xen_claim_active_sockets(int **psock, int **pro_sock)
> > +{
> > +	int *sock, *ro_sock;
> > +	const char *soc_str = xs_daemon_socket();
> > +	const char *soc_str_ro = xs_daemon_socket_ro();
> > +	int n;
> > +
> > +	n = sd_listen_fds(0);
> > +	if (n <= 0) {
> > +		sd_notifyf(0, "STATUS=Failed to get any active sockets: %s\n"
> > +			   "ERRNO=%i",
> > +			   strerror(errno),
> > +			   errno);
> > +		barf_perror("sd_listen_fds() failed\n");
> > +	} else if (n > 2) {
> > +		fprintf(stderr, SD_ERR "Expected 2 fds but given %d\n", n);
> > +		sd_notifyf(0, "STATUS=Mismatch on number (2): %s\n"
> > +			   "ERRNO=%d",
> 
> You've used %u everywhere else...

Sorry this was not clear, I don't see usage of %u in my patches.

> Also, you aren't handling the n == 1 case, is that supposed to be an
> error or not? Either "Expected 2" is wrong or the conditions need to
> differ.

Good catch, yeah that should be an error as well.

> Perhaps you wanted to send n in the STATUS?

There's an fprintf(stderr) right above above that provides the n variables,
when you use fprintf(stderr) that goes the systemd journal as well.

> Anyway, none of that seems super critical so perhaps you'd prefer to
> make whichever changes are appropriate in a followup patch?

Sure.

> (I notice that the same comment seems to apply to the ocaml case in the
> too...)

OK I'll send a follow up.

  Luis