[libvirt PATCH 0/9] src: some improvements to systemd unit files

These were suggested by Lennart in https://gitlab.com/libvirt/libvirt/-/issues/489 Daniel P. Berrangé (9): src: remove After=local-fs.target from systemd units src: remote deps on ip[6]tables/firewalld.service from systemd units util: remove pointless wrappers for setrlimit/getrlimit util: add helper for raising the max files limit rpc: automatically raise max file limit in all daemons src: set max open file limit to match systemd >= 240 defaults util: relax requirement for logind to be running src: remove dep on systemd-logind.service from unit files util: add logging about node suspend availability src/ch/virtchd.service.in | 11 ++--- src/interface/virtinterfaced.service.in | 1 - src/libvirt_private.syms | 1 + src/libxl/virtxend.service.in | 1 - src/locking/virtlockd.service.in | 8 ++-- src/logging/virtlogd.service.in | 11 ++--- src/lxc/virtlxcd.service.in | 11 ++--- src/network/virtnetworkd.service.in | 4 -- src/node_device/virtnodedevd.service.in | 1 - src/nwfilter/virtnwfilterd.service.in | 1 - src/qemu/virtqemud.service.in | 11 ++--- src/remote/libvirtd.service.in | 14 ++----- src/remote/virtproxyd.service.in | 1 - src/rpc/virnetdaemon.c | 3 ++ src/secret/virtsecretd.service.in | 1 - src/storage/virtstoraged.service.in | 1 - src/util/virnodesuspend.c | 3 ++ src/util/virprocess.c | 56 ++++++++++++++++--------- src/util/virprocess.h | 1 + src/util/virstring.c | 6 +++ src/util/virsystemd.c | 12 ++++++ src/vbox/virtvboxd.service.in | 1 - src/vz/virtvzd.service.in | 1 - tests/virshtest.c | 1 + tools/virsh.c | 2 +- 25 files changed, 88 insertions(+), 76 deletions(-) -- 2.40.1

