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@redhat.com> --- 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)
[...]