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

[John Ferlan]

On 04/17/2014 07:43 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
> active 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
> libvirt driver that stores status xml for its objects). The difference
> is that /var/run is cleared out when the host reboots, so you can 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 networkUpdateAllState(),
> 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
> libvirtd is restarted, any network with a status file will be marked
> as active (unless the network uses a bridge device and that device for
> some reason doesn't exist).
> ---
> Changes from V1:
> 
> * rename networkFindActiveConfigs() to networkUpdateAllState() (rather
>   than networkFindInactiveConfigs()
> 
> * undo the change in order of calling the above function
>   vs. virNetworkReadAllConfigs(), just in case that would cause some
>   undetected regression.
> 
> * extricate the reading of pidfiles from the switch statement that
>   behaves differently for different types of networks - those networks
>   that don't use dnsmasq/radvd will not have any pidfiles for them
>   anyway, so it becomes a NOP (and if a new network type that *does*
>   use one of those processes is created, it will automatically work
>   correctly here.)
> 
> 
>  src/conf/network_conf.c     |  1 +
>  src/network/bridge_driver.c | 69 ++++++++++++++++++++++++++++++++-------------
>  2 files changed, 51 insertions(+), 19 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 */

Typing and thinking - it's a dangerous combination... Since the driver
initialization now "moves" files from /lib/ to /pid/ without regard to
whether they were active previously in /lib/ - is it possible that other
code will erroneously mark something active?

The prior code would look through the state files to mark active... This
code says, I found a state file thus I must be active.  Of course only
true for migration case - so hmm.... does the previous patch need a tweak?

>  
>   cleanup:
>      VIR_FREE(configFile);
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 57dfb2d..0c879b9 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -328,38 +328,69 @@ networkBridgeDummyNicName(const char *brname)
>      return nicname;
>  }
>  
> +/* Update the internal status of all allegedly active networks
> + * according to external conditions on the host (i.e. anything that
> + * isn't stored directly in each network's state file). */
>  static void
> -networkFindActiveConfigs(virNetworkDriverStatePtr driver)
> +networkUpdateAllState(virNetworkDriverStatePtr driver)
>  {
>      size_t i;
>  
>      for (i = 0; i < driver->networks.count; i++) {
>          virNetworkObjPtr obj = driver->networks.objs[i];
>  
> +        if (!obj->active)
> +           continue;
> +
>          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;
> +            break;

Comment is still misleading...

ACK with that change

John

>  
> -            /* Try and read dnsmasq/radvd pids if any */
> -            if (obj->def->ips && (obj->def->nips > 0)) {
> -                char *radvdpidbase;
> +        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:
> +            /* so far no extra checks */
> +            break;
>  
> -                ignore_value(virPidFileReadIfAlive(driverState->pidDir, obj->def->name,
> -                                                   &obj->dnsmasqPid,
> -                                                   dnsmasqCapsGetBinaryPath(driver->dnsmasqCaps)));
> +        case VIR_NETWORK_FORWARD_HOSTDEV:
> +            /* so far no extra checks */
> +            break;
> +        }
>  
> -                if (!(radvdpidbase = networkRadvdPidfileBasename(obj->def->name)))
> -                    goto cleanup;
> -                ignore_value(virPidFileReadIfAlive(driverState->pidDir, radvdpidbase,
> -                                                   &obj->radvdPid, RADVD));
> -                VIR_FREE(radvdpidbase);
> -            }
> +        /* Try and read dnsmasq/radvd pids of active networks */
> +        if (obj->active && obj->def->ips && (obj->def->nips > 0)) {
> +            char *radvdpidbase;
> +
> +            ignore_value(virPidFileReadIfAlive(driverState->pidDir,
> +                                               obj->def->name,
> +                                               &obj->dnsmasqPid,
> +                                               dnsmasqCapsGetBinaryPath(driver->dnsmasqCaps)));
> +            radvdpidbase = networkRadvdPidfileBasename(obj->def->name);
> +            if (!radvdpidbase)
> +                break;
> +            ignore_value(virPidFileReadIfAlive(driverState->pidDir,
> +                                               radvdpidbase,
> +                                               &obj->radvdPid, RADVD));
> +            VIR_FREE(radvdpidbase);
>          }
>  
> -    cleanup:
>          virNetworkObjUnlock(obj);
>      }
>  
> @@ -591,7 +622,7 @@ networkStateInitialize(bool privileged,
>                                   driverState->networkAutostartDir) < 0)
>          goto error;
>  
> -    networkFindActiveConfigs(driverState);
> +    networkUpdateAllState(driverState);
>      networkReloadFirewallRules(driverState);
>      networkRefreshDaemons(driverState);
>  
>