[libvirt] [PATCH] systemd: directly notify systemd instead of using sd_notify

[Daniel P. Berrange]

On Mon, Jun 06, 2016 at 04:59:24PM +0200, Peter Krempa wrote:
> On Mon, Jun 06, 2016 at 15:50:46 +0100, Daniel Berrange wrote:
> > On Mon, Jun 06, 2016 at 04:46:50PM +0200, Peter Krempa wrote:
> > > On Mon, Jun 06, 2016 at 15:16:50 +0100, Daniel Berrange wrote:
> > > > The sd_notify method is used to tell systemd when libvirtd
> > > > has finished starting up. All it does is send a datagram
> > > > containing the string parameter to systemd on a UNIX socket
> > > > named in the NOTIFY_SOCKET environment variable. Rather than
> > > > pulling in the systemd libraries for this, just code the
> > > > notification directly in libvirt as this is a stable ABI
> > > > from systemd's POV which explicitly allows independant
> > > > implementations:
> > > > 
> > > > See "Reimplementable Independently" column in the
> > > > "$NOTIFY_SOCKET Daemon Notifications" row:
> > > > 
> > > > https://www.freedesktop.org/wiki/Software/systemd/InterfacePortabilityAndStabilityChart/
> > > > 
> > > > Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> > > > ---
> > > >  configure.ac              |  2 --
> > > >  libvirt.spec.in           | 12 -----------
> > > >  m4/virt-systemd-daemon.m4 | 34 -------------------------------
> > > >  src/Makefile.am           |  4 ++--
> > > >  src/util/virsystemd.c     | 52 ++++++++++++++++++++++++++++++++++++++++++-----
> > > >  5 files changed, 49 insertions(+), 55 deletions(-)
> > > >  delete mode 100644 m4/virt-systemd-daemon.m4
> > > 
> > > [...]
> > > 
> > > > diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c
> > > > index 4883f94..71b8f33 100644
> > > > --- a/src/util/virsystemd.c
> > > > +++ b/src/util/virsystemd.c
> > > 
> > > [...]
> > > 
> > > > @@ -480,9 +482,49 @@ int virSystemdTerminateMachine(const char *name)
> > > >  void
> > > >  virSystemdNotifyStartup(void)
> > > >  {
> > > > -#ifdef WITH_SYSTEMD_DAEMON
> > > > -    sd_notify(0, "READY=1");
> > > > -#endif
> > > > +#ifdef HAVE_SYS_UN_H
> > > > +    const char *path;
> > > > +    const char *msg = "READY=1";
> > > > +    int fd;
> > > > +    struct sockaddr_un un = {
> > > > +        .sun_family = AF_UNIX,
> > > > +    };
> > > > +    struct iovec iov = {
> > > > +        .iov_base = (char *)msg,
> > > > +        .iov_len = strlen(msg),
> > > > +    };
> > > > +    struct msghdr mh = {
> > > > +        .msg_name = &un,
> > > > +        .msg_namelen = sizeof(un),
> > > > +        .msg_iov = &iov,
> > > > +        .msg_iovlen = 1,
> > > > +    };
> > > > +
> > > > +    if (!(path = virGetEnvBlockSUID("NOTIFY_SOCKET"))) {
> > > > +        VIR_DEBUG("Skipping systemd notify, not requested");
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    if (strlen(path) > sizeof(un.sun_path)) {
> > > 
> > > Off-by-one by not considering the trailing \0
> > 
> > UNIX socket addresses are *not* NUL-terminated strings. The full
> > 108 bytes in the 'sun_path' field are considered to be the path.
> > So even if you do have a NUL in your string, everything following
> > it is also considered part of the address - its why its important
> > to ensure the sun_path is set to all-zeros :-)
> 
> Ah. I learned something at least.
> 
> But then there's still a problem as virStrcpy doesn't know about this
> unix socket quirk and it subtracts '1' from the third argument before
> comparing it with the length of the second argument and won't copy
> anything in the corner case if the path has exactly 108 bytes.

Ok, I'll repost using memcpy() to make it clearer that we're really
not trying to create a NUL-terminated string.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|