[libvirt] [PATCH 1/2] networkUpdateState: Create virMacMap module more frequently

[Laine Stump]

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 at 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;
>  }
>  
>