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

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

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@kazak.uk.xensource.com>?
> > > 
> > > 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. 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.

  Luis