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

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