[libvirt] [PATCH v2 0/3] 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. v2: - added a test case for testing virHashEqual Regards, Stefan
Add function to compare two hash tables for equality. --- src/util/hash.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/hash.h | 12 ++++++++++++ 2 files changed, 58 insertions(+) Index: libvirt-iterator/src/util/hash.c =================================================================== --- libvirt-iterator.orig/src/util/hash.c +++ libvirt-iterator/src/util/hash.c @@ -663,3 +663,49 @@ virHashKeyValuePairPtr virHashGetItems(v return iter.sortArray; } + +struct virHashEqualData +{ + bool equal; + const virHashTablePtr table2; + virHashValueComparator compar; +}; + +static int virHashEqualSearcher(const void *payload, const void *name, + const void *data) +{ + struct virHashEqualData *vhed = (void *)data; + const void *value; + + value = virHashLookup(vhed->table2, name); + if (!value || + vhed->compar(value, payload) != 0) { + /* key is missing in 2nd table or values are different */ + vhed->equal = false; + /* stop 'iteration' */ + return 1; + } + return 0; +} + +bool virHashEqual(const virHashTablePtr table1, + const virHashTablePtr table2, + virHashValueComparator compar) +{ + struct virHashEqualData data = { + .equal = true, + .table2 = table2, + .compar = compar, + }; + + if (table1 == table2) + return true; + + if (!table1 || !table2 || + virHashSize(table1) != virHashSize(table2)) + return false; + + virHashSearch(table1, virHashEqualSearcher, &data); + + return data.equal; +} Index: libvirt-iterator/src/util/hash.h =================================================================== --- libvirt-iterator.orig/src/util/hash.h +++ libvirt-iterator/src/util/hash.h @@ -154,6 +154,18 @@ virHashKeyValuePairPtr virHashGetItems(v virHashKeyComparator compar); /* + * Compare two tables for equality: the lookup of a key's value in + * both tables must result in an equivalent value. + * The caller must pass in a comparator function for comparing the values + * of two keys. + */ +typedef int (*virHashValueComparator)(const void *value1, const void *value2); +bool virHashEqual(const virHashTablePtr table1, + const virHashTablePtr table2, + virHashValueComparator compar); + + +/* * Iterators */ int virHashForEach(virHashTablePtr table, virHashIterator iter, void *data);
On 01/18/2012 09:20 AM, Stefan Berger wrote:
Add function to compare two hash tables for equality.
--- src/util/hash.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/hash.h | 12 ++++++++++++ 2 files changed, 58 insertions(+)
ACK. Pretty straightforward. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Add a test case to test the virHashEqual function. --- tests/hashtest.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) Index: libvirt-iterator/tests/hashtest.c =================================================================== --- libvirt-iterator.orig/tests/hashtest.c +++ libvirt-iterator/tests/hashtest.c @@ -576,6 +576,83 @@ cleanup: static int +testHashEqualCompValue(const void *value1, const void *value2) +{ + return strcmp (value1, value2); +} + + +static int +testHashEqual(const void *data ATTRIBUTE_UNUSED) +{ + virHashTablePtr hash1, hash2; + int ret = -1; + char keya[] = "a"; + char keyb[] = "b"; + char keyc[] = "c"; + char value1[] = "1"; + char value2[] = "2"; + char value3[] = "3"; + char value4[] = "4"; + + if (!(hash1 = virHashCreate(0, NULL)) || + !(hash2 = virHashCreate(0, NULL)) || + virHashAddEntry(hash1, keya, value1) < 0 || + virHashAddEntry(hash1, keyb, value2) < 0 || + virHashAddEntry(hash1, keyc, value3) < 0 || + virHashAddEntry(hash2, keya, value1) < 0 || + virHashAddEntry(hash2, keyb, value2) < 0) { + if (virTestGetVerbose()) { + testError("\nfailed to create hashes"); + } + goto cleanup; + } + + if (virHashEqual(hash1, hash2, testHashEqualCompValue)) { + if (virTestGetVerbose()) { + testError("\nfailed equal test for different number of elements"); + } + goto cleanup; + } + + if (virHashAddEntry(hash2, keyc, value4) < 0) { + if (virTestGetVerbose()) { + testError("\nfailed to add element to hash2"); + } + goto cleanup; + } + + if (virHashEqual(hash1, hash2, testHashEqualCompValue)) { + if (virTestGetVerbose()) { + testError("\nfailed equal test for same number of elements"); + } + goto cleanup; + } + + if (virHashUpdateEntry(hash2, keyc, value3) < 0) { + if (virTestGetVerbose()) { + testError("\nfailed to update element in hash2"); + } + goto cleanup; + } + + if (!virHashEqual(hash1, hash2, testHashEqualCompValue)) { + if (virTestGetVerbose()) { + testError("\nfailed equal test for equal hash tables"); + } + goto cleanup; + } + + ret = 0; + +cleanup: + virHashFree(hash1); + virHashFree(hash2); + return ret; +} + + +static int mymain(void) { int ret = 0; @@ -612,6 +689,7 @@ mymain(void) DO_TEST("RemoveSet", RemoveSet); DO_TEST("Search", Search); DO_TEST("GetItems", GetItems); + DO_TEST("Equal", Equal); return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE; }
On 01/18/2012 09:20 AM, Stefan Berger wrote:
Add a test case to test the virHashEqual function.
--- tests/hashtest.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+)
Even better - you test your new API. I would have squashed this into 1/3, but separate patches is okay as well.
static int +testHashEqualCompValue(const void *value1, const void *value2) +{ + return strcmp (value1, value2);
Style: no space before (. Suggestion - maybe use strcasecmp, and pass in strings that differ in case to prove that definitions of 'equal' data values is user-defined. ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/20/2012 07:16 PM, Eric Blake wrote:
On 01/18/2012 09:20 AM, Stefan Berger wrote:
Add a test case to test the virHashEqual function.
--- tests/hashtest.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) Even better - you test your new API. I would have squashed this into 1/3, but separate patches is okay as well.
static int +testHashEqualCompValue(const void *value1, const void *value2) +{ + return strcmp (value1, value2); Style: no space before (.
c&p from the test case above...
Suggestion - maybe use strcasecmp, and pass in strings that differ in case to prove that definitions of 'equal' data values is user-defined.
I changed that to use c_strcasecmp() with values having different case. Pushed. Thanks. Stefan
On 18.01.2012 17:20, Stefan Berger wrote:
+ +static int +testHashEqual(const void *data ATTRIBUTE_UNUSED) +{ + virHashTablePtr hash1, hash2; + int ret = -1; + char keya[] = "a"; + char keyb[] = "b"; + char keyc[] = "c"; + char value1[] = "1"; + char value2[] = "2"; + char value3[] = "3"; + char value4[] = "4"; + + if (!(hash1 = virHashCreate(0, NULL)) || + !(hash2 = virHashCreate(0, NULL)) ||
Actually, if the first virHashCreate() returns NULL, hash2 remains uninitialized and we jump
+ virHashAddEntry(hash1, keya, value1) < 0 || + virHashAddEntry(hash1, keyb, value2) < 0 || + virHashAddEntry(hash1, keyc, value3) < 0 || + virHashAddEntry(hash2, keya, value1) < 0 || + virHashAddEntry(hash2, keyb, value2) < 0) { + if (virTestGetVerbose()) { + testError("\nfailed to create hashes"); + } + goto cleanup; + }
over here and do free() on uninitialized pointer.
+ +cleanup: + virHashFree(hash1); + virHashFree(hash2); + return ret; +}
Therefore I am pushing this under trivial and build-breaker rules (yeah, one thing - and perhaps the only one - i like about 4.6 gcc is enhanced static analysis as I spotted warning while compiling current HEAD): Author: Michal Privoznik <mprivozn@redhat.com> Date: Tue Jan 24 12:09:42 2012 +0100 hashtest: Initialize variable in virHashEqual test One of latest patches (b7bcb22ce2) enhanced testing for virHashEqual. However, hash2 variable might be used uninitialized. diff --git a/tests/hashtest.c b/tests/hashtest.c index 6c45b01..441672c 100644 --- a/tests/hashtest.c +++ b/tests/hashtest.c @@ -583,7 +583,7 @@ testHashEqualCompValue(const void *value1, const void *value2) static int testHashEqual(const void *data ATTRIBUTE_UNUSED) { - virHashTablePtr hash1, hash2; + virHashTablePtr hash1, hash2 = NULL; int ret = -1; char keya[] = "a"; char keyb[] = "b";
On 01/24/2012 06:19 AM, Michal Privoznik wrote:
Therefore I am pushing this under trivial and build-breaker rules (yeah, one thing - and perhaps the only one - i like about 4.6 gcc is enhanced static analysis as I spotted warning while compiling current HEAD):
Author: Michal Privoznik<mprivozn@redhat.com> Date: Tue Jan 24 12:09:42 2012 +0100
hashtest: Initialize variable in virHashEqual test
One of latest patches (b7bcb22ce2) enhanced testing for virHashEqual. However, hash2 variable might be used uninitialized.
diff --git a/tests/hashtest.c b/tests/hashtest.c index 6c45b01..441672c 100644 --- a/tests/hashtest.c +++ b/tests/hashtest.c @@ -583,7 +583,7 @@ testHashEqualCompValue(const void *value1, const void *value2) static int testHashEqual(const void *data ATTRIBUTE_UNUSED) { - virHashTablePtr hash1, hash2; + virHashTablePtr hash1, hash2 = NULL; int ret = -1; char keya[] = "a"; char keyb[] = "b";
ACK.
Compare two filter definitions for equality and only rebuild/instantiate the new filter if the two 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' Unforuntately this more involved than a mere memcmp() on the structures. --- src/Makefile.am | 1 src/conf/nwfilter_conf.c | 213 ++++++++++++++++++++ src/conf/nwfilter_params.c | 18 + src/conf/nwfilter_params.h | 2 src/conf/nwfilter_protocols.c | 447 ++++++++++++++++++++++++++++++++++++++++++ src/conf/nwfilter_protocols.h | 56 +++++ 6 files changed, 737 insertions(+) Index: libvirt-iterator/src/conf/nwfilter_conf.c =================================================================== --- libvirt-iterator.orig/src/conf/nwfilter_conf.c +++ libvirt-iterator/src/conf/nwfilter_conf.c @@ -42,6 +42,7 @@ #include "memory.h" #include "virterror_internal.h" #include "datatypes.h" +#include "nwfilter_protocols.h" #include "nwfilter_params.h" #include "nwfilter_conf.h" #include "domain_conf.h" @@ -140,6 +141,10 @@ static const struct int_map chain_priori INTMAP_ENTRY_LAST, }; +static bool virNWFilterDefEqual(const virNWFilterDefPtr def1, + const virNWFilterDefPtr def2, + bool cmpUUIDs); + /* * only one filter update allowed */ @@ -407,6 +412,134 @@ virNWFilterRuleDefAddString(virNWFilterR return nwf->strings[nwf->nstrings-1]; } +static bool +virNWFilterRuleDefEqual(const virNWFilterRuleDefPtr def1, + const virNWFilterRuleDefPtr def2) +{ + unsigned int i, j; + bool equal; + + if (def1 == def2) + return true; + + if (!def1 || !def2) + return false; + + if (def1->priority != def2->priority || + def1->flags != def2->flags || + def1->action != def2->action || + def1->tt != def2->tt || + def1->prtclType != def2->prtclType) + return false; + + switch (def1->prtclType) { + case VIR_NWFILTER_RULE_PROTOCOL_NONE: + return true; + case VIR_NWFILTER_RULE_PROTOCOL_MAC: + equal = virNWFilterEthHdrFilterDefEqual(&def1->p.ethHdrFilter, + &def2->p.ethHdrFilter); + break; + case VIR_NWFILTER_RULE_PROTOCOL_VLAN: + equal = virNWFilterVlanHdrFilterDefEqual(&def1->p.vlanHdrFilter, + &def2->p.vlanHdrFilter); + break; + case VIR_NWFILTER_RULE_PROTOCOL_STP: + equal = virNWFilterStpHdrFilterDefEqual(&def1->p.stpHdrFilter, + &def2->p.stpHdrFilter); + case VIR_NWFILTER_RULE_PROTOCOL_ARP: + case VIR_NWFILTER_RULE_PROTOCOL_RARP: + equal = virNWFilterArpHdrFilterDefEqual(&def1->p.arpHdrFilter, + &def2->p.arpHdrFilter); + break; + case VIR_NWFILTER_RULE_PROTOCOL_IP: + equal = virNWFilterIpHdrFilterDefEqual(&def1->p.ipHdrFilter, + &def2->p.ipHdrFilter); + break; + case VIR_NWFILTER_RULE_PROTOCOL_IPV6: + equal = virNWFilterIpv6HdrFilterDefEqual(&def1->p.ipv6HdrFilter, + &def2->p.ipv6HdrFilter); + break; + case VIR_NWFILTER_RULE_PROTOCOL_TCP: + case VIR_NWFILTER_RULE_PROTOCOL_TCPoIPV6: + equal = virNWFilterTcpHdrFilterDefEqual(&def1->p.tcpHdrFilter, + &def2->p.tcpHdrFilter); + break; + case VIR_NWFILTER_RULE_PROTOCOL_ICMP: + case VIR_NWFILTER_RULE_PROTOCOL_ICMPV6: + equal = virNWFilterIcmpHdrFilterDefEqual(&def1->p.icmpHdrFilter, + &def2->p.icmpHdrFilter); + break; + case VIR_NWFILTER_RULE_PROTOCOL_IGMP: + equal = virNWFilterIgmpHdrFilterDefEqual(&def1->p.igmpHdrFilter, + &def2->p.igmpHdrFilter); + break; + case VIR_NWFILTER_RULE_PROTOCOL_UDP: + case VIR_NWFILTER_RULE_PROTOCOL_UDPoIPV6: + equal = virNWFilterUdpHdrFilterDefEqual(&def1->p.udpHdrFilter, + &def2->p.udpHdrFilter); + break; + case VIR_NWFILTER_RULE_PROTOCOL_UDPLITE: + case VIR_NWFILTER_RULE_PROTOCOL_UDPLITEoIPV6: + equal = virNWFilterUdpliteHdrFilterDefEqual(&def1->p.udpliteHdrFilter, + &def2->p.udpliteHdrFilter); + break; + case VIR_NWFILTER_RULE_PROTOCOL_ESP: + case VIR_NWFILTER_RULE_PROTOCOL_ESPoIPV6: + equal = virNWFilterEspHdrFilterDefEqual(&def1->p.espHdrFilter, + &def2->p.espHdrFilter); + break; + case VIR_NWFILTER_RULE_PROTOCOL_AH: + case VIR_NWFILTER_RULE_PROTOCOL_AHoIPV6: + equal = virNWFilterAhHdrFilterDefEqual(&def1->p.ahHdrFilter, + &def2->p.ahHdrFilter); + break; + case VIR_NWFILTER_RULE_PROTOCOL_SCTP: + case VIR_NWFILTER_RULE_PROTOCOL_SCTPoIPV6: + equal = virNWFilterSctpHdrFilterDefEqual(&def1->p.sctpHdrFilter, + &def2->p.sctpHdrFilter); + break; + case VIR_NWFILTER_RULE_PROTOCOL_ALL: + case VIR_NWFILTER_RULE_PROTOCOL_ALLoIPV6: + equal = virNWFilterAllHdrFilterDefEqual(&def1->p.allHdrFilter, + &def2->p.allHdrFilter); + break; + case VIR_NWFILTER_RULE_PROTOCOL_LAST: + return false; + } + + if (!equal) + return false; + + if (def1->nVarAccess != def2->nVarAccess) + return false; + + for (i = 0; i < def1->nVarAccess; i++) { + equal = false; + for (j = 0; j < def2->nVarAccess; j++) { + equal = virNWFilterVarAccessEqual(def1->varAccess[i], + def2->varAccess[j]); + if (equal) + break; + } + if (!equal) + return false; + } + + if (def1->nstrings != def2->nstrings) + return false; + + for (i = 0; i < def1->nstrings; i++) { + equal = false; + for (j = 0; j < def2->nstrings; j++) { + equal = STREQ(def1->strings[i], def2->strings[j]); + if (equal) + break; + } + if (!equal) + return false; + } + return true; +} void virNWFilterObjRemove(virNWFilterObjListPtr nwfilters, @@ -2817,6 +2950,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)) { @@ -3226,6 +3367,22 @@ virNWFilterIncludeDefFormat(virNWFilterI return virBufferContentAndReset(&buf); } +static bool +virNWFilterIncludeDefEqual(const virNWFilterIncludeDefPtr def1, + const virNWFilterIncludeDefPtr def2) +{ + if (def1 == def2) + return true; + + if (!def1 || !def2) + return false; + + if (!STREQ(def1->filterref, def2->filterref) || + !virNWFilterHashTableEqual(def1->params, def2->params)) + return false; + + return true; +} static char * virNWFilterEntryFormat(virNWFilterEntryPtr entry) @@ -3235,6 +3392,22 @@ virNWFilterEntryFormat(virNWFilterEntryP return virNWFilterIncludeDefFormat(entry->include); } +static bool +virNWFilterEntryEqual(const virNWFilterEntryPtr def1, + const virNWFilterEntryPtr def2) +{ + if (def1 == def2) + return true; + + if (!def1 || !def2) + return false; + + if (!virNWFilterRuleDefEqual(def1->rule, def2->rule) || + !virNWFilterIncludeDefEqual(def1->include, def2->include)) + return false; + + return true; +} char * virNWFilterDefFormat(virNWFilterDefPtr def) @@ -3278,6 +3451,46 @@ virNWFilterDefFormat(virNWFilterDefPtr d return NULL; } +/* Compare two filter definitions for equality. + * Both filters must have the same content and all content must appear in the + * same order. + * + * @def1: the 1st filter + * @def2: the 2nd filter + * @cmpUUIDs: whether to compare the UUIDs of the two filters + * + * Returns true if both filters are equal, false otherwise. + */ +static bool +virNWFilterDefEqual(const virNWFilterDefPtr def1, const virNWFilterDefPtr def2, + bool cmpUUIDs) +{ + unsigned int i; + bool equal; + + if (def1 == def2) + return true; + + if (!def1 || !def2) + return false; + + if (!STREQ(def1->name, def2->name) || + (cmpUUIDs && memcmp(def1->uuid, def2->uuid, sizeof(def1->uuid)) != 0) || + !STREQ(def1->chainsuffix, def2->chainsuffix) || + def1->chainPriority != def2->chainPriority || + def1->nentries != def2->nentries) + return false; + + /* the order of the filter entries must be the same in both */ + for (i = 0; i < def1->nentries; i++) { + equal = virNWFilterEntryEqual(def1->filterEntries[i], + def2->filterEntries[i]); + if (!equal) + return false; + } + + return true; +} char *virNWFilterConfigFile(const char *dir, const char *name) Index: libvirt-iterator/src/conf/nwfilter_params.c =================================================================== --- libvirt-iterator.orig/src/conf/nwfilter_params.c +++ libvirt-iterator/src/conf/nwfilter_params.c @@ -747,6 +747,19 @@ err_exit: return -1; } +bool +virNWFilterHashTableEqual(const virNWFilterHashTablePtr hash1, + const virNWFilterHashTablePtr hash2) +{ + if (hash1 == hash2) + return true; + + if (!hash1 || !hash2) + return false; + + return virHashEqual(hash1->hashTable, hash2->hashTable, + (virHashValueComparator)strcmp); +} static bool isValidVarName(const char *var) @@ -896,6 +909,11 @@ bool virNWFilterVarAccessEqual(const virNWFilterVarAccessPtr a, const virNWFilterVarAccessPtr b) { + if (a == b) + return true; + if (!a || !b) + return false; + if (a->accessType != b->accessType) return false; Index: libvirt-iterator/src/conf/nwfilter_params.h =================================================================== --- libvirt-iterator.orig/src/conf/nwfilter_params.h +++ libvirt-iterator/src/conf/nwfilter_params.h @@ -84,6 +84,8 @@ void *virNWFilterHashTableRemoveEntry(vi const char *name); int virNWFilterHashTablePutAll(virNWFilterHashTablePtr src, virNWFilterHashTablePtr dest); +bool virNWFilterHashTableEqual(const virNWFilterHashTablePtr hash1, + const virNWFilterHashTablePtr hash2); # define VALID_VARNAME \ "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_" Index: libvirt-iterator/src/Makefile.am =================================================================== --- libvirt-iterator.orig/src/Makefile.am +++ libvirt-iterator/src/Makefile.am @@ -152,6 +152,7 @@ NETWORK_CONF_SOURCES = \ # Network filter driver generic impl APIs NWFILTER_PARAM_CONF_SOURCES = \ conf/nwfilter_params.c conf/nwfilter_params.h \ + conf/nwfilter_protocols.c conf/nwfilter_protocols.h \ conf/nwfilter_conf.h NWFILTER_CONF_SOURCES = \ Index: libvirt-iterator/src/conf/nwfilter_protocols.c =================================================================== --- /dev/null +++ libvirt-iterator/src/conf/nwfilter_protocols.c @@ -0,0 +1,447 @@ +/* + * nwfilter_protocols.c: protocol handling + * + * Copyright (C) 2011 IBM Corporation + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: Stefan Berger <stefanb@us.ibm.com> + */ + +#include <config.h> + +#include "internal.h" + +#include "uuid.h" +#include "virterror_internal.h" +#include "datatypes.h" +#include "nwfilter_params.h" +#include "nwfilter_conf.h" +#include "nwfilter_protocols.h" + +#define VIR_FROM_THIS VIR_FROM_NWFILTER + +#define NWITEMDESCEQUAL(ITEM) \ + virNWFilterNWItemDescEqual(&def1->ITEM, &def2->ITEM) + +static bool +virNWFilterNWItemDescEqual(const nwItemDescPtr def1, + const nwItemDescPtr def2) +{ + if (def1 == def2) + return true; + + if (!def1 || !def2) + return false; + + if (def1->flags != def2->flags || + !virNWFilterVarAccessEqual(def1->varAccess, def2->varAccess) || + def1->datatype != def2->datatype) + return false; + + switch (def1->datatype) { + case DATATYPE_STRINGCOPY: + if (strcmp(def1->u.string, def2->u.string)) + return false; + break; + case DATATYPE_UINT16: + case DATATYPE_UINT8: + case DATATYPE_UINT16_HEX: + case DATATYPE_UINT8_HEX: + case DATATYPE_MACADDR: + case DATATYPE_MACMASK: + case DATATYPE_IPADDR: + case DATATYPE_IPMASK: + case DATATYPE_IPV6ADDR: + case DATATYPE_IPV6MASK: + case DATATYPE_STRING: + case DATATYPE_BOOLEAN: + case DATATYPE_UINT32: + case DATATYPE_UINT32_HEX: + if (memcmp(&def1->u, &def2->u, sizeof(def1->u))) + return false; + break; + case DATATYPE_LAST: + return false; + } + + return true; +} + +static bool +virNWFilterEthHdrDataDefEqual(const ethHdrDataDefPtr def1, + const ethHdrDataDefPtr def2) +{ + if (def1 == def2) + return true; + + if (!def1 || !def2) + return false; + + if (!NWITEMDESCEQUAL(dataSrcMACAddr) || + !NWITEMDESCEQUAL(dataSrcMACMask) || + !NWITEMDESCEQUAL(dataDstMACAddr) || + !NWITEMDESCEQUAL(dataDstMACMask)) + return false; + + return true; +} + +bool +virNWFilterEthHdrFilterDefEqual(const ethHdrFilterDefPtr def1, + const ethHdrFilterDefPtr def2) +{ + if (def1 == def2) + return true; + + if (!def1 || !def2) + return false; + + if (!virNWFilterEthHdrDataDefEqual(&def1->ethHdr, &def2->ethHdr) || + !NWITEMDESCEQUAL(dataProtocolID) || + !NWITEMDESCEQUAL(dataComment)) + return false; + + return true; +} + +bool +virNWFilterVlanHdrFilterDefEqual(const vlanHdrFilterDefPtr def1, + const vlanHdrFilterDefPtr def2) +{ + if (def1 == def2) + return true; + + if (!def1 || !def2) + return false; + + if (!virNWFilterEthHdrDataDefEqual(&def1->ethHdr, &def2->ethHdr) || + !NWITEMDESCEQUAL(dataVlanID) || + !NWITEMDESCEQUAL(dataVlanEncap) || + !NWITEMDESCEQUAL(dataComment)) + return false; + + return true; +} + +bool +virNWFilterStpHdrFilterDefEqual(const stpHdrFilterDefPtr def1, + const stpHdrFilterDefPtr def2) +{ + if (def1 == def2) + return true; + + if (!def1 || !def2) + return false; + + if (!virNWFilterEthHdrDataDefEqual(&def1->ethHdr, &def2->ethHdr) || + !NWITEMDESCEQUAL(dataType) || + !NWITEMDESCEQUAL(dataFlags) || + !NWITEMDESCEQUAL(dataRootPri) || + !NWITEMDESCEQUAL(dataRootPriHi) || + !NWITEMDESCEQUAL(dataRootAddr) || + !NWITEMDESCEQUAL(dataRootAddrMask) || + !NWITEMDESCEQUAL(dataRootCost) || + !NWITEMDESCEQUAL(dataRootCostHi) || + !NWITEMDESCEQUAL(dataSndrPrio) || + !NWITEMDESCEQUAL(dataSndrPrioHi) || + !NWITEMDESCEQUAL(dataSndrAddr) || + !NWITEMDESCEQUAL(dataSndrAddrMask) || + !NWITEMDESCEQUAL(dataPort) || + !NWITEMDESCEQUAL(dataPortHi) || + !NWITEMDESCEQUAL(dataAge) || + !NWITEMDESCEQUAL(dataAgeHi) || + !NWITEMDESCEQUAL(dataMaxAge) || + !NWITEMDESCEQUAL(dataMaxAgeHi) || + !NWITEMDESCEQUAL(dataHelloTime) || + !NWITEMDESCEQUAL(dataHelloTimeHi) || + !NWITEMDESCEQUAL(dataFwdDelay) || + !NWITEMDESCEQUAL(dataFwdDelayHi) || + !NWITEMDESCEQUAL(dataComment)) + return false; + + return true; +} + +bool +virNWFilterArpHdrFilterDefEqual(const arpHdrFilterDefPtr def1, + const arpHdrFilterDefPtr def2) +{ + if (def1 == def2) + return true; + + if (!def1 || !def2) + return false; + + if (!virNWFilterEthHdrDataDefEqual(&def1->ethHdr, &def2->ethHdr) || + !NWITEMDESCEQUAL(dataHWType) || + !NWITEMDESCEQUAL(dataProtocolType) || + !NWITEMDESCEQUAL(dataOpcode) || + !NWITEMDESCEQUAL(dataARPSrcMACAddr) || + !NWITEMDESCEQUAL(dataARPSrcIPAddr) || + !NWITEMDESCEQUAL(dataARPDstMACAddr) || + !NWITEMDESCEQUAL(dataARPDstIPAddr) || + !NWITEMDESCEQUAL(dataGratuitousARP) || + !NWITEMDESCEQUAL(dataComment)) + return false; + + return true; +} + +static bool +virNWFilterIpHdrDataDefEqual(const ipHdrDataDefPtr def1, + const ipHdrDataDefPtr def2) +{ + if (def1 == def2) + return true; + + if (!def1 || !def2) + return false; + + if (!NWITEMDESCEQUAL(dataIPVersion) || + !NWITEMDESCEQUAL(dataSrcIPAddr) || + !NWITEMDESCEQUAL(dataSrcIPMask) || + !NWITEMDESCEQUAL(dataDstIPAddr) || + !NWITEMDESCEQUAL(dataDstIPMask) || + !NWITEMDESCEQUAL(dataProtocolID) || + !NWITEMDESCEQUAL(dataSrcIPFrom) || + !NWITEMDESCEQUAL(dataSrcIPTo) || + !NWITEMDESCEQUAL(dataDstIPFrom) || + !NWITEMDESCEQUAL(dataDstIPTo) || + !NWITEMDESCEQUAL(dataDSCP) || + !NWITEMDESCEQUAL(dataState) || + !NWITEMDESCEQUAL(dataConnlimitAbove) || + !NWITEMDESCEQUAL(dataComment)) + return false; + + return true; +} + +static bool +virNWFilterPortDataDefEqual(const portDataDefPtr def1, + const portDataDefPtr def2) +{ + if (def1 == def2) + return true; + + if (!def1 || !def2) + return false; + + if (!NWITEMDESCEQUAL(dataSrcPortStart) || + !NWITEMDESCEQUAL(dataSrcPortEnd) || + !NWITEMDESCEQUAL(dataDstPortStart) || + !NWITEMDESCEQUAL(dataDstPortEnd)) + return false; + + return true; +} + +bool +virNWFilterIpHdrFilterDefEqual(const ipHdrFilterDefPtr def1, + const ipHdrFilterDefPtr def2) +{ + if (def1 == def2) + return true; + + if (!def1 || !def2) + return false; + + if (!virNWFilterEthHdrDataDefEqual(&def1->ethHdr, &def2->ethHdr) || + !virNWFilterIpHdrDataDefEqual(&def1->ipHdr, &def2->ipHdr) || + !virNWFilterPortDataDefEqual(&def1->portData, &def2->portData)) + return false; + + return true; +} + +bool +virNWFilterIpv6HdrFilterDefEqual(const ipv6HdrFilterDefPtr def1, + const ipv6HdrFilterDefPtr def2) +{ + if (def1 == def2) + return true; + + if (!def1 || !def2) + return false; + + if (!virNWFilterEthHdrDataDefEqual(&def1->ethHdr, &def2->ethHdr) || + !virNWFilterIpHdrDataDefEqual(&def1->ipHdr, &def2->ipHdr) || + !virNWFilterPortDataDefEqual(&def1->portData, &def2->portData)) + return false; + + return true; +} + +bool +virNWFilterIcmpHdrFilterDefEqual(const icmpHdrFilterDefPtr def1, + const icmpHdrFilterDefPtr def2) +{ + if (def1 == def2) + return true; + + if (!def1 || !def2) + return false; + + if (!NWITEMDESCEQUAL(dataSrcMACAddr) || + !virNWFilterIpHdrDataDefEqual(&def1->ipHdr, &def2->ipHdr) || + !NWITEMDESCEQUAL(dataICMPType) || + !NWITEMDESCEQUAL(dataICMPCode) || + !NWITEMDESCEQUAL(dataStateFlags)) + return false; + + return true; +} + +bool +virNWFilterAllHdrFilterDefEqual(const allHdrFilterDefPtr def1, + const allHdrFilterDefPtr def2) +{ + if (def1 == def2) + return true; + + if (!def1 || !def2) + return false; + + if (!NWITEMDESCEQUAL(dataSrcMACAddr) || + !virNWFilterIpHdrDataDefEqual(&def1->ipHdr, &def2->ipHdr)) + return false; + + return true; +} + +bool +virNWFilterIgmpHdrFilterDefEqual(const igmpHdrFilterDefPtr def1, + const igmpHdrFilterDefPtr def2) +{ + if (def1 == def2) + return true; + + if (!def1 || !def2) + return false; + + if (!NWITEMDESCEQUAL(dataSrcMACAddr) || + !virNWFilterIpHdrDataDefEqual(&def1->ipHdr, &def2->ipHdr)) + return false; + + return true; +} + +bool +virNWFilterTcpHdrFilterDefEqual(const tcpHdrFilterDefPtr def1, + const tcpHdrFilterDefPtr def2) +{ + if (def1 == def2) + return true; + + if (!def1 || !def2) + return false; + + if (!NWITEMDESCEQUAL(dataSrcMACAddr) || + !virNWFilterIpHdrDataDefEqual(&def1->ipHdr, &def2->ipHdr) || + !virNWFilterPortDataDefEqual(&def1->portData, &def2->portData) || + !NWITEMDESCEQUAL(dataTCPOption) || + !NWITEMDESCEQUAL(dataTCPFlags)) + return false; + + return true; +} + +bool +virNWFilterUdpHdrFilterDefEqual(const udpHdrFilterDefPtr def1, + const udpHdrFilterDefPtr def2) +{ + if (def1 == def2) + return true; + + if (!def1 || !def2) + return false; + + if (!NWITEMDESCEQUAL(dataSrcMACAddr) || + !virNWFilterIpHdrDataDefEqual(&def1->ipHdr, &def2->ipHdr) || + !virNWFilterPortDataDefEqual(&def1->portData, &def2->portData)) + return false; + + return true; +} + +bool +virNWFilterSctpHdrFilterDefEqual(const sctpHdrFilterDefPtr def1, + const sctpHdrFilterDefPtr def2) +{ + if (def1 == def2) + return true; + + if (!def1 || !def2) + return false; + + if (!NWITEMDESCEQUAL(dataSrcMACAddr) || + !virNWFilterIpHdrDataDefEqual(&def1->ipHdr, &def2->ipHdr) || + !virNWFilterPortDataDefEqual(&def1->portData, &def2->portData)) + return false; + + return true; +} + +bool +virNWFilterEspHdrFilterDefEqual(const espHdrFilterDefPtr def1, + const espHdrFilterDefPtr def2) +{ + if (def1 == def2) + return true; + + if (!def1 || !def2) + return false; + + if (!NWITEMDESCEQUAL(dataSrcMACAddr) || + !virNWFilterIpHdrDataDefEqual(&def1->ipHdr, &def2->ipHdr)) + return false; + + return true; +} + +bool +virNWFilterAhHdrFilterDefEqual(const ahHdrFilterDefPtr def1, + const ahHdrFilterDefPtr def2) +{ + if (def1 == def2) + return true; + + if (!def1 || !def2) + return false; + + if (!NWITEMDESCEQUAL(dataSrcMACAddr) || + !virNWFilterIpHdrDataDefEqual(&def1->ipHdr, &def2->ipHdr)) + return false; + + return true; +} + +bool +virNWFilterUdpliteHdrFilterDefEqual(const udpliteHdrFilterDefPtr def1, + const udpliteHdrFilterDefPtr def2) +{ + if (def1 == def2) + return true; + + if (!def1 || !def2) + return false; + + if (!NWITEMDESCEQUAL(dataSrcMACAddr) || + !virNWFilterIpHdrDataDefEqual(&def1->ipHdr, &def2->ipHdr)) + return false; + + return true; +} + Index: libvirt-iterator/src/conf/nwfilter_protocols.h =================================================================== --- /dev/null +++ libvirt-iterator/src/conf/nwfilter_protocols.h @@ -0,0 +1,56 @@ +/* + * nwfilter_protocols.h: protocol handling + * + * Copyright (C) 2011 IBM Corporation + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: Stefan Berger <stefanb@us.ibm.com> + */ + +#include "datatypes.h" +#include "nwfilter_params.h" +#include "nwfilter_conf.h" + +bool virNWFilterEthHdrFilterDefEqual(const ethHdrFilterDefPtr def1, + const ethHdrFilterDefPtr def2); +bool virNWFilterVlanHdrFilterDefEqual(const vlanHdrFilterDefPtr def1, + const vlanHdrFilterDefPtr def2); +bool virNWFilterStpHdrFilterDefEqual(const stpHdrFilterDefPtr def1, + const stpHdrFilterDefPtr def2); +bool virNWFilterArpHdrFilterDefEqual(const arpHdrFilterDefPtr def1, + const arpHdrFilterDefPtr def2); +bool virNWFilterIpHdrFilterDefEqual(const ipHdrFilterDefPtr def1, + const ipHdrFilterDefPtr def2); +bool virNWFilterIpv6HdrFilterDefEqual(const ipv6HdrFilterDefPtr def1, + const ipv6HdrFilterDefPtr def2); +bool virNWFilterIcmpHdrFilterDefEqual(const icmpHdrFilterDefPtr def1, + const icmpHdrFilterDefPtr def2); +bool virNWFilterAllHdrFilterDefEqual(const allHdrFilterDefPtr def1, + const allHdrFilterDefPtr def2); +bool virNWFilterIgmpHdrFilterDefEqual(const igmpHdrFilterDefPtr def1, + const igmpHdrFilterDefPtr def2); +bool virNWFilterTcpHdrFilterDefEqual(const tcpHdrFilterDefPtr def1, + const tcpHdrFilterDefPtr def2); +bool virNWFilterUdpHdrFilterDefEqual(const udpHdrFilterDefPtr def1, + const udpHdrFilterDefPtr def2); +bool virNWFilterSctpHdrFilterDefEqual(const sctpHdrFilterDefPtr def1, + const sctpHdrFilterDefPtr def2); +bool virNWFilterEspHdrFilterDefEqual(const espHdrFilterDefPtr def1, + const espHdrFilterDefPtr def2); +bool virNWFilterAhHdrFilterDefEqual(const ahHdrFilterDefPtr def1, + const ahHdrFilterDefPtr def2); +bool virNWFilterUdpliteHdrFilterDefEqual(const udpliteHdrFilterDefPtr def1, + const udpliteHdrFilterDefPtr def2);
On 01/18/2012 09:20 AM, Stefan Berger wrote:
Compare two filter definitions for equality and only rebuild/instantiate the new filter if the two 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'
Unforuntately this more involved than a mere memcmp() on the structures.
s/Unforuntately/Unfortunately/
--- src/Makefile.am | 1 src/conf/nwfilter_conf.c | 213 ++++++++++++++++++++ src/conf/nwfilter_params.c | 18 + src/conf/nwfilter_params.h | 2 src/conf/nwfilter_protocols.c | 447 ++++++++++++++++++++++++++++++++++++++++++ src/conf/nwfilter_protocols.h | 56 +++++ 6 files changed, 737 insertions(+)
Fails 'make syntax-check': prohibit_empty_lines_at_EOF src/conf/nwfilter_protocols.c maint.mk: empty line(s) or no newline at EOF And while fixing that, I noticed that you didn't bump any copyright years. Emacs users can get this for free with this in ~/.emacs (not sure if vim has a comparable hook): (require 'copyright) (defun my-copyright-update (&optional arg) "My improvements to `copyright-update'." (interactive "*P") (and (not (eq major-mode 'fundamental-mode)) (copyright-update arg)) nil) (add-hook 'before-save-hook 'my-copyright-update)
Index: libvirt-iterator/src/conf/nwfilter_params.c =================================================================== --- libvirt-iterator.orig/src/conf/nwfilter_params.c +++ libvirt-iterator/src/conf/nwfilter_params.c @@ -747,6 +747,19 @@ err_exit: return -1; }
+bool +virNWFilterHashTableEqual(const virNWFilterHashTablePtr hash1, + const virNWFilterHashTablePtr hash2) +{ + if (hash1 == hash2) + return true; + + if (!hash1 || !hash2) + return false; + + return virHashEqual(hash1->hashTable, hash2->hashTable, + (virHashValueComparator)strcmp);
Indentation - I'd remove 3 spaces, so that the case lines up with hash1 on the line before.
Index: libvirt-iterator/src/conf/nwfilter_protocols.c =================================================================== --- /dev/null +++ libvirt-iterator/src/conf/nwfilter_protocols.c @@ -0,0 +1,447 @@ +/* + * nwfilter_protocols.c: protocol handling + * + * Copyright (C) 2011 IBM Corporation
It's 2012, now :)
+ +#define VIR_FROM_THIS VIR_FROM_NWFILTER + +#define NWITEMDESCEQUAL(ITEM) \ + virNWFilterNWItemDescEqual(&def1->ITEM, &def2->ITEM)
You don't have to change if you don't want, but it took me a while to parse that name. Maybe NW_ITEM_DESC_EQUAL would be more readable.
Index: libvirt-iterator/src/conf/nwfilter_protocols.h =================================================================== --- /dev/null +++ libvirt-iterator/src/conf/nwfilter_protocols.h @@ -0,0 +1,56 @@ +/* + * nwfilter_protocols.h: protocol handling + * + * Copyright (C) 2011 IBM Corporation
2012 Big, but looks like a rather mechanical conversion of the various protocols into a highly repetitive algorithm for comparisons. I didn't notice any major problems, so: ACK with nits fixed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/23/2012 04:59 PM, Eric Blake wrote:
On 01/18/2012 09:20 AM, Stefan Berger wrote:
Compare two filter definitions for equality and only rebuild/instantiate the new filter if the two 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'
Unforuntately this more involved than a mere memcmp() on the structures.
2012
Big, but looks like a rather mechanical conversion of the various protocols into a highly repetitive algorithm for comparisons. I didn't notice any major problems, so:
ACK with nits fixed.
After some consideration I decided not to push this patch but go a different route: have the new XML parsed, convert new and old internal representation of the filters to XML and compare those. This will be much less code. Stefan
participants (3)
-
Eric Blake -
Michal Privoznik -
Stefan Berger