[libvirt] [PATCH RFC 0/4]] nwfilter: don't reinstantiate filters if they are not changed
The series adds optimization in network filters instantiation as suggested in [1]. Applied on top of [2]. However this approach has drawback I'm unfortunately discovered too late) Next steps will left us with no network filters after this series applied: systemctl stop libvirtd systemctl restart firewalld systemctl start libvirtd In case of system update libvirt binaries ctime will change and filters will be reinstalled. [1] https://www.redhat.com/archives/libvir-list/2018-October/msg00657.html [2] https://www.redhat.com/archives/libvir-list/2018-October/msg00787.html Nikolay Shirokovskiy (4): nwfilter: add nwfilter hash nwfilter: don't reinstantiate filters if they are not changed nwfilter: force filters reinstantiation on firewalld reload nwfilter: force filters reinstantiation on binary update src/conf/virnwfilterbindingobj.c | 40 +++++++++ src/conf/virnwfilterbindingobj.h | 10 +++ src/conf/virnwfilterobj.c | 145 +++++++++++++++++++++++++++++++++ src/conf/virnwfilterobj.h | 9 ++ src/libvirt_private.syms | 6 ++ src/nwfilter/nwfilter_driver.c | 11 ++- src/nwfilter/nwfilter_gentech_driver.c | 67 +++++++++++++-- src/nwfilter/nwfilter_gentech_driver.h | 6 +- 8 files changed, 283 insertions(+), 11 deletions(-) -- 1.8.3.1
For filter without references to other filters hash is just sha-256 of filter's xml. For filters with references hash is sha-256 of string consisting of filter's self hash and hashes of directly referenced filters. If filter is not complete that is some of filters it's referencing directly or indirectly are missing the hash is set to NULL as well as if there was OOM on hash caculation. So having NULL hash is regular situation. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/virnwfilterobj.c | 145 +++++++++++++++++++++++++++++++++ src/conf/virnwfilterobj.h | 9 ++ src/libvirt_private.syms | 3 + src/nwfilter/nwfilter_driver.c | 3 + src/nwfilter/nwfilter_gentech_driver.c | 2 + 5 files changed, 162 insertions(+) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 0136a0d..4cc81df 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -28,6 +28,7 @@ #include "virlog.h" #include "virnwfilterobj.h" #include "virstring.h" +#include "vircrypto.h" #define VIR_FROM_THIS VIR_FROM_NWFILTER @@ -40,6 +41,8 @@ struct _virNWFilterObj { virNWFilterDefPtr def; virNWFilterDefPtr newDef; + + char *hash; }; struct _virNWFilterObjList { @@ -82,6 +85,13 @@ virNWFilterObjGetNewDef(virNWFilterObjPtr obj) } +char* +virNWFilterObjGetHash(virNWFilterObjPtr obj) +{ + return obj->hash; +} + + bool virNWFilterObjWantRemoved(virNWFilterObjPtr obj) { @@ -307,6 +317,139 @@ virNWFilterDefEqual(const virNWFilterDef *def1, } +static void +virNWFilterObjInvalidateHash(virNWFilterObjListPtr nwfilters, + virNWFilterObjPtr obj) +{ + virNWFilterDefPtr def = obj->def; + size_t i; + + if (!obj->hash) + return; + + if (obj->newDef) { + VIR_FREE(obj->hash); + return; + } + + for (i = 0; i < def->nentries; i++) { + virNWFilterObjPtr child; + char *filterref; + + if (def->filterEntries[i]->rule || + !def->filterEntries[i]->include) + continue; + + filterref = def->filterEntries[i]->include->filterref; + + if (!(child = virNWFilterObjListFindByName(nwfilters, filterref))) { + VIR_FREE(obj->hash); + return; + } + + virNWFilterObjInvalidateHash(nwfilters, child); + + if (!child->hash) { + VIR_FREE(obj->hash); + virNWFilterObjUnlock(child); + return; + } + + virNWFilterObjUnlock(child); + } +} + + +void +virNWFilterObjListInvalidateHash(virNWFilterObjListPtr nwfilters) +{ + size_t i; + virNWFilterObjPtr obj; + + for (i = 0; i < nwfilters->count; i++) { + obj = nwfilters->objs[i]; + virNWFilterObjLock(obj); + virNWFilterObjInvalidateHash(nwfilters, obj); + virNWFilterObjUnlock(obj); + } +} + + +static void +virNWFilterObjUpdateHash(virNWFilterObjListPtr nwfilters, + virNWFilterObjPtr obj) +{ + virNWFilterDefPtr def = obj->newDef ? obj->newDef : obj->def; + size_t i; + char *hash; + char *xml; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (obj->hash) + return; + + if (!(xml = virNWFilterDefFormat(def))) + return; + + if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, xml, &hash)) + goto cleanup; + + virBufferAsprintf(&buf, "%s\n", hash); + VIR_FREE(hash); + + for (i = 0; i < def->nentries; i++) { + char *filterref; + virNWFilterObjPtr child; + + if (def->filterEntries[i]->rule || + !def->filterEntries[i]->include) + continue; + + filterref = def->filterEntries[i]->include->filterref; + + if (!(child = virNWFilterObjListFindByName(nwfilters, filterref))) + goto cleanup; + + virNWFilterObjUpdateHash(nwfilters, child); + + if (!child->hash) { + virNWFilterObjUnlock(child); + goto cleanup; + } + + virBufferAsprintf(&buf, "%s\n", child->hash); + + virNWFilterObjUnlock(child); + } + + if (virBufferCheckError(&buf) < 0) + goto cleanup; + + hash = virBufferContentAndReset(&buf); + ignore_value(virCryptoHashString(VIR_CRYPTO_HASH_SHA256, hash, &obj->hash)); + VIR_FREE(hash); + + cleanup: + virBufferFreeAndReset(&buf); + VIR_FREE(xml); +} + + +void +virNWFilterObjListUpdateHash(virNWFilterObjListPtr nwfilters) +{ + size_t i; + virNWFilterObjPtr obj; + + for (i = 0; i < nwfilters->count; i++) { + obj = nwfilters->objs[i]; + virNWFilterObjLock(obj); + virNWFilterObjUpdateHash(nwfilters, obj); + virNWFilterObjUnlock(obj); + } +} + + virNWFilterObjPtr virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, virNWFilterDefPtr def) @@ -555,6 +698,8 @@ virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters, virNWFilterObjUnlock(obj); } + virNWFilterObjListUpdateHash(nwfilters); + VIR_DIR_CLOSE(dir); return ret; } diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index 4a54dd5..1c41a3e 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -50,6 +50,9 @@ virNWFilterObjGetDef(virNWFilterObjPtr obj); virNWFilterDefPtr virNWFilterObjGetNewDef(virNWFilterObjPtr obj); +char* +virNWFilterObjGetHash(virNWFilterObjPtr obj); + bool virNWFilterObjWantRemoved(virNWFilterObjPtr obj); @@ -114,4 +117,10 @@ virNWFilterObjLock(virNWFilterObjPtr obj); void virNWFilterObjUnlock(virNWFilterObjPtr obj); +void +virNWFilterObjListInvalidateHash(virNWFilterObjListPtr nwfilters); + +void +virNWFilterObjListUpdateHash(virNWFilterObjListPtr nwfilters); + #endif /* VIRNWFILTEROBJ_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 335210c..a7cfe80 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1077,6 +1077,7 @@ virNWFilterBindingObjListRemove; # conf/virnwfilterobj.h virNWFilterObjGetDef; +virNWFilterObjGetHash; virNWFilterObjGetNewDef; virNWFilterObjListAssignDef; virNWFilterObjListExport; @@ -1085,10 +1086,12 @@ virNWFilterObjListFindByUUID; virNWFilterObjListFindInstantiateFilter; virNWFilterObjListFree; virNWFilterObjListGetNames; +virNWFilterObjListInvalidateHash; virNWFilterObjListLoadAllConfigs; virNWFilterObjListNew; virNWFilterObjListNumOfNWFilters; virNWFilterObjListRemove; +virNWFilterObjListUpdateHash; virNWFilterObjLock; virNWFilterObjTestUnassignDef; virNWFilterObjUnlock; diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 1ab906f..5591c0b 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -570,6 +570,8 @@ nwfilterDefineXML(virConnectPtr conn, def = NULL; objdef = virNWFilterObjGetDef(obj); + virNWFilterObjListUpdateHash(driver->nwfilters); + if (virNWFilterSaveConfig(driver->configDir, objdef) < 0) { virNWFilterObjListRemove(driver->nwfilters, obj); goto cleanup; @@ -616,6 +618,7 @@ nwfilterUndefine(virNWFilterPtr nwfilter) goto cleanup; virNWFilterObjListRemove(driver->nwfilters, obj); + virNWFilterObjListInvalidateHash(driver->nwfilters); obj = NULL; ret = 0; diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index e5dea91..d64621b 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -1066,6 +1066,8 @@ virNWFilterBuildAll(virNWFilterDriverStatePtr driver, virNWFilterBuildIter, &data); } else { + virNWFilterObjListInvalidateHash(driver->nwfilters); + virNWFilterObjListUpdateHash(driver->nwfilters); data.step = STEP_SWITCH; virNWFilterBindingObjListForEach(driver->bindings, virNWFilterBuildIter, -- 1.8.3.1
On 10/18/18 2:49 AM, Nikolay Shirokovskiy wrote:
For filter without references to other filters hash is just sha-256 of filter's xml. For filters with references hash is sha-256 of string consisting of filter's self hash and hashes of directly referenced filters.
I didn't read the entire patch, but if you're just comparing hashes of the filters' XML, then you aren't actually checking to see if *someone else* outside libvirt has modified the actual rules in ebtables/iptables (and someday not *too* far away nbtables, or maybe firewalld "rich rules"). That is the really important thing we need to check for. One of the reasons for a libvirtd restart to delete and reload all the rules is to clean up after some third party has "moved our cheese" and our filter rules no longer work as expected, and just checking that our own internal data is unchanged doesn't help that problem. (BTW, on a slightly related topic - since I've lately been thinking about making libvirt work properly on systems with nbtables and in particular firewalld with an nbtables backend, I've been wondering if our whole model of "deleting old rules" based on our current idea of the libvirt xml ==> eb/iptables backend isn't improper. Maybe we should be saving the actual rules that we sent out in the status, and undoing those. The rules we need to delete could be *very* different from what the new libvirtd is generating (e.g. we decided we didn't need one of the rules anymore so it's no longer generated, or we're now using nbtables/ebpf/sub-etha-senso-tables or whatever).
If filter is not complete that is some of filters it's referencing directly or indirectly are missing the hash is set to NULL as well as if there was OOM on hash caculation. So having NULL hash is regular situation.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/virnwfilterobj.c | 145 +++++++++++++++++++++++++++++++++ src/conf/virnwfilterobj.h | 9 ++ src/libvirt_private.syms | 3 + src/nwfilter/nwfilter_driver.c | 3 + src/nwfilter/nwfilter_gentech_driver.c | 2 + 5 files changed, 162 insertions(+)
diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 0136a0d..4cc81df 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -28,6 +28,7 @@ #include "virlog.h" #include "virnwfilterobj.h" #include "virstring.h" +#include "vircrypto.h"
#define VIR_FROM_THIS VIR_FROM_NWFILTER
@@ -40,6 +41,8 @@ struct _virNWFilterObj {
virNWFilterDefPtr def; virNWFilterDefPtr newDef; + + char *hash; };
struct _virNWFilterObjList { @@ -82,6 +85,13 @@ virNWFilterObjGetNewDef(virNWFilterObjPtr obj) }
+char* +virNWFilterObjGetHash(virNWFilterObjPtr obj) +{ + return obj->hash; +} + + bool virNWFilterObjWantRemoved(virNWFilterObjPtr obj) { @@ -307,6 +317,139 @@ virNWFilterDefEqual(const virNWFilterDef *def1, }
+static void +virNWFilterObjInvalidateHash(virNWFilterObjListPtr nwfilters, + virNWFilterObjPtr obj) +{ + virNWFilterDefPtr def = obj->def; + size_t i; + + if (!obj->hash) + return; + + if (obj->newDef) { + VIR_FREE(obj->hash); + return; + } + + for (i = 0; i < def->nentries; i++) { + virNWFilterObjPtr child; + char *filterref; + + if (def->filterEntries[i]->rule || + !def->filterEntries[i]->include) + continue; + + filterref = def->filterEntries[i]->include->filterref; + + if (!(child = virNWFilterObjListFindByName(nwfilters, filterref))) { + VIR_FREE(obj->hash); + return; + } + + virNWFilterObjInvalidateHash(nwfilters, child); + + if (!child->hash) { + VIR_FREE(obj->hash); + virNWFilterObjUnlock(child); + return; + } + + virNWFilterObjUnlock(child); + } +} + + +void +virNWFilterObjListInvalidateHash(virNWFilterObjListPtr nwfilters) +{ + size_t i; + virNWFilterObjPtr obj; + + for (i = 0; i < nwfilters->count; i++) { + obj = nwfilters->objs[i]; + virNWFilterObjLock(obj); + virNWFilterObjInvalidateHash(nwfilters, obj); + virNWFilterObjUnlock(obj); + } +} + + +static void +virNWFilterObjUpdateHash(virNWFilterObjListPtr nwfilters, + virNWFilterObjPtr obj) +{ + virNWFilterDefPtr def = obj->newDef ? obj->newDef : obj->def; + size_t i; + char *hash; + char *xml; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (obj->hash) + return; + + if (!(xml = virNWFilterDefFormat(def))) + return; + + if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, xml, &hash)) + goto cleanup; + + virBufferAsprintf(&buf, "%s\n", hash); + VIR_FREE(hash); + + for (i = 0; i < def->nentries; i++) { + char *filterref; + virNWFilterObjPtr child; + + if (def->filterEntries[i]->rule || + !def->filterEntries[i]->include) + continue; + + filterref = def->filterEntries[i]->include->filterref; + + if (!(child = virNWFilterObjListFindByName(nwfilters, filterref))) + goto cleanup; + + virNWFilterObjUpdateHash(nwfilters, child); + + if (!child->hash) { + virNWFilterObjUnlock(child); + goto cleanup; + } + + virBufferAsprintf(&buf, "%s\n", child->hash); + + virNWFilterObjUnlock(child); + } + + if (virBufferCheckError(&buf) < 0) + goto cleanup; + + hash = virBufferContentAndReset(&buf); + ignore_value(virCryptoHashString(VIR_CRYPTO_HASH_SHA256, hash, &obj->hash)); + VIR_FREE(hash); + + cleanup: + virBufferFreeAndReset(&buf); + VIR_FREE(xml); +} + + +void +virNWFilterObjListUpdateHash(virNWFilterObjListPtr nwfilters) +{ + size_t i; + virNWFilterObjPtr obj; + + for (i = 0; i < nwfilters->count; i++) { + obj = nwfilters->objs[i]; + virNWFilterObjLock(obj); + virNWFilterObjUpdateHash(nwfilters, obj); + virNWFilterObjUnlock(obj); + } +} + + virNWFilterObjPtr virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, virNWFilterDefPtr def) @@ -555,6 +698,8 @@ virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters, virNWFilterObjUnlock(obj); }
+ virNWFilterObjListUpdateHash(nwfilters); + VIR_DIR_CLOSE(dir); return ret; } diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index 4a54dd5..1c41a3e 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -50,6 +50,9 @@ virNWFilterObjGetDef(virNWFilterObjPtr obj); virNWFilterDefPtr virNWFilterObjGetNewDef(virNWFilterObjPtr obj);
+char* +virNWFilterObjGetHash(virNWFilterObjPtr obj); + bool virNWFilterObjWantRemoved(virNWFilterObjPtr obj);
@@ -114,4 +117,10 @@ virNWFilterObjLock(virNWFilterObjPtr obj); void virNWFilterObjUnlock(virNWFilterObjPtr obj);
+void +virNWFilterObjListInvalidateHash(virNWFilterObjListPtr nwfilters); + +void +virNWFilterObjListUpdateHash(virNWFilterObjListPtr nwfilters); + #endif /* VIRNWFILTEROBJ_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 335210c..a7cfe80 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1077,6 +1077,7 @@ virNWFilterBindingObjListRemove;
# conf/virnwfilterobj.h virNWFilterObjGetDef; +virNWFilterObjGetHash; virNWFilterObjGetNewDef; virNWFilterObjListAssignDef; virNWFilterObjListExport; @@ -1085,10 +1086,12 @@ virNWFilterObjListFindByUUID; virNWFilterObjListFindInstantiateFilter; virNWFilterObjListFree; virNWFilterObjListGetNames; +virNWFilterObjListInvalidateHash; virNWFilterObjListLoadAllConfigs; virNWFilterObjListNew; virNWFilterObjListNumOfNWFilters; virNWFilterObjListRemove; +virNWFilterObjListUpdateHash; virNWFilterObjLock; virNWFilterObjTestUnassignDef; virNWFilterObjUnlock; diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 1ab906f..5591c0b 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -570,6 +570,8 @@ nwfilterDefineXML(virConnectPtr conn, def = NULL; objdef = virNWFilterObjGetDef(obj);
+ virNWFilterObjListUpdateHash(driver->nwfilters); + if (virNWFilterSaveConfig(driver->configDir, objdef) < 0) { virNWFilterObjListRemove(driver->nwfilters, obj); goto cleanup; @@ -616,6 +618,7 @@ nwfilterUndefine(virNWFilterPtr nwfilter) goto cleanup;
virNWFilterObjListRemove(driver->nwfilters, obj); + virNWFilterObjListInvalidateHash(driver->nwfilters); obj = NULL; ret = 0;
diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index e5dea91..d64621b 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -1066,6 +1066,8 @@ virNWFilterBuildAll(virNWFilterDriverStatePtr driver, virNWFilterBuildIter, &data); } else { + virNWFilterObjListInvalidateHash(driver->nwfilters); + virNWFilterObjListUpdateHash(driver->nwfilters); data.step = STEP_SWITCH; virNWFilterBindingObjListForEach(driver->bindings, virNWFilterBuildIter,
On 19.10.2018 18:33, Laine Stump wrote:
On 10/18/18 2:49 AM, Nikolay Shirokovskiy wrote:
For filter without references to other filters hash is just sha-256 of filter's xml. For filters with references hash is sha-256 of string consisting of filter's self hash and hashes of directly referenced filters.
I didn't read the entire patch, but if you're just comparing hashes of the filters' XML, then you aren't actually checking to see if *someone else* outside libvirt has modified the actual rules in ebtables/iptables (and someday not *too* far away nbtables, or maybe firewalld "rich rules"). That is the really important thing we need to check for. One of the reasons for a libvirtd restart to delete and reload all the rules is to clean up after some third party has "moved our cheese" and our filter rules no longer work as expected, and just checking that our own internal data is unchanged doesn't help that problem.
Agree. I mentioned the same issue in the cover letter. Need to rework series)
(BTW, on a slightly related topic - since I've lately been thinking about making libvirt work properly on systems with nbtables and in particular firewalld with an nbtables backend, I've been wondering if our whole model of "deleting old rules" based on our current idea of the libvirt xml ==> eb/iptables backend isn't improper. Maybe we should be saving the actual rules that we sent out in the status, and undoing those. The rules we need to delete could be *very* different from what the new libvirtd is generating (e.g. we decided we didn't need one of the rules anymore so it's no longer generated, or we're now using nbtables/ebpf/sub-etha-senso-tables or whatever).
The approach of not keeping firewall rules status is that libvirt keeps all rules in 4 chains: libvirt-O-$IFACE, libvirt-I-$IFACE, libvirt-J-$IFACE, libvirt-O-$IFACE (the last two are temporary) and their subchains. This names are fixed so we can always cleanup all rules installed by libvirt. Not sure of nbtables though - can we clean chains installed thru iptables/ebtables thru nbtables. Nikolay
If filter is not complete that is some of filters it's referencing directly or indirectly are missing the hash is set to NULL as well as if there was OOM on hash caculation. So having NULL hash is regular situation.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/virnwfilterobj.c | 145 +++++++++++++++++++++++++++++++++ src/conf/virnwfilterobj.h | 9 ++ src/libvirt_private.syms | 3 + src/nwfilter/nwfilter_driver.c | 3 + src/nwfilter/nwfilter_gentech_driver.c | 2 + 5 files changed, 162 insertions(+)
diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 0136a0d..4cc81df 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -28,6 +28,7 @@ #include "virlog.h" #include "virnwfilterobj.h" #include "virstring.h" +#include "vircrypto.h"
#define VIR_FROM_THIS VIR_FROM_NWFILTER
@@ -40,6 +41,8 @@ struct _virNWFilterObj {
virNWFilterDefPtr def; virNWFilterDefPtr newDef; + + char *hash; };
struct _virNWFilterObjList { @@ -82,6 +85,13 @@ virNWFilterObjGetNewDef(virNWFilterObjPtr obj) }
+char* +virNWFilterObjGetHash(virNWFilterObjPtr obj) +{ + return obj->hash; +} + + bool virNWFilterObjWantRemoved(virNWFilterObjPtr obj) { @@ -307,6 +317,139 @@ virNWFilterDefEqual(const virNWFilterDef *def1, }
+static void +virNWFilterObjInvalidateHash(virNWFilterObjListPtr nwfilters, + virNWFilterObjPtr obj) +{ + virNWFilterDefPtr def = obj->def; + size_t i; + + if (!obj->hash) + return; + + if (obj->newDef) { + VIR_FREE(obj->hash); + return; + } + + for (i = 0; i < def->nentries; i++) { + virNWFilterObjPtr child; + char *filterref; + + if (def->filterEntries[i]->rule || + !def->filterEntries[i]->include) + continue; + + filterref = def->filterEntries[i]->include->filterref; + + if (!(child = virNWFilterObjListFindByName(nwfilters, filterref))) { + VIR_FREE(obj->hash); + return; + } + + virNWFilterObjInvalidateHash(nwfilters, child); + + if (!child->hash) { + VIR_FREE(obj->hash); + virNWFilterObjUnlock(child); + return; + } + + virNWFilterObjUnlock(child); + } +} + + +void +virNWFilterObjListInvalidateHash(virNWFilterObjListPtr nwfilters) +{ + size_t i; + virNWFilterObjPtr obj; + + for (i = 0; i < nwfilters->count; i++) { + obj = nwfilters->objs[i]; + virNWFilterObjLock(obj); + virNWFilterObjInvalidateHash(nwfilters, obj); + virNWFilterObjUnlock(obj); + } +} + + +static void +virNWFilterObjUpdateHash(virNWFilterObjListPtr nwfilters, + virNWFilterObjPtr obj) +{ + virNWFilterDefPtr def = obj->newDef ? obj->newDef : obj->def; + size_t i; + char *hash; + char *xml; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (obj->hash) + return; + + if (!(xml = virNWFilterDefFormat(def))) + return; + + if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, xml, &hash)) + goto cleanup; + + virBufferAsprintf(&buf, "%s\n", hash); + VIR_FREE(hash); + + for (i = 0; i < def->nentries; i++) { + char *filterref; + virNWFilterObjPtr child; + + if (def->filterEntries[i]->rule || + !def->filterEntries[i]->include) + continue; + + filterref = def->filterEntries[i]->include->filterref; + + if (!(child = virNWFilterObjListFindByName(nwfilters, filterref))) + goto cleanup; + + virNWFilterObjUpdateHash(nwfilters, child); + + if (!child->hash) { + virNWFilterObjUnlock(child); + goto cleanup; + } + + virBufferAsprintf(&buf, "%s\n", child->hash); + + virNWFilterObjUnlock(child); + } + + if (virBufferCheckError(&buf) < 0) + goto cleanup; + + hash = virBufferContentAndReset(&buf); + ignore_value(virCryptoHashString(VIR_CRYPTO_HASH_SHA256, hash, &obj->hash)); + VIR_FREE(hash); + + cleanup: + virBufferFreeAndReset(&buf); + VIR_FREE(xml); +} + + +void +virNWFilterObjListUpdateHash(virNWFilterObjListPtr nwfilters) +{ + size_t i; + virNWFilterObjPtr obj; + + for (i = 0; i < nwfilters->count; i++) { + obj = nwfilters->objs[i]; + virNWFilterObjLock(obj); + virNWFilterObjUpdateHash(nwfilters, obj); + virNWFilterObjUnlock(obj); + } +} + + virNWFilterObjPtr virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, virNWFilterDefPtr def) @@ -555,6 +698,8 @@ virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters, virNWFilterObjUnlock(obj); }
+ virNWFilterObjListUpdateHash(nwfilters); + VIR_DIR_CLOSE(dir); return ret; } diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index 4a54dd5..1c41a3e 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -50,6 +50,9 @@ virNWFilterObjGetDef(virNWFilterObjPtr obj); virNWFilterDefPtr virNWFilterObjGetNewDef(virNWFilterObjPtr obj);
+char* +virNWFilterObjGetHash(virNWFilterObjPtr obj); + bool virNWFilterObjWantRemoved(virNWFilterObjPtr obj);
@@ -114,4 +117,10 @@ virNWFilterObjLock(virNWFilterObjPtr obj); void virNWFilterObjUnlock(virNWFilterObjPtr obj);
+void +virNWFilterObjListInvalidateHash(virNWFilterObjListPtr nwfilters); + +void +virNWFilterObjListUpdateHash(virNWFilterObjListPtr nwfilters); + #endif /* VIRNWFILTEROBJ_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 335210c..a7cfe80 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1077,6 +1077,7 @@ virNWFilterBindingObjListRemove;
# conf/virnwfilterobj.h virNWFilterObjGetDef; +virNWFilterObjGetHash; virNWFilterObjGetNewDef; virNWFilterObjListAssignDef; virNWFilterObjListExport; @@ -1085,10 +1086,12 @@ virNWFilterObjListFindByUUID; virNWFilterObjListFindInstantiateFilter; virNWFilterObjListFree; virNWFilterObjListGetNames; +virNWFilterObjListInvalidateHash; virNWFilterObjListLoadAllConfigs; virNWFilterObjListNew; virNWFilterObjListNumOfNWFilters; virNWFilterObjListRemove; +virNWFilterObjListUpdateHash; virNWFilterObjLock; virNWFilterObjTestUnassignDef; virNWFilterObjUnlock; diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 1ab906f..5591c0b 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -570,6 +570,8 @@ nwfilterDefineXML(virConnectPtr conn, def = NULL; objdef = virNWFilterObjGetDef(obj);
+ virNWFilterObjListUpdateHash(driver->nwfilters); + if (virNWFilterSaveConfig(driver->configDir, objdef) < 0) { virNWFilterObjListRemove(driver->nwfilters, obj); goto cleanup; @@ -616,6 +618,7 @@ nwfilterUndefine(virNWFilterPtr nwfilter) goto cleanup;
virNWFilterObjListRemove(driver->nwfilters, obj); + virNWFilterObjListInvalidateHash(driver->nwfilters); obj = NULL; ret = 0;
diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index e5dea91..d64621b 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -1066,6 +1066,8 @@ virNWFilterBuildAll(virNWFilterDriverStatePtr driver, virNWFilterBuildIter, &data); } else { + virNWFilterObjListInvalidateHash(driver->nwfilters); + virNWFilterObjListUpdateHash(driver->nwfilters); data.step = STEP_SWITCH; virNWFilterBindingObjListForEach(driver->bindings, virNWFilterBuildIter,
Skip binding's filter reinstantiation if it is not changed since it was instantiated last time. The purpose it to fasten libvirtd restart at least if filters won't changed, see RFC [1]. Thus we need to keep instantiated filter hash for binding in binding's status. This patch skips filters reinstantiation on firewalld reloads too but this will be fixed in next patch. [1] https://www.redhat.com/archives/libvir-list/2018-October/msg00657.html Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/virnwfilterbindingobj.c | 20 +++++++++++++ src/conf/virnwfilterbindingobj.h | 7 +++++ src/libvirt_private.syms | 2 ++ src/nwfilter/nwfilter_driver.c | 2 ++ src/nwfilter/nwfilter_gentech_driver.c | 52 +++++++++++++++++++++++++++++++--- src/nwfilter/nwfilter_gentech_driver.h | 3 ++ 6 files changed, 82 insertions(+), 4 deletions(-) diff --git a/src/conf/virnwfilterbindingobj.c b/src/conf/virnwfilterbindingobj.c index d145fe32..355981e 100644 --- a/src/conf/virnwfilterbindingobj.c +++ b/src/conf/virnwfilterbindingobj.c @@ -36,6 +36,7 @@ struct _virNWFilterBindingObj { bool removing; virNWFilterBindingDefPtr def; + char *filterhash; }; @@ -103,6 +104,22 @@ virNWFilterBindingObjSetRemoving(virNWFilterBindingObjPtr obj, } +void +virNWFilterBindingObjSetFilterhash(virNWFilterBindingObjPtr obj, + char *filterhash) +{ + VIR_FREE(obj->filterhash); + obj->filterhash = filterhash; +} + + +char* +virNWFilterBindingObjGetFilterhash(virNWFilterBindingObjPtr obj) +{ + return obj->filterhash; +} + + /** * virNWFilterBindingObjEndAPI: * @obj: binding object @@ -207,6 +224,8 @@ virNWFilterBindingObjParseXML(xmlDocPtr doc, if (!(ret = virNWFilterBindingObjNew())) return NULL; + ret->filterhash = virXPathString("string(./filterhash)", ctxt); + if (!(node = virXPathNode("./filterbinding", ctxt))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("filter binding status missing content")); @@ -284,6 +303,7 @@ virNWFilterBindingObjFormat(const virNWFilterBindingObj *obj) virBufferAddLit(&buf, "<filterbindingstatus>\n"); virBufferAdjustIndent(&buf, 2); + virBufferAsprintf(&buf, "<filterhash>%s</filterhash>\n", obj->filterhash); if (virNWFilterBindingDefFormatBuf(&buf, obj->def) < 0) { virBufferFreeAndReset(&buf); diff --git a/src/conf/virnwfilterbindingobj.h b/src/conf/virnwfilterbindingobj.h index 21ae85b..fbcee03 100644 --- a/src/conf/virnwfilterbindingobj.h +++ b/src/conf/virnwfilterbindingobj.h @@ -46,6 +46,13 @@ virNWFilterBindingObjSetRemoving(virNWFilterBindingObjPtr obj, bool removing); void +virNWFilterBindingObjSetFilterhash(virNWFilterBindingObjPtr obj, + char *filterhash); + +char* +virNWFilterBindingObjGetFilterhash(virNWFilterBindingObjPtr obj); + +void virNWFilterBindingObjEndAPI(virNWFilterBindingObjPtr *obj); char * diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a7cfe80..cc3aaba 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1057,11 +1057,13 @@ virNWFilterBindingObjDelete; virNWFilterBindingObjEndAPI; virNWFilterBindingObjFormat; virNWFilterBindingObjGetDef; +virNWFilterBindingObjGetFilterhash; virNWFilterBindingObjGetRemoving; virNWFilterBindingObjNew; virNWFilterBindingObjParseFile; virNWFilterBindingObjSave; virNWFilterBindingObjSetDef; +virNWFilterBindingObjSetFilterhash; virNWFilterBindingObjSetRemoving; diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 5591c0b..5d25d65 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -778,6 +778,8 @@ nwfilterBindingCreateXML(virConnectPtr conn, ret = NULL; goto cleanup; } + + virNWFilterBindingUpdateHash(driver->nwfilters, obj); virNWFilterBindingObjSave(obj, driver->bindingDir); cleanup: diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index d64621b..46b1144 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -982,10 +982,12 @@ enum { static int virNWFilterBuildOne(virNWFilterDriverStatePtr driver, - virNWFilterBindingDefPtr binding, + virNWFilterBindingObjPtr bindingobj, virHashTablePtr skipInterfaces, int step) { + virNWFilterBindingDefPtr binding = virNWFilterBindingObjGetDef(bindingobj); + virNWFilterObjPtr filter; bool skipIface; int ret = 0; VIR_DEBUG("Building filter for portdev=%s step=%d", binding->portdevname, step); @@ -1009,13 +1011,39 @@ virNWFilterBuildOne(virNWFilterDriverStatePtr driver, break; case STEP_SWITCH: - if (!virHashLookup(skipInterfaces, binding->portdevname)) + if (!virHashLookup(skipInterfaces, binding->portdevname)) { ret = virNWFilterTearOldFilter(binding); + + virNWFilterBindingUpdateHash(driver->nwfilters, bindingobj); + virNWFilterBindingObjSave(bindingobj, driver->bindingDir); + } break; case STEP_APPLY_CURRENT: + if ((filter = virNWFilterObjListFindByName(driver->nwfilters, + binding->filter))) { + char *filterhash = virNWFilterObjGetHash(filter); + char *bindinghash = virNWFilterBindingObjGetFilterhash(bindingobj); + + if (filterhash && bindinghash && STREQ(filterhash, bindinghash)) { + VIR_DEBUG("skip binding reinstantiating owner=%s portdevname=%s" + " filter=%s", + binding->ownername, binding->portdevname, + binding->filter); + + virNWFilterObjUnlock(filter); + break; + } + + virNWFilterObjUnlock(filter); + } + ret = virNWFilterInstantiateFilter(driver, binding); + if (ret == 0) { + virNWFilterBindingUpdateHash(driver->nwfilters, bindingobj); + virNWFilterBindingObjSave(bindingobj, driver->bindingDir); + } break; } @@ -1033,9 +1061,8 @@ static int virNWFilterBuildIter(virNWFilterBindingObjPtr binding, void *opaque) { struct virNWFilterBuildData *data = opaque; - virNWFilterBindingDefPtr def = virNWFilterBindingObjGetDef(binding); - return virNWFilterBuildOne(data->driver, def, + return virNWFilterBuildOne(data->driver, binding, data->skipInterfaces, data->step); } @@ -1084,3 +1111,20 @@ virNWFilterBuildAll(virNWFilterDriverStatePtr driver, } return ret; } + + +void +virNWFilterBindingUpdateHash(virNWFilterObjListPtr nwfilters, + virNWFilterBindingObjPtr binding) +{ + virNWFilterObjPtr filter; + virNWFilterBindingDefPtr def = virNWFilterBindingObjGetDef(binding); + char *filterhash = NULL; + + if ((filter = virNWFilterObjListFindByName(nwfilters, def->filter))) { + ignore_value(VIR_STRDUP_QUIET(filterhash, virNWFilterObjGetHash(filter))); + virNWFilterObjUnlock(filter); + } + + virNWFilterBindingObjSetFilterhash(binding, filterhash); +} diff --git a/src/nwfilter/nwfilter_gentech_driver.h b/src/nwfilter/nwfilter_gentech_driver.h index 2cd19c9..3c96c34 100644 --- a/src/nwfilter/nwfilter_gentech_driver.h +++ b/src/nwfilter/nwfilter_gentech_driver.h @@ -57,4 +57,7 @@ virHashTablePtr virNWFilterCreateVarHashmap(const char *macaddr, int virNWFilterBuildAll(virNWFilterDriverStatePtr driver, bool newFilters); +void virNWFilterBindingUpdateHash(virNWFilterObjListPtr nwfilters, + virNWFilterBindingObjPtr binding); + #endif -- 1.8.3.1
We need reinstantiation because reload will flush rules installed by libvirtd. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/nwfilter/nwfilter_driver.c | 6 +++--- src/nwfilter/nwfilter_gentech_driver.c | 13 +++++++++---- src/nwfilter/nwfilter_gentech_driver.h | 3 ++- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 5d25d65..db04868 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -168,7 +168,7 @@ virNWFilterTriggerRebuildImpl(void *opaque) { virNWFilterDriverStatePtr nwdriver = opaque; - return virNWFilterBuildAll(nwdriver, true); + return virNWFilterBuildAll(nwdriver, true, false); } @@ -264,7 +264,7 @@ nwfilterStateInitialize(bool privileged, if (virNWFilterBindingObjListLoadAllConfigs(driver->bindings, driver->bindingDir) < 0) goto error; - if (virNWFilterBuildAll(driver, false) < 0) + if (virNWFilterBuildAll(driver, false, false) < 0) goto error; nwfilterDriverUnlock(); @@ -319,7 +319,7 @@ nwfilterStateReload(void) virNWFilterUnlockFilterUpdates(); - virNWFilterBuildAll(driver, false); + virNWFilterBuildAll(driver, false, true); nwfilterDriverUnlock(); diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 46b1144..a5b3e1a 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -984,7 +984,8 @@ static int virNWFilterBuildOne(virNWFilterDriverStatePtr driver, virNWFilterBindingObjPtr bindingobj, virHashTablePtr skipInterfaces, - int step) + int step, + bool force) { virNWFilterBindingDefPtr binding = virNWFilterBindingObjGetDef(bindingobj); virNWFilterObjPtr filter; @@ -1020,7 +1021,8 @@ virNWFilterBuildOne(virNWFilterDriverStatePtr driver, break; case STEP_APPLY_CURRENT: - if ((filter = virNWFilterObjListFindByName(driver->nwfilters, + if (!force && + (filter = virNWFilterObjListFindByName(driver->nwfilters, binding->filter))) { char *filterhash = virNWFilterObjGetHash(filter); char *bindinghash = virNWFilterBindingObjGetFilterhash(bindingobj); @@ -1055,6 +1057,7 @@ struct virNWFilterBuildData { virNWFilterDriverStatePtr driver; virHashTablePtr skipInterfaces; int step; + bool force; }; static int @@ -1063,15 +1066,17 @@ virNWFilterBuildIter(virNWFilterBindingObjPtr binding, void *opaque) struct virNWFilterBuildData *data = opaque; return virNWFilterBuildOne(data->driver, binding, - data->skipInterfaces, data->step); + data->skipInterfaces, data->step, data->force); } int virNWFilterBuildAll(virNWFilterDriverStatePtr driver, - bool newFilters) + bool newFilters, + bool force) { struct virNWFilterBuildData data = { .driver = driver, + .force = force, }; int ret = 0; diff --git a/src/nwfilter/nwfilter_gentech_driver.h b/src/nwfilter/nwfilter_gentech_driver.h index 3c96c34..bdf3daa 100644 --- a/src/nwfilter/nwfilter_gentech_driver.h +++ b/src/nwfilter/nwfilter_gentech_driver.h @@ -55,7 +55,8 @@ virHashTablePtr virNWFilterCreateVarHashmap(const char *macaddr, const virNWFilterVarValue *value); int virNWFilterBuildAll(virNWFilterDriverStatePtr driver, - bool newFilters); + bool newFilters, + bool force); void virNWFilterBindingUpdateHash(virNWFilterObjListPtr nwfilters, virNWFilterBindingObjPtr binding); -- 1.8.3.1
This helps us bring correct firewall rules if previous binary install them incorrectly. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/virnwfilterbindingobj.c | 20 ++++++++++++++++++++ src/conf/virnwfilterbindingobj.h | 3 +++ src/libvirt_private.syms | 1 + src/nwfilter/nwfilter_gentech_driver.c | 4 +++- 4 files changed, 27 insertions(+), 1 deletion(-) diff --git a/src/conf/virnwfilterbindingobj.c b/src/conf/virnwfilterbindingobj.c index 355981e..09b757a 100644 --- a/src/conf/virnwfilterbindingobj.c +++ b/src/conf/virnwfilterbindingobj.c @@ -37,6 +37,7 @@ struct _virNWFilterBindingObj { bool removing; virNWFilterBindingDefPtr def; char *filterhash; + time_t libvirtCtime; }; @@ -110,6 +111,7 @@ virNWFilterBindingObjSetFilterhash(virNWFilterBindingObjPtr obj, { VIR_FREE(obj->filterhash); obj->filterhash = filterhash; + obj->libvirtCtime = virGetSelfLastChanged(); } @@ -120,6 +122,12 @@ virNWFilterBindingObjGetFilterhash(virNWFilterBindingObjPtr obj) } +time_t +virNWFilterBindingObjGetLibvirtCtime(virNWFilterBindingObjPtr obj) +{ + return obj->libvirtCtime; +} + /** * virNWFilterBindingObjEndAPI: * @obj: binding object @@ -220,12 +228,22 @@ virNWFilterBindingObjParseXML(xmlDocPtr doc, { virNWFilterBindingObjPtr ret; xmlNodePtr node; + long long int ctime; if (!(ret = virNWFilterBindingObjNew())) return NULL; ret->filterhash = virXPathString("string(./filterhash)", ctxt); + if (virXPathBoolean("boolean(./libvirtctime)", ctxt) > 0) { + if (virXPathLongLong("string(./libvirtctime)", ctxt, &ctime) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid libvirtctime format")); + goto cleanup; + } + ret->libvirtCtime = (time_t)ctime; + } + if (!(node = virXPathNode("./filterbinding", ctxt))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("filter binding status missing content")); @@ -304,6 +322,8 @@ virNWFilterBindingObjFormat(const virNWFilterBindingObj *obj) virBufferAdjustIndent(&buf, 2); virBufferAsprintf(&buf, "<filterhash>%s</filterhash>\n", obj->filterhash); + virBufferAsprintf(&buf, "<libvirtctime>%llu</libvirtctime>\n", + (long long) obj->libvirtCtime); if (virNWFilterBindingDefFormatBuf(&buf, obj->def) < 0) { virBufferFreeAndReset(&buf); diff --git a/src/conf/virnwfilterbindingobj.h b/src/conf/virnwfilterbindingobj.h index fbcee03..ab949f8 100644 --- a/src/conf/virnwfilterbindingobj.h +++ b/src/conf/virnwfilterbindingobj.h @@ -52,6 +52,9 @@ virNWFilterBindingObjSetFilterhash(virNWFilterBindingObjPtr obj, char* virNWFilterBindingObjGetFilterhash(virNWFilterBindingObjPtr obj); +time_t +virNWFilterBindingObjGetLibvirtCtime(virNWFilterBindingObjPtr obj); + void virNWFilterBindingObjEndAPI(virNWFilterBindingObjPtr *obj); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index cc3aaba..368ee01 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1058,6 +1058,7 @@ virNWFilterBindingObjEndAPI; virNWFilterBindingObjFormat; virNWFilterBindingObjGetDef; virNWFilterBindingObjGetFilterhash; +virNWFilterBindingObjGetLibvirtCtime; virNWFilterBindingObjGetRemoving; virNWFilterBindingObjNew; virNWFilterBindingObjParseFile; diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index a5b3e1a..94c2c5b 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -1026,8 +1026,10 @@ virNWFilterBuildOne(virNWFilterDriverStatePtr driver, binding->filter))) { char *filterhash = virNWFilterObjGetHash(filter); char *bindinghash = virNWFilterBindingObjGetFilterhash(bindingobj); + time_t libvirtCtime = virNWFilterBindingObjGetLibvirtCtime(bindingobj); - if (filterhash && bindinghash && STREQ(filterhash, bindinghash)) { + if (libvirtCtime == virGetSelfLastChanged() && + filterhash && bindinghash && STREQ(filterhash, bindinghash)) { VIR_DEBUG("skip binding reinstantiating owner=%s portdevname=%s" " filter=%s", binding->ownername, binding->portdevname, -- 1.8.3.1
participants (2)
-
Laine Stump -
Nikolay Shirokovskiy