On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote: > Remove the callbacks that the nwfilter driver registers with the domain > object config layer. Instead make the current helper methods call into > the public API for creating/deleting nwfilter bindings. > > Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx> > --- > src/conf/domain_nwfilter.c | 124 +++++++++++++++++++++---- > src/conf/domain_nwfilter.h | 13 --- > src/libvirt_private.syms | 1 - > src/nwfilter/nwfilter_driver.c | 84 +++-------------- > src/nwfilter/nwfilter_gentech_driver.c | 42 --------- > src/nwfilter/nwfilter_gentech_driver.h | 4 - > 6 files changed, 120 insertions(+), 148 deletions(-) > I ran the more "aggressive" Avocado tests based on an old bz (https://bugzilla.redhat.com/show_bug.cgi?id=1034807) and got the following in my debug libvirtd output: 2018-06-15 15:46:18.683+0000: 18817: error : virNWFilterBindingLookupByPortDev:589 : portdev in virNWFilterBindingLookupByPortDev must not be NULL 2018-06-15 15:46:18.684+0000: 18817: error : virNWFilterBindingLookupByPortDev:589 : portdev in virNWFilterBindingLookupByPortDev must not be NULL The first thing the test does is remove the defined interface: <interface type='bridge'> <mac address='52:54:00:9a:9b:9c'/> <source bridge='virbr0'/> <model type='virtio'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> </interface> and then replaces/adds with a new interface: <interface type="bridge"> <mac address="52:54:00:9a:9b:9c" /> <source bridge="virbr0" /> <model type="virtio" /> <address bus="0x00" domain="0x0000" function="0x0" slot="0x03" type="pci" /> <filterref filter="allow-arp" /> </interface> for the test domain via device attach. Then 2 threads are started - 1 that continually starts/destroys the domain and 1 that continually removes/adds allow-arp. The actual logic in the test is busted because starting up a domain without a defined filter will fail and the thread doing the start/stop has no retry (try/except) logic. When I added the try/except logic and toned down the retry logic a bit I could get the test to pass with a few adjustments to the libvirt code here. Ironically, when the test goes too fast it caused my CPU's to get hot and generate a false positive failure because there were dmesg events related to core temperature above threshold. Anyway, I've noted a couple of places I think adjustments could/should be made - hopefully they make sense as I started looking "backwards" to see where things go introduced. > diff --git a/src/conf/domain_nwfilter.c b/src/conf/domain_nwfilter.c > index 7570e0ae83..ed45394918 100644 > --- a/src/conf/domain_nwfilter.c > +++ b/src/conf/domain_nwfilter.c [...] > > + > int > virDomainConfNWFilterInstantiate(const char *vmname, > const unsigned char *vmuuid, > virDomainNetDefPtr net) Revisiting my comment from patch 13 - it is possible to enter here with net->filter being NULL. Is it worth returning 0 in that case under the assumption that there is no filter so that the callers then don't have to make that check? If portdev/ifname is NULL, not much is going to be found as well. > { > - if (nwfilterDriver != NULL) > - return nwfilterDriver->instantiateFilter(vmname, vmuuid, net); > + virConnectPtr conn = virGetConnectNWFilter(); > + virNWFilterBindingDefPtr def = NULL; > + virNWFilterBindingPtr binding = NULL; > + char *xml; > + int ret = -1; > + > + VIR_DEBUG("vmname=%s portdev=%s filter=%s", > + vmname, NULLSTR(net->ifname), NULLSTR(net->filter)); Both being NULL probably is not a good thing - what filter for what portdev? > + > + if (!conn) > + goto cleanup; > + Based on patch 16/19 comments and the thought above: if (!net->ifname || (binding = virNWFilterBindingLookupByPortDev(conn, net->ifname))) { ret = 0; goto cleanup; } where the !net->ifname may avoids the patch 13 comment issue and the ret = 0 when finding the binding handles the filter already loaded issue. The filter would be loaded for a running guest (after at least the second libvirtd restart) by virNWFilterBindingObjListLoadAllConfigs. > + if (!(def = virNWFilterBindingDefForNet(vmname, vmuuid, net))) > + goto cleanup; > + > + if (!(xml = virNWFilterBindingDefFormat(def))) > + goto cleanup; > + > + if (!(binding = virNWFilterBindingCreateXML(conn, xml, 0))) > + goto cleanup; > + > + ret = 0; > + > + cleanup: > + VIR_FREE(xml); > + virNWFilterBindingDefFree(def); > + virObjectUnref(binding); > + virObjectUnref(conn); > + return ret; > +} > + > + > +static void > +virDomainConfNWFilterTeardownImpl(virConnectPtr conn, > + virDomainNetDefPtr net) > +{ > + virNWFilterBindingPtr binding; > > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > - _("No network filter driver available")); > - return -1; > + binding = virNWFilterBindingLookupByPortDev(conn, net->ifname); > + if (!binding) virNWFilterBindingLookupByPortDev will generate an error when there's no filter defined for @net (if you're running libvirtd in a debugger you see it). Shouldn't the call be guarded by a "if (!net->filter)"? if (!net->filter || !binding = vir...()) return; if not, then we probably should reset the last error since we're just returning (error as in [1]). > + return; > + > + virNWFilterBindingDelete(binding); > + > + virObjectUnref(binding); > } > > + > void > virDomainConfNWFilterTeardown(virDomainNetDefPtr net) > { > - if (nwfilterDriver != NULL) > - nwfilterDriver->teardownFilter(net); > + virConnectPtr conn = virGetConnectNWFilter(); > + > + if (!conn) > + return; > + > + virDomainConfNWFilterTeardownImpl(conn, net); > + > + virObjectUnref(conn); > } > may as well add the blank line here from ... > void > virDomainConfVMNWFilterTeardown(virDomainObjPtr vm) > { > size_t i; > + virConnectPtr conn = virGetConnectNWFilter(); > + > + if (!conn) > + return; > + ... here... or at least remove this extra blank line. > + > + for (i = 0; i < vm->def->nnets; i++) > + virDomainConfNWFilterTeardownImpl(conn, vm->def->nets[i]); > > - if (nwfilterDriver != NULL) { > - for (i = 0; i < vm->def->nnets; i++) > - virDomainConfNWFilterTeardown(vm->def->nets[i]); > - } > + virObjectUnref(conn); > } [...] > > diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c > index 2b6856a36c..26e6e76b3b 100644 > --- a/src/nwfilter/nwfilter_driver.c > +++ b/src/nwfilter/nwfilter_driver.c > @@ -654,67 +654,6 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter, > } > > [...] > static virNWFilterBindingPtr > nwfilterBindingLookupByPortDev(virConnectPtr conn, > const char *portdev) > @@ -725,8 +664,11 @@ nwfilterBindingLookupByPortDev(virConnectPtr conn, > > obj = virNWFilterBindingObjListFindByPortDev(driver->bindings, > portdev); > - if (!obj) > + if (!obj) { > + virReportError(VIR_ERR_NO_NWFILTER_BINDING, > + _("no nwfilter binding for port dev '%s'"), portdev);454 [1] Adding this error here for someone running debug will cause those virDomainConfNWFilterTeardownImpl consumers w/o a filter to display this message. Of course removing it means the callers all have to add some sort of message. John > goto cleanup; > + } > > def = virNWFilterBindingObjGetDef(obj); > if (virNWFilterBindingLookupByPortDevEnsureACL(conn, def) < 0) [...] -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
