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