[libvirt] [PATCH 4/5] network: set macvtap/hostdev networks active if their state file exists

[John Ferlan]

On 04/10/2014 09:19 AM, Laine Stump wrote:
> libvirt attempts to determine at startup time which networks are
> already active, and set their active flags. Previously it has done
> this by assuming that all networks are inactive, then setting the flag
> if the network has a bridge device associated with it, and that bridge
> device exists. This is not useful for macvtap and hostdev based
> networks, since they do not use a bridge device.
> 
> Of course the reason that such a check had to be done was that the
> presence of a status file in the network "stateDir" couldn't be
> trusted as an indicator of whether or not a network was active. This
> was due to the network driver mistakenly using
> /var/lib/libvirt/network to store the status files, rather than
> /var/run/libvirt/network (similar to what is done by every other
> libirt driver that stores status xml for its objects). The difference
> is that /var/run is cleared out when the host reboots, so you cvqan be
> assured that the state file you are seeing isn't just left over from a
> previous boot of the host.
> 
> Now that the network driver has been switched to using
> /var/run/libvirt/network for status, we can also modify it to assume
> that any network with an existing status file is by definition active
> - we do this when reading the status file. To fine tune the results,
> networkFindActiveConfigs() is changed to networkFindInactiveConfigs(),
> and only sets active = 0 if the conditions for particular network
> types are *not* met.
> 
> The result is that during the first run of libvirtd after the host
> boots, there are no status files, so no networks are active. Any time
> it is restarted, and network with a status file will be marked as
> inactive (unless it uses a bridge device and that device for some
> reason doesn't exist).
> ---
>  src/conf/network_conf.c     |  1 +
>  src/network/bridge_driver.c | 49 +++++++++++++++++++++++++++++++++------------
>  2 files changed, 37 insertions(+), 13 deletions(-)
> 
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index 56c4a09..69ad929 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -3060,6 +3060,7 @@ virNetworkLoadState(virNetworkObjListPtr nets,
>          net->floor_sum = floor_sum_val;
>  
>      net->taint = taint;
> +    net->active = 1; /* any network with a state file is by definition active */
>  
>   cleanup:
>      VIR_FREE(configFile);
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 98d10bd..7beab79 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -328,7 +328,7 @@ networkBridgeDummyNicName(const char *brname)
>  }
>  
>  static void
> -networkFindActiveConfigs(virNetworkDriverStatePtr driver)
> +networkFindInactiveConfigs(virNetworkDriverStatePtr driver)
>  {
>      size_t i;
>  
> @@ -336,29 +336,51 @@ networkFindActiveConfigs(virNetworkDriverStatePtr driver)
>          virNetworkObjPtr obj = driver->networks.objs[i];
>  
>          virNetworkObjLock(obj);
> -
> -        /* If bridge exists, then mark it active */
> -        if (obj->def->bridge &&
> -            virNetDevExists(obj->def->bridge) == 1) {
> -            obj->active = 1;
> +        switch (obj->def->forward.type) {
> +        case VIR_NETWORK_FORWARD_NONE:
> +        case VIR_NETWORK_FORWARD_NAT:
> +        case VIR_NETWORK_FORWARD_ROUTE:
> +            /* If bridge exists, then mark it active */
> +            if (!(obj->def->bridge && virNetDevExists(obj->def->bridge) == 1)) {
> +                obj->active = 0;

Goes against comment...  double negatives in if condition are throwing
me off.  The switch/case break almost got me too (thinking that the
break was to break out of the for loop).

> +                break;
> +            }
>  
>              /* Try and read dnsmasq/radvd pids if any */
>              if (obj->def->ips && (obj->def->nips > 0)) {
>                  char *radvdpidbase;
>  
> -                ignore_value(virPidFileReadIfAlive(driverState->pidDir, obj->def->name,
> +                ignore_value(virPidFileReadIfAlive(driverState->pidDir,
> +                                                   obj->def->name,
>                                                     &obj->dnsmasqPid,
>                                                     dnsmasqCapsGetBinaryPath(driver->dnsmasqCaps)));
> -
> -                if (!(radvdpidbase = networkRadvdPidfileBasename(obj->def->name)))
> -                    goto cleanup;
> -                ignore_value(virPidFileReadIfAlive(driverState->pidDir, radvdpidbase,
> +                radvdpidbase = networkRadvdPidfileBasename(obj->def->name);
> +                if (!radvdpidbase)
> +                    break;
> +                ignore_value(virPidFileReadIfAlive(driverState->pidDir,
> +                                                   radvdpidbase,
>                                                     &obj->radvdPid, RADVD));
>                  VIR_FREE(radvdpidbase);

I'm confused as to why the code would be doing this as the active value
isn't changed as a result of this.  Was it just "convenient" to do it
here?  In the context of the old code, if we're marking active, then
this code runs.  In the context of the new code, for only specific types
of network forward types that we've ostensibly already deemed active
somewhere (virNetworkLoadState()?) we'll then run this code... and I'm
assuming that if we get this far obj->active == 1 - whether that's true
or not I'm not 100%, but I assume you'd know.  Of course, that could be
the problem you're trying to resolve in 5...

Furthermore, since the check for inactive is before the load of the
config now, are you getting what you expected in the right place here?
Previously it'd be load all configs, check active.  Now it's check
inactive, load all configs.  If the above is loaded during that process,
then the above is all for naught.


ACK pending your thoughts on this above hunk.  Might need to see a v2...

John

>              }
> +            break;
> +
> +        case VIR_NETWORK_FORWARD_BRIDGE:
> +            if (obj->def->bridge) {
> +                if (virNetDevExists(obj->def->bridge) != 1)
> +                    obj->active = 0;
> +                break;
> +            }
> +            /* intentionally drop through to common case for all
> +             * macvtap networks (forward='bridge' with no bridge
> +             * device defined is macvtap using its 'bridge' mode)
> +             */
> +        case VIR_NETWORK_FORWARD_PRIVATE:
> +        case VIR_NETWORK_FORWARD_VEPA:
> +        case VIR_NETWORK_FORWARD_PASSTHROUGH:
> +        case VIR_NETWORK_FORWARD_HOSTDEV:
> +            break;
>          }
>  
> -    cleanup:
>          virNetworkObjUnlock(obj);
>      }
>  
> @@ -542,12 +564,13 @@ networkStateInitialize(bool privileged,
>                                 driverState->stateDir) < 0)
>          goto error;
>  
> +    networkFindInactiveConfigs(driverState);
> +
>      if (virNetworkLoadAllConfigs(&driverState->networks,
>                                   driverState->networkConfigDir,
>                                   driverState->networkAutostartDir) < 0)
>          goto error;
>  
> -    networkFindActiveConfigs(driverState);
>      networkReloadFirewallRules(driverState);
>      networkRefreshDaemons(driverState);
>  
>