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