All services are ordered after local-fs.target unless they have set DefaultDependencies=no, which we do not do. https://gitlab.com/libvirt/libvirt/-/issues/489 Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/ch/virtchd.service.in | 1 - src/interface/virtinterfaced.service.in | 1 - src/libxl/virtxend.service.in | 1 - src/lxc/virtlxcd.service.in | 1 - src/network/virtnetworkd.service.in | 1 - src/node_device/virtnodedevd.service.in | 1 - src/nwfilter/virtnwfilterd.service.in | 1 - src/qemu/virtqemud.service.in | 1 - src/remote/libvirtd.service.in | 1 - src/remote/virtproxyd.service.in | 1 - src/secret/virtsecretd.service.in | 1 - src/storage/virtstoraged.service.in | 1 - src/util/virstring.c | 6 ++++++ src/vbox/virtvboxd.service.in | 1 - src/vz/virtvzd.service.in | 1 - 15 files changed, 6 insertions(+), 14 deletions(-) diff --git a/src/ch/virtchd.service.in b/src/ch/virtchd.service.in index a07c04a845..6e3b13446f 100644 --- a/src/ch/virtchd.service.in +++ b/src/ch/virtchd.service.in @@ -8,7 +8,6 @@ Wants=systemd-machined.service After=network.target After=dbus.service After=apparmor.service -After=local-fs.target After=remote-fs.target After=systemd-logind.service After=systemd-machined.service diff --git a/src/interface/virtinterfaced.service.in b/src/interface/virtinterfaced.service.in index 1be3ab32dc..5cb2cd19dc 100644 --- a/src/interface/virtinterfaced.service.in +++ b/src/interface/virtinterfaced.service.in @@ -7,7 +7,6 @@ Requires=virtinterfaced-admin.socket After=network.target After=dbus.service After=apparmor.service -After=local-fs.target Documentation=man:virtinterfaced(8) Documentation=https://libvirt.org diff --git a/src/libxl/virtxend.service.in b/src/libxl/virtxend.service.in index abb1972777..c6a88f7fe9 100644 --- a/src/libxl/virtxend.service.in +++ b/src/libxl/virtxend.service.in @@ -8,7 +8,6 @@ Wants=virtlockd.socket After=network.target After=dbus.service After=apparmor.service -After=local-fs.target After=remote-fs.target After=xencommons.service Conflicts=xendomains.service diff --git a/src/lxc/virtlxcd.service.in b/src/lxc/virtlxcd.service.in index 2623f7375a..06c70ccde2 100644 --- a/src/lxc/virtlxcd.service.in +++ b/src/lxc/virtlxcd.service.in @@ -8,7 +8,6 @@ Wants=systemd-machined.service After=network.target After=dbus.service After=apparmor.service -After=local-fs.target After=remote-fs.target After=systemd-logind.service After=systemd-machined.service diff --git a/src/network/virtnetworkd.service.in b/src/network/virtnetworkd.service.in index 48423e777d..f35cccb8f7 100644 --- a/src/network/virtnetworkd.service.in +++ b/src/network/virtnetworkd.service.in @@ -10,7 +10,6 @@ After=iptables.service After=ip6tables.service After=dbus.service After=apparmor.service -After=local-fs.target Documentation=man:virtnetworkd(8) Documentation=https://libvirt.org diff --git a/src/node_device/virtnodedevd.service.in b/src/node_device/virtnodedevd.service.in index 3ceed30f29..2ac41db32e 100644 --- a/src/node_device/virtnodedevd.service.in +++ b/src/node_device/virtnodedevd.service.in @@ -7,7 +7,6 @@ Requires=virtnodedevd-admin.socket After=network.target After=dbus.service After=apparmor.service -After=local-fs.target Documentation=man:virtnodedevd(8) Documentation=https://libvirt.org diff --git a/src/nwfilter/virtnwfilterd.service.in b/src/nwfilter/virtnwfilterd.service.in index 37fa54d684..d6e98240a8 100644 --- a/src/nwfilter/virtnwfilterd.service.in +++ b/src/nwfilter/virtnwfilterd.service.in @@ -7,7 +7,6 @@ Requires=virtnwfilterd-admin.socket After=network.target After=dbus.service After=apparmor.service -After=local-fs.target Documentation=man:virtnwfilterd(8) Documentation=https://libvirt.org diff --git a/src/qemu/virtqemud.service.in b/src/qemu/virtqemud.service.in index 032cbcbbf0..46917b746d 100644 --- a/src/qemu/virtqemud.service.in +++ b/src/qemu/virtqemud.service.in @@ -10,7 +10,6 @@ Wants=systemd-machined.service After=network.target After=dbus.service After=apparmor.service -After=local-fs.target After=remote-fs.target After=systemd-logind.service After=systemd-machined.service diff --git a/src/remote/libvirtd.service.in b/src/remote/libvirtd.service.in index 11507207a1..b691d35938 100644 --- a/src/remote/libvirtd.service.in +++ b/src/remote/libvirtd.service.in @@ -16,7 +16,6 @@ After=ip6tables.service After=dbus.service After=iscsid.service After=apparmor.service -After=local-fs.target After=remote-fs.target After=systemd-logind.service After=systemd-machined.service diff --git a/src/remote/virtproxyd.service.in b/src/remote/virtproxyd.service.in index dd3bdf3429..9b829641f7 100644 --- a/src/remote/virtproxyd.service.in +++ b/src/remote/virtproxyd.service.in @@ -7,7 +7,6 @@ Requires=virtproxyd-admin.socket After=network.target After=dbus.service After=apparmor.service -After=local-fs.target Documentation=man:virtproxyd(8) Documentation=https://libvirt.org diff --git a/src/secret/virtsecretd.service.in b/src/secret/virtsecretd.service.in index 774cfc3ecd..3804fe553b 100644 --- a/src/secret/virtsecretd.service.in +++ b/src/secret/virtsecretd.service.in @@ -7,7 +7,6 @@ Requires=virtsecretd-admin.socket After=network.target After=dbus.service After=apparmor.service -After=local-fs.target Documentation=man:virtsecretd(8) Documentation=https://libvirt.org diff --git a/src/storage/virtstoraged.service.in b/src/storage/virtstoraged.service.in index e1a1ea6820..235fbc6798 100644 --- a/src/storage/virtstoraged.service.in +++ b/src/storage/virtstoraged.service.in @@ -8,7 +8,6 @@ After=network.target After=dbus.service After=iscsid.service After=apparmor.service -After=local-fs.target After=remote-fs.target Documentation=man:virtstoraged(8) Documentation=https://libvirt.org diff --git a/src/util/virstring.c b/src/util/virstring.c index 6b728ff047..e189b9de31 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -50,6 +50,7 @@ virStrToLong_i(char const *s, char **end_ptr, int base, int *result) errno = 0; val = g_ascii_strtoll(s, &p, base); + g_assert(errno != EAGAIN); err = (errno || (!end_ptr && *p) || p == s || (int) val != val); if (end_ptr) *end_ptr = p; @@ -71,6 +72,7 @@ virStrToLong_ui(char const *s, char **end_ptr, int base, unsigned int *result) errno = 0; val = g_ascii_strtoull(s, &p, base); + g_assert(errno != EAGAIN); /* This one's tricky. We _want_ to allow "-1" as shorthand for * UINT_MAX regardless of whether long is 32-bit or 64-bit. But @@ -103,6 +105,7 @@ virStrToLong_uip(char const *s, char **end_ptr, int base, unsigned int *result) errno = 0; val = g_ascii_strtoull(s, &p, base); + g_assert(errno != EAGAIN); err = (memchr(s, '-', p - s) || errno || (!end_ptr && *p) || p == s || (unsigned int) val != val); if (end_ptr) @@ -160,6 +163,7 @@ virStrToLong_ulp(char const *s, char **end_ptr, int base, errno = 0; val = g_ascii_strtoull(s, &p, base); + g_assert(errno != EAGAIN); err = (memchr(s, '-', p - s) || errno || (!end_ptr && *p) || p == s || (unsigned long) val != val); if (end_ptr) @@ -202,6 +206,7 @@ virStrToLong_ull(char const *s, char **end_ptr, int base, errno = 0; val = g_ascii_strtoull(s, &p, base); + g_assert(errno != EAGAIN); err = (errno || (!end_ptr && *p) || p == s); if (end_ptr) *end_ptr = p; @@ -223,6 +228,7 @@ virStrToLong_ullp(char const *s, char **end_ptr, int base, errno = 0; val = g_ascii_strtoull(s, &p, base); + g_assert(errno != EAGAIN); err = (memchr(s, '-', p - s) || errno || (!end_ptr && *p) || p == s); if (end_ptr) diff --git a/src/vbox/virtvboxd.service.in b/src/vbox/virtvboxd.service.in index e73206591a..a567ed2443 100644 --- a/src/vbox/virtvboxd.service.in +++ b/src/vbox/virtvboxd.service.in @@ -7,7 +7,6 @@ Requires=virtvboxd-admin.socket After=network.target After=dbus.service After=apparmor.service -After=local-fs.target After=remote-fs.target Documentation=man:virtvboxd(8) Documentation=https://libvirt.org diff --git a/src/vz/virtvzd.service.in b/src/vz/virtvzd.service.in index bd98d96262..5521e89e10 100644 --- a/src/vz/virtvzd.service.in +++ b/src/vz/virtvzd.service.in @@ -7,7 +7,6 @@ Requires=virtvzd-admin.socket After=network.target After=dbus.service After=apparmor.service -After=local-fs.target After=remote-fs.target Documentation=man:virtvzd(8) Documentation=https://libvirt.org -- 2.40.1

