[libvirt] [PATCH v3 0/2] nwfilter: Compare filters for equality when updating
When a filter is updated, compare it and the original one for equality so that unnecessary instantiations of rules can be avoided. v3: - introducing a function to instantiate all running VMs' filters; call it upon nwfilter driver reload - reworked function comparing filters; determining whether two filters are equal now by comparing their XML representations v2: - added a test case for testing virHashEqual Regards, Stefan
Introduce a function that rebuilds all running VMs' filters. Call this function when reloading the nwfilter driver. This addresses a problem introduced by the 2nd patch that typically causes no filters to be reinstantiate anymore upon driver reload since their XML has not changed. Yet the current behavior is that upon a SIGHUP all filters get reinstantiated. --- src/conf/nwfilter_conf.c | 23 +++++++++++++++++++++++ src/conf/nwfilter_conf.h | 3 +++ src/libvirt_private.syms | 1 + src/nwfilter/nwfilter_driver.c | 2 ++ src/nwfilter/nwfilter_gentech_driver.c | 12 +++++++++++- 5 files changed, 40 insertions(+), 1 deletion(-) Index: libvirt-iterator/src/conf/nwfilter_conf.c =================================================================== --- libvirt-iterator.orig/src/conf/nwfilter_conf.c +++ libvirt-iterator/src/conf/nwfilter_conf.c @@ -2723,6 +2723,29 @@ virNWFilterCallbackDriversUnlock(void) static virHashIterator virNWFilterDomainFWUpdateCB; +/** + * virNWFilterInstFiltersOnAllVMs: + * Apply all filters on all running VMs. Don't terminate in case of an + * error. This should be called upon reloading of the driver. + */ +int +virNWFilterInstFiltersOnAllVMs(virConnectPtr conn) +{ + int i; + struct domUpdateCBStruct cb = { + .conn = conn, + .err = 0, /* ignored here */ + .step = STEP_APPLY_CURRENT, + .skipInterfaces = NULL, /* not needed */ + }; + + for (i = 0; i < nCallbackDriver; i++) + callbackDrvArray[i]->vmFilterRebuild(conn, + virNWFilterDomainFWUpdateCB, + &cb); + + return 0; +} static int virNWFilterTriggerVMFilterRebuild(virConnectPtr conn) Index: libvirt-iterator/src/conf/nwfilter_conf.h =================================================================== --- libvirt-iterator.orig/src/conf/nwfilter_conf.h +++ libvirt-iterator/src/conf/nwfilter_conf.h @@ -577,6 +577,7 @@ enum UpdateStep { STEP_APPLY_NEW, STEP_TEAR_NEW, STEP_TEAR_OLD, + STEP_APPLY_CURRENT, }; struct domUpdateCBStruct { @@ -722,6 +723,8 @@ void virNWFilterUnlockFilterUpdates(void int virNWFilterConfLayerInit(virHashIterator domUpdateCB); void virNWFilterConfLayerShutdown(void); +int virNWFilterInstFiltersOnAllVMs(virConnectPtr conn); + # define virNWFilterReportError(code, fmt...) \ virReportErrorHelper(VIR_FROM_NWFILTER, code, __FILE__, \ __FUNCTION__, __LINE__, fmt) Index: libvirt-iterator/src/nwfilter/nwfilter_driver.c =================================================================== --- libvirt-iterator.orig/src/nwfilter/nwfilter_driver.c +++ libvirt-iterator/src/nwfilter/nwfilter_driver.c @@ -162,6 +162,8 @@ nwfilterDriverReload(void) { virNWFilterCallbackDriversUnlock(); nwfilterDriverUnlock(driverState); + virNWFilterInstFiltersOnAllVMs(conn); + virConnectClose(conn); } Index: libvirt-iterator/src/libvirt_private.syms =================================================================== --- libvirt-iterator.orig/src/libvirt_private.syms +++ libvirt-iterator/src/libvirt_private.syms @@ -811,6 +811,7 @@ virNWFilterConfLayerShutdown; virNWFilterDefFormat; virNWFilterDefFree; virNWFilterDefParseString; +virNWFilterInstFiltersOnAllVMs; virNWFilterJumpTargetTypeToString; virNWFilterLoadAllConfigs; virNWFilterLockFilterUpdates; Index: libvirt-iterator/src/nwfilter/nwfilter_gentech_driver.c =================================================================== --- libvirt-iterator.orig/src/nwfilter/nwfilter_gentech_driver.c +++ libvirt-iterator/src/nwfilter/nwfilter_gentech_driver.c @@ -1122,7 +1122,7 @@ virNWFilterDomainFWUpdateCB(void *payloa virDomainObjPtr obj = payload; virDomainDefPtr vm = obj->def; struct domUpdateCBStruct *cb = data; - int i; + int i, err; bool skipIface; virDomainObjLock(obj); @@ -1156,6 +1156,16 @@ virNWFilterDomainFWUpdateCB(void *payloa cb->err = virNWFilterTearOldFilter(net); } break; + + case STEP_APPLY_CURRENT: + err = virNWFilterInstantiateFilter(cb->conn, + vm->uuid, + net); + if (err) + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("Failure while applying current filter on " + "VM %s"), vm->name); + break; } if (cb->err) break;
On 01/25/2012 11:58 AM, Stefan Berger wrote:
Introduce a function that rebuilds all running VMs' filters. Call this function when reloading the nwfilter driver.
This addresses a problem introduced by the 2nd patch that typically causes no filters to be reinstantiate anymore upon driver reload since their XML has not changed. Yet the current behavior is that upon a SIGHUP all filters get reinstantiated.
--- src/conf/nwfilter_conf.c | 23 +++++++++++++++++++++++ src/conf/nwfilter_conf.h | 3 +++ src/libvirt_private.syms | 1 + src/nwfilter/nwfilter_driver.c | 2 ++ src/nwfilter/nwfilter_gentech_driver.c | 12 +++++++++++- 5 files changed, 40 insertions(+), 1 deletion(-)
ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Compare two filters' XML for equality and only rebuild/instantiate the new filter if the new and current filters are found to be different. This improves performance during an update of a filter with no obvious change or the reloading of filters during a 'kill -SIGHUP' --- src/conf/nwfilter_conf.c | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) Index: libvirt-iterator/src/conf/nwfilter_conf.c =================================================================== --- libvirt-iterator.orig/src/conf/nwfilter_conf.c +++ libvirt-iterator/src/conf/nwfilter_conf.c @@ -2809,6 +2809,35 @@ virNWFilterTestUnassignDef(virConnectPtr return rc; } +static bool +virNWFilterDefEqual(const virNWFilterDefPtr def1, virNWFilterDefPtr def2, + bool cmpUUIDs) +{ + bool ret = false; + unsigned char rem_uuid[VIR_UUID_BUFLEN]; + char *xml1, *xml2 = NULL; + + if (!cmpUUIDs) { + /* make sure the UUIDs are equal */ + memcpy(rem_uuid, def2->uuid, sizeof(rem_uuid)); + memcpy(def2->uuid, def1->uuid, sizeof(def2->uuid)); + } + + if (!(xml1 = virNWFilterDefFormat(def1)) || + !(xml2 = virNWFilterDefFormat(def2))) + goto cleanup; + + ret = STREQ(xml1, xml2); + + if (!cmpUUIDs) + memcpy(def2->uuid, rem_uuid, sizeof(rem_uuid)); + +cleanup: + VIR_FREE(xml1); + VIR_FREE(xml2); + + return ret; +} virNWFilterObjPtr virNWFilterObjAssignDef(virConnectPtr conn, @@ -2840,6 +2869,14 @@ virNWFilterObjAssignDef(virConnectPtr co virNWFilterLockFilterUpdates(); if ((nwfilter = virNWFilterObjFindByName(nwfilters, def->name))) { + + if (virNWFilterDefEqual(def, nwfilter->def, false)) { + virNWFilterDefFree(nwfilter->def); + nwfilter->def = def; + virNWFilterUnlockFilterUpdates(); + return nwfilter; + } + nwfilter->newDef = def; /* trigger the update on VMs referencing the filter */ if (virNWFilterTriggerVMFilterRebuild(conn)) {
On 01/25/2012 11:58 AM, Stefan Berger wrote:
Compare two filters' XML for equality and only rebuild/instantiate the new filter if the new and current filters are found to be different. This improves performance during an update of a filter with no obvious change or the reloading of filters during a 'kill -SIGHUP'
--- src/conf/nwfilter_conf.c | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)
Index: libvirt-iterator/src/conf/nwfilter_conf.c =================================================================== --- libvirt-iterator.orig/src/conf/nwfilter_conf.c +++ libvirt-iterator/src/conf/nwfilter_conf.c @@ -2809,6 +2809,35 @@ virNWFilterTestUnassignDef(virConnectPtr return rc; }
+static bool +virNWFilterDefEqual(const virNWFilterDefPtr def1, virNWFilterDefPtr def2, + bool cmpUUIDs) +{ + bool ret = false; + unsigned char rem_uuid[VIR_UUID_BUFLEN]; + char *xml1, *xml2 = NULL; + + if (!cmpUUIDs) { + /* make sure the UUIDs are equal */ + memcpy(rem_uuid, def2->uuid, sizeof(rem_uuid)); + memcpy(def2->uuid, def1->uuid, sizeof(def2->uuid)); + } + + if (!(xml1 = virNWFilterDefFormat(def1)) || + !(xml2 = virNWFilterDefFormat(def2))) + goto cleanup; + + ret = STREQ(xml1, xml2); + + if (!cmpUUIDs) + memcpy(def2->uuid, rem_uuid, sizeof(rem_uuid)); + +cleanup:
Misplaced label. You need to slide it up two lines, and unconditionally call the memcpy() to undo things when !cmpUUIDs. ACK with that fixed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/25/2012 04:41 PM, Eric Blake wrote:
On 01/25/2012 11:58 AM, Stefan Berger wrote:
Compare two filters' XML for equality and only rebuild/instantiate the new filter if the new and current filters are found to be different. This improves performance during an update of a filter with no obvious change or the reloading of filters during a 'kill -SIGHUP'
--- src/conf/nwfilter_conf.c | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)
Index: libvirt-iterator/src/conf/nwfilter_conf.c =================================================================== --- libvirt-iterator.orig/src/conf/nwfilter_conf.c +++ libvirt-iterator/src/conf/nwfilter_conf.c @@ -2809,6 +2809,35 @@ virNWFilterTestUnassignDef(virConnectPtr return rc; }
+static bool +virNWFilterDefEqual(const virNWFilterDefPtr def1, virNWFilterDefPtr def2, + bool cmpUUIDs) +{ + bool ret = false; + unsigned char rem_uuid[VIR_UUID_BUFLEN]; + char *xml1, *xml2 = NULL; + + if (!cmpUUIDs) { + /* make sure the UUIDs are equal */ + memcpy(rem_uuid, def2->uuid, sizeof(rem_uuid)); + memcpy(def2->uuid, def1->uuid, sizeof(def2->uuid)); + } + + if (!(xml1 = virNWFilterDefFormat(def1)) || + !(xml2 = virNWFilterDefFormat(def2))) + goto cleanup; + + ret = STREQ(xml1, xml2); + + if (!cmpUUIDs) + memcpy(def2->uuid, rem_uuid, sizeof(rem_uuid)); + +cleanup: Misplaced label. You need to slide it up two lines, and unconditionally call the memcpy() to undo things when !cmpUUIDs.
ACK with that fixed.
Pushed both patches with fixes applied.
participants (2)
-
Eric Blake -
Stefan Berger