[PATCH 0/3] src: fix virtnetworkd blocking (delaying) system shutdown
We recently wired up inhibitors in the network driver to prevent auto-shutdown of libvirtd/virtnetworkd when virtual networks are active. This is to prevent the repeated re-creation of firewall rules which happens on daemon startup. Unfortunately we forgot that an inhibition on libvirt daemon shutdown *also* gets unconditionally turned into an inhibition on OS shutdown :-( In fixing this I realized this is actually the second such mistakes, because a few years ago we made the virtsecretd daemon inhibit when ephemeral secrets are present, and that really has no justification for blocking system shutdown either. Daniel P. Berrangé (3): util: introduce object for holding a system inhibitor lock src: convert drivers over to new virInhibitor APIs rpc: remove logind support for virNetDaemon po/POTFILES | 1 + src/libvirt_private.syms | 7 + src/libxl/libxl_conf.h | 9 +- src/libxl/libxl_domain.c | 6 +- src/libxl/libxl_driver.c | 15 ++- src/lxc/lxc_conf.h | 9 +- src/lxc/lxc_driver.c | 13 +- src/lxc/lxc_process.c | 9 +- src/network/bridge_driver.c | 20 +-- src/network/bridge_driver_conf.h | 9 +- src/qemu/qemu_conf.h | 9 +- src/qemu/qemu_driver.c | 12 +- src/qemu/qemu_process.c | 9 +- src/rpc/virnetdaemon.c | 78 ----------- src/secret/secret_driver.c | 46 +++---- src/util/meson.build | 1 + src/util/virinhibitor.c | 214 +++++++++++++++++++++++++++++++ src/util/virinhibitor.h | 58 +++++++++ 18 files changed, 361 insertions(+), 164 deletions(-) create mode 100644 src/util/virinhibitor.c create mode 100644 src/util/virinhibitor.h -- 2.46.0
The system inhibitor locks are currently handled by code in the virNetDaemon class. The driver code invokes a callback provided by the daemon when it wants to start or end inhibition. When the first inhibition is started, the daemon will call out to logind to apply it system wide. This has many flaws * A single message is registered with logind regardless of what driver holds the inhibition * An inhibition of daemon shutdown can't be acquired without also inhibiting system shutdown * Config of the inhibitions cannot be tailored by the driver The new virInhibitor object addresses these: * The object directly manages an inhibition with logind privately to the driver, enabling custom messages to be set. * It is possible to acquire an inhibition locally to the daemon without forwarding it to logind. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- po/POTFILES | 1 + src/libvirt_private.syms | 7 ++ src/util/meson.build | 1 + src/util/virinhibitor.c | 214 +++++++++++++++++++++++++++++++++++++++ src/util/virinhibitor.h | 58 +++++++++++ 5 files changed, 281 insertions(+) create mode 100644 src/util/virinhibitor.c create mode 100644 src/util/virinhibitor.h diff --git a/po/POTFILES b/po/POTFILES index 3514aa3dca..c71e439fe3 100644 --- a/po/POTFILES +++ b/po/POTFILES @@ -293,6 +293,7 @@ src/util/virhostcpu.c src/util/virhostmem.c src/util/virhostuptime.c src/util/viridentity.c +src/util/virinhibitor.c src/util/virinitctl.c src/util/viriscsi.c src/util/virjson.c diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c931003fad..adc3e3064f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2608,6 +2608,13 @@ virIdentitySetUserName; virIdentitySetX509DName; +# util/virinhibitor.h +virInhibitorFree; +virInhibitorHold; +virInhibitorNew; +virInhibitorRelease; + + # util/virinitctl.h virInitctlFifos; virInitctlSetRunLevel; diff --git a/src/util/meson.build b/src/util/meson.build index 30f71b0227..69ef49139a 100644 --- a/src/util/meson.build +++ b/src/util/meson.build @@ -45,6 +45,7 @@ util_sources = [ 'virhostmem.c', 'virhostuptime.c', 'viridentity.c', + 'virinhibitor.c', 'virinitctl.c', 'viriscsi.c', 'virjson.c', diff --git a/src/util/virinhibitor.c b/src/util/virinhibitor.c new file mode 100644 index 0000000000..647bdc9fbb --- /dev/null +++ b/src/util/virinhibitor.c @@ -0,0 +1,214 @@ +/* + * virinhibitor.c: helper APIs for inhibiting host actions + * + * Copyright (C) 2024 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#include <config.h> + +#include "virinhibitor.h" +#include "virgdbus.h" +#include "virsystemd.h" +#include "virfile.h" +#include "virlog.h" +#include "virenum.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +VIR_LOG_INIT("util.inhibitor"); + +struct _virInhibitor { + GMutex lock; + size_t count; + int fd; + + char *what; + char *who; + char *why; + const char *mode; + + virInhibitorAction action; + void *actionData; +}; + +VIR_ENUM_DECL(virInhibitorMode); + +VIR_ENUM_IMPL(virInhibitorMode, + VIR_INHIBITOR_MODE_LAST, + "block", "delay"); + +#ifdef G_OS_UNIX +/* As per: https://www.freedesktop.org/wiki/Software/systemd/inhibit */ +static int +virInhibitorAcquire(const char *what, + const char *who, + const char *why, + const char *mode, + int *inhibitorFD) +{ + g_autoptr(GVariant) reply = NULL; + g_autoptr(GUnixFDList) replyFD = NULL; + g_autoptr(GVariant) message = NULL; + GDBusConnection *systemBus; + int fd; + int rc; + + VIR_DEBUG("what=%s who=%s why=%s mode=%s", + NULLSTR(what), NULLSTR(who), NULLSTR(why), NULLSTR(mode)); + + if (!(systemBus = virGDBusGetSystemBus())) { + VIR_DEBUG("system dbus not available, skipping system inhibitor"); + return 0; + } + + if (virSystemdHasLogind() < 0) { + VIR_DEBUG("logind not available, skipping system inhibitor"); + return 0; + } + + message = g_variant_new("(ssss)", what, who, why, mode); + + rc = virGDBusCallMethodWithFD(systemBus, + &reply, + G_VARIANT_TYPE("(h)"), + &replyFD, + NULL, + "org.freedesktop.login1", + "/org/freedesktop/login1", + "org.freedesktop.login1.Manager", + "Inhibit", + message, + NULL); + + if (rc < 0) + return -1; + + if (g_unix_fd_list_get_length(replyFD) <= 0) { + VIR_DEBUG("Missing inhibitor FD in logind reply"); + return -1; + } + + fd = g_unix_fd_list_get(replyFD, 0, NULL); + if (fd < 0) { + VIR_DEBUG("Unable to get inhibitor FD from logind reply"); + return -1; + } + + *inhibitorFD = fd; + VIR_DEBUG("Got inhibitor FD %d", fd); + return 0; +} +#endif + + +static char * +virInhibitorWhatFormat(virInhibitorWhat what) +{ + const char *whatstr[] = { + "sleep", + "shutdown", + "idle", + "handle-power-key", + "handle-suspend-key", + "handle-hibernate-key", + "handle-lid-switch", + }; + GString *str = g_string_new(""); + size_t i; + + for (i = 0; i < G_N_ELEMENTS(whatstr); i++) { + if (what & (1 << i)) { + if (str->len) + g_string_append(str, ":"); + g_string_append(str, whatstr[i]); + } + } + + return g_string_free(str, FALSE); +} + + +virInhibitor *virInhibitorNew(virInhibitorWhat what, + const char *who, + const char *why, + virInhibitorMode mode, + virInhibitorAction action, + void *actionData) +{ + virInhibitor *inhibitor = g_new0(virInhibitor, 1); + + inhibitor->fd = -1; + inhibitor->what = virInhibitorWhatFormat(what); + inhibitor->who = g_strdup(who); + inhibitor->why = g_strdup(why); + inhibitor->mode = virInhibitorModeTypeToString(mode); + inhibitor->action = action; + inhibitor->actionData = actionData; + + return inhibitor; +} + +void virInhibitorHold(virInhibitor *inhibitor) +{ + g_mutex_lock(&inhibitor->lock); + + if (inhibitor->count == 0) { + if (inhibitor->action) { + inhibitor->action(true, inhibitor->actionData); + } +#ifdef G_OS_UNIX + if (virInhibitorAcquire( + inhibitor->what, inhibitor->who, inhibitor->why, + inhibitor->mode, &inhibitor->fd) < 0) { + VIR_ERROR(_("Failed to acquire inhibitor: %1$s"), + virGetLastErrorMessage()); + virResetLastError(); + } +#else + VIR_DEBUG("No inhibitor implementation on non-UNIX platforms"); +#endif + } + inhibitor->count++; + g_mutex_unlock(&inhibitor->lock); +} + + +void virInhibitorRelease(virInhibitor *inhibitor) +{ + g_mutex_lock(&inhibitor->lock); + inhibitor->count--; + if (inhibitor->count == 0) { + VIR_FORCE_CLOSE(inhibitor->fd); + if (inhibitor->action) { + inhibitor->action(false, inhibitor->actionData); + } + } + g_mutex_unlock(&inhibitor->lock); +} + + +void virInhibitorFree(virInhibitor *inhibitor) +{ + if (!inhibitor) + return; + + g_free(inhibitor->what); + g_free(inhibitor->who); + g_free(inhibitor->why); + VIR_FORCE_CLOSE(inhibitor->fd); + g_free(inhibitor); +} diff --git a/src/util/virinhibitor.h b/src/util/virinhibitor.h new file mode 100644 index 0000000000..0a1c445d41 --- /dev/null +++ b/src/util/virinhibitor.h @@ -0,0 +1,58 @@ +/* + * virinhibitor.h: helper APIs for inhibiting host actions + * + * Copyright (C) 2024 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#pragma once + +#include "internal.h" + +typedef struct _virInhibitor virInhibitor; + +typedef enum { + VIR_INHIBITOR_WHAT_NONE = 0, + VIR_INHIBITOR_WHAT_SLEEP = (1 << 1), + VIR_INHIBITOR_WHAT_SHUTDOWN = (1 << 2), + VIR_INHIBITOR_WHAT_IDLE = (1 << 3), + VIR_INHIBITOR_WHAT_POWER_KEY = (1 << 4), + VIR_INHIBITOR_WHAT_SUSPEND_KEY = (1 << 5), + VIR_INHIBITOR_WHAT_HIBERNATE_KEY = (1 << 6), + VIR_INHIBITOR_WHAT_LID_SWITCH = (1 << 7), +} virInhibitorWhat; + +typedef enum { + VIR_INHIBITOR_MODE_BLOCK, + VIR_INHIBITOR_MODE_DELAY, + + VIR_INHIBITOR_MODE_LAST +} virInhibitorMode; + +typedef void (*virInhibitorAction)(bool inhibited, + void *opaque); + +virInhibitor *virInhibitorNew(virInhibitorWhat what, + const char *who, + const char *why, + virInhibitorMode mode, + virInhibitorAction action, + void *actionData); + +void virInhibitorHold(virInhibitor *inhibitor); +void virInhibitorRelease(virInhibitor *inhibitor); + +void virInhibitorFree(virInhibitor *inhibitor); -- 2.46.0
This initial conversion of the drivers switches them over to use the virInhibitor APIs in local daemon only mode. Communication to logind is still handled by the virNetDaemon class logic. This mostly just replaces upto 3 fields in the driver state with a single new virInhibitor object, but otherwise should not change functionality besides replacing atomics with mutex protected APIs. The exception is the LXC driver which has been trying to inhibit shutdown shutdown but silently failing to, since nothing ever remembered to set the 'inhibitCallback' pointer in the driver state struct. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libxl/libxl_conf.h | 9 +++---- src/libxl/libxl_domain.c | 6 ++--- src/libxl/libxl_driver.c | 15 +++++++---- src/lxc/lxc_conf.h | 9 +++---- src/lxc/lxc_driver.c | 13 +++++++-- src/lxc/lxc_process.c | 9 +++---- src/network/bridge_driver.c | 20 +++++++------- src/network/bridge_driver_conf.h | 9 +++---- src/qemu/qemu_conf.h | 9 +++---- src/qemu/qemu_driver.c | 12 ++++++--- src/qemu/qemu_process.c | 9 +++---- src/secret/secret_driver.c | 46 +++++++++++++------------------- 12 files changed, 80 insertions(+), 86 deletions(-) diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h index 7087b41079..0edcde079d 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -35,6 +35,7 @@ #include "virfirmware.h" #include "libxl_capabilities.h" #include "libxl_logger.h" +#include "virinhibitor.h" #define LIBXL_DRIVER_EXTERNAL_NAME "Xen" /* @@ -117,12 +118,8 @@ struct _libxlDriverPrivate { /* pid file FD, ensures two copies of the driver can't use the same root */ int lockFD; - /* Atomic inc/dec only */ - unsigned int nactive; - - /* Immutable pointers. Caller must provide locking */ - virStateInhibitCallback inhibitCallback; - void *inhibitOpaque; + /* Immutable pointer, self-locking APIs */ + virInhibitor *inhibitor; /* Immutable pointer, self-locking APIs */ virDomainObjList *domains; diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index cad6c9ce42..a049cdb30f 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -873,8 +873,7 @@ libxlDomainCleanup(libxlDriverPrivate *driver, priv->deathW = NULL; } - if (g_atomic_int_dec_and_test(&driver->nactive) && driver->inhibitCallback) - driver->inhibitCallback(false, driver->inhibitOpaque); + virInhibitorRelease(driver->inhibitor); /* Release auto-allocated graphics ports */ for (i = 0; i < vm->def->ngraphics; i++) { @@ -1421,8 +1420,7 @@ libxlDomainStart(libxlDriverPrivate *driver, return -1; } - if (g_atomic_int_add(&driver->nactive, 1) == 0 && driver->inhibitCallback) - driver->inhibitCallback(true, driver->inhibitOpaque); + virInhibitorHold(driver->inhibitor); event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, restore_fd < 0 ? diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index e72553603d..eb293172f7 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -437,8 +437,7 @@ libxlReconnectDomain(virDomainObj *vm, virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_UNKNOWN); - if (g_atomic_int_add(&driver->nactive, 1) == 0 && driver->inhibitCallback) - driver->inhibitCallback(true, driver->inhibitOpaque); + virInhibitorHold(driver->inhibitor); /* Enable domain death events */ libxl_evenable_domain_death(cfg->ctx, vm->def->id, 0, &priv->deathW); @@ -514,6 +513,7 @@ libxlStateCleanup(void) virObjectUnref(libxl_driver->domainEventState); virSysinfoDefFree(libxl_driver->hostsysinfo); + virInhibitorFree(libxl_driver->inhibitor); if (libxl_driver->lockFD != -1) virPidFileRelease(libxl_driver->config->stateDir, "driver", libxl_driver->lockFD); @@ -675,9 +675,6 @@ libxlStateInitialize(bool privileged, return VIR_DRV_STATE_INIT_ERROR; } - libxl_driver->inhibitCallback = callback; - libxl_driver->inhibitOpaque = opaque; - /* Allocate bitmap for vnc port reservation */ if (!(libxl_driver->reservedGraphicsPorts = virPortAllocatorRangeNew(_("VNC"), @@ -709,6 +706,14 @@ libxlStateInitialize(bool privileged, if (libxlDriverConfigLoadFile(cfg, driverConf) < 0) goto error; + libxl_driver->inhibitor = virInhibitorNew( + VIR_INHIBITOR_WHAT_NONE, + _("Libvirt Xen"), + _("Xen virtual machines are running"), + VIR_INHIBITOR_MODE_DELAY, + callback, + opaque); + /* Register the callbacks providing access to libvirt's event loop */ libxl_osevent_register_hooks(cfg->ctx, &libxl_osevent_callbacks, cfg->ctx); diff --git a/src/lxc/lxc_conf.h b/src/lxc/lxc_conf.h index a639e3989f..1969ed74cb 100644 --- a/src/lxc/lxc_conf.h +++ b/src/lxc/lxc_conf.h @@ -30,6 +30,7 @@ #include "virsysinfo.h" #include "virclosecallbacks.h" #include "virhostdev.h" +#include "virinhibitor.h" #define LXC_DRIVER_NAME "LXC" @@ -75,12 +76,8 @@ struct _virLXCDriver { /* Immutable pointer, lockless APIs */ virSysinfoDef *hostsysinfo; - /* Atomic inc/dec only */ - unsigned int nactive; - - /* Immutable pointers. Caller must provide locking */ - virStateInhibitCallback inhibitCallback; - void *inhibitOpaque; + /* Immutable pointer, self-locking APIs */ + virInhibitor *inhibitor; /* Immutable pointer, self-locking APIs */ virDomainObjList *domains; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 2488940feb..08516c8297 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1398,8 +1398,8 @@ static virDrvStateInitResult lxcStateInitialize(bool privileged, const char *root, bool monolithic G_GNUC_UNUSED, - virStateInhibitCallback callback G_GNUC_UNUSED, - void *opaque G_GNUC_UNUSED) + virStateInhibitCallback callback, + void *opaque) { virLXCDriverConfig *cfg = NULL; bool autostart = true; @@ -1451,6 +1451,14 @@ lxcStateInitialize(bool privileged, if (virLXCLoadDriverConfig(cfg, SYSCONFDIR "/libvirt/lxc.conf") < 0) goto cleanup; + lxc_driver->inhibitor = virInhibitorNew( + VIR_INHIBITOR_WHAT_NONE, + _("Libvirt LXC"), + _("LXC containers are running"), + VIR_INHIBITOR_MODE_DELAY, + callback, + opaque); + if (!(lxc_driver->securityManager = lxcSecurityInit(cfg))) goto cleanup; @@ -1555,6 +1563,7 @@ static int lxcStateCleanup(void) virObjectUnref(lxc_driver->caps); virObjectUnref(lxc_driver->securityManager); virObjectUnref(lxc_driver->xmlopt); + virInhibitorFree(lxc_driver->inhibitor); if (lxc_driver->lockFD != -1) virPidFileRelease(lxc_driver->config->stateDir, "driver", lxc_driver->lockFD); diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index cd8bcfc282..c2982244f0 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -203,8 +203,7 @@ static void virLXCProcessCleanup(virLXCDriver *driver, vm->pid = 0; vm->def->id = -1; - if (g_atomic_int_dec_and_test(&driver->nactive) && driver->inhibitCallback) - driver->inhibitCallback(false, driver->inhibitOpaque); + virInhibitorRelease(driver->inhibitor); virLXCDomainReAttachHostDevices(driver, vm->def); @@ -1466,8 +1465,7 @@ int virLXCProcessStart(virLXCDriver * driver, if (virCommandHandshakeNotify(cmd) < 0) goto cleanup; - if (g_atomic_int_add(&driver->nactive, 1) == 0 && driver->inhibitCallback) - driver->inhibitCallback(true, driver->inhibitOpaque); + virInhibitorHold(driver->inhibitor); /* The first synchronization point is when the controller creates CGroups. */ if (lxcContainerWaitForContinue(handshakefds[0]) < 0) { @@ -1665,8 +1663,7 @@ virLXCProcessReconnectDomain(virDomainObj *vm, virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_UNKNOWN); - if (g_atomic_int_add(&driver->nactive, 1) == 0 && driver->inhibitCallback) - driver->inhibitCallback(true, driver->inhibitOpaque); + virInhibitorHold(driver->inhibitor); if (!(priv->monitor = virLXCProcessConnectMonitor(driver, vm))) goto error; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index e700a614a9..ce793c12ef 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -504,8 +504,7 @@ networkUpdateState(virNetworkObj *obj, if (virNetworkObjIsActive(obj)) { virNetworkObjPortForEach(obj, networkUpdatePort, obj); - if (g_atomic_int_add(&driver->nactive, 1) == 0 && driver->inhibitCallback) - driver->inhibitCallback(true, driver->inhibitOpaque); + virInhibitorHold(driver->inhibitor); } /* Try and read dnsmasq pids of both active and inactive networks, just in @@ -644,9 +643,6 @@ networkStateInitialize(bool privileged, goto error; } - network_driver->inhibitCallback = callback; - network_driver->inhibitOpaque = opaque; - network_driver->privileged = privileged; if (!(network_driver->xmlopt = networkDnsmasqCreateXMLConf())) @@ -655,6 +651,14 @@ networkStateInitialize(bool privileged, if (!(network_driver->config = cfg = virNetworkDriverConfigNew(privileged))) goto error; + network_driver->inhibitor = virInhibitorNew( + VIR_INHIBITOR_WHAT_NONE, + _("Libvirt Network"), + _("Virtual networks are active"), + VIR_INHIBITOR_MODE_DELAY, + callback, + opaque); + if ((network_driver->lockFD = virPidFileAcquire(cfg->stateDir, "driver", getpid())) < 0) goto error; @@ -2432,8 +2436,7 @@ networkStartNetwork(virNetworkDriverState *driver, obj, network_driver->xmlopt) < 0) goto cleanup; - if (g_atomic_int_add(&driver->nactive, 1) == 0 && driver->inhibitCallback) - driver->inhibitCallback(true, driver->inhibitOpaque); + virInhibitorHold(driver->inhibitor); virNetworkObjSetActive(obj, true); VIR_INFO("Network '%s' started up", def->name); @@ -2509,8 +2512,7 @@ networkShutdownNetwork(virNetworkDriverState *driver, virNetworkObjSetActive(obj, false); - if (g_atomic_int_dec_and_test(&driver->nactive) && driver->inhibitCallback) - driver->inhibitCallback(false, driver->inhibitOpaque); + virInhibitorRelease(driver->inhibitor); virNetworkObjUnsetDefTransient(obj); return ret; diff --git a/src/network/bridge_driver_conf.h b/src/network/bridge_driver_conf.h index 1beed01efb..2a2e2bc16d 100644 --- a/src/network/bridge_driver_conf.h +++ b/src/network/bridge_driver_conf.h @@ -27,6 +27,7 @@ #include "virnetworkobj.h" #include "object_event.h" #include "virfirewall.h" +#include "virinhibitor.h" typedef struct _virNetworkDriverConfig virNetworkDriverConfig; struct _virNetworkDriverConfig { @@ -49,12 +50,8 @@ typedef struct _virNetworkDriverState virNetworkDriverState; struct _virNetworkDriverState { virMutex lock; - /* Atomic inc/dec only */ - unsigned int nactive; - - /* Immutable pointers. Caller must provide locking */ - virStateInhibitCallback inhibitCallback; - void *inhibitOpaque; + /* Immutable pointer, self-locking APIs */ + virInhibitor *inhibitor; /* Read-only */ bool privileged; diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 23a900193e..42cdb6f883 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -42,6 +42,7 @@ #include "virfile.h" #include "virfilecache.h" #include "virfirmware.h" +#include "virinhibitor.h" #define QEMU_DRIVER_NAME "QEMU" @@ -257,16 +258,12 @@ struct _virQEMUDriver { /* Atomic increment only */ int lastvmid; - /* Atomic inc/dec only */ - unsigned int nactive; - /* Immutable values */ bool privileged; char *embeddedRoot; - /* Immutable pointers. Caller must provide locking */ - virStateInhibitCallback inhibitCallback; - void *inhibitOpaque; + /* Immutable pointer, self-locking APIs */ + virInhibitor *inhibitor; /* Immutable pointer, self-locking APIs */ virDomainObjList *domains; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f1a633fdd3..c4cb9b2a9a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -573,9 +573,6 @@ qemuStateInitialize(bool privileged, return VIR_DRV_STATE_INIT_ERROR; } - qemu_driver->inhibitCallback = callback; - qemu_driver->inhibitOpaque = opaque; - qemu_driver->privileged = privileged; qemu_driver->hostarch = virArchFromHost(); if (root != NULL) @@ -675,6 +672,14 @@ qemuStateInitialize(bool privileged, goto error; } + qemu_driver->inhibitor = virInhibitorNew( + VIR_INHIBITOR_WHAT_NONE, + _("Libvirt QEMU"), + _("QEMU/KVM virtual machines are running"), + VIR_INHIBITOR_MODE_DELAY, + callback, + opaque); + if ((qemu_driver->lockFD = virPidFileAcquire(cfg->stateDir, "driver", getpid())) < 0) goto error; @@ -1065,6 +1070,7 @@ qemuStateCleanup(void) ebtablesContextFree(qemu_driver->ebtables); virObjectUnref(qemu_driver->domains); virObjectUnref(qemu_driver->nbdkitCapsCache); + virInhibitorFree(qemu_driver->inhibitor); if (qemu_driver->lockFD != -1) virPidFileRelease(qemu_driver->config->stateDir, "driver", qemu_driver->lockFD); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index bee7a39e4e..c3b6f95493 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5809,8 +5809,7 @@ qemuProcessInit(virQEMUDriver *driver, qemuDomainSetFakeReboot(vm, false); virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_STARTING_UP); - if (g_atomic_int_add(&driver->nactive, 1) == 0 && driver->inhibitCallback) - driver->inhibitCallback(true, driver->inhibitOpaque); + virInhibitorHold(driver->inhibitor); /* Run an early hook to set-up missing devices */ if (qemuProcessStartHook(driver, vm, @@ -8812,8 +8811,7 @@ void qemuProcessStop(virQEMUDriver *driver, if (priv->eventThread) g_object_unref(g_steal_pointer(&priv->eventThread)); - if (g_atomic_int_dec_and_test(&driver->nactive) && driver->inhibitCallback) - driver->inhibitCallback(false, driver->inhibitOpaque); + virInhibitorRelease(driver->inhibitor); /* Clear network bandwidth */ virDomainClearNetBandwidth(vm->def); @@ -9571,8 +9569,7 @@ qemuProcessReconnect(void *opaque) goto error; } - if (g_atomic_int_add(&driver->nactive, 1) == 0 && driver->inhibitCallback) - driver->inhibitCallback(true, driver->inhibitOpaque); + virInhibitorHold(driver->inhibitor); cleanup: if (jobStarted) diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index a2d6b879d0..04c3ca49f1 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -41,6 +41,7 @@ #include "viraccessapicheck.h" #include "secret_event.h" #include "virutil.h" +#include "virinhibitor.h" #define VIR_FROM_THIS VIR_FROM_SECRET @@ -67,9 +68,8 @@ struct _virSecretDriverState { /* Immutable pointer, self-locking APIs */ virObjectEventState *secretEventState; - /* Immutable pointers. Caller must provide locking */ - virStateInhibitCallback inhibitCallback; - void *inhibitOpaque; + /* Immutable pointer, self-locking APIs */ + virInhibitor *inhibitor; }; static virSecretDriverState *driver; @@ -90,23 +90,6 @@ secretObjFromSecret(virSecretPtr secret) } -static bool -secretNumOfEphemeralSecretsHelper(virConnectPtr conn G_GNUC_UNUSED, - virSecretDef *def) -{ - return def->isephemeral; -} - - -static int -secretNumOfEphemeralSecrets(void) -{ - return virSecretObjListNumOfSecrets(driver->secrets, - secretNumOfEphemeralSecretsHelper, - NULL); -} - - /* Driver functions */ static int @@ -271,6 +254,10 @@ secretDefineXML(virConnectPtr conn, objDef->uuid, objDef->usage_type, objDef->usage_id); + + if (objDef->isephemeral) + virInhibitorHold(driver->inhibitor); + goto cleanup; restore_backup: @@ -288,8 +275,6 @@ secretDefineXML(virConnectPtr conn, virSecretDefFree(def); virSecretObjEndAPI(&obj); - if (secretNumOfEphemeralSecrets() > 0) - driver->inhibitCallback(true, driver->inhibitOpaque); virObjectEventStateQueue(driver->secretEventState, event); @@ -440,6 +425,9 @@ secretUndefine(virSecretPtr secret) VIR_SECRET_EVENT_UNDEFINED, 0); + if (def->isephemeral) + virInhibitorRelease(driver->inhibitor); + virSecretObjDeleteData(obj); virSecretObjListRemove(driver->secrets, obj); @@ -450,9 +438,6 @@ secretUndefine(virSecretPtr secret) cleanup: virSecretObjEndAPI(&obj); - if (secretNumOfEphemeralSecrets() == 0) - driver->inhibitCallback(false, driver->inhibitOpaque); - virObjectEventStateQueue(driver->secretEventState, event); return ret; @@ -469,6 +454,7 @@ secretStateCleanupLocked(void) VIR_FREE(driver->configDir); virObjectUnref(driver->secretEventState); + virInhibitorFree(driver->inhibitor); if (driver->lockFD != -1) virPidFileRelease(driver->stateDir, "driver", driver->lockFD); @@ -502,8 +488,6 @@ secretStateInitialize(bool privileged, driver->lockFD = -1; driver->secretEventState = virObjectEventStateNew(); driver->privileged = privileged; - driver->inhibitCallback = callback; - driver->inhibitOpaque = opaque; if (root) { driver->embeddedRoot = g_strdup(root); @@ -535,6 +519,14 @@ secretStateInitialize(bool privileged, goto error; } + driver->inhibitor = virInhibitorNew( + VIR_INHIBITOR_WHAT_NONE, + _("Libvirt Secret"), + _("Ephemeral secrets are loaded"), + VIR_INHIBITOR_MODE_DELAY, + callback, + opaque); + if ((driver->lockFD = virPidFileAcquire(driver->stateDir, "driver", getpid())) < 0) goto error; -- 2.46.0
On 12/17/24 12:15, Daniel P. Berrangé wrote:
This initial conversion of the drivers switches them over to use the virInhibitor APIs in local daemon only mode. Communication to logind is still handled by the virNetDaemon class logic.
This mostly just replaces upto 3 fields in the driver state with a single new virInhibitor object, but otherwise should not change functionality besides replacing atomics with mutex protected APIs.
The exception is the LXC driver which has been trying to inhibit shutdown shutdown but silently failing to, since nothing ever remembered to set the 'inhibitCallback' pointer in the driver state struct.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libxl/libxl_conf.h | 9 +++---- src/libxl/libxl_domain.c | 6 ++--- src/libxl/libxl_driver.c | 15 +++++++---- src/lxc/lxc_conf.h | 9 +++---- src/lxc/lxc_driver.c | 13 +++++++-- src/lxc/lxc_process.c | 9 +++---- src/network/bridge_driver.c | 20 +++++++------- src/network/bridge_driver_conf.h | 9 +++---- src/qemu/qemu_conf.h | 9 +++---- src/qemu/qemu_driver.c | 12 ++++++--- src/qemu/qemu_process.c | 9 +++---- src/secret/secret_driver.c | 46 +++++++++++++------------------- 12 files changed, 80 insertions(+), 86 deletions(-)
diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h index 7087b41079..0edcde079d 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -35,6 +35,7 @@ #include "virfirmware.h" #include "libxl_capabilities.h" #include "libxl_logger.h" +#include "virinhibitor.h"
#define LIBXL_DRIVER_EXTERNAL_NAME "Xen" /* @@ -117,12 +118,8 @@ struct _libxlDriverPrivate { /* pid file FD, ensures two copies of the driver can't use the same root */ int lockFD;
- /* Atomic inc/dec only */ - unsigned int nactive; - - /* Immutable pointers. Caller must provide locking */ - virStateInhibitCallback inhibitCallback; - void *inhibitOpaque; + /* Immutable pointer, self-locking APIs */ + virInhibitor *inhibitor;
/* Immutable pointer, self-locking APIs */ virDomainObjList *domains; diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index cad6c9ce42..a049cdb30f 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -873,8 +873,7 @@ libxlDomainCleanup(libxlDriverPrivate *driver, priv->deathW = NULL; }
- if (g_atomic_int_dec_and_test(&driver->nactive) && driver->inhibitCallback) - driver->inhibitCallback(false, driver->inhibitOpaque); + virInhibitorRelease(driver->inhibitor);
/* Release auto-allocated graphics ports */ for (i = 0; i < vm->def->ngraphics; i++) { @@ -1421,8 +1420,7 @@ libxlDomainStart(libxlDriverPrivate *driver, return -1; }
- if (g_atomic_int_add(&driver->nactive, 1) == 0 && driver->inhibitCallback) - driver->inhibitCallback(true, driver->inhibitOpaque); + virInhibitorHold(driver->inhibitor);
event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, restore_fd < 0 ? diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index e72553603d..eb293172f7 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -437,8 +437,7 @@ libxlReconnectDomain(virDomainObj *vm, virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_UNKNOWN);
- if (g_atomic_int_add(&driver->nactive, 1) == 0 && driver->inhibitCallback) - driver->inhibitCallback(true, driver->inhibitOpaque); + virInhibitorHold(driver->inhibitor);
/* Enable domain death events */ libxl_evenable_domain_death(cfg->ctx, vm->def->id, 0, &priv->deathW); @@ -514,6 +513,7 @@ libxlStateCleanup(void)
virObjectUnref(libxl_driver->domainEventState); virSysinfoDefFree(libxl_driver->hostsysinfo); + virInhibitorFree(libxl_driver->inhibitor);
if (libxl_driver->lockFD != -1) virPidFileRelease(libxl_driver->config->stateDir, "driver", libxl_driver->lockFD); @@ -675,9 +675,6 @@ libxlStateInitialize(bool privileged, return VIR_DRV_STATE_INIT_ERROR; }
- libxl_driver->inhibitCallback = callback; - libxl_driver->inhibitOpaque = opaque; - /* Allocate bitmap for vnc port reservation */ if (!(libxl_driver->reservedGraphicsPorts = virPortAllocatorRangeNew(_("VNC"), @@ -709,6 +706,14 @@ libxlStateInitialize(bool privileged, if (libxlDriverConfigLoadFile(cfg, driverConf) < 0) goto error;
+ libxl_driver->inhibitor = virInhibitorNew( + VIR_INHIBITOR_WHAT_NONE, + _("Libvirt Xen"), + _("Xen virtual machines are running"), + VIR_INHIBITOR_MODE_DELAY, + callback, + opaque);
A bit funky formatting here and in the rest of the patch. But I can live with that. Michal
The virNetDaemon code now only concerns itself with preventing auto shutdown of the local daemon. Logind is now handled by the new virInhibitor object, for QEMU, LXC and LibXL. This fixes two notable bugs * Running virtual networks would prevent system shutdown * Loaded ephemeral secrets would prevent system shutdown Fixes 9e3cc0ff5e81ed2056a6a528893fd2cb5609d70b Fixes 37800af9a400385801da6d73654249fdb51a93d8 Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libxl/libxl_driver.c | 2 +- src/lxc/lxc_driver.c | 2 +- src/qemu/qemu_driver.c | 2 +- src/rpc/virnetdaemon.c | 78 ---------------------------------------- 4 files changed, 3 insertions(+), 81 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index eb293172f7..2c0cf75562 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -707,7 +707,7 @@ libxlStateInitialize(bool privileged, goto error; libxl_driver->inhibitor = virInhibitorNew( - VIR_INHIBITOR_WHAT_NONE, + VIR_INHIBITOR_WHAT_SHUTDOWN, _("Libvirt Xen"), _("Xen virtual machines are running"), VIR_INHIBITOR_MODE_DELAY, diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 08516c8297..cc4b2a05d6 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1452,7 +1452,7 @@ lxcStateInitialize(bool privileged, goto cleanup; lxc_driver->inhibitor = virInhibitorNew( - VIR_INHIBITOR_WHAT_NONE, + VIR_INHIBITOR_WHAT_SHUTDOWN, _("Libvirt LXC"), _("LXC containers are running"), VIR_INHIBITOR_MODE_DELAY, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c4cb9b2a9a..95a0e3a170 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -673,7 +673,7 @@ qemuStateInitialize(bool privileged, } qemu_driver->inhibitor = virInhibitorNew( - VIR_INHIBITOR_WHAT_NONE, + VIR_INHIBITOR_WHAT_SHUTDOWN, _("Libvirt QEMU"), _("QEMU/KVM virtual machines are running"), VIR_INHIBITOR_MODE_DELAY, diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c index 9795418126..e4c6261536 100644 --- a/src/rpc/virnetdaemon.c +++ b/src/rpc/virnetdaemon.c @@ -31,7 +31,6 @@ #include "virutil.h" #include "virfile.h" #include "virnetserver.h" -#include "virgdbus.h" #include "virhash.h" #include "virprocess.h" #include "virsystemd.h" @@ -80,7 +79,6 @@ struct _virNetDaemon { int autoShutdownTimerID; bool autoShutdownTimerActive; size_t autoShutdownInhibitions; - int autoShutdownInhibitFd; }; @@ -109,7 +107,6 @@ virNetDaemonDispose(void *obj) virEventRemoveHandle(dmn->sigwatch); #endif /* !WIN32 */ - VIR_FORCE_CLOSE(dmn->autoShutdownInhibitFd); g_free(dmn->stateStopThread); g_clear_pointer(&dmn->servers, g_hash_table_unref); @@ -150,7 +147,6 @@ virNetDaemonNew(void) #endif /* !WIN32 */ dmn->privileged = geteuid() == 0; - dmn->autoShutdownInhibitFd = -1; virProcessActivateMaxFiles(); @@ -491,66 +487,6 @@ virNetDaemonAutoShutdown(virNetDaemon *dmn, } -#ifdef G_OS_UNIX -/* As per: https://www.freedesktop.org/wiki/Software/systemd/inhibit */ -static void -virNetDaemonCallInhibit(virNetDaemon *dmn, - const char *what, - const char *who, - const char *why, - const char *mode) -{ - g_autoptr(GVariant) reply = NULL; - g_autoptr(GUnixFDList) replyFD = NULL; - g_autoptr(GVariant) message = NULL; - GDBusConnection *systemBus; - int fd; - int rc; - - VIR_DEBUG("dmn=%p what=%s who=%s why=%s mode=%s", - dmn, NULLSTR(what), NULLSTR(who), NULLSTR(why), NULLSTR(mode)); - - if (virSystemdHasLogind() < 0) - return; - - if (!(systemBus = virGDBusGetSystemBus())) - return; - - message = g_variant_new("(ssss)", what, who, why, mode); - - rc = virGDBusCallMethodWithFD(systemBus, - &reply, - G_VARIANT_TYPE("(h)"), - &replyFD, - NULL, - "org.freedesktop.login1", - "/org/freedesktop/login1", - "org.freedesktop.login1.Manager", - "Inhibit", - message, - NULL); - - if (rc < 0) - return; - - if (g_unix_fd_list_get_length(replyFD) <= 0) - return; - - fd = g_unix_fd_list_get(replyFD, 0, NULL); - if (fd < 0) - return; - - if (dmn->autoShutdownInhibitions) { - dmn->autoShutdownInhibitFd = fd; - VIR_DEBUG("Got inhibit FD %d", fd); - } else { - /* We stopped the last VM since we made the inhibit call */ - VIR_DEBUG("Closing inhibit FD %d", fd); - VIR_FORCE_CLOSE(fd); - } -} -#endif - void virNetDaemonAddShutdownInhibition(virNetDaemon *dmn) { @@ -559,15 +495,6 @@ virNetDaemonAddShutdownInhibition(virNetDaemon *dmn) dmn->autoShutdownInhibitions++; VIR_DEBUG("dmn=%p inhibitions=%zu", dmn, dmn->autoShutdownInhibitions); - -#ifdef G_OS_UNIX - if (dmn->autoShutdownInhibitions == 1) - virNetDaemonCallInhibit(dmn, - "shutdown", - _("Libvirt"), - _("Virtual machines need to be saved"), - "delay"); -#endif } @@ -579,11 +506,6 @@ virNetDaemonRemoveShutdownInhibition(virNetDaemon *dmn) dmn->autoShutdownInhibitions--; VIR_DEBUG("dmn=%p inhibitions=%zu", dmn, dmn->autoShutdownInhibitions); - - if (dmn->autoShutdownInhibitions == 0) { - VIR_DEBUG("Closing inhibit FD %d", dmn->autoShutdownInhibitFd); - VIR_FORCE_CLOSE(dmn->autoShutdownInhibitFd); - } } -- 2.46.0
On 12/17/24 12:15, Daniel P. Berrangé wrote:
We recently wired up inhibitors in the network driver to prevent auto-shutdown of libvirtd/virtnetworkd when virtual networks are active. This is to prevent the repeated re-creation of firewall rules which happens on daemon startup.
Unfortunately we forgot that an inhibition on libvirt daemon shutdown *also* gets unconditionally turned into an inhibition on OS shutdown :-(
In fixing this I realized this is actually the second such mistakes, because a few years ago we made the virtsecretd daemon inhibit when ephemeral secrets are present, and that really has no justification for blocking system shutdown either.
Daniel P. Berrangé (3): util: introduce object for holding a system inhibitor lock src: convert drivers over to new virInhibitor APIs rpc: remove logind support for virNetDaemon
po/POTFILES | 1 + src/libvirt_private.syms | 7 + src/libxl/libxl_conf.h | 9 +- src/libxl/libxl_domain.c | 6 +- src/libxl/libxl_driver.c | 15 ++- src/lxc/lxc_conf.h | 9 +- src/lxc/lxc_driver.c | 13 +- src/lxc/lxc_process.c | 9 +- src/network/bridge_driver.c | 20 +-- src/network/bridge_driver_conf.h | 9 +- src/qemu/qemu_conf.h | 9 +- src/qemu/qemu_driver.c | 12 +- src/qemu/qemu_process.c | 9 +- src/rpc/virnetdaemon.c | 78 ----------- src/secret/secret_driver.c | 46 +++---- src/util/meson.build | 1 + src/util/virinhibitor.c | 214 +++++++++++++++++++++++++++++++ src/util/virinhibitor.h | 58 +++++++++ 18 files changed, 361 insertions(+), 164 deletions(-) create mode 100644 src/util/virinhibitor.c create mode 100644 src/util/virinhibitor.h
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Daniel P. Berrangé -
Michal Prívozník