Currently to deal with auto-shutdown libvirtd must periodically poll all stateful drivers. Thus sucks because it requires acquiring both the driver lock and locks on every single virtual machine. Instead pass in a "inhibit" callback to virStateInitialize which drivers can invoke whenever they want to inhibit shutdown due to existance of active VMs.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> ---
@@ -772,6 +762,18 @@ static int daemonSetupSignals(virNetServerPtr srv) return 0; }
+ +static void daemonInhibitCallback(bool inhibit, void *opaque) +{ + virNetServerPtr srv = opaque; + + if (inhibit) + virNetServerAddShutdownInhibition(srv); + else + virNetServerRemoveShutdownInhibition(srv); +}
Nice. Is the intent that drivers call this once on the first guest to start, and again on the last guest stopped, or once on every single guest start/stop action? Either way, it seems like the inhibition has to be reference counted to deal with more than one driver having a reason for inhibition among a single libvirtd service.
+++ b/src/nwfilter/nwfilter_driver.c @@ -165,7 +165,9 @@ nwfilterDriverInstallDBusMatches(DBusConnection *sysbus ATTRIBUTE_UNUSED) * Initialization function for the QEmu daemon */ static int -nwfilterDriverStartup(bool privileged) +nwfilterDriverStartup(bool privileged ATTRIBUTE_UNUSED, + virStateInhibitCallback callback ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) {
Here, you aren't remembering the callback...
char *base = NULL; DBusConnection *sysbus = NULL; @@ -305,27 +307,6 @@ nwfilterDriverReload(void) { return 0; }
-/** - * virNWFilterActive: - * - * Checks if the nwfilter driver is active, i.e. has an active nwfilter - * - * Returns 1 if active, 0 otherwise - */ -static int -nwfilterDriverActive(void) { - int ret; - - if (!driverState) - return 0; - - nwfilterDriverLock(driverState); - ret = driverState->nwfilters.count ? 1 : 0; - ret |= driverState->watchingFirewallD; - nwfilterDriverUnlock(driverState); - - return ret;
But the old code could inhibit shutdown if a nwfilter was active. Is this intentional?
+ +void virNetServerAddShutdownInhibition(virNetServerPtr srv) +{ + virNetServerLock(srv); + srv->autoShutdownInhibitions++; + virNetServerUnlock(srv); +} + + +void virNetServerRemoveShutdownInhibition(virNetServerPtr srv) +{ + virNetServerLock(srv); + srv->autoShutdownInhibitions--; + virNetServerUnlock(srv); +}
Do these have to obtain full-blown locks, or can you use atomic increments outside of any other locking?
static int -storageDriverStartup(bool privileged) +storageDriverStartup(bool privileged, + virStateInhibitCallback callback ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED)
Another case of ignoring the callback...
{ char *base = NULL;
@@ -202,33 +204,6 @@ storageDriverReload(void) { return 0; }
-/** - * virStorageActive: - * - * Checks if the storage driver is active, i.e. has an active pool - * - * Returns 1 if active, 0 otherwise - */ -static int -storageDriverActive(void) { - unsigned int i; - int active = 0; - - if (!driverState) - return 0; - - storageDriverLock(driverState); - - for (i = 0 ; i < driverState->pools.count ; i++) { - virStoragePoolObjLock(driverState->pools.objs[i]); - if (virStoragePoolObjIsActive(driverState->pools.objs[i])) - active = 1; - virStoragePoolObjUnlock(driverState->pools.objs[i]); - } - - storageDriverUnlock(driverState); - return active;
...where the old code could inhibit shutdown. Intentional? Overall, the idea is nice, but answers to my questions will determine whether you need a v2.