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

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

On Wed, May 21, 2014 at 05:39:31PM +0100, Ian Campbell wrote:
> On Wed, 2014-05-21 at 18:24 +0200, Luis R. Rodriguez wrote:
> > On Wed, May 21, 2014 at 03:35:19PM +0100, Ian Campbell wrote:
> > > On Tue, 2014-05-20 at 05:31 -0700, Luis R. Rodriguez wrote:
> > > > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> > > > 
> > > > This adds support for systemd into libxenstore.so to enable usage
> > > > through cxenstored and oxenstored. The way we provide support for
> > > > systemd is to *not* compile systemd with -lsystemd-daemon but instead
> > > > to look for libsystemd-daemon.so at run time if the binary was compiled
> > > > with support for systemd.
> > > 
> > > This is not what either Ian or I intended to suggest. It is perfectly
> > > fine for the binary to be dynamically linked against -lsystemd-daemon in
> > > the normal way (and for the resulting binary packages to depend on the
> > > libsystemd-daemon package etc) if the headers etc are present at compile
> > > time.
> > > 
> > > What we were after was for the actual use of systemd to be runtime not
> > > compile time. IOW it is fine for xenstored to require the library to be
> > > installed, but not that systemd be the init which is in use.
> > 
> > In order to work dynamically *and* automatically at run time, that is
> > to let the binary figure out what is best, the dynamic linker was the
> > best choice. The reason is that we have to consider a binary that
> > can run on systems that do not have the systemd libraries present,
> 
> This is not a requirement. If the systemd libraries were present at
> build time then it is acceptable to require them at runtime even if
> systemd is not actually in use. (Of course other than he
> "is_systemd_being_used" function nothing in the library really ought to
> get called in this case)

It was not clear that this was not a requirement and hence the use of
dlopen() and dlsym(). Now that that work is done though up to you guys
to decide what you want, this option works now, I've tested it, so
just let me know.

I believe the existing use integration provides the most flexibility.

> > or that have them but didn't use sytemd as the init process.
> 
> That case must work but if it required dlopen then something is horribly
> broken somewhere.

If a requirement was not to produce binaries to work on non systemd systems
then yes, otherwise no.

> >  This solution
> > covers all those cases.
> > 
> > > > diff --git a/tools/xenstore/xs_systemd.c b/tools/xenstore/xs_systemd.c
> > > > new file mode 100644
> > > > index 0000000..814e0fc
> > > > --- /dev/null
> > > > +++ b/tools/xenstore/xs_systemd.c
> > > 
> > > > +
> > > > +/*
> > > > + * 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>?
> > 
> > Did you see the mode? Its an example of how different preferences can
> > be tucked away under it, but we can always statically peg associations.
> 
> I have no idea what you are trying to ask/say here...

There are seval uses of the structure I defined, one is the making it
easier to call sd_is_socket_unix() with already prepared data, the other
use case is to chmod() the socket on the path given that while systemd
service unit files does support specifying permissions on the sockets
its a wide set permission setting, so changes between permissions must
be handled manually right now. See the use of chmod on xs_claim_active_socket() 
Its another example of the use of the structure format I laid out instead
of defines.

> > The purpose of the list was also to allow a switch on the type to
> > alternate on sd_is_socket_*() calls but of course if we're not growing
> > the xenstore implementation this means we only have to deal with one
> > type and can use that statically.
> 
> ...or here.
> 
> If socket activation is enabled then both of the things we are passed
> should be unix domain sockets 

It doesn't matter, systemd documentation recommends for a sanity check
on the type of socket given back to us by systemd, and hance the use
of sd_is_socket_unix().

> so I don't understand why you would need
> to "alternate" on anything there either.

What I mean by alternating on different sd_is_socket_*() calls is if we were to
grow xenstored with different type of sockets and make the code a bit more
dynamic. In such case we'd just throw a switch on the active_socket->type and
call the appropriate sd_is_socket_*().

> And if a new thing were added
> to the list then its index would imply that it was whatever type it must
> be, because you'd have written it in the spec.

Even so you are still recommended to call sd_is_socket_*() for the type of
expected socket. What I did tries to make the code easier to grow and
manage.

> > Up to you, just consider the above and let me know.
> 
> The list should go.

OK...

  Luis