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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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.

>  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);
        }
        
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.


Ian.


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