4:02 p.m.
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(a)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)
[...]