Re: [libvirt] [PATCH v3 16/20] nwfilter: keep track of active filter bindings

On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote: > Currently the nwfilter driver does not keep any record of what filter > bindings it has active. This means that when it needs to recreate > filters, it has to rely on triggering callbacks provided by the virt > drivers. This introduces a hash table recording the virNWFilterBinding > objects so the driver has a record of all active filters. > > Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; > --- > src/conf/virnwfilterobj.h | 4 ++ > src/nwfilter/nwfilter_driver.c | 86 ++++++++++++++++++++++++---------- > 2 files changed, 65 insertions(+), 25 deletions(-) > [...] > diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c > index 7202691646..2388e925fc 100644 > --- a/src/nwfilter/nwfilter_driver.c > +++ b/src/nwfilter/nwfilter_driver.c > @@ -38,7 +38,6 @@ > #include "domain_conf.h" > #include "domain_nwfilter.h" > #include "nwfilter_driver.h" > -#include "virnwfilterbindingdef.h" > #include "nwfilter_gentech_driver.h" > #include "configmake.h" > #include "virfile.h" > @@ -174,7 +173,6 @@ nwfilterStateInitialize(bool privileged, > virStateInhibitCallback callback ATTRIBUTE_UNUSED, > void *opaque ATTRIBUTE_UNUSED) > { [...] > > if (virNWFilterObjListLoadAllConfigs(driver->nwfilters, driver->configDir) < 0) > goto error; > > + if (virNWFilterBindingObjListLoadAllConfigs(driver->bindings, driver->bindingDir) < 0) > + goto error; > + Because of this... [1] > nwfilterDriverUnlock(); > > return 0; > [...] > @@ -647,13 +656,37 @@ nwfilterInstantiateFilter(const char *vmname, > const unsigned char *vmuuid, > virDomainNetDefPtr net) > { > - virNWFilterBindingDefPtr binding; > + virNWFilterBindingObjPtr obj; > + virNWFilterBindingDefPtr def; > int ret; > > - if (!(binding = virNWFilterBindingDefForNet(vmname, vmuuid, net))) > + obj = virNWFilterBindingObjListFindByPortDev(driver->bindings, net->ifname); > + if (obj) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Filter already present for NIC %s"), net->ifname); [1] If I stop at this patch, start a domain with a filter, then restart libvirtd, then that will cause a guest running with a filter to exit and not just disappear - as it can be restarted. The error is: 2018-06-18 16:49:07.405+0000: 3978: error : nwfilterInstantiateFilter:666 : internal error: Filter already present for NIC vnet1 Because once I have this patch built/running the /var/run/libvirt/nwfilter-binding/vnet1.xml exists and get's loaded when virNWFilterBindingObjListLoadAllConfigs is called at StateInitialize. I only saw this because I found later in patch 20 the failure comes from nwfilterBindingCreateXML instead when virDomainConfNWFilterInstantiate is called. I worked my way back to this point. Not sure which would be the "best" solution at this point. Should we wait to do [1] until patch 20 or perhaps just not have an error here.

NB: If the guest was running at a point up through patch 15 then it won't exit on the first libvirtd restart (since the obj dir doesn't exist), but the issue shows up on the 2nd restart. In general the code is fine to me, but just need to handle this one issue in some way.