From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Luis R. Rodriguez" Subject: Re: [PATCH v7 2/8] cxenstored: add support for systemd active sockets Date: Sat, 26 Jul 2014 00:45:19 +0200 Message-ID: <20140725224519.GC4191@wotan.suse.de> References: <1405639699-13494-1-git-send-email-mcgrof@do-not-panic.com> <1405639699-13494-3-git-send-email-mcgrof@do-not-panic.com> <1406214613.25535.0.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1XAoEs-00050b-Ok for xen-devel@lists.xenproject.org; Fri, 25 Jul 2014 22:45:22 +0000 Content-Disposition: inline In-Reply-To: <1406214613.25535.0.camel@kazak.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: Ian Campbell Cc: xen-devel@lists.xenproject.org, "Luis R. Rodriguez" List-Id: xen-devel@lists.xenproject.org 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" > > > > 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 > > --- > > 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 > > #include > > #include > > +#include > > > > #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 > > +#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