[libvirt] [PATCH] systemd: directly notify systemd instead of using sd_notify
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/InterfacePortabilityAndSta... Signed-off-by: Daniel P. Berrange <berrange@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/configure.ac b/configure.ac index 74c33b3..a63b912 100644 --- a/configure.ac +++ b/configure.ac @@ -256,7 +256,6 @@ LIBVIRT_CHECK_SANLOCK LIBVIRT_CHECK_SASL LIBVIRT_CHECK_SELINUX LIBVIRT_CHECK_SSH2 -LIBVIRT_CHECK_SYSTEMD_DAEMON LIBVIRT_CHECK_UDEV LIBVIRT_CHECK_WIRESHARK LIBVIRT_CHECK_NSS @@ -2787,7 +2786,6 @@ LIBVIRT_RESULT_SANLOCK LIBVIRT_RESULT_SASL LIBVIRT_RESULT_SELINUX LIBVIRT_RESULT_SSH2 -LIBVIRT_RESULT_SYSTEMD_DAEMON LIBVIRT_RESULT_UDEV LIBVIRT_RESULT_WIRESHARK LIBVIRT_RESULT_NSS diff --git a/libvirt.spec.in b/libvirt.spec.in index 8b88eef..b93a53c 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -79,7 +79,6 @@ %define with_firewalld 0%{!?_without_firewalld:0} %define with_libssh2 0%{!?_without_libssh2:0} %define with_wireshark 0%{!?_without_wireshark:0} -%define with_systemd_daemon 0%{!?_without_systemd_daemon:0} %define with_pm_utils 1 # Finally set the OS / architecture specific special cases @@ -133,7 +132,6 @@ # Fedora has systemd, libvirt still used sysvinit there. %if 0%{?fedora} || 0%{?rhel} >= 7 %define with_systemd 1 - %define with_systemd_daemon 1 %define with_pm_utils 0 %endif @@ -268,9 +266,6 @@ BuildRequires: python %if %{with_systemd} BuildRequires: systemd-units %endif -%if %{with_systemd_daemon} -BuildRequires: systemd-devel -%endif %if %{with_xen} || %{with_libxl} BuildRequires: xen-devel %endif @@ -1061,12 +1056,6 @@ rm -rf .git %define arg_wireshark --without-wireshark-dissector %endif -%if %{with_systemd_daemon} - %define arg_systemd_daemon --with-systemd-daemon -%else - %define arg_systemd_daemon --without-systemd-daemon -%endif - %if %{with_pm_utils} %define arg_pm_utils --with-pm-utils %else @@ -1157,7 +1146,6 @@ rm -f po/stamp-po --with-driver-modules \ %{?arg_firewalld} \ %{?arg_wireshark} \ - %{?arg_systemd_daemon} \ %{?arg_pm_utils} \ --with-nss-plugin \ %{arg_packager} \ diff --git a/m4/virt-systemd-daemon.m4 b/m4/virt-systemd-daemon.m4 deleted file mode 100644 index 8516e41..0000000 --- a/m4/virt-systemd-daemon.m4 +++ /dev/null @@ -1,34 +0,0 @@ -dnl The libsystemd-daemon.so library -dnl -dnl Copyright (C) 2012-2013 Red Hat, Inc. -dnl -dnl This library is free software; you can redistribute it and/or -dnl modify it under the terms of the GNU Lesser General Public -dnl License as published by the Free Software Foundation; either -dnl version 2.1 of the License, or (at your option) any later version. -dnl -dnl This library is distributed in the hope that it will be useful, -dnl but WITHOUT ANY WARRANTY; without even the implied warranty of -dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU -dnl Lesser General Public License for more details. -dnl -dnl You should have received a copy of the GNU Lesser General Public -dnl License along with this library. If not, see -dnl <http://www.gnu.org/licenses/>. -dnl - -AC_DEFUN([LIBVIRT_CHECK_SYSTEMD_DAEMON],[ - LIBVIRT_CHECK_PKG([SYSTEMD_DAEMON], [libsystemd-daemon], [0.27.1]) - - old_CFLAGS="$CFLAGS" - old_LIBS="$LIBS" - CFLAGS="$CFLAGS $SYSTEMD_DAEMON_CFLAGS" - LIBS="$LIBS $SYSTEMD_DAEMON_LIBS" - AC_CHECK_FUNCS([sd_notify]) - CFLAGS="$old_CFLAGS" - LIBS="$old_LIBS" -]) - -AC_DEFUN([LIBVIRT_RESULT_SYSTEMD_DAEMON],[ - LIBVIRT_RESULT_LIB([SYSTEMD_DAEMON]) -]) diff --git a/src/Makefile.am b/src/Makefile.am index 12b66c2..11c79df 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1106,12 +1106,12 @@ libvirt_util_la_SOURCES = \ libvirt_util_la_CFLAGS = $(CAPNG_CFLAGS) $(YAJL_CFLAGS) $(LIBNL_CFLAGS) \ $(AM_CFLAGS) $(AUDIT_CFLAGS) $(DEVMAPPER_CFLAGS) \ $(DBUS_CFLAGS) $(LDEXP_LIBM) $(NUMACTL_CFLAGS) \ - $(SYSTEMD_DAEMON_CFLAGS) $(POLKIT_CFLAGS) $(GNUTLS_CFLAGS) \ + $(POLKIT_CFLAGS) $(GNUTLS_CFLAGS) \ -I$(srcdir)/conf libvirt_util_la_LIBADD = $(CAPNG_LIBS) $(YAJL_LIBS) $(LIBNL_LIBS) \ $(THREAD_LIBS) $(AUDIT_LIBS) $(DEVMAPPER_LIBS) \ $(LIB_CLOCK_GETTIME) $(DBUS_LIBS) $(MSCOM_LIBS) $(LIBXML_LIBS) \ - $(SECDRIVER_LIBS) $(NUMACTL_LIBS) $(SYSTEMD_DAEMON_LIBS) \ + $(SECDRIVER_LIBS) $(NUMACTL_LIBS) \ $(POLKIT_LIBS) 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 @@ -21,8 +21,9 @@ #include <config.h> -#ifdef WITH_SYSTEMD_DAEMON -# include <systemd/sd-daemon.h> +#include <sys/socket.h> +#ifdef HAVE_SYS_UN_H +# include <sys/un.h> #endif #include "virsystemd.h" @@ -34,6 +35,7 @@ #include "virutil.h" #include "virlog.h" #include "virerror.h" +#include "virfile.h" #define VIR_FROM_THIS VIR_FROM_SYSTEMD @@ -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)) { + VIR_WARN("Systemd notify socket path '%s' too long", path); + return; + } + + fd = socket(AF_UNIX, SOCK_DGRAM, 0); + if (fd < 0) { + VIR_WARN("Unable to create socket FD"); + return; + } + + ignore_value(virStrcpy(un.sun_path, path, sizeof(un.sun_path))); + if (un.sun_path[0] == '@') + un.sun_path[0] = '\0'; + + if (sendmsg(fd, &mh, MSG_NOSIGNAL) < 0) + VIR_WARN("Failed to notify systemd"); + + VIR_FORCE_CLOSE(fd); +#endif /* HAVE_SYS_UN_H */ } static int -- 2.5.5
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/InterfacePortabilityAndSta...
Signed-off-by: Daniel P. Berrange <berrange@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
+ VIR_WARN("Systemd notify socket path '%s' too long", path); + return; + }
ACK
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/InterfacePortabilityAndSta...
Signed-off-by: Daniel P. Berrange <berrange@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 :-)
+ VIR_WARN("Systemd notify socket path '%s' too long", path); + return; + }
ACK
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 :|
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/InterfacePortabilityAndSta...
Signed-off-by: Daniel P. Berrange <berrange@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.
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/InterfacePortabilityAndSta...
Signed-off-by: Daniel P. Berrange <berrange@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 :|
participants (2)
-
Daniel P. Berrange -
Peter Krempa