On 03/22/2017 10:43 AM, Michal Privoznik wrote:
The virMacMap module is there for dumping [domain, <list of is MACs>] pairs into a file so that libvirt_guest NSS module can use it. Whenever a interface is allocated from network (e.g. on domani startup or NIC hotplug), network is notified and so is
s/domani/domain/
virMacMap module subsequently. The module update functions networkMacMgrAdd() and networkMacMgrDel() handle the case when there's no module gracefully.
"gracefully handle the case when there's no module"
Problem is, the module is created
The problem is...
if and only if network is freshly started, or if daemon restarts
*the* daemon
and network previously had the module.
This is not very user friendly - if users want to use the NSS module they need to destroy their network and bring it up again (and subsequently all the domains using it).
(note that it's no longer quite as bad - as of a few days ago, if you restart a network, restarting libvirtd will reconnect all guests that were previously connected to that network. Still not nice of course...)
One disadvantage of this approach implemented here is that one may get just partial results: any already running network does not record mac maps, thus only newly plugged domains will be stored in the module. The network restart scenario is not touched by this of course. But one can argue that older libvirts had never recorded the mac maps anyway.
It's unclear above where you're talking about the way the code was, and the way it is after this patch...
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index a753cd78b..0ea8e2348 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -470,6 +470,7 @@ networkUpdateState(virNetworkObjPtr obj, { virNetworkDriverStatePtr driver = opaque; dnsmasqCapsPtr dnsmasq_caps = networkGetDnsmasqCaps(driver); + char *macMapFile = NULL; int ret = -1;
virObjectLock(obj); @@ -486,6 +487,13 @@ networkUpdateState(virNetworkObjPtr obj, /* If bridge doesn't exist, then mark it inactive */ if (!(obj->def->bridge && virNetDevExists(obj->def->bridge) == 1)) obj->active = 0; + + if (!(macMapFile = networkMacMgrFileName(driver, obj->def->bridge))) + goto cleanup; + + if (!(obj->macmap = virMacMapNew(macMapFile))) + goto cleanup; + break;
... but from what I can understand, the only differences are: 1) the macMapFile used to be regenerated right after reading the radvdpidfile (which in most cases doesn't exist because I think most of the time that same duty is handled by dnsmasq instead), and now it is regenerated *just before* reading radvdpidfile. 2) it used to be regenerated for any network with a def->bridge (so it would also happen for "unmanaged" bridge networks, where libvirt just points to an already-existing bridge), and it is now regenerated only for networks where libvirt creates and destroys the bridge (and might have a dnsmasq instance running. So I'm confused - I must be missing something obvious. Can you explain a bit more?
case VIR_NETWORK_FORWARD_BRIDGE: @@ -512,7 +520,6 @@ networkUpdateState(virNetworkObjPtr obj, /* Try and read dnsmasq/radvd pids of active networks */ if (obj->active && obj->def->ips && (obj->def->nips > 0)) { char *radvdpidbase; - char *macMapFile;
ignore_value(virPidFileReadIfAlive(driver->pidDir, obj->def->name, @@ -527,23 +534,13 @@ networkUpdateState(virNetworkObjPtr obj, radvdpidbase, &obj->radvdPid, RADVD)); VIR_FREE(radvdpidbase); - - if (!(macMapFile = networkMacMgrFileName(driver, obj->def->bridge))) - goto cleanup; - - if (virFileExists(macMapFile) && - !(obj->macmap = virMacMapNew(macMapFile))) { - VIR_FREE(macMapFile); - goto cleanup; - } - - VIR_FREE(macMapFile); }
ret = 0; cleanup: virObjectUnlock(obj); virObjectUnref(dnsmasq_caps); + VIR_FREE(macMapFile); return ret; }