On Wed, Jun 21, 2023 at 14:32:24 +0100, Daniel P. Berrangé wrote:
All services are ordered after local-fs.target unless they have set DefaultDependencies=no, which we do not do.
https://gitlab.com/libvirt/libvirt/-/issues/489 Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/ch/virtchd.service.in | 1 - src/interface/virtinterfaced.service.in | 1 - src/libxl/virtxend.service.in | 1 - src/lxc/virtlxcd.service.in | 1 - src/network/virtnetworkd.service.in | 1 - src/node_device/virtnodedevd.service.in | 1 - src/nwfilter/virtnwfilterd.service.in | 1 - src/qemu/virtqemud.service.in | 1 - src/remote/libvirtd.service.in | 1 - src/remote/virtproxyd.service.in | 1 - src/secret/virtsecretd.service.in | 1 - src/storage/virtstoraged.service.in | 1 - src/util/virstring.c | 6 ++++++ src/vbox/virtvboxd.service.in | 1 - src/vz/virtvzd.service.in | 1 - 15 files changed, 6 insertions(+), 14 deletions(-)
diff --git a/src/util/virstring.c b/src/util/virstring.c index 6b728ff047..e189b9de31 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -50,6 +50,7 @@ virStrToLong_i(char const *s, char **end_ptr, int base, int *result)
errno = 0; val = g_ascii_strtoll(s, &p, base); + g_assert(errno != EAGAIN); err = (errno || (!end_ptr && *p) || p == s || (int) val != val); if (end_ptr) *end_ptr = p; @@ -71,6 +72,7 @@ virStrToLong_ui(char const *s, char **end_ptr, int base, unsigned int *result)
errno = 0; val = g_ascii_strtoull(s, &p, base); + g_assert(errno != EAGAIN);
/* This one's tricky. We _want_ to allow "-1" as shorthand for * UINT_MAX regardless of whether long is 32-bit or 64-bit. But @@ -103,6 +105,7 @@ virStrToLong_uip(char const *s, char **end_ptr, int base, unsigned int *result)
errno = 0; val = g_ascii_strtoull(s, &p, base); + g_assert(errno != EAGAIN); err = (memchr(s, '-', p - s) || errno || (!end_ptr && *p) || p == s || (unsigned int) val != val); if (end_ptr) @@ -160,6 +163,7 @@ virStrToLong_ulp(char const *s, char **end_ptr, int base,
errno = 0; val = g_ascii_strtoull(s, &p, base); + g_assert(errno != EAGAIN); err = (memchr(s, '-', p - s) || errno || (!end_ptr && *p) || p == s || (unsigned long) val != val); if (end_ptr) @@ -202,6 +206,7 @@ virStrToLong_ull(char const *s, char **end_ptr, int base,
errno = 0; val = g_ascii_strtoull(s, &p, base); + g_assert(errno != EAGAIN); err = (errno || (!end_ptr && *p) || p == s); if (end_ptr) *end_ptr = p; @@ -223,6 +228,7 @@ virStrToLong_ullp(char const *s, char **end_ptr, int base,
errno = 0; val = g_ascii_strtoull(s, &p, base); + g_assert(errno != EAGAIN); err = (memchr(s, '-', p - s) || errno || (!end_ptr && *p) || p == s); if (end_ptr)
You've accidentally added some debugging code to this patch. For the systemd stuff: Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On Thu, Jun 22, 2023 at 09:27:48AM +0200, Peter Krempa wrote:
On Wed, Jun 21, 2023 at 14:32:24 +0100, Daniel P. Berrangé wrote:
All services are ordered after local-fs.target unless they have set DefaultDependencies=no, which we do not do.
https://gitlab.com/libvirt/libvirt/-/issues/489 Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/ch/virtchd.service.in | 1 - src/interface/virtinterfaced.service.in | 1 - src/libxl/virtxend.service.in | 1 - src/lxc/virtlxcd.service.in | 1 - src/network/virtnetworkd.service.in | 1 - src/node_device/virtnodedevd.service.in | 1 - src/nwfilter/virtnwfilterd.service.in | 1 - src/qemu/virtqemud.service.in | 1 - src/remote/libvirtd.service.in | 1 - src/remote/virtproxyd.service.in | 1 - src/secret/virtsecretd.service.in | 1 - src/storage/virtstoraged.service.in | 1 - src/util/virstring.c | 6 ++++++ src/vbox/virtvboxd.service.in | 1 - src/vz/virtvzd.service.in | 1 - 15 files changed, 6 insertions(+), 14 deletions(-)
diff --git a/src/util/virstring.c b/src/util/virstring.c index 6b728ff047..e189b9de31 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -50,6 +50,7 @@ virStrToLong_i(char const *s, char **end_ptr, int base, int *result)
errno = 0; val = g_ascii_strtoll(s, &p, base); + g_assert(errno != EAGAIN); err = (errno || (!end_ptr && *p) || p == s || (int) val != val); if (end_ptr) *end_ptr = p; @@ -71,6 +72,7 @@ virStrToLong_ui(char const *s, char **end_ptr, int base, unsigned int *result)
errno = 0; val = g_ascii_strtoull(s, &p, base); + g_assert(errno != EAGAIN);
/* This one's tricky. We _want_ to allow "-1" as shorthand for * UINT_MAX regardless of whether long is 32-bit or 64-bit. But @@ -103,6 +105,7 @@ virStrToLong_uip(char const *s, char **end_ptr, int base, unsigned int *result)
errno = 0; val = g_ascii_strtoull(s, &p, base); + g_assert(errno != EAGAIN); err = (memchr(s, '-', p - s) || errno || (!end_ptr && *p) || p == s || (unsigned int) val != val); if (end_ptr) @@ -160,6 +163,7 @@ virStrToLong_ulp(char const *s, char **end_ptr, int base,
errno = 0; val = g_ascii_strtoull(s, &p, base); + g_assert(errno != EAGAIN); err = (memchr(s, '-', p - s) || errno || (!end_ptr && *p) || p == s || (unsigned long) val != val); if (end_ptr) @@ -202,6 +206,7 @@ virStrToLong_ull(char const *s, char **end_ptr, int base,
errno = 0; val = g_ascii_strtoull(s, &p, base); + g_assert(errno != EAGAIN); err = (errno || (!end_ptr && *p) || p == s); if (end_ptr) *end_ptr = p; @@ -223,6 +228,7 @@ virStrToLong_ullp(char const *s, char **end_ptr, int base,
errno = 0; val = g_ascii_strtoull(s, &p, base); + g_assert(errno != EAGAIN); err = (memchr(s, '-', p - s) || errno || (!end_ptr && *p) || p == s); if (end_ptr)
You've accidentally added some debugging code to this patch.
Doh yes, this was leftover from debugging the problem with glib mutexes splattering errno.
For the systemd stuff:
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

The unit files both have After=network.target, and this in turn implies After=network-pre.target. Both iptables.service & ip6tables.service have Before=network-pre.target since Fedora >= 35 and RHEL >= 8.4. When we first added the deps on ip[6]tables.service in commit 0756415f147dda15a417bd79eef9a62027d176e6 Author: Laine Stump <laine@redhat.com> Date: Fri May 1 00:05:50 2020 -0400 systemd: start libvirtd after firewalld/iptables services the Before=network-pre.target didn't exist, but we can rely on it now given our supported platforms matrix. The firewalld.service has similarly has a Before=network-pre.target, even when we took that commit above, so this dep was in face never actually needed. This answers the question posed in that above commit message about firewalld ordering. https://gitlab.com/libvirt/libvirt/-/issues/489 Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/network/virtnetworkd.service.in | 3 --- src/remote/libvirtd.service.in | 3 --- 2 files changed, 6 deletions(-) diff --git a/src/network/virtnetworkd.service.in b/src/network/virtnetworkd.service.in index f35cccb8f7..3d7374715d 100644 --- a/src/network/virtnetworkd.service.in +++ b/src/network/virtnetworkd.service.in @@ -5,9 +5,6 @@ Requires=virtnetworkd.socket Requires=virtnetworkd-ro.socket Requires=virtnetworkd-admin.socket After=network.target -After=firewalld.service -After=iptables.service -After=ip6tables.service After=dbus.service After=apparmor.service Documentation=man:virtnetworkd(8) diff --git a/src/remote/libvirtd.service.in b/src/remote/libvirtd.service.in index b691d35938..afda257228 100644 --- a/src/remote/libvirtd.service.in +++ b/src/remote/libvirtd.service.in @@ -10,9 +10,6 @@ Wants=libvirtd-admin.socket Wants=virtlockd.socket Wants=systemd-machined.service After=network.target -After=firewalld.service -After=iptables.service -After=ip6tables.service After=dbus.service After=iscsid.service After=apparmor.service -- 2.40.1

On Wed, Jun 21, 2023 at 14:32:25 +0100, Daniel P. Berrangé wrote:
The unit files both have After=network.target, and this in turn implies After=network-pre.target. Both iptables.service & ip6tables.service have Before=network-pre.target since Fedora >= 35 and RHEL >= 8.4.
Is this also true for ubuntu 20.04?
When we first added the deps on ip[6]tables.service in
commit 0756415f147dda15a417bd79eef9a62027d176e6 Author: Laine Stump <laine@redhat.com> Date: Fri May 1 00:05:50 2020 -0400
systemd: start libvirtd after firewalld/iptables services
the Before=network-pre.target didn't exist, but we can rely on it now given our supported platforms matrix.
The firewalld.service has similarly has a Before=network-pre.target, even when we took that commit above, so this dep was in face never actually needed. This answers the question posed in that above commit message about firewalld ordering.
https://gitlab.com/libvirt/libvirt/-/issues/489 Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/network/virtnetworkd.service.in | 3 --- src/remote/libvirtd.service.in | 3 --- 2 files changed, 6 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On Thu, Jun 22, 2023 at 04:00:50PM +0200, Peter Krempa wrote:
On Wed, Jun 21, 2023 at 14:32:25 +0100, Daniel P. Berrangé wrote:
The unit files both have After=network.target, and this in turn implies After=network-pre.target. Both iptables.service & ip6tables.service have Before=network-pre.target since Fedora >= 35 and RHEL >= 8.4.
Is this also true for ubuntu 20.04?
The iptables.service / ip6tables.service is a Fedora specific addon, not typically part of other distros. It is glue to the old Fedora sysvinit script for iptables. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

These wrappers added no semantic difference over calling the system function directly. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virprocess.c | 22 ++-------------------- 1 file changed, 2 insertions(+), 20 deletions(-) diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 2fd9b406a1..0462ae8465 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -736,24 +736,6 @@ virProcessPrLimit(pid_t pid G_GNUC_UNUSED, } #endif -#if WITH_GETRLIMIT -static int -virProcessGetRLimit(int resource, - struct rlimit *old_limit) -{ - return getrlimit(resource, old_limit); -} -#endif /* WITH_GETRLIMIT */ - -#if WITH_SETRLIMIT -static int -virProcessSetRLimit(int resource, - const struct rlimit *new_limit) -{ - return setrlimit(resource, new_limit); -} -#endif /* WITH_SETRLIMIT */ - #if WITH_GETRLIMIT static const char* virProcessLimitResourceToLabel(int resource) @@ -876,7 +858,7 @@ virProcessGetLimit(pid_t pid, if (virProcessGetLimitFromProc(pid, resource, old_limit) == 0) return 0; - if (same_process && virProcessGetRLimit(resource, old_limit) == 0) + if (same_process && getrlimit(resource, old_limit) == 0) return 0; return -1; @@ -895,7 +877,7 @@ virProcessSetLimit(pid_t pid, if (virProcessPrLimit(pid, resource, new_limit, NULL) == 0) return 0; - if (same_process && virProcessSetRLimit(resource, new_limit) == 0) + if (same_process && setrlimit(resource, new_limit) == 0) return 0; return -1; -- 2.40.1

