[libvirt] [PATCH 08/10] Replace polling for active VMs with signalling by drivers

[Eric Blake]

> 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 at 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.