> 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@xxxxxxxxxx>
> ---
> @@ -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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list