On Wed, Jun 21, 2023 at 14:32:26 +0100, Daniel P. Berrangé wrote:
These wrappers added no semantic difference over calling the system function directly.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virprocess.c | 22 ++-------------------- 1 file changed, 2 insertions(+), 20 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

Historically the max files limit for processes has always been 1024, because going beyond this is incompatible with the select() function. None the less most apps these days will use poll() so should not be limited in this way. Since systemd >= 240, the hard limit will be 500k, while the soft limit remains at 1k. Applications which don't use select() should raise their soft limit to match the hard limit during their startup. This function provides a convenient helper to do this limit raising. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virprocess.c | 34 ++++++++++++++++++++++++++++++++++ src/util/virprocess.h | 1 + 3 files changed, 36 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fb7ad9c855..3071dec13a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3171,6 +3171,7 @@ virPortAllocatorSetUsed; # util/virprocess.h virProcessAbort; +virProcessActivateMaxFiles; virProcessExitWithStatus; virProcessGetAffinity; virProcessGetMaxMemLock; diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 0462ae8465..a26683f333 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -1048,6 +1048,35 @@ virProcessSetMaxFiles(pid_t pid, unsigned int files) return 0; } + +void +virProcessActivateMaxFiles(void) +{ + struct rlimit maxfiles = {0}; + + /* + * Ignore errors since we might be inside a container with seccomp + * filters and limits preset to suitable values. + */ + if (getrlimit(RLIMIT_NOFILE, &maxfiles) < 0) { + VIR_DEBUG("Unable to fetch process max files limit: %s", + g_strerror(errno)); + return; + } + + VIR_DEBUG("Initial max files was %llu", (unsigned long long)maxfiles.rlim_cur); + + maxfiles.rlim_cur = maxfiles.rlim_max; + + if (setrlimit(RLIMIT_NOFILE, &maxfiles) < 0) { + VIR_DEBUG("Unable to set process max files limit to %llu: %s", + (unsigned long long)maxfiles.rlim_cur, g_strerror(errno)); + return; + } + + VIR_DEBUG("Raised max files to %llu", (unsigned long long)maxfiles.rlim_cur); +} + #else /* ! (WITH_SETRLIMIT && defined(RLIMIT_NOFILE)) */ int virProcessSetMaxFiles(pid_t pid G_GNUC_UNUSED, @@ -1056,6 +1085,11 @@ virProcessSetMaxFiles(pid_t pid G_GNUC_UNUSED, virReportSystemError(ENOSYS, "%s", _("Not supported on this platform")); return -1; } + +void +virProcessActivateMaxFiles(void) +{ +} #endif /* ! (WITH_SETRLIMIT && defined(RLIMIT_NOFILE)) */ #if WITH_SETRLIMIT && defined(RLIMIT_CORE) diff --git a/src/util/virprocess.h b/src/util/virprocess.h index c18f87e80e..6008cca4af 100644 --- a/src/util/virprocess.h +++ b/src/util/virprocess.h @@ -81,6 +81,7 @@ int virProcessSetMaxMemLock(pid_t pid, unsigned long long bytes) G_NO_INLINE; int virProcessSetMaxProcesses(pid_t pid, unsigned int procs); int virProcessSetMaxFiles(pid_t pid, unsigned int files); int virProcessSetMaxCoreSize(pid_t pid, unsigned long long bytes); +void virProcessActivateMaxFiles(void); int virProcessGetMaxMemLock(pid_t pid, unsigned long long *bytes) G_NO_INLINE; -- 2.40.1

