Re: [Xen-devel] [PATCH v5 02/14] libxenstore.so: add support for systemd

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2014-05-21 at 19:15 +0200, Luis R. Rodriguez wrote:
> On Wed, May 21, 2014 at 05:48:37PM +0100, Ian Campbell wrote:
> > On Wed, 2014-05-21 at 18:32 +0200, Luis R. Rodriguez wrote:
> > > On Wed, May 21, 2014 at 03:56:54PM +0100, Ian Campbell wrote:
> > > > On Wed, 2014-05-21 at 15:35 +0100, Ian Campbell wrote:
> > > > 
> > > > > > +
> > > > > > +/*
> > > > > > + * We list stdin, stdout and stderr simply for documentation 
> > > > > > purposes
> > > > > > + * and to help our array size fit the number of expected sockets we
> > > > > > + * as sd_listen_fds() will return 5 for example if you set the 
> > > > > > socket
> > > > > > + * service with 2 sockets.
> > > > > 
> > > > > Please can we get rid of this list (which is bad enough in itself but
> > > > > the three spurious entries are ludicrous) and just
> > > > > #define SOCKET_RW_INDEX SD_LISTEN_FDS_START
> > > > > #define SOCKET_RO_INDEX SDL_LISTEN_FDS_START + 1
> > > > > etc and use those for lookups, as I described in
> > > > > <1399971222.11314.27.camel@xxxxxxxxxxxxxxxxxxxxxx>?
> > > > 
> > > > Also I thought that we decided that the mapping from the indexes here to
> > > > the functionality need to be documented as well, I'm not seeing that, at
> > > > least not based on a skim of the other subject lines in this series.
> > > 
> > > I didn't like the #define approach
> > 
> > The indexes are into the systemd provided environment variable of the
> > fds which it has produced for us. It *must* be documented what
> > xenstored's expectations are wrt what is in that array. Non optional and
> > completely orthogonal to the use of #defines or anything else.
> 
> Sure.
> 
> > >  so wanted to pitch it again under
> > > new usage, I'll be sure to add something once a final approach is
> > > decided.
> > > 
> > > Please note that another reason for tucking things away under a list
> > > and library was to *eventually* remove the different definitions of
> > > these socket's paths.
> > 
> > >  That code is already split up but can be brought
> > > together under libxenstore and for the systemd case the list can be
> > > used to to do an O(1) lookup of the socket path in the list. If we
> > > use just #defines we would just be adding more #defines for other
> > > things other than the file descriptor for systemd, we'd have it also
> > > for mode, and paths. The list therefore seemed more attractive.
> > 
> > I'm afraid non of this makes any sense to me.
> > 
> > If this mad list of paths and indexes scheme is The Systemd Way then
> > please provide references. Otherwise:
> > 
> > systemd case:
> > 
> >         int get_handle(int index)
> >         {
> >            return sd_listen_fds(index);
> 
> That's not the way to use sd_listen_fds(). More on this below.
> 
> >         }
> >         
> > no extra uses of the path or need for #defines for modes or anything
> > like that.
> > 
> > non-systemd case:
> >    switch(index)
> >    case SOCKET_WR_INDEX:
> >         open /var/run/xensotred/socket by whatever means.
> 
> The struct I defined is not something part of systemd, in fact there
> are not many example codes out there that use multiple sockets but the
> approach to use multiple sockets is to actually only call sd_listen_fds()
> once.

OK, so fds = sd_listen_fds() once then return fds[index] above instead
(modulo the checks you want to do).

>  That returns the list of sockets that systemd was configured through
> service units that process should have, systemd would have activated them
> for us, we just then carry them over. We do sanity checks with the number
> returned, so if we expected systemd to have set up 2 sockets for us we
> should check for that number and then *additionally* systemd *does*
> recommend that we validate the type of socket is what we expected and
> hence the usage of sd_is_socket_unix(). The arguments for
> sd_is_socket_unix() is what I set up in a struct mapped by the path.
> This is what xs_claim_active_socket() ends up doing for us.

Passing a NULL path to sd_is_socket_unix() is valid though, which means
you don't need the path in two places (the unit file and here). The
length and listening should both always be 0, so you don't need those
either. The mode for chmod seems strange to me, does systemd not let you
configure that? In any case a table lookup for that one thing seems like
overkill to me, you could put it in the switch for instance.

Ian.


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