On Wed, Jun 21, 2023 at 14:32:27 +0100, Daniel P. Berrangé wrote:
Historically the max files limit for processes has always been 1024, because going beyond this is incompatible with the select() function. None the less most apps these days will use poll() so should not be limited in this way.
Since systemd >= 240, the hard limit will be 500k, while the soft limit remains at 1k. Applications which don't use select() should raise their soft limit to match the hard limit during their startup.
This function provides a convenient helper to do this limit raising.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virprocess.c | 34 ++++++++++++++++++++++++++++++++++ src/util/virprocess.h | 1 + 3 files changed, 36 insertions(+)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

None of our daemons use select(), so it is safe to raise the max file limit to its maximum on startup. https://gitlab.com/libvirt/libvirt/-/issues/489 Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/rpc/virnetdaemon.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c index 1292faa078..554b8852e4 100644 --- a/src/rpc/virnetdaemon.c +++ b/src/rpc/virnetdaemon.c @@ -33,6 +33,7 @@ #include "virnetserver.h" #include "virgdbus.h" #include "virhash.h" +#include "virprocess.h" #include "virsystemd.h" #define VIR_FROM_THIS VIR_FROM_RPC @@ -151,6 +152,8 @@ virNetDaemonNew(void) dmn->privileged = geteuid() == 0; dmn->autoShutdownInhibitFd = -1; + virProcessActivateMaxFiles(); + if (virEventRegisterDefaultImpl() < 0) goto error; -- 2.40.1

On Wed, Jun 21, 2023 at 14:32:28 +0100, Daniel P. Berrangé wrote:
None of our daemons use select(), so it is safe to raise the max file limit to its maximum on startup.
https://gitlab.com/libvirt/libvirt/-/issues/489 Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/rpc/virnetdaemon.c | 3 +++
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

Since systemd 240, all services get an open file hard limit of 500k, and a soft limit of 1024. This limit means apps are safe to use select() by default which is limited to 1024 FDs. Apps which don't use select() are expected to simply set their soft limit to match the hard limit during startup. With our current unit file settings we've been effectively reducing the max open files we have on most modern systems. https://gitlab.com/libvirt/libvirt/-/issues/489 Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/ch/virtchd.service.in | 9 ++++----- src/locking/virtlockd.service.in | 8 ++++---- src/logging/virtlogd.service.in | 11 ++++------- src/lxc/virtlxcd.service.in | 9 ++++----- src/qemu/virtqemud.service.in | 9 ++++----- src/remote/libvirtd.service.in | 9 ++++----- tests/virshtest.c | 1 + tools/virsh.c | 2 +- 8 files changed, 26 insertions(+), 32 deletions(-) diff --git a/src/ch/virtchd.service.in b/src/ch/virtchd.service.in index 6e3b13446f..22314bc907 100644 --- a/src/ch/virtchd.service.in +++ b/src/ch/virtchd.service.in @@ -22,11 +22,10 @@ ExecStart=@sbindir@/virtchd $VIRTCHD_ARGS ExecReload=/bin/kill -HUP $MAINPID KillMode=process Restart=on-failure -# At least 2 FD per guest (eg ch monitor + ch socket). -# eg if we want to support 4096 guests, we'll typically need 8192 FDs -# If changing this, also consider virtlogd.service & virtlockd.service -# limits which are also related to number of guests -LimitNOFILE=8192 +# Raise hard limits to match behaviour of systemd >= 240. +# During startup, daemon will set soft limit to match hard limit +# per systemd recommendations +LimitNOFile=512000:1024 # The cgroups pids controller can limit the number of tasks started by # the daemon, which can limit the number of domains for some hypervisors. # A conservative default of 8 tasks per guest results in a TasksMax of diff --git a/src/locking/virtlockd.service.in b/src/locking/virtlockd.service.in index 23054369d5..f1792dcb43 100644 --- a/src/locking/virtlockd.service.in +++ b/src/locking/virtlockd.service.in @@ -15,10 +15,10 @@ ExecReload=/bin/kill -USR1 $MAINPID # cause the machine to be fenced (rebooted), so make # sure we discourage OOM killer OOMScoreAdjust=-900 -# Needs to allow for max guests * average disks per guest -# libvirtd.service written to expect 4096 guests, so if we -# allow for 10 disks per guest, we get: -LimitNOFILE=40960 +# Raise hard limits to match behaviour of systemd >= 240. +# During startup, daemon will set soft limit to match hard limit +# per systemd recommendations +LimitNOFile=512000:1024 [Install] Also=virtlockd.socket diff --git a/src/logging/virtlogd.service.in b/src/logging/virtlogd.service.in index e4aecd46a7..cef4053f59 100644 --- a/src/logging/virtlogd.service.in +++ b/src/logging/virtlogd.service.in @@ -15,13 +15,10 @@ ExecReload=/bin/kill -USR1 $MAINPID # cause the machine to be fenced (rebooted), so make # sure we discourage OOM killer OOMScoreAdjust=-900 -# Need to have at least one file open per guest (eg QEMU -# stdio log), but might be more (eg serial console logs) -# A common case is OpenStack which often has up to 4 file -# handles per guest. -# libvirtd.service written to expect 4096 guests, so if we -# guess at 4 files per guest here that is 16k: -LimitNOFILE=16384 +# Raise hard limits to match behaviour of systemd >= 240. +# During startup, daemon will set soft limit to match hard limit +# per systemd recommendations +LimitNOFile=512000:1024 [Install] Also=virtlogd.socket diff --git a/src/lxc/virtlxcd.service.in b/src/lxc/virtlxcd.service.in index 06c70ccde2..59d7d26657 100644 --- a/src/lxc/virtlxcd.service.in +++ b/src/lxc/virtlxcd.service.in @@ -22,11 +22,10 @@ ExecStart=@sbindir@/virtlxcd $VIRTLXCD_ARGS ExecReload=/bin/kill -HUP $MAINPID KillMode=process Restart=on-failure -# At least 1 FD per guest, often 2 (eg qemu monitor + qemu agent). -# eg if we want to support 4096 guests, we'll typically need 8192 FDs -# If changing this, also consider virtlogd.service & virtlockd.service -# limits which are also related to number of guests -LimitNOFILE=8192 +# Raise hard limits to match behaviour of systemd >= 240. +# During startup, daemon will set soft limit to match hard limit +# per systemd recommendations +LimitNOFile=512000:1024 # The cgroups pids controller can limit the number of tasks started by # the daemon, which can limit the number of domains for some hypervisors. # A conservative default of 8 tasks per guest results in a TasksMax of diff --git a/src/qemu/virtqemud.service.in b/src/qemu/virtqemud.service.in index 46917b746d..7e02f7ab51 100644 --- a/src/qemu/virtqemud.service.in +++ b/src/qemu/virtqemud.service.in @@ -24,11 +24,10 @@ ExecStart=@sbindir@/virtqemud $VIRTQEMUD_ARGS ExecReload=/bin/kill -HUP $MAINPID KillMode=process Restart=on-failure -# At least 1 FD per guest, often 2 (eg qemu monitor + qemu agent). -# eg if we want to support 4096 guests, we'll typically need 8192 FDs -# If changing this, also consider virtlogd.service & virtlockd.service -# limits which are also related to number of guests -LimitNOFILE=8192 +# Raise hard limits to match behaviour of systemd >= 240. +# During startup, daemon will set soft limit to match hard limit +# per systemd recommendations +LimitNOFile=512000:1024 # The cgroups pids controller can limit the number of tasks started by # the daemon, which can limit the number of domains for some hypervisors. # A conservative default of 8 tasks per guest results in a TasksMax of diff --git a/src/remote/libvirtd.service.in b/src/remote/libvirtd.service.in index afda257228..28bcdb1220 100644 --- a/src/remote/libvirtd.service.in +++ b/src/remote/libvirtd.service.in @@ -29,11 +29,10 @@ ExecStart=@sbindir@/libvirtd $LIBVIRTD_ARGS ExecReload=/bin/kill -HUP $MAINPID KillMode=process Restart=on-failure -# At least 1 FD per guest, often 2 (eg qemu monitor + qemu agent). -# eg if we want to support 4096 guests, we'll typically need 8192 FDs -# If changing this, also consider virtlogd.service & virtlockd.service -# limits which are also related to number of guests -LimitNOFILE=8192 +# Raise hard limits to match behaviour of systemd >= 240. +# During startup, daemon will set soft limit to match hard limit +# per systemd recommendations +LimitNOFile=512000:1024 # The cgroups pids controller can limit the number of tasks started by # the daemon, which can limit the number of domains for some hypervisors. # A conservative default of 8 tasks per guest results in a TasksMax of diff --git a/tests/virshtest.c b/tests/virshtest.c index cf834bb847..022cd075f9 100644 --- a/tests/virshtest.c +++ b/tests/virshtest.c @@ -118,6 +118,7 @@ testCompareOutputLit(const char *expectData, cmd = virCommandNewArgs(argv); + virCommandAddEnvPassCommon(cmd); virCommandAddEnvString(cmd, "LANG=C"); virCommandSetInputBuffer(cmd, empty); virCommandSetOutputBuffer(cmd, &actualData); diff --git a/tools/virsh.c b/tools/virsh.c index 963e886860..55a1ffca86 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -123,7 +123,7 @@ virshConnect(vshControl *ctl, const char *uri, bool readonly) keepalive_forced = true; } - if (virPolkitAgentAvailable() && + if (0 && virPolkitAgentAvailable() && !(pkagent = virPolkitAgentCreate())) virResetLastError(); -- 2.40.1

On Wed, Jun 21, 2023 at 14:32:29 +0100, Daniel P. Berrangé wrote:
Since systemd 240, all services get an open file hard limit of 500k, and a soft limit of 1024. This limit means apps are safe to use select() by default which is limited to 1024 FDs. Apps which don't use select() are expected to simply set their soft limit to match the hard limit during startup.
With our current unit file settings we've been effectively reducing the max open files we have on most modern systems.
https://gitlab.com/libvirt/libvirt/-/issues/489 Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/ch/virtchd.service.in | 9 ++++----- src/locking/virtlockd.service.in | 8 ++++---- src/logging/virtlogd.service.in | 11 ++++------- src/lxc/virtlxcd.service.in | 9 ++++----- src/qemu/virtqemud.service.in | 9 ++++----- src/remote/libvirtd.service.in | 9 ++++----- tests/virshtest.c | 1 + tools/virsh.c | 2 +- 8 files changed, 26 insertions(+), 32 deletions(-)
[...]
diff --git a/tests/virshtest.c b/tests/virshtest.c index cf834bb847..022cd075f9 100644 --- a/tests/virshtest.c +++ b/tests/virshtest.c @@ -118,6 +118,7 @@ testCompareOutputLit(const char *expectData,
cmd = virCommandNewArgs(argv);
+ virCommandAddEnvPassCommon(cmd); virCommandAddEnvString(cmd, "LANG=C"); virCommandSetInputBuffer(cmd, empty); virCommandSetOutputBuffer(cmd, &actualData); diff --git a/tools/virsh.c b/tools/virsh.c index 963e886860..55a1ffca86 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -123,7 +123,7 @@ virshConnect(vshControl *ctl, const char *uri, bool readonly) keepalive_forced = true; }
- if (virPolkitAgentAvailable() && + if (0 && virPolkitAgentAvailable() && !(pkagent = virPolkitAgentCreate())) virResetLastError();
More irrelevant debug code accidentally added. For the unit file bits: Reviewed-by: Peter Krempa <pkrempa@redhat.com>

Wed, 21 Jun 2023 14:32:29 +0100 Daniel P. Berrangé <berrange@redhat.com>:
-LimitNOFILE=8192 +LimitNOFile=512000:1024
Did someone really rename that knob in upstream systemd? My copy of systemd.directives(7) still lists the uppercase variant. As a result this change means the knob is now not recognized anymore. Olaf

On Wed, Aug 02, 2023 at 10:49:55AM +0200, Olaf Hering wrote:
Wed, 21 Jun 2023 14:32:29 +0100 Daniel P. Berrangé <berrange@redhat.com>:
-LimitNOFILE=8192 +LimitNOFile=512000:1024
Did someone really rename that knob in upstream systemd? My copy of systemd.directives(7) still lists the uppercase variant. As a result this change means the knob is now not recognized anymore.
No, its a mistake. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Historically we wanted to check if logind was actually running, not merely activatable, because on systems where systemd is installed, but the OS is booted into non-systemd init, we want to fallback to pm-utils. Requiring logind to be running, however, forces us to serialize libvirtd startup on startup of logind which is undesirable. We can relax this dependancy if we check whether systemd itself is running, which implies that logind will activated when we need it. https://gitlab.com/libvirt/libvirt/-/issues/489 Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virsystemd.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c index 3112a1ba80..cd4de0eef8 100644 --- a/src/util/virsystemd.c +++ b/src/util/virsystemd.c @@ -184,9 +184,21 @@ virSystemdHasLogind(void) return ret; } + /* + * Want to use logind if: + * - logind is already running + * Or + * - logind is not running, but this is a systemd host + * (rely on dbus activation) + */ if ((ret = virGDBusIsServiceRegistered("org.freedesktop.login1")) == -1) return ret; + if (ret == -2) { + if ((ret = virGDBusIsServiceRegistered("org.freedesktop.systemd1")) == -1) + return ret; + } + g_atomic_int_set(&virSystemdHasLogindCachedValue, ret); return ret; } -- 2.40.1

On Wed, Jun 21, 2023 at 14:32:30 +0100, Daniel P. Berrangé wrote:
Historically we wanted to check if logind was actually running, not merely activatable, because on systems where systemd is installed, but the OS is booted into non-systemd init, we want to fallback to pm-utils.
Requiring logind to be running, however, forces us to serialize libvirtd startup on startup of logind which is undesirable. We can relax this dependancy if we check whether systemd itself is running, which implies that logind will activated when we need it.
https://gitlab.com/libvirt/libvirt/-/issues/489 Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virsystemd.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c index 3112a1ba80..cd4de0eef8 100644 --- a/src/util/virsystemd.c +++ b/src/util/virsystemd.c @@ -184,9 +184,21 @@ virSystemdHasLogind(void) return ret; }
+ /* + * Want to use logind if: + * - logind is already running + * Or + * - logind is not running, but this is a systemd host + * (rely on dbus activation) + */ if ((ret = virGDBusIsServiceRegistered("org.freedesktop.login1")) == -1) return ret;
+ if (ret == -2) { + if ((ret = virGDBusIsServiceRegistered("org.freedesktop.systemd1")) == -1) + return ret; + }
Why not simply just check that we are on a systemd host? Can logind be used without systemd?

On Thu, Jun 22, 2023 at 10:23:57AM +0200, Peter Krempa wrote:
On Wed, Jun 21, 2023 at 14:32:30 +0100, Daniel P. Berrangé wrote:
Historically we wanted to check if logind was actually running, not merely activatable, because on systems where systemd is installed, but the OS is booted into non-systemd init, we want to fallback to pm-utils.
Requiring logind to be running, however, forces us to serialize libvirtd startup on startup of logind which is undesirable. We can relax this dependancy if we check whether systemd itself is running, which implies that logind will activated when we need it.
https://gitlab.com/libvirt/libvirt/-/issues/489 Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virsystemd.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c index 3112a1ba80..cd4de0eef8 100644 --- a/src/util/virsystemd.c +++ b/src/util/virsystemd.c @@ -184,9 +184,21 @@ virSystemdHasLogind(void) return ret; }
+ /* + * Want to use logind if: + * - logind is already running + * Or + * - logind is not running, but this is a systemd host + * (rely on dbus activation) + */ if ((ret = virGDBusIsServiceRegistered("org.freedesktop.login1")) == -1) return ret;
+ if (ret == -2) { + if ((ret = virGDBusIsServiceRegistered("org.freedesktop.systemd1")) == -1) + return ret; + }
Why not simply just check that we are on a systemd host? Can logind be used without systemd?
I believe it can. I recall that when Debian supported a choice between systemd and sysvinit, logind could still be used in the sysvinit scenario in order to support GNOME which had a mandatory dep on logind. So I wasn't comfortable entirely removing the check for logind. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On Thu, Jun 22, 2023 at 09:49:41AM +0100, Daniel P. Berrangé wrote:
On Thu, Jun 22, 2023 at 10:23:57AM +0200, Peter Krempa wrote:
On Wed, Jun 21, 2023 at 14:32:30 +0100, Daniel P. Berrangé wrote:
+ /* + * Want to use logind if: + * - logind is already running + * Or + * - logind is not running, but this is a systemd host + * (rely on dbus activation) + */ if ((ret = virGDBusIsServiceRegistered("org.freedesktop.login1")) == -1) return ret;
+ if (ret == -2) { + if ((ret = virGDBusIsServiceRegistered("org.freedesktop.systemd1")) == -1) + return ret; + }
Why not simply just check that we are on a systemd host? Can logind be used without systemd?
I believe it can. I recall that when Debian supported a choice between systemd and sysvinit, logind could still be used in the sysvinit scenario in order to support GNOME which had a mandatory dep on logind.
So I wasn't comfortable entirely removing the check for logind.
Yeah, there's an alternative/hacked implementation called elogind which can be used without systemd. On a Debian VM configured to use the classic sysvinit, for example, I get $ dbus-send --system --print-reply --dest=org.freedesktop.DBus /org/freedesktop/DBus org.freedesktop.DBus.ListNames method return time=1687442871.393925 sender=org.freedesktop.DBus -> destination=:1.4 serial=3 reply_serial=2 array [ string "org.freedesktop.DBus" string "org.freedesktop.login1" string ":1.4" string ":1.0" string ":1.1" ] Now the interesting question is whether we will (and should) run any of the code in the virsystemd module on such a machine... -- Andrea Bolognani / Red Hat / Virtualization

On Wed, Jun 21, 2023 at 14:32:30 +0100, Daniel P. Berrangé wrote:
Historically we wanted to check if logind was actually running, not merely activatable, because on systems where systemd is installed, but the OS is booted into non-systemd init, we want to fallback to pm-utils.
Requiring logind to be running, however, forces us to serialize libvirtd startup on startup of logind which is undesirable. We can relax this dependancy if we check whether systemd itself is running, which implies that logind will activated when we need it.
https://gitlab.com/libvirt/libvirt/-/issues/489 Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virsystemd.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
Based on the discussion in the other thread: Reviewed-by: Peter Krempa <pkrempa@redhat.com>

After the previous commit we no longer require that logind is actually running, it merely has to be activatable. https://gitlab.com/libvirt/libvirt/-/issues/489 Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/ch/virtchd.service.in | 1 - src/lxc/virtlxcd.service.in | 1 - src/qemu/virtqemud.service.in | 1 - src/remote/libvirtd.service.in | 1 - 4 files changed, 4 deletions(-) diff --git a/src/ch/virtchd.service.in b/src/ch/virtchd.service.in index 22314bc907..be242fea78 100644 --- a/src/ch/virtchd.service.in +++ b/src/ch/virtchd.service.in @@ -9,7 +9,6 @@ After=network.target After=dbus.service After=apparmor.service After=remote-fs.target -After=systemd-logind.service After=systemd-machined.service Documentation=man:virtchd(8) Documentation=https://libvirt.org diff --git a/src/lxc/virtlxcd.service.in b/src/lxc/virtlxcd.service.in index 59d7d26657..b615a3f32d 100644 --- a/src/lxc/virtlxcd.service.in +++ b/src/lxc/virtlxcd.service.in @@ -9,7 +9,6 @@ After=network.target After=dbus.service After=apparmor.service After=remote-fs.target -After=systemd-logind.service After=systemd-machined.service Documentation=man:virtlxcd(8) Documentation=https://libvirt.org diff --git a/src/qemu/virtqemud.service.in b/src/qemu/virtqemud.service.in index 7e02f7ab51..e3dc738b8f 100644 --- a/src/qemu/virtqemud.service.in +++ b/src/qemu/virtqemud.service.in @@ -11,7 +11,6 @@ After=network.target After=dbus.service After=apparmor.service After=remote-fs.target -After=systemd-logind.service After=systemd-machined.service Documentation=man:virtqemud(8) Documentation=https://libvirt.org diff --git a/src/remote/libvirtd.service.in b/src/remote/libvirtd.service.in index 28bcdb1220..abac58cb2c 100644 --- a/src/remote/libvirtd.service.in +++ b/src/remote/libvirtd.service.in @@ -14,7 +14,6 @@ After=dbus.service After=iscsid.service After=apparmor.service After=remote-fs.target -After=systemd-logind.service After=systemd-machined.service After=xencommons.service Conflicts=xendomains.service -- 2.40.1

On Wed, Jun 21, 2023 at 14:32:31 +0100, Daniel P. Berrangé wrote:
After the previous commit we no longer require that logind is actually running, it merely has to be activatable.
https://gitlab.com/libvirt/libvirt/-/issues/489 Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/ch/virtchd.service.in | 1 - src/lxc/virtlxcd.service.in | 1 - src/qemu/virtqemud.service.in | 1 - src/remote/libvirtd.service.in | 1 -
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virnodesuspend.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/util/virnodesuspend.c b/src/util/virnodesuspend.c index 01f387d6fa..91a7f10eb9 100644 --- a/src/util/virnodesuspend.c +++ b/src/util/virnodesuspend.c @@ -249,6 +249,7 @@ virNodeSuspendSupportsTargetPMUtils(unsigned int target, bool *supported) * (i.e., the PM capability is supported) */ *supported = (status == 0); + VIR_DEBUG("Node systemd pm-utils target %d: %d", target, *supported); return 0; } @@ -257,6 +258,7 @@ static int virNodeSuspendSupportsTargetPMUtils(unsigned int target G_GNUC_UNUSED, bool *supported G_GNUC_UNUSED) { + VIR_DEBUG("Node systemd pm-utils target %d: unsupported platform", target); return -2; } #endif /* ! WITH_PM_UTILS */ @@ -282,6 +284,7 @@ virNodeSuspendSupportsTargetSystemd(unsigned int target, bool *supported) return ret; } + VIR_DEBUG("Node systemd systemd target %d: %d", target, ret); return ret; } -- 2.40.1

On Wed, Jun 21, 2023 at 14:32:32 +0100, Daniel P. Berrangé wrote:
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virnodesuspend.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/util/virnodesuspend.c b/src/util/virnodesuspend.c index 01f387d6fa..91a7f10eb9 100644 --- a/src/util/virnodesuspend.c +++ b/src/util/virnodesuspend.c @@ -249,6 +249,7 @@ virNodeSuspendSupportsTargetPMUtils(unsigned int target, bool *supported) * (i.e., the PM capability is supported) */ *supported = (status == 0); + VIR_DEBUG("Node systemd pm-utils target %d: %d", target, *supported);
...
return 0; } @@ -257,6 +258,7 @@ static int virNodeSuspendSupportsTargetPMUtils(unsigned int target G_GNUC_UNUSED, bool *supported G_GNUC_UNUSED) { + VIR_DEBUG("Node systemd pm-utils target %d: unsupported platform", target);
... extra 'systemd' ?
return -2; } #endif /* ! WITH_PM_UTILS */ @@ -282,6 +284,7 @@ virNodeSuspendSupportsTargetSystemd(unsigned int target, bool *supported) return ret; }
+ VIR_DEBUG("Node systemd systemd target %d: %d", target, ret);
systemd systemd?
return ret; }
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On Thu, Jun 22, 2023 at 10:27:15AM +0200, Peter Krempa wrote:
On Wed, Jun 21, 2023 at 14:32:32 +0100, Daniel P. Berrangé wrote:
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virnodesuspend.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/util/virnodesuspend.c b/src/util/virnodesuspend.c index 01f387d6fa..91a7f10eb9 100644 --- a/src/util/virnodesuspend.c +++ b/src/util/virnodesuspend.c @@ -249,6 +249,7 @@ virNodeSuspendSupportsTargetPMUtils(unsigned int target, bool *supported) * (i.e., the PM capability is supported) */ *supported = (status == 0); + VIR_DEBUG("Node systemd pm-utils target %d: %d", target, *supported);
...
return 0; } @@ -257,6 +258,7 @@ static int virNodeSuspendSupportsTargetPMUtils(unsigned int target G_GNUC_UNUSED, bool *supported G_GNUC_UNUSED) { + VIR_DEBUG("Node systemd pm-utils target %d: unsupported platform", target);
... extra 'systemd' ?
return -2; } #endif /* ! WITH_PM_UTILS */ @@ -282,6 +284,7 @@ virNodeSuspendSupportsTargetSystemd(unsigned int target, bool *supported) return ret; }
+ VIR_DEBUG("Node systemd systemd target %d: %d", target, ret);
systemd systemd?
All the above cases where supposed to say 'Node suspend ....' not "Node systemd ....'
return ret; }
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (4)
-
Andrea Bolognani
-
Daniel P. Berrangé
-
Olaf Hering
-
Peter Krempa