[libvirt] [PATCH V4 00/10] Make inner workings of nwfilters more flexible + extensions
The following series of patches re-does some of the inner workings of nwfilters with the goal to enable users to write filters that have other than the system-known chains supported right now ('root','arp','rarp','ipv4' and 'ipv6'). Ideally users should be able to provide a chain name in the chains XML attribute and either be able to jump to it as an 'action' or have the chain created automatically as it is the case right now for those chains enumerated before. The latter is now added in this patch series as well. I am first introducing internal priorities for the chains mentioned above so that their creation can be made more flexible -- currently their creation and the order in which they are accessed is hardcoded. This largely does away with the hardcoded stuff. All assigned priorities have negative values. Later on the priorities for the chains are made accessible via an XML attribute. Further, filters will be automatically accessed from the (ebtables) interface 'root' chain using the prefix of the name of the chain. As an example, the following filter will be accessed from the root chain for 'arp' packets since its name 'arp-xyz' has the prefix 'arp'. <filter name='test-arp-xyz' chain='arp-xyz' priority='-650'> <uuid>94abeecc-c956-0ac8-1f49-a06ee8995688</uuid> <rule action='accept' direction='out' priority='100'> <arp opcode='Request_Reverse' arpsrcmacaddr='$MAC' arpdstmacaddr='$MAC' arpsrcipaddr='0.0.0.0' arpdstipaddr='0.0.0.0'/> </rule> <rule action='accept' direction='inout' priority='500'/> </filter> In another step the priorities of rules is extended to also allow negativ values. This then allows the creation of rules and chains in the interface 'root' chain to be mixed so that the following layout becomes possible: Bridge chain: libvirt-I-vnet0, entries: 6, policy: ACCEPT -p IPv4 -j I-vnet0-ipv4 -p ARP -j I-vnet0-arp -p ARP -j ACCEPT -p 0x8035 -j I-vnet0-rarp -p 0x835 -j ACCEPT -j DROP In the above list of rules the '-p ARP -j ACCEPT' can now be found in between the 'jumps' to protocol-specific chains, which allows for more efficient rule evaluation. I did testing with the test cases in libvirt-tck as well as those in the tests/ directory and did not see any regressions. v4: - assign priority to chain according to name prefix - change default (internal) priorities of chains (by +200) - fix memory leak Regards, Stefan
Add a function to the virHashTable for getting an array of the hash table's keys and have the keys (optionally) sorted. Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/libvirt_private.syms | 3 + src/util/hash.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/hash.h | 14 ++++++ 3 files changed, 115 insertions(+) Index: libvirt-acl/src/util/hash.c =================================================================== --- libvirt-acl.orig/src/util/hash.c +++ libvirt-acl/src/util/hash.c @@ -618,3 +618,101 @@ void *virHashSearch(virHashTablePtr tabl return NULL; } + + +struct getKeysIter +{ + virHashTablePtr table; + void **array; + virHashKeyValuePair *sortArray; + unsigned arrayIdx; + bool error; +}; + +static void virHashGetKeysIterator(void *payload, + const void *name, void *data) +{ + struct getKeysIter *iter = data; + void *key; + + if (iter->error) + return; + + key = iter->table->keyCopy(name); + + if (key == NULL) { + virReportOOMError(); + iter->error = true; + return; + } + + if (iter->sortArray) { + iter->sortArray[iter->arrayIdx].key = key; + iter->sortArray[iter->arrayIdx].value = payload; + } else { + iter->array[iter->arrayIdx] = iter->table->keyCopy(name); + } + iter->arrayIdx++; +} + +void virHashFreeKeys(virHashTablePtr table, void **keys) +{ + unsigned i = 0; + + if (keys == NULL) + return; + + while (keys[i]) + table->keyFree(keys[i++]); + + VIR_FREE(keys); +} + +typedef int (*qsort_comp)(const void *, const void *); + +void **virHashGetKeys(virHashTablePtr table, + virHashKeyComparator compar) +{ + int i, numElems = virHashSize(table); + struct getKeysIter iter = { + .table = table, + .arrayIdx = 0, + .error = false, + }; + + if (numElems < 0) { + return NULL; + } + + if (VIR_ALLOC_N(iter.array, numElems + 1)) { + virReportOOMError(); + return NULL; + } + + /* null-terminate array */ + iter.array[numElems] = NULL; + + if (compar && + VIR_ALLOC_N(iter.sortArray, numElems)) { + VIR_FREE(iter.array); + virReportOOMError(); + return NULL; + } + + virHashForEach(table, virHashGetKeysIterator, &iter); + if (iter.error) { + VIR_FREE(iter.sortArray); + virHashFreeKeys(table, iter.array); + return NULL; + } + + if (compar) { + qsort(&iter.sortArray[0], numElems, sizeof(iter.sortArray[0]), + (qsort_comp)compar); + for (i = 0; i < numElems; i++) + iter.array[i] = iter.sortArray[i].key; + VIR_FREE(iter.sortArray); + } + + return iter.array; +} Index: libvirt-acl/src/util/hash.h =================================================================== --- libvirt-acl.orig/src/util/hash.h +++ libvirt-acl/src/util/hash.h @@ -130,6 +130,20 @@ void *virHashLookup(virHashTablePtr tabl */ void *virHashSteal(virHashTablePtr table, const void *name); +/* + * Get the set of keys and have them optionally sorted. + */ +typedef struct _virHashKeyValuePair virHashKeyValuePair; +typedef virHashKeyValuePair *virHashKeyValuePairPtr; +struct _virHashKeyValuePair { + void *key; + const void *value; +}; +typedef int (*virHashKeyComparator)(const virHashKeyValuePairPtr , + const virHashKeyValuePairPtr ); +void **virHashGetKeys(virHashTablePtr table, virHashKeyComparator compar); +void virHashFreeKeys(virHashTablePtr table, void **); + /* * Iterators Index: libvirt-acl/src/libvirt_private.syms =================================================================== --- libvirt-acl.orig/src/libvirt_private.syms +++ libvirt-acl/src/libvirt_private.syms @@ -559,6 +559,8 @@ virHashAddEntry; virHashCreate; virHashForEach; virHashFree; +virHashFreeKeys; +virHashGetKeys; virHashLookup; virHashRemoveEntry; virHashRemoveSet; @@ -566,6 +568,7 @@ virHashSearch; virHashSize; virHashSteal; virHashTableSize; +virHashUpdateEntry; # hooks.h
On 10/26/2011 05:36 AM, Stefan Berger wrote:
Add a function to the virHashTable for getting an array of the hash table's keys and have the keys (optionally) sorted.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
--- src/libvirt_private.syms | 3 + src/util/hash.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/hash.h | 14 ++++++ 3 files changed, 115 insertions(+)
Reordering for review purposes:
Index: libvirt-acl/src/util/hash.h =================================================================== --- libvirt-acl.orig/src/util/hash.h +++ libvirt-acl/src/util/hash.h @@ -130,6 +130,20 @@ void *virHashLookup(virHashTablePtr tabl */ void *virHashSteal(virHashTablePtr table, const void *name);
+/* + * Get the set of keys and have them optionally sorted. + */ +typedef struct _virHashKeyValuePair virHashKeyValuePair; +typedef virHashKeyValuePair *virHashKeyValuePairPtr; +struct _virHashKeyValuePair { + void *key;
Why isn't this 'const void *key'?
+ const void *value; +}; +typedef int (*virHashKeyComparator)(const virHashKeyValuePairPtr , + const virHashKeyValuePairPtr ); +void **virHashGetKeys(virHashTablePtr table, virHashKeyComparator compar);
So the caller only knows how large the returned array is by calling virHashSize? I take it passing NULL for compar is what gets the keys in an unsorted order. Should this return 'const void **', that is, an array that can be modified, but whose elements are const void * pointers into the hash table, where the keys are not modifiable through this array view?
+void virHashFreeKeys(virHashTablePtr table, void **);
I'm not sure we need this. It seems like the array returned by virHashGetKeys should be able to just pass directly to VIR_FREE, without reference to which table it was copied from, since all the elements of the array are just pointers into the hash table. I guess I could see using this if you are cloning keys in the process of creating the array, but I'm not sure that cloning keys is worth it.
* Iterators Index: libvirt-acl/src/libvirt_private.syms =================================================================== --- libvirt-acl.orig/src/libvirt_private.syms +++ libvirt-acl/src/libvirt_private.syms @@ -559,6 +559,8 @@ virHashAddEntry; virHashCreate; virHashForEach; virHashFree; +virHashFreeKeys;
This line might not be needed, per my above questioning.
+virHashGetKeys; virHashLookup; virHashRemoveEntry; virHashRemoveSet; @@ -566,6 +568,7 @@ virHashSearch; virHashSize; virHashSteal; virHashTableSize; +virHashUpdateEntry;
Did we really have this one missing?
+++ libvirt-acl/src/util/hash.c @@ -618,3 +618,101 @@ void *virHashSearch(virHashTablePtr tabl
return NULL; } + + +struct getKeysIter +{ + virHashTablePtr table; + void **array; + virHashKeyValuePair *sortArray; + unsigned arrayIdx; + bool error;
If you go with my idea of VIR_FREE() to clean up the returned array, then you don't need bool error.
+}; + +static void virHashGetKeysIterator(void *payload, + const void *name, void *data) +{ + struct getKeysIter *iter = data; + void *key; + + if (iter->error) + return; + + key = iter->table->keyCopy(name); + + if (key == NULL) { + virReportOOMError(); + iter->error = true; + return; + }
These statements disappear, replaced by: key = name; (const-correcness may require that you use const void * in more places).
+ + if (iter->sortArray) { + iter->sortArray[iter->arrayIdx].key = key; + iter->sortArray[iter->arrayIdx].value = payload; + } else { + iter->array[iter->arrayIdx] = iter->table->keyCopy(name);
and this becomes iter->array[iter->arrayIdx] = key;
+ } + iter->arrayIdx++; +} + +void virHashFreeKeys(virHashTablePtr table, void **keys) +{ + unsigned i = 0; + + if (keys == NULL) + return; + + while (keys[i]) + table->keyFree(keys[i++]); + + VIR_FREE(keys); +}
then this function disappears.
+ +typedef int (*qsort_comp)(const void *, const void *); + +void **virHashGetKeys(virHashTablePtr table, + virHashKeyComparator compar)
This function needs better documentation; compare to virHashForEach to see an example. Mention that the returned array must be passed to VIR_FREE
+{ + int i, numElems = virHashSize(table); + struct getKeysIter iter = { + .table = table, + .arrayIdx = 0, + .error = false,
You need to explicitly initialize .sortArray to NULL, otherwise, virHashGetKeysIterator will see it as uninitialized garbage, and may attempt to store into .sortArray instead of the intended .array.
+ }; + + if (numElems < 0) { + return NULL; + } + + if (VIR_ALLOC_N(iter.array, numElems + 1)) { + virReportOOMError(); + return NULL; + } + + /* null-terminate array */ + iter.array[numElems] = NULL;
The array is already NULL-terminated, by virtue of VIR_ALLOC_N, so this line is not necessary.
+ + if (compar && + VIR_ALLOC_N(iter.sortArray, numElems)) { + VIR_FREE(iter.array); + virReportOOMError(); + return NULL; + } + + virHashForEach(table, virHashGetKeysIterator, &iter); + if (iter.error) { + VIR_FREE(iter.sortArray); + virHashFreeKeys(table, iter.array); + return NULL; + }
By returning const void* into the array, the virHashForEach no longer does allocation (just copying in-hash pointers into the array), so it can't fail, and this if statement can be dropped.
+ + if (compar) { + qsort(&iter.sortArray[0], numElems, sizeof(iter.sortArray[0]), + (qsort_comp)compar); + for (i = 0; i < numElems; i++) + iter.array[i] = iter.sortArray[i].key; + VIR_FREE(iter.sortArray); + } + + return iter.array; +}
Also a meta-question. I guess I can see where this might be useful - the act of sorting hash keys is not worth reinventing with every caller. And making the user store data in a sorted array instead of a hash table hurts lookup performance; so if lookup is more frequent than listing, but listing must be sorted, then this is a useful abstraction. But why are we just returning keys, instead of key/value pairs? If we're going to do sorting on key-value pairs, would it be any easier to hand the user back key-value pairs, so they don't have to do key lookups on every element of the returned array? Also, some food for thought. Gnulib provides a GPLv3+ implementation of a sorted list backed by a hash table, which provides the best of both ideals of sorted traversal and fast lookup. While we can't use that module for licensing reasons, the data structure used there may prove an interesting study for what you are using this function for. http://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/gl_list.h In particular, if you make the hash payload carry linked-list data alongside the normal lookup value, and maintain the list order on insertion, then you have the benefit of both orderly traversal and fast lookups, without having to add any new functions to util/hash.h, but at the expense of more legwork in the caller. So, if we ever have more than one client that wants to maintain a sorted list while still having fast key lookup, it may make more sense to provide a src/util/sortedhash.h that wraps hash.h with the additional legwork to provide the sorting. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 11/16/2011 07:32 PM, Eric Blake wrote:
On 10/26/2011 05:36 AM, Stefan Berger wrote:
Add a function to the virHashTable for getting an array of the hash table's keys and have the keys (optionally) sorted.
Signed-off-by: Stefan Berger<stefanb@linux.vnet.ibm.com>
--- src/libvirt_private.syms | 3 + src/util/hash.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/hash.h | 14 ++++++ 3 files changed, 115 insertions(+) Reordering for review purposes:
Index: libvirt-acl/src/util/hash.h =================================================================== --- libvirt-acl.orig/src/util/hash.h +++ libvirt-acl/src/util/hash.h @@ -130,6 +130,20 @@ void *virHashLookup(virHashTablePtr tabl */ void *virHashSteal(virHashTablePtr table, const void *name);
+/* + * Get the set of keys and have them optionally sorted. + */ +typedef struct _virHashKeyValuePair virHashKeyValuePair; +typedef virHashKeyValuePair *virHashKeyValuePairPtr; +struct _virHashKeyValuePair { + void *key; Why isn't this 'const void *key'? My bad.
I converted everything. It now looks like this. * * Get the set of keys and have them optionally sorted. */ typedef struct _virHashKeyValuePair virHashKeyValuePair; typedef virHashKeyValuePair *virHashKeyValuePairPtr; struct _virHashKeyValuePair { const void *key; const void *value; }; typedef int (*virHashKeyComparator)(const virHashKeyValuePairPtr , const virHashKeyValuePairPtr ); virHashKeyValuePairPtr virHashGetItems(virHashTablePtr table, virHashKeyComparator compar); I hope this addresses your concerns. I do need the sorting and left it in there without wanting to either wrap it or offload it into the caller - if that's ok. Stefan
For better handling of the sorting of chains introduce an internally used priority. Use a lookup table to store the priorities. For now their actual values do not matter just that the values cause the chains to be properly sorted through changes in the following patches. However, the values are chosen as negative so that once they are sorted along with filtering rules (whose priority may only be positive for now) they will always be instantiated before them (lower values cause instantiation before higher values). This is done to maintain backwards compatibility. v3: - increased filter priorities to have more room before them Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/conf/nwfilter_conf.c | 14 ++++++++++++++ src/conf/nwfilter_conf.h | 12 ++++++++++++ src/nwfilter/nwfilter_ebiptables_driver.c | 4 ++++ src/nwfilter/nwfilter_ebiptables_driver.h | 1 + 4 files changed, 31 insertions(+) Index: libvirt-acl/src/conf/nwfilter_conf.h =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_conf.h +++ libvirt-acl/src/conf/nwfilter_conf.h @@ -357,8 +357,18 @@ enum virNWFilterEbtablesTableType { }; +# define MIN_RULE_PRIORITY 0 # define MAX_RULE_PRIORITY 1000 +# define NWFILTER_MIN_FILTER_PRIORITY -1000 +# define NWFILTER_MAX_FILTER_PRIORITY MAX_RULE_PRIORITY + +# define NWFILTER_ROOT_FILTER_PRI 0 +# define NWFILTER_IPV4_FILTER_PRI -700 +# define NWFILTER_IPV6_FILTER_PRI -600 +# define NWFILTER_ARP_FILTER_PRI -500 +# define NWFILTER_RARP_FILTER_PRI -400 + enum virNWFilterRuleFlags { RULE_FLAG_NO_STATEMATCH = (1 << 0), RULE_FLAG_STATE_NEW = (1 << 1), @@ -436,6 +446,7 @@ enum virNWFilterChainSuffixType { VIR_NWFILTER_CHAINSUFFIX_LAST, }; +typedef int32_t virNWFilterChainPriority; typedef struct _virNWFilterDef virNWFilterDef; typedef virNWFilterDef *virNWFilterDefPtr; @@ -445,6 +456,7 @@ struct _virNWFilterDef { unsigned char uuid[VIR_UUID_BUFLEN]; int chainsuffix; /*enum virNWFilterChainSuffixType */ + virNWFilterChainPriority chainPriority; int nentries; virNWFilterEntryPtr *filterEntries; Index: libvirt-acl/src/conf/nwfilter_conf.c =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_conf.c +++ libvirt-acl/src/conf/nwfilter_conf.c @@ -124,6 +124,14 @@ struct int_map { #define INTMAP_ENTRY(ATT, VAL) { .attr = ATT, .val = VAL } #define INTMAP_ENTRY_LAST { .val = NULL } +static const struct int_map chain_priorities[] = { + INTMAP_ENTRY(NWFILTER_ROOT_FILTER_PRI, "root"), + INTMAP_ENTRY(NWFILTER_IPV4_FILTER_PRI, "ipv4"), + INTMAP_ENTRY(NWFILTER_IPV6_FILTER_PRI, "ipv6"), + INTMAP_ENTRY(NWFILTER_ARP_FILTER_PRI , "arp" ), + INTMAP_ENTRY(NWFILTER_RARP_FILTER_PRI, "rarp"), + INTMAP_ENTRY_LAST, +}; /* * only one filter update allowed @@ -2030,6 +2038,12 @@ virNWFilterDefParseXML(xmlXPathContextPt _("unknown chain suffix '%s'"), chain); goto cleanup; } + /* assign an implicit priority -- support XML attribute later */ + if (intMapGetByString(chain_priorities, chain, 0, + &ret->chainPriority) == false) { + ret->chainPriority = (NWFILTER_MAX_FILTER_PRIORITY + + NWFILTER_MIN_FILTER_PRIORITY) / 2; + } } uuid = virXPathString("string(./uuid)", ctxt); Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.h =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.h +++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.h @@ -36,6 +36,7 @@ typedef ebiptablesRuleInst *ebiptablesRu struct _ebiptablesRuleInst { char *commandTemplate; enum virNWFilterChainSuffixType neededProtocolChain; + virNWFilterChainPriority chainPriority; char chainprefix; /* I for incoming, O for outgoing */ unsigned int priority; enum RuleType ruleType; Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c @@ -327,6 +327,7 @@ static int ebiptablesAddRuleInst(virNWFilterRuleInstPtr res, char *commandTemplate, enum virNWFilterChainSuffixType neededChain, + virNWFilterChainPriority chainPriority, char chainprefix, unsigned int priority, enum RuleType ruleType) @@ -340,6 +341,7 @@ ebiptablesAddRuleInst(virNWFilterRuleIns inst->commandTemplate = commandTemplate; inst->neededProtocolChain = neededChain; + inst->chainPriority = chainPriority; inst->chainprefix = chainprefix; inst->priority = priority; inst->ruleType = ruleType; @@ -1588,6 +1590,7 @@ _iptablesCreateRuleInstance(int directio return ebiptablesAddRuleInst(res, virBufferContentAndReset(final), nwfilter->chainsuffix, + nwfilter->chainPriority, '\0', rule->priority, (isIPv6) ? RT_IP6TABLES : RT_IPTABLES); @@ -2337,6 +2340,7 @@ ebtablesCreateRuleInstance(char chainPre return ebiptablesAddRuleInst(res, virBufferContentAndReset(&buf), nwfilter->chainsuffix, + nwfilter->chainPriority, chainPrefix, rule->priority, RT_EBTABLES);
On 10/26/2011 05:36 AM, Stefan Berger wrote:
For better handling of the sorting of chains introduce an internally used priority. Use a lookup table to store the priorities. For now their actual values do not matter just that the values cause the chains to be properly sorted through changes in the following patches. However, the values are chosen as negative so that once they are sorted along with filtering rules (whose priority may only be positive for now) they will always be instantiated before them (lower values cause instantiation before higher values). This is done to maintain backwards compatibility.
v3: - increased filter priorities to have more room before them
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
--- src/conf/nwfilter_conf.c | 14 ++++++++++++++ src/conf/nwfilter_conf.h | 12 ++++++++++++ src/nwfilter/nwfilter_ebiptables_driver.c | 4 ++++ src/nwfilter/nwfilter_ebiptables_driver.h | 1 + 4 files changed, 31 insertions(+)
ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Use the previously introduced chain priorities to sort the chains for access from an interface's 'root' table and have them created in the proper order. This gets rid of a lot of code that was previously creating the chains in a more hardcoded way. To determine what protocol a filter is used for evaluation do prefix- matching, i.e., the filter 'arp' is used to filter for the 'arp' protocol, 'ipv4' for the 'ipv4' protocol and 'arp-xyz' will also be used to filter for the 'arp' protocol following the prefix 'arp' in its name. Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/nwfilter/nwfilter_ebiptables_driver.c | 130 ++++++++++++++++++++++-------- 1 file changed, 98 insertions(+), 32 deletions(-) Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c @@ -2774,6 +2774,7 @@ ebtablesCreateTmpSubChain(virBufferPtr b int incoming, const char *ifname, enum l3_proto_idx protoidx, + const char *filtername, int stopOnError) { char rootchain[MAX_CHAINNAME_LENGTH], chain[MAX_CHAINNAME_LENGTH]; @@ -2781,7 +2782,8 @@ ebtablesCreateTmpSubChain(virBufferPtr b : CHAINPREFIX_HOST_OUT_TEMP; PRINT_ROOT_CHAIN(rootchain, chainPrefix, ifname); - PRINT_CHAIN(chain, chainPrefix, ifname, l3_protocols[protoidx].val); + PRINT_CHAIN(chain, chainPrefix, ifname, + (filtername) ? filtername : l3_protocols[protoidx].val); virBufferAsprintf(buf, CMD_DEF("%s -t %s -N %s") CMD_SEPARATOR @@ -3288,6 +3290,13 @@ ebiptablesRuleOrderSort(const void *a, c return ((*insta)->priority - (*instb)->priority); } +static int +ebiptablesFilterOrderSort(const virHashKeyValuePairPtr a, + const virHashKeyValuePairPtr b) +{ + return *(virNWFilterChainPriority *)a->value - + *(virNWFilterChainPriority *)b->value; +} static void iptablesCheckBridgeNFCallEnabled(bool isIPv6) @@ -3327,6 +3336,56 @@ iptablesCheckBridgeNFCallEnabled(bool is } } +/* + * Given a filtername determine the protocol it is used for evaluating + * We do prefix-matching to determine the protocol. + */ +static enum l3_proto_idx +ebtablesGetProtoIdxByFiltername(const char *filtername) +{ + enum l3_proto_idx idx; + + for (idx = 0; idx < L3_PROTO_LAST_IDX; idx++) { + if (STRPREFIX(filtername, l3_protocols[idx].val)) { + return idx; + } + } + + return -1; +} + +static int +ebtablesCreateTmpRootAndSubChains(virBufferPtr buf, + const char *ifname, + virHashTablePtr chains, int direction) +{ + int rc = 0, i; + + if (virHashSize(chains) != 0) { + char **filter_names; + + ebtablesCreateTmpRootChain(buf, direction, ifname, 1); + filter_names = (char **)virHashGetKeys(chains, + ebiptablesFilterOrderSort); + if (filter_names == NULL) { + virReportOOMError(); + rc = 1; + goto err_exit; + } + for (i = 0; filter_names[i]; i++) { + enum l3_proto_idx idx = ebtablesGetProtoIdxByFiltername( + filter_names[i]); + if ((int)idx < 0) + continue; + ebtablesCreateTmpSubChain(buf, direction, ifname, idx, + filter_names[i], 1); + } + virHashFreeKeys(chains, (void **)filter_names); + } + + err_exit: + return rc; +} static int ebiptablesApplyNewRules(virConnectPtr conn ATTRIBUTE_UNUSED, @@ -3337,24 +3396,43 @@ ebiptablesApplyNewRules(virConnectPtr co int i; int cli_status; ebiptablesRuleInstPtr *inst = (ebiptablesRuleInstPtr *)_inst; - int chains_in = 0, chains_out = 0; virBuffer buf = VIR_BUFFER_INITIALIZER; + virHashTablePtr chains_in_set = virHashCreate(10, NULL), + chains_out_set = virHashCreate(10, NULL); bool haveIptables = false; bool haveIp6tables = false; + if (!chains_in_set || !chains_out_set) { + virReportOOMError(); + goto exit_free_sets; + } + if (nruleInstances > 1 && inst) qsort(inst, nruleInstances, sizeof(inst[0]), ebiptablesRuleOrderSort); + /* scan the rules to see which chains need to be created */ for (i = 0; i < nruleInstances; i++) { sa_assert (inst); if (inst[i]->ruleType == RT_EBTABLES) { - if (inst[i]->chainprefix == CHAINPREFIX_HOST_IN_TEMP) - chains_in |= (1 << inst[i]->neededProtocolChain); - else - chains_out |= (1 << inst[i]->neededProtocolChain); + const char *name = virNWFilterChainSuffixTypeToString( + inst[i]->neededProtocolChain); + if (inst[i]->chainprefix == CHAINPREFIX_HOST_IN_TEMP) { + if (virHashUpdateEntry(chains_in_set, name, + &inst[i]->chainPriority)) { + virReportOOMError(); + goto exit_free_sets; + } + } else { + if (virHashUpdateEntry(chains_out_set, name, + &inst[i]->chainPriority)) { + virReportOOMError(); + goto exit_free_sets; + } + } } } + /* cleanup whatever may exist */ if (ebtables_cmd_path) { ebtablesUnlinkTmpRootChain(&buf, 1, ifname); ebtablesUnlinkTmpRootChain(&buf, 0, ifname); @@ -3364,30 +3442,11 @@ ebiptablesApplyNewRules(virConnectPtr co ebiptablesExecCLI(&buf, &cli_status); } - if (chains_in != 0) - ebtablesCreateTmpRootChain(&buf, 1, ifname, 1); - if (chains_out != 0) - ebtablesCreateTmpRootChain(&buf, 0, ifname, 1); - - if (chains_in & (1 << VIR_NWFILTER_CHAINSUFFIX_IPv4)) - ebtablesCreateTmpSubChain(&buf, 1, ifname, L3_PROTO_IPV4_IDX, 1); - if (chains_out & (1 << VIR_NWFILTER_CHAINSUFFIX_IPv4)) - ebtablesCreateTmpSubChain(&buf, 0, ifname, L3_PROTO_IPV4_IDX, 1); - - if (chains_in & (1 << VIR_NWFILTER_CHAINSUFFIX_IPv6)) - ebtablesCreateTmpSubChain(&buf, 1, ifname, L3_PROTO_IPV6_IDX, 1); - if (chains_out & (1 << VIR_NWFILTER_CHAINSUFFIX_IPv6)) - ebtablesCreateTmpSubChain(&buf, 0, ifname, L3_PROTO_IPV6_IDX, 1); - - /* keep arp,rarp as last */ - if (chains_in & (1 << VIR_NWFILTER_CHAINSUFFIX_ARP)) - ebtablesCreateTmpSubChain(&buf, 1, ifname, L3_PROTO_ARP_IDX, 1); - if (chains_out & (1 << VIR_NWFILTER_CHAINSUFFIX_ARP)) - ebtablesCreateTmpSubChain(&buf, 0, ifname, L3_PROTO_ARP_IDX, 1); - if (chains_in & (1 << VIR_NWFILTER_CHAINSUFFIX_RARP)) - ebtablesCreateTmpSubChain(&buf, 1, ifname, L3_PROTO_RARP_IDX, 1); - if (chains_out & (1 << VIR_NWFILTER_CHAINSUFFIX_RARP)) - ebtablesCreateTmpSubChain(&buf, 0, ifname, L3_PROTO_RARP_IDX, 1); + /* create needed chains */ + if (ebtablesCreateTmpRootAndSubChains(&buf, ifname, chains_in_set , 1) || + ebtablesCreateTmpRootAndSubChains(&buf, ifname, chains_out_set, 0)) { + goto tear_down_tmpebchains; + } if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0) goto tear_down_tmpebchains; @@ -3477,14 +3536,17 @@ ebiptablesApplyNewRules(virConnectPtr co iptablesCheckBridgeNFCallEnabled(true); } - if (chains_in != 0) + if (virHashSize(chains_in_set) != 0) ebtablesLinkTmpRootChain(&buf, 1, ifname, 1); - if (chains_out != 0) + if (virHashSize(chains_out_set) != 0) ebtablesLinkTmpRootChain(&buf, 0, ifname, 1); if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0) goto tear_down_ebsubchains_and_unlink; + virHashFree(chains_in_set); + virHashFree(chains_out_set); + return 0; tear_down_ebsubchains_and_unlink: @@ -3519,6 +3581,10 @@ tear_down_tmpebchains: "interface %s."), ifname); +exit_free_sets: + virHashFree(chains_in_set); + virHashFree(chains_out_set); + return 1; }
On 10/26/2011 05:36 AM, Stefan Berger wrote:
Use the previously introduced chain priorities to sort the chains for access from an interface's 'root' table and have them created in the proper order. This gets rid of a lot of code that was previously creating the chains in a more hardcoded way.
To determine what protocol a filter is used for evaluation do prefix- matching, i.e., the filter 'arp' is used to filter for the 'arp' protocol, 'ipv4' for the 'ipv4' protocol and 'arp-xyz' will also be used to filter for the 'arp' protocol following the prefix 'arp' in its name.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
--- src/nwfilter/nwfilter_ebiptables_driver.c | 130 ++++++++++++++++++++++-------- 1 file changed, 98 insertions(+), 32 deletions(-)
+static int +ebiptablesFilterOrderSort(const virHashKeyValuePairPtr a, + const virHashKeyValuePairPtr b) +{ + return *(virNWFilterChainPriority *)a->value - + *(virNWFilterChainPriority *)b->value; +}
I tend to worry about someone passing INT_MAX and INT_MIN to a qsort algorithm, then getting the wrong comparison sense because of integer overflow when it uses straight subtraction; so I like to add a comment explaining why I know that the valid input was capped and overflow is impossible.
static void iptablesCheckBridgeNFCallEnabled(bool isIPv6) @@ -3327,6 +3336,56 @@ iptablesCheckBridgeNFCallEnabled(bool is } }
+/* + * Given a filtername determine the protocol it is used for evaluating + * We do prefix-matching to determine the protocol. + */ +static enum l3_proto_idx +ebtablesGetProtoIdxByFiltername(const char *filtername) +{ + enum l3_proto_idx idx; + + for (idx = 0; idx < L3_PROTO_LAST_IDX; idx++) { + if (STRPREFIX(filtername, l3_protocols[idx].val)) { + return idx; + } + }
This works as long as none of the entries in l3_protocols are a prefix of any other entry; is it worth a comment at the declaration of l3_protocols warning about this assumption for when new protocol names are added to the table?
+ + return -1; +} + +static int +ebtablesCreateTmpRootAndSubChains(virBufferPtr buf, + const char *ifname, + virHashTablePtr chains, int direction) +{ + int rc = 0, i; + + if (virHashSize(chains) != 0) { + char **filter_names; + + ebtablesCreateTmpRootChain(buf, direction, ifname, 1); + filter_names = (char **)virHashGetKeys(chains, + ebiptablesFilterOrderSort);
This area of code will be impacted by my comments on 1/10.
+ if (filter_names == NULL) { + virReportOOMError();
Your implementation of virHashGetKeys already reported OOM error for all error paths except for numElems < 0; but that means that this call to virReportOOMError() is either redundant (a second report), or misleading (if numElems < 0, you are not OOM, but have a programming bug on hand for passing in an invalid hash table in the first place).
+ rc = 1;
Can we get this updated to use -1 for failures, so that there's less to clean up later when we fix this file to match libvirt conventions?
+ goto err_exit; + } + for (i = 0; filter_names[i]; i++) { + enum l3_proto_idx idx = ebtablesGetProtoIdxByFiltername( + filter_names[i]); + if ((int)idx < 0) + continue; + ebtablesCreateTmpSubChain(buf, direction, ifname, idx, + filter_names[i], 1); + } + virHashFreeKeys(chains, (void **)filter_names);
Oh, this reminds me of some additional feedback I meant to give on 1/10 - as part of the documentation you add, be sure to mention that the returned array is only valid as long as the underlying hash table is not modified (especially if you alter things to return in-hash pointers rather than cloning keys) - adding or removing elements, or deleting the hash table, will invalidate the returned key array.
@@ -3337,24 +3396,43 @@ ebiptablesApplyNewRules(virConnectPtr co int i; int cli_status; ebiptablesRuleInstPtr *inst = (ebiptablesRuleInstPtr *)_inst; - int chains_in = 0, chains_out = 0; virBuffer buf = VIR_BUFFER_INITIALIZER; + virHashTablePtr chains_in_set = virHashCreate(10, NULL), + chains_out_set = virHashCreate(10, NULL);
Style-wise, I would list this as two declarations separated by ';', rather than one declaration of two variables separated by ','. That is, I find it slightly hard to read stacked declarations that involve multi-argument function calls as the initializers. If patch 1/10 were used as-is, then this patch looks good except for a few easy-to-fix nits. But more likely, this will also need a v5 due to rebasing on top of changes to how you grab a sorted list of hash keys (or key-value pairs). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 11/16/2011 08:02 PM, Eric Blake wrote:
> On 10/26/2011 05:36 AM, Stefan Berger wrote:
>> Use the previously introduced chain priorities to sort the chains for access
>> from an interface's 'root' table and have them created in the proper order.
>> This gets rid of a lot of code that was previously creating the chains in a
>> more hardcoded way.
>>
>> To determine what protocol a filter is used for evaluation do prefix-
>> matching, i.e., the filter 'arp' is used to filter for the 'arp' protocol,
>> 'ipv4' for the 'ipv4' protocol and 'arp-xyz' will also be used to filter
>> for the 'arp' protocol following the prefix 'arp' in its name.
>>
>> Signed-off-by: Stefan Berger<stefanb@linux.vnet.ibm.com>
>>
>> ---
>> src/nwfilter/nwfilter_ebiptables_driver.c | 130 ++++++++++++++++++++++--------
>> 1 file changed, 98 insertions(+), 32 deletions(-)
>>
>>
>> +static int
>> +ebiptablesFilterOrderSort(const virHashKeyValuePairPtr a,
>> + const virHashKeyValuePairPtr b)
>> +{
>> + return *(virNWFilterChainPriority *)a->value -
>> + *(virNWFilterChainPriority *)b->value;
>> +}
> I tend to worry about someone passing INT_MAX and INT_MIN to a qsort
> algorithm, then getting the wrong comparison sense because of integer
> overflow when it uses straight subtraction; so I like to add a comment
> explaining why I know that the valid input was capped and overflow is
> impossible.
>
Values here are limited to range [-1000, 1000]. Added that as a comment now.
>>
>> static void
>> iptablesCheckBridgeNFCallEnabled(bool isIPv6)
>> @@ -3327,6 +3336,56 @@ iptablesCheckBridgeNFCallEnabled(bool is
>> }
>> }
>>
>> +/*
>> + * Given a filtername determine the protocol it is used for evaluating
>> + * We do prefix-matching to determine the protocol.
>> + */
>> +static enum l3_proto_idx
>> +ebtablesGetProtoIdxByFiltername(const char *filtername)
>> +{
>> + enum l3_proto_idx idx;
>> +
>> + for (idx = 0; idx< L3_PROTO_LAST_IDX; idx++) {
>> + if (STRPREFIX(filtername, l3_protocols[idx].val)) {
>> + return idx;
>> + }
>> + }
> This works as long as none of the entries in l3_protocols are a prefix
> of any other entry; is it worth a comment at the declaration of
> l3_protocols warning about this assumption for when new protocol names
> are added to the table?
>
Added a comment there. Though if "abc" comes before "ab", that would
still work, though entries have to be sorted that way then.
>> +
>> + return -1;
>> +}
>> +
>> +static int
>> +ebtablesCreateTmpRootAndSubChains(virBufferPtr buf,
>> + const char *ifname,
>> + virHashTablePtr chains, int direction)
>> +{
>> + int rc = 0, i;
>> +
>> + if (virHashSize(chains) != 0) {
>> + char **filter_names;
>> +
>> + ebtablesCreateTmpRootChain(buf, direction, ifname, 1);
>> + filter_names = (char **)virHashGetKeys(chains,
>> + ebiptablesFilterOrderSort);
> This area of code will be impacted by my comments on 1/10.
>
Conversion done.
>> + if (filter_names == NULL) {
>> + virReportOOMError();
> Your implementation of virHashGetKeys already reported OOM error for all
> error paths except for numElems< 0; but that means that this call to
> virReportOOMError() is either redundant (a second report), or misleading
> (if numElems< 0, you are not OOM, but have a programming bug on hand
> for passing in an invalid hash table in the first place).
>
So I am not reporting an error anymore. If the hash table 'chains' was
NULL, which would then also lead to '<', it would not end up in this
function at all but would have been intercepted earlier.
>> + rc = 1;
> Can we get this updated to use -1 for failures, so that there's less to
> clean up later when we fix this file to match libvirt conventions?
>
Did that now.
There will also be a patch converting all the nwfilter code to return
'-1' upon failure. I got that in my queue, but only for later.
>> + goto err_exit;
>> + }
>> + for (i = 0; filter_names[i]; i++) {
>> + enum l3_proto_idx idx = ebtablesGetProtoIdxByFiltername(
>> + filter_names[i]);
>> + if ((int)idx< 0)
>> + continue;
>> + ebtablesCreateTmpSubChain(buf, direction, ifname, idx,
>> + filter_names[i], 1);
>> + }
>> + virHashFreeKeys(chains, (void **)filter_names);
> Oh, this reminds me of some additional feedback I meant to give on 1/10
> - as part of the documentation you add, be sure to mention that the
> returned array is only valid as long as the underlying hash table is not
> modified (especially if you alter things to return in-hash pointers
> rather than cloning keys) - adding or removing elements, or deleting the
> hash table, will invalidate the returned key array.
>
I added that also as comment to the virHashGetItems() description.
>> @@ -3337,24 +3396,43 @@ ebiptablesApplyNewRules(virConnectPtr co
>> int i;
>> int cli_status;
>> ebiptablesRuleInstPtr *inst = (ebiptablesRuleInstPtr *)_inst;
>> - int chains_in = 0, chains_out = 0;
>> virBuffer buf = VIR_BUFFER_INITIALIZER;
>> + virHashTablePtr chains_in_set = virHashCreate(10, NULL),
>> + chains_out_set = virHashCreate(10, NULL);
> Style-wise, I would list this as two declarations separated by ';',
> rather than one declaration of two variables separated by ','. That is,
> I find it slightly hard to read stacked declarations that involve
> multi-argument function calls as the initializers.
>
Fixed.
> If patch 1/10 were used as-is, then this patch looks good except for a
> few easy-to-fix nits. But more likely, this will also need a v5 due to
> rebasing on top of changes to how you grab a sorted list of hash keys
> (or key-value pairs).
>
Thanks for the review.
Stefan
Use scripts for the renaming and cleaning up of chains. This allows us to get rid of some of the code that is only capable of renaming and removing chains whose names are hardcoded. A shell function 'collect_chains' is introduced that is given the name of an ebtables chain and then recursively determines the names of all chanins that are accessed from this chain and its sub-chains using 'jumps'. The resulting list of chain names is then used to delete all the found chains by first flushing and then deleting them. The same function is also used for renaming temporary filters to their final names. I tested this with the bash and dash as script interpreters. v2: - setting IFS to intended value; works with bash and dash (phew!) Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/nwfilter/nwfilter_ebiptables_driver.c | 204 +++++++++++++++++------------- 1 file changed, 117 insertions(+), 87 deletions(-) Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c @@ -91,6 +91,49 @@ static char *gawk_cmd_path; #define PRINT_CHAIN(buf, prefix, ifname, suffix) \ snprintf(buf, sizeof(buf), "%c-%s-%s", prefix, ifname, suffix) +static const char ebtables_script_func_collect_chains[] = + "collect_chains()\n" + "{\n" + " local sc\n" + " sc=$(%s -t %s -L $1 | \\\n" + " sed -n \"/Bridge chain*/,$ s/.*\\-j \\([%s]-.*\\)/\\1/p\")\n" + " for tmp in `echo \"$sc\"`; do\n" + " [ -n \"$tmp\" ] && sc=\"$sc $(collect_chains $tmp)\"\n" + " done\n" + " echo \"$sc\"\n" + "}\n"; + +static const char ebiptables_script_func_rm_chains[] = + "rm_chains()\n" + "{\n" + " for tmp in `echo \"$1\"`; do [ -n \"$tmp\" ] && %s -t %s -F $tmp; done\n" + " for tmp in `echo \"$1\"`; do [ -n \"$tmp\" ] && %s -t %s -X $tmp; done\n" + "}\n"; + +static const char ebiptables_script_func_rename_chains[] = + "rename_chains()\n" + "{\n" + " for tmp in `echo \"$1\"`; do\n" + " if [ -n \"$tmp\" ]; then\n" + " tmp2=`expr substr \"$tmp\" 1 1`\n" + " if [ $tmp2 = \"%c\" ]; then\n" + " %s -t %s -E \"$tmp\" \"%c\"`expr substr \"$tmp\" 2 33`\n" + " elif [ $tmp2 = \"%c\" ]; then\n" + " %s -t %s -E \"$tmp\" \"%c\"`expr substr \"$tmp\" 2 33`\n" + " fi\n" + " fi\n" + " done\n" + "}\n"; + +static const char ebiptables_script_set_ifs[] = + "IFS=$' \\t\\n'\n" + "[ `expr length \"$IFS\"` != 3 ] && " + "IFS=$(echo | %s '{ print \"\\t\\n \"}')\n"; + +#define NWFILTER_FUNC_COLLECT_CHAINS ebtables_script_func_collect_chains +#define NWFILTER_FUNC_DELETE_CHAINS ebiptables_script_func_rm_chains +#define NWFILTER_FUNC_RENAME_CHAINS ebiptables_script_func_rename_chains +#define NWFILTER_FUNC_SET_IFS ebiptables_script_set_ifs #define VIRT_IN_CHAIN "libvirt-in" #define VIRT_OUT_CHAIN "libvirt-out" @@ -2805,95 +2848,65 @@ ebtablesCreateTmpSubChain(virBufferPtr b return 0; } - -static int -_ebtablesRemoveSubChain(virBufferPtr buf, - int incoming, - const char *ifname, - enum l3_proto_idx protoidx, - int isTempChain) +static int _ebtablesRemoveSubChains(virBufferPtr buf, + const char *ifname, + const char *chains) { - char rootchain[MAX_CHAINNAME_LENGTH], chain[MAX_CHAINNAME_LENGTH]; - char chainPrefix; - - if (isTempChain) { - chainPrefix =(incoming) ? CHAINPREFIX_HOST_IN_TEMP - : CHAINPREFIX_HOST_OUT_TEMP; - } else { - chainPrefix =(incoming) ? CHAINPREFIX_HOST_IN - : CHAINPREFIX_HOST_OUT; - } + char rootchain[MAX_CHAINNAME_LENGTH]; + unsigned i; - PRINT_ROOT_CHAIN(rootchain, chainPrefix, ifname); - PRINT_CHAIN(chain, chainPrefix, ifname, l3_protocols[protoidx].val); - - virBufferAsprintf(buf, - "%s -t %s -D %s -p 0x%x -j %s" CMD_SEPARATOR - "%s -t %s -F %s" CMD_SEPARATOR - "%s -t %s -X %s" CMD_SEPARATOR, + virBufferAsprintf(buf, NWFILTER_FUNC_COLLECT_CHAINS, + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, chains); + virBufferAsprintf(buf, NWFILTER_FUNC_DELETE_CHAINS, ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, - rootchain, l3_protocols[protoidx].attr, chain, + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE); - ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, chain, + virBufferAsprintf(buf, NWFILTER_FUNC_SET_IFS, gawk_cmd_path); + virBufferAddLit(buf, "a=\""); + for (i = 0; chains[i] != 0; i++) { + PRINT_ROOT_CHAIN(rootchain, chains[i], ifname); + virBufferAsprintf(buf, "$(collect_chains %s) ", rootchain); + } + virBufferAddLit(buf, "\"\n"); - ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, chain); + for (i = 0; chains[i] != 0; i++) { + PRINT_ROOT_CHAIN(rootchain, chains[i], ifname); + virBufferAsprintf(buf, + "%s -t %s -F %s\n", + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, + rootchain); + } + virBufferAddLit(buf, "rm_chains \"$a\"\n"); return 0; } - -static int -ebtablesRemoveSubChain(virBufferPtr buf, - int incoming, - const char *ifname, - enum l3_proto_idx protoidx) -{ - return _ebtablesRemoveSubChain(buf, - incoming, ifname, protoidx, 0); -} - - static int ebtablesRemoveSubChains(virBufferPtr buf, const char *ifname) { - enum l3_proto_idx i; - - for (i = 0; i < L3_PROTO_LAST_IDX; i++) { - ebtablesRemoveSubChain(buf, 1, ifname, i); - ebtablesRemoveSubChain(buf, 0, ifname, i); - } + char chains[3] = { + CHAINPREFIX_HOST_IN, + CHAINPREFIX_HOST_OUT, + 0 + }; - return 0; + return _ebtablesRemoveSubChains(buf, ifname, chains); } - -static int -ebtablesRemoveTmpSubChain(virBufferPtr buf, - int incoming, - const char *ifname, - enum l3_proto_idx protoidx) -{ - return _ebtablesRemoveSubChain(buf, - incoming, ifname, protoidx, 1); -} - - static int ebtablesRemoveTmpSubChains(virBufferPtr buf, const char *ifname) { - enum l3_proto_idx i; - - for (i = 0; i < L3_PROTO_LAST_IDX; i++) { - ebtablesRemoveTmpSubChain(buf, 1, ifname, i); - ebtablesRemoveTmpSubChain(buf, 0, ifname, i); - } + char chains[3] = { + CHAINPREFIX_HOST_IN_TEMP, + CHAINPREFIX_HOST_OUT_TEMP, + 0 + }; - return 0; + return _ebtablesRemoveSubChains(buf, ifname, chains); } - static int ebtablesRenameTmpSubChain(virBufferPtr buf, int incoming, @@ -2920,31 +2933,51 @@ ebtablesRenameTmpSubChain(virBufferPtr b return 0; } - static int -ebtablesRenameTmpSubChains(virBufferPtr buf, +ebtablesRenameTmpRootChain(virBufferPtr buf, + int incoming, const char *ifname) { - enum l3_proto_idx i; + return ebtablesRenameTmpSubChain(buf, incoming, ifname, NULL); +} - for (i = 0; i < L3_PROTO_LAST_IDX; i++) { - ebtablesRenameTmpSubChain (buf, 1, ifname, l3_protocols[i].val); - ebtablesRenameTmpSubChain (buf, 0, ifname, l3_protocols[i].val); +static int +ebtablesRenameTmpSubAndRootChains(virBufferPtr buf, + const char *ifname) +{ + char rootchain[MAX_CHAINNAME_LENGTH]; + unsigned i; + char chains[3] = { + CHAINPREFIX_HOST_IN_TEMP, + CHAINPREFIX_HOST_OUT_TEMP, + 0}; + + virBufferAsprintf(buf, NWFILTER_FUNC_COLLECT_CHAINS, + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, chains); + virBufferAsprintf(buf, NWFILTER_FUNC_RENAME_CHAINS, + CHAINPREFIX_HOST_IN_TEMP, + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, + CHAINPREFIX_HOST_IN, + CHAINPREFIX_HOST_OUT_TEMP, + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, + CHAINPREFIX_HOST_OUT); + + virBufferAsprintf(buf, NWFILTER_FUNC_SET_IFS, gawk_cmd_path); + virBufferAddLit(buf, "a=\""); + for (i = 0; chains[i] != 0; i++) { + PRINT_ROOT_CHAIN(rootchain, chains[i], ifname); + virBufferAsprintf(buf, "$(collect_chains %s) ", rootchain); } + virBufferAddLit(buf, "\"\n"); - return 0; -} + virBufferAddLit(buf, "rename_chains \"$a\"\n"); + ebtablesRenameTmpRootChain(buf, 1, ifname); + ebtablesRenameTmpRootChain(buf, 0, ifname); -static int -ebtablesRenameTmpRootChain(virBufferPtr buf, - int incoming, - const char *ifname) -{ - return ebtablesRenameTmpSubChain(buf, incoming, ifname, NULL); + return 0; } - static void ebiptablesInstCommand(virBufferPtr buf, const char *templ, char cmd, int pos, @@ -3654,9 +3687,7 @@ ebiptablesTearOldRules(virConnectPtr con ebtablesRemoveRootChain(&buf, 1, ifname); ebtablesRemoveRootChain(&buf, 0, ifname); - ebtablesRenameTmpSubChains(&buf, ifname); - ebtablesRenameTmpRootChain(&buf, 1, ifname); - ebtablesRenameTmpRootChain(&buf, 0, ifname); + ebtablesRenameTmpSubAndRootChains(&buf, ifname); ebiptablesExecCLI(&buf, &cli_status); } @@ -3741,12 +3772,11 @@ ebiptablesAllTeardown(const char *ifname ebtablesUnlinkRootChain(&buf, 1, ifname); ebtablesUnlinkRootChain(&buf, 0, ifname); + ebtablesRemoveSubChains(&buf, ifname); + ebtablesRemoveRootChain(&buf, 1, ifname); ebtablesRemoveRootChain(&buf, 0, ifname); - - ebtablesRemoveSubChains(&buf, ifname); } - ebiptablesExecCLI(&buf, &cli_status); return 0;
On 10/26/2011 05:36 AM, Stefan Berger wrote:
Use scripts for the renaming and cleaning up of chains. This allows us to get rid of some of the code that is only capable of renaming and removing chains whose names are hardcoded.
Resuming where I left off, in the hopes that this helps before you post v5...
A shell function 'collect_chains' is introduced that is given the name of an ebtables chain and then recursively determines the names of all chanins that are accessed from this chain and its sub-chains using 'jumps'.
s/chanins/chains/
The resulting list of chain names is then used to delete all the found chains by first flushing and then deleting them.
The same function is also used for renaming temporary filters to their final names.
I tested this with the bash and dash as script interpreters.
Well, there's still the overriding design nag that we want to avoid shell interpretation if at all possible, but that's a _much_ bigger rewrite for another day, so I can live with this current patch (it's not making our use of sh any worse than it already was).
+static const char ebtables_script_func_collect_chains[] =
Reading shell code that has been quoted into a C string literal to be passed through printf is an interesting exercise :)
+ "collect_chains()\n"
Making sure I understand the rest of this patch, this function is reached in two places by the rest of your patch, with code like: + virBufferAsprintf(buf, "$(collect_chains %s) ", rootchain); thus, you are using one command substitution per rootchain, where this function then outputs words to be parsed as part of a resulting command. That's a lot of processes; would it be worth using a shell for loop instead of a command-substitution per rootchain
+ "{\n" + " local sc\n"
'local' is not portable; POSIX doesn't guarantee it. I suppose we can get away with it since both bash and dash support it, and this code is specific to Linux where we know /bin/sh will be either bash or dash, but it would be even nicer if we could stick to POSIX syntax (which means avoiding 'local' and treating all variables are global, and you have to be careful that variables used inside a function are not likely to cause conflicts with any other variables used globally).
+ " sc=$(%s -t %s -L $1 | \\\n"
Just so I'm making sure I understand things, this line is paired with ebtables_cmd_path and EBTABLES_DEFAULT_TABLE, which results in something like this in shell ($1 being a rootchain name, fitting the pattern libvirt-%c-%s, so I think you're okay that $1 wasn't quoted): sc=$(/path/to/iptables -t nat -L $1 | ... Can you please include a comment mentioning the typical output from such a command that you intend to be parsing? Technically, you don't need backslash-newline after pipe; a trailing pipe is sufficient to continue a statement to the next line in shell. But it doesn't hurt to leave it in either.
+ " sed -n \"/Bridge chain*/,$ s/.*\\-j \\([%s]-.*\\)/\\1/p\")\n" 1 2 3 4 5
1. Did you really mean to match lines with "Bridge chai" or "Bridge chainnn"? That * is probably superfluous. 2. "$ " is not portable shell. To be portable, it should be "\$ " or '$ '. 3. \- is not a portable sed escape. But you don't need escaping for '-', since searching for a literal - in a sed expression works just fine. 4. This %s matches up to the 'chains' argument in this code: + virBufferAsprintf(buf, NWFILTER_FUNC_COLLECT_CHAINS, + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, chains); which I in turn traced back to: + char chains[3] = { + CHAINPREFIX_HOST_IN_TEMP, // 'J' + CHAINPREFIX_HOST_OUT_TEMP, // 'P' + 0}; so it looks like you are taking a line that looks like: Bridge chain ... -j J-... and turning it into: J-... while ignoring all other lines. 5. Use of "\(\)/\1" in shell is unusual (it's well-defined by POSIX that the backslash is preserved literally if the next character is not backquote, dollar, backslash, or newline), but for safety reason, I prefer to write it as "\\(\\)/\\1" to make it obvious that in the shell code I meant a literal backslash rather than relying on POSIX rules. Put all of these together, and this line should probably be: " sed -n \"/Bridge chain/,\\$ s/.*-j \\\\([%s]-.*\\\\)/\\\\1/p\")\n"
+ " for tmp in `echo \"$sc\"`; do\n"
I prefer $() over ``. But this echo is a wasted process - no need to echo when you can just use $sc directly: " for tmp in $sc; do\n"
+ " [ -n \"$tmp\" ] && sc=\"$sc $(collect_chains $tmp)\"\n"
Odd to be assigning to $sc in the middle of a loop based on the contents of $sc, and problematic if we change $sc to be global instead of local. Furthermore, [ -n $tmp ] will always be true (whether with your 'for tmp in `echo "$sc"`' or my 'for tmp in $sc', the point remains that word splitting is done, so each value of $tmp will be non-empty and contain no spaces).
+ " done\n" + " echo \"$sc\"\n" + "}\n";
Ah, so the end result is that you echo each name found, as well as recurse on each name found to echo any dependent names. Is order important (all names from the first chain must be output before any names from the recursive calls)? If not, then I can rewrite this function to avoid local sc in the first place. Here, in shell form (I'll leave you to convert it back into C string literal form): collect_chains() { for tmp in $(iptables -t nat -L $1 | \ sed -n "/Bridge chain/,\$ s/.*-j \\([JP]-.*\\)/\\1/p"); do echo $tmp collect_chains $tmp done }
+ +static const char ebiptables_script_func_rm_chains[] = + "rm_chains()\n"
It looks like you are calling rm_chains() with a single argument containing a whitespace separated list of arguments. + virBufferAddLit(buf, "rm_chains \"$a\"\n"); Why not just call rm_chains with multiple arguments? virBufferAddLit(buf, "rm_chains $a\n");
+ "{\n" + " for tmp in `echo \"$1\"`; do [ -n \"$tmp\" ] && %s -t %s -F $tmp; done\n" + " for tmp in `echo \"$1\"`; do [ -n \"$tmp\" ] && %s -t %s -X $tmp; done\n" + "}\n";
Is it essential that all chains be flushed before any are deleted? If so, then keeping this as two for loops makes sense; if not, then these could be combined. Same story about `echo "$1"` being identical to the much simpler $1. Same story about $tmp always being non-empty in a shell for-loop where the tokens were created by word-splitting. With one argument containing a quoted list, I'd write this as: rm_chains() { for tmp in $1; do %s -t %s -F $tmp; done for tmp in $1; do %s -t %s -X $tmp; done } with multiple arguments (unquoted $a), I'd write it as: rm_chains() { for tmp in $*; do %s -t %s -F $tmp; done for tmp in $*; do %s -t %s -X $tmp; done } <sidenote> A point that might be worth a separate cleanup patch: it looks awkward to have to write the literal name of the script and the table name in multiple places into the script: + virBufferAsprintf(buf, NWFILTER_FUNC_DELETE_CHAINS, + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE); generating: rm_chains() { for tmp in $*; do /path/to/iptables -t nat -f $tmp; done... .. rm_chains $a Why not change your shell script to set up a shell variable up front for ebtables_cmd_path and EBTABLES_DEFAULT_TABLE, and use that as a shell variable instead of as a printf substitution? That is, have your overall script look like: TABLESCMD=/path/to/iptables TABLE=nat rm_chains() { for tmp in $*; do $TABLESCMD -t $TABLE -f $tmp; done ... rm_chains $a where the script was generated by: virBufferAsprintf(buf, "TABLESCMD=%s\nTABLES=%s\n", ebtables_cmd_path, EBTABLES_DEFAULT_TABLE); virBufferAsprintf(buf, NWFILTER_FUNC_DELETE_CHAINS); ... virBufferAddLit(buf, "rm_chains $a\n"); </sidenote>
+ +static const char ebiptables_script_func_rename_chains[] = + "rename_chains()\n" + "{\n" + " for tmp in `echo \"$1\"`; do\n"
Again, `echo "$1"` is overkill for $1,
+ " if [ -n \"$tmp\" ]; then\n"
and testing for non-empty $tmp is wasted effort, if the loop is over a word-split $1.
+ " tmp2=`expr substr \"$tmp\" 1 1`\n"
expr substr is not portable to POSIX. But you don't need it, when a shell case statement, followed by a POSIX variable expansion, will do the same.
+ " if [ $tmp2 = \"%c\" ]; then\n" + " %s -t %s -E \"$tmp\" \"%c\"`expr substr \"$tmp\" 2 33`\n" + " elif [ $tmp2 = \"%c\" ]; then\n" + " %s -t %s -E \"$tmp\" \"%c\"`expr substr \"$tmp\" 2 33`\n" + " fi\n" + " fi\n" + " done\n" + "}\n";
I'd write this as: rename_chains() { for tmp in $1; do case $tmp in J*) /path/to/iptables -t nat -E $tmp I${tmp#?} ;; P*) /path/to/iptables -t nat -E $tmp O${tmp#?} ;; esac done } or back in your C-string format: "rename_chains()\n" "{\n" " for tmp in $1; do\n" " case $tmp in\n" " %c*) %s -t %s -E $tmp %c${tmp#?} ;;\n" " %c*) %s -t %s -E $tmp %c${tmp#?} ;;\n" " esac\n" " done\n" "}\n"
+ +static const char ebiptables_script_set_ifs[] = + "IFS=$' \\t\\n'\n" + "[ `expr length \"$IFS\"` != 3 ] && " + "IFS=$(echo | %s '{ print \"\\t\\n \"}')\n";
Oy - my head hurts! $'' is not portable; neither is expr length. And doing IFS=$() is just wrong - that eats the trailing newline, leaving you with just IFS=space-tab instead of the intended IFS=space-tab-newline. Just do: static const char ebiptables_script_set_ifs[] = "nl='\n" "'; IFS=' ''\t'$nl\n" and you're done. No need for shenanigans with command substitution, expr, or awk. [Side note - the only reason I write ' ''\t' rather than ' \t' is that I don't like seeing space-tab in generated scripts; some editors have a tendency to corrupt space-tab in the name of being "helpful", so separating the characters avoids that problem.]
+ +#define NWFILTER_FUNC_COLLECT_CHAINS ebtables_script_func_collect_chains +#define NWFILTER_FUNC_DELETE_CHAINS ebiptables_script_func_rm_chains
This particular change in names made it a bit harder to search the rest of your patch (I had to search two different strings to see where the function definition was emitted, vs. where it was called); I'd suggest either naming the function delete_chains, or the macro NWFILTER_FUNC_RM_CHAINS, for consistency.
+#define NWFILTER_FUNC_RENAME_CHAINS ebiptables_script_func_rename_chains +#define NWFILTER_FUNC_SET_IFS ebiptables_script_set_ifs
#define VIRT_IN_CHAIN "libvirt-in" #define VIRT_OUT_CHAIN "libvirt-out" @@ -2805,95 +2848,65 @@ ebtablesCreateTmpSubChain(virBufferPtr b return 0; }
Phew - made it past the function definitions. Now, on to how they are used. [By the way, in case it's not obvious with my lengthy review above, I _like_ the fact that you are using shell functions to consolidate effort - as long as we are using a scripting language, we might as well use all of its power - and it does mean that any future attempt to refactor this away from a temporary shell script into libvirt primitives will have to keep that in mind]
- -static int -_ebtablesRemoveSubChain(virBufferPtr buf, - int incoming, - const char *ifname, - enum l3_proto_idx protoidx, - int isTempChain) +static int _ebtablesRemoveSubChains(virBufferPtr buf, + const char *ifname, + const char *chains) { - char rootchain[MAX_CHAINNAME_LENGTH], chain[MAX_CHAINNAME_LENGTH]; - char chainPrefix; - - if (isTempChain) { - chainPrefix =(incoming) ? CHAINPREFIX_HOST_IN_TEMP - : CHAINPREFIX_HOST_OUT_TEMP; - } else { - chainPrefix =(incoming) ? CHAINPREFIX_HOST_IN - : CHAINPREFIX_HOST_OUT; - } + char rootchain[MAX_CHAINNAME_LENGTH]; + unsigned i;
- PRINT_ROOT_CHAIN(rootchain, chainPrefix, ifname); - PRINT_CHAIN(chain, chainPrefix, ifname, l3_protocols[protoidx].val); - - virBufferAsprintf(buf, - "%s -t %s -D %s -p 0x%x -j %s" CMD_SEPARATOR - "%s -t %s -F %s" CMD_SEPARATOR - "%s -t %s -X %s" CMD_SEPARATOR, + virBufferAsprintf(buf, NWFILTER_FUNC_COLLECT_CHAINS, + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, chains); + virBufferAsprintf(buf, NWFILTER_FUNC_DELETE_CHAINS, ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, - rootchain, l3_protocols[protoidx].attr, chain, + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE);
- ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, chain, + virBufferAsprintf(buf, NWFILTER_FUNC_SET_IFS, gawk_cmd_path);
Given my above reformulation of SET_IFS, you don't need gawk_cmd_path here.
+ virBufferAddLit(buf, "a=\""); + for (i = 0; chains[i] != 0; i++) { + PRINT_ROOT_CHAIN(rootchain, chains[i], ifname); + virBufferAsprintf(buf, "$(collect_chains %s) ", rootchain); + } + virBufferAddLit(buf, "\"\n");
This results in the shell code: a="$(collect_chains name) $(collect_chains name) " If you added looping in the shell definition of collect_chains, this could instead be: a=$(collect_chains name name) Not essential, but food for thought for a smaller script.
- ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, chain); + for (i = 0; chains[i] != 0; i++) { + PRINT_ROOT_CHAIN(rootchain, chains[i], ifname); + virBufferAsprintf(buf, + "%s -t %s -F %s\n", + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, + rootchain); + } + virBufferAddLit(buf, "rm_chains \"$a\"\n");
This results in: rm_chains "$a" See my earlier comments where using $* instead of $1 would let you write this as: rm_chains $a instead.
+static int +ebtablesRenameTmpSubAndRootChains(virBufferPtr buf, + const char *ifname) +{ + char rootchain[MAX_CHAINNAME_LENGTH]; + unsigned i; + char chains[3] = { + CHAINPREFIX_HOST_IN_TEMP, + CHAINPREFIX_HOST_OUT_TEMP, + 0}; + + virBufferAsprintf(buf, NWFILTER_FUNC_COLLECT_CHAINS, + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, chains);
This means you are outputting the function definition from two different call paths. It doesn't hurt to (re-)define a function to the same value, while avoiding the function if it isn't used; on the other hand, I'm wondering if it is worth outputting the functions as part of the file header rather than risking having the function definitions appear multiple times throughout the temporary script. I can live with your current approach, so don't bother refactoring things unless you want to. [I speak from experience - I'm one of the autoconf maintainers, and solving the problem of how to generate shell script output based on m4 input, where pre-requisite shell functions are emitted at most once, up front, but used multiple times through the rest of the script, all in order to reduce the total size of the overall script, proved to be an interesting challenge in writing a code generator.]
+ virBufferAsprintf(buf, NWFILTER_FUNC_RENAME_CHAINS, + CHAINPREFIX_HOST_IN_TEMP, + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, + CHAINPREFIX_HOST_IN, + CHAINPREFIX_HOST_OUT_TEMP, + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, + CHAINPREFIX_HOST_OUT); + + virBufferAsprintf(buf, NWFILTER_FUNC_SET_IFS, gawk_cmd_path); + virBufferAddLit(buf, "a=\""); + for (i = 0; chains[i] != 0; i++) { + PRINT_ROOT_CHAIN(rootchain, chains[i], ifname); + virBufferAsprintf(buf, "$(collect_chains %s) ", rootchain); }
Similar comments about collect_chains usage possibly being easier to understand if it includes a shell for loop over multiple arguments. Looking forward to seeing what v5 looks like! -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 11/17/2011 12:33 PM, Eric Blake wrote:
> On 10/26/2011 05:36 AM, Stefan Berger wrote:
>> The resulting list of chain names is then used to delete all the found
>> chains by first flushing and then deleting them.
>>
>> The same function is also used for renaming temporary filters to their final
>> names.
>>
>> I tested this with the bash and dash as script interpreters.
> Well, there's still the overriding design nag that we want to avoid
> shell interpretation if at all possible, but that's a _much_ bigger
> rewrite for another day, so I can live with this current patch (it's not
> making our use of sh any worse than it already was).
>
Agree.
>>
>> +static const char ebtables_script_func_collect_chains[] =
> Reading shell code that has been quoted into a C string literal to be
> passed through printf is an interesting exercise :)
>
>> + "collect_chains()\n"
> Making sure I understand the rest of this patch, this function is
> reached in two places by the rest of your patch, with code like:
> + virBufferAsprintf(buf, "$(collect_chains %s) ", rootchain);
> thus, you are using one command substitution per rootchain, where this
> function then outputs words to be parsed as part of a resulting command.
>
Yes, so this function starts out with the name of an ebtables chain and
tries to determine all dependent chains of it, i.e., 'subchains'.
#> ebtables -t nat -L libvirt-I-tck-test205002
Bridge table: nat
Bridge chain: libvirt-I-tck-test205002, entries: 5, policy: ACCEPT
-p IPv4 -j I-tck-test205002-ipv4
-p ARP -j I-tck-test205002-arp
-p 0x8035 -j I-tck-test205002-rarp
-p 0x835 -j ACCEPT
-j DROP
Assuming I have the interface 'tck-test205002', I then run this function
to find the chains
I-tck-test205002-ipv4, I-tck-test205002-arp and I-tck-test-205002-rarp
and then visit
each one of those and try to find the chains they are 'jumping into'.
> That's a lot of processes; would it be worth using a shell for loop
> instead of a command-substitution per rootchain
>
I followed your suggested code below.
[...]
>
> sc=$(/path/to/iptables -t nat -L $1 | ...
>
> Can you please include a comment mentioning the typical output from such
> a command that you intend to be parsing?
>
See above (uses ebtables rather than iptables).
> Technically, you don't need backslash-newline after pipe; a trailing
> pipe is sufficient to continue a statement to the next line in shell.
> But it doesn't hurt to leave it in either.
>
>> + " sed -n \"/Bridge chain*/,$ s/.*\\-j \\([%s]-.*\\)/\\1/p\")\n"
> 1 2 3 4 5
>
> 1. Did you really mean to match lines with "Bridge chai" or "Bridge
> chainnn"? That * is probably superfluous.
>
No, that was wrong.
> 2. "$ " is not portable shell. To be portable, it should be "\$ " or '$ '.
>
Didn't know...
> 3. \- is not a portable sed escape. But you don't need escaping for
> '-', since searching for a literal - in a sed expression works just fine.
>
> 4. This %s matches up to the 'chains' argument in this code:
> + virBufferAsprintf(buf, NWFILTER_FUNC_COLLECT_CHAINS,
> + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, chains);
> which I in turn traced back to:
> + char chains[3] = {
> + CHAINPREFIX_HOST_IN_TEMP, // 'J'
> + CHAINPREFIX_HOST_OUT_TEMP, // 'P'
> + 0};
> so it looks like you are taking a line that looks like:
> Bridge chain ... -j J-...
> and turning it into:
> J-...
> while ignoring all other lines.
>
> 5. Use of "\(\)/\1" in shell is unusual (it's well-defined by POSIX that
> the backslash is preserved literally if the next character is not
> backquote, dollar, backslash, or newline), but for safety reason, I
> prefer to write it as "\\(\\)/\\1" to make it obvious that in the shell
> code I meant a literal backslash rather than relying on POSIX rules.
>
> Put all of these together, and this line should probably be:
>
> " sed -n \"/Bridge chain/,\\$ s/.*-j \\\\([%s]-.*\\\\)/\\\\1/p\")\n"
>
Thanks.
> Ah, so the end result is that you echo each name found, as well as
> recurse on each name found to echo any dependent names. Is order
> important (all names from the first chain must be output before any
> names from the recursive calls)? If not, then I can rewrite this
> function to avoid local sc in the first place. Here, in shell form
> (I'll leave you to convert it back into C string literal form):
>
> collect_chains()
> {
> for tmp in $(iptables -t nat -L $1 | \
> sed -n "/Bridge chain/,\$ s/.*-j \\([JP]-.*\\)/\\1/p"); do
> echo $tmp
> collect_chains $tmp
> done
> }
>
>
I took this 'as-is'. Thanks.
>> +
>> +static const char ebiptables_script_func_rm_chains[] =
>> + "rm_chains()\n"
> It looks like you are calling rm_chains() with a single argument
> containing a whitespace separated list of arguments.
> + virBufferAddLit(buf, "rm_chains \"$a\"\n");
>
> Why not just call rm_chains with multiple arguments?
> virBufferAddLit(buf, "rm_chains $a\n");
>
>> + "{\n"
>> + " for tmp in `echo \"$1\"`; do [ -n \"$tmp\" ]&& %s -t %s -F $tmp; done\n"
>> + " for tmp in `echo \"$1\"`; do [ -n \"$tmp\" ]&& %s -t %s -X $tmp; done\n"
>> + "}\n";
> Is it essential that all chains be flushed before any are deleted? If
If they are flushed first they cannot reference each other. So that
prevents not being able to delete one because it's still referenced by
another one and then also the order in which the names are being
processed doesn't matter as much.
> so, then keeping this as two for loops makes sense; if not, then these
> could be combined.
>
> Same story about `echo "$1"` being identical to the much simpler $1.
>
> Same story about $tmp always being non-empty in a shell for-loop where
> the tokens were created by word-splitting.
>
I had a lot of problems when adding support for dash to it. Probably the
IFS was wrong at some point and so I added lots of [ -n $tmp ].
> With one argument containing a quoted list, I'd write this as:
>
> rm_chains()
> {
> for tmp in $1; do %s -t %s -F $tmp; done
> for tmp in $1; do %s -t %s -X $tmp; done
> }
>
> with multiple arguments (unquoted $a), I'd write it as:
>
> rm_chains()
> {
> for tmp in $*; do %s -t %s -F $tmp; done
> for tmp in $*; do %s -t %s -X $tmp; done
> }
>
I took the 2nd version.
> <sidenote>
> A point that might be worth a separate cleanup patch: it looks awkward
> to have to write the literal name of the script and the table name in
> multiple places into the script:
> + virBufferAsprintf(buf, NWFILTER_FUNC_DELETE_CHAINS,
> + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE,
> + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE);
>
> generating:
>
> rm_chains()
> {
> for tmp in $*; do /path/to/iptables -t nat -f $tmp; done...
> ..
> rm_chains $a
>
>
> Why not change your shell script to set up a shell variable up front for
> ebtables_cmd_path and EBTABLES_DEFAULT_TABLE, and use that as a shell
> variable instead of as a printf substitution?
As you mentioned, let's defer this to another patch.
> I'd write this as:
> rename_chains()
> {
> for tmp in $1; do
> case $tmp in
> J*) /path/to/iptables -t nat -E $tmp I${tmp#?} ;;
> P*) /path/to/iptables -t nat -E $tmp O${tmp#?} ;;
> esac
> done
> }
>
> or back in your C-string format:
>
> "rename_chains()\n"
> "{\n"
> " for tmp in $1; do\n"
> " case $tmp in\n"
> " %c*) %s -t %s -E $tmp %c${tmp#?} ;;\n"
> " %c*) %s -t %s -E $tmp %c${tmp#?} ;;\n"
> " esac\n"
> " done\n"
> "}\n"
>
Took this 'as-is'.
>> +
>> +static const char ebiptables_script_set_ifs[] =
>> + "IFS=$' \\t\\n'\n"
>> + "[ `expr length \"$IFS\"` != 3 ]&& "
>> + "IFS=$(echo | %s '{ print \"\\t\\n \"}')\n";
> Oy - my head hurts! $'' is not portable; neither is expr length. And
sorry for this unwanted 'side-effect' :-)
> doing IFS=$() is just wrong - that eats the trailing newline, leaving
> you with just IFS=space-tab instead of the intended
> IFS=space-tab-newline. Just do:
>
> static const char ebiptables_script_set_ifs[] =
> "nl='\n"
> "'; IFS=' ''\t'$nl\n"
>
> and you're done. No need for shenanigans with command substitution,
> expr, or awk.
I took it as-is now. Thanks.
> [Side note - the only reason I write ' ''\t' rather than ' \t' is that I
> don't like seeing space-tab in generated scripts; some editors have a
> tendency to corrupt space-tab in the name of being "helpful", so
> separating the characters avoids that problem.]
>
>> +
>> +#define NWFILTER_FUNC_COLLECT_CHAINS ebtables_script_func_collect_chains
>> +#define NWFILTER_FUNC_DELETE_CHAINS ebiptables_script_func_rm_chains
> This particular change in names made it a bit harder to search the rest
> of your patch (I had to search two different strings to see where the
> function definition was emitted, vs. where it was called); I'd suggest
> either naming the function delete_chains, or the macro
> NWFILTER_FUNC_RM_CHAINS, for consistency.
Fixed.
>> +#define NWFILTER_FUNC_RENAME_CHAINS ebiptables_script_func_rename_chains
>> +#define NWFILTER_FUNC_SET_IFS ebiptables_script_set_ifs
>>
>> #define VIRT_IN_CHAIN "libvirt-in"
>> #define VIRT_OUT_CHAIN "libvirt-out"
>> @@ -2805,95 +2848,65 @@ ebtablesCreateTmpSubChain(virBufferPtr b
>> return 0;
>> }
> Phew - made it past the function definitions. Now, on to how they are
> used. [By the way, in case it's not obvious with my lengthy review
> above, I _like_ the fact that you are using shell functions to
> consolidate effort - as long as we are using a scripting language, we
> might as well use all of its power - and it does mean that any future
> attempt to refactor this away from a temporary shell script into libvirt
> primitives will have to keep that in mind]
>
>>
>> -
>> -static int
>> -_ebtablesRemoveSubChain(virBufferPtr buf,
>> - int incoming,
>> - const char *ifname,
>> - enum l3_proto_idx protoidx,
>> - int isTempChain)
>> +static int _ebtablesRemoveSubChains(virBufferPtr buf,
>> + const char *ifname,
>> + const char *chains)
>> {
>> - char rootchain[MAX_CHAINNAME_LENGTH], chain[MAX_CHAINNAME_LENGTH];
>> - char chainPrefix;
>> -
>> - if (isTempChain) {
>> - chainPrefix =(incoming) ? CHAINPREFIX_HOST_IN_TEMP
>> - : CHAINPREFIX_HOST_OUT_TEMP;
>> - } else {
>> - chainPrefix =(incoming) ? CHAINPREFIX_HOST_IN
>> - : CHAINPREFIX_HOST_OUT;
>> - }
>> + char rootchain[MAX_CHAINNAME_LENGTH];
>> + unsigned i;
>>
>> - PRINT_ROOT_CHAIN(rootchain, chainPrefix, ifname);
>> - PRINT_CHAIN(chain, chainPrefix, ifname, l3_protocols[protoidx].val);
>> -
>> - virBufferAsprintf(buf,
>> - "%s -t %s -D %s -p 0x%x -j %s" CMD_SEPARATOR
>> - "%s -t %s -F %s" CMD_SEPARATOR
>> - "%s -t %s -X %s" CMD_SEPARATOR,
>> + virBufferAsprintf(buf, NWFILTER_FUNC_COLLECT_CHAINS,
>> + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, chains);
>> + virBufferAsprintf(buf, NWFILTER_FUNC_DELETE_CHAINS,
>> ebtables_cmd_path, EBTABLES_DEFAULT_TABLE,
>> - rootchain, l3_protocols[protoidx].attr, chain,
>> + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE);
>>
>> - ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, chain,
>> + virBufferAsprintf(buf, NWFILTER_FUNC_SET_IFS, gawk_cmd_path);
> Given my above reformulation of SET_IFS, you don't need gawk_cmd_path here.
>
Fixed.
>> + virBufferAddLit(buf, "a=\"");
>> + for (i = 0; chains[i] != 0; i++) {
>> + PRINT_ROOT_CHAIN(rootchain, chains[i], ifname);
>> + virBufferAsprintf(buf, "$(collect_chains %s) ", rootchain);
>> + }
>> + virBufferAddLit(buf, "\"\n");
> This results in the shell code:
>
> a="$(collect_chains name) $(collect_chains name) "
>
> If you added looping in the shell definition of collect_chains, this
> could instead be:
>
> a=$(collect_chains name name)
>
> Not essential, but food for thought for a smaller script.
Did that now.
>>
>> - ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, chain);
>> + for (i = 0; chains[i] != 0; i++) {
>> + PRINT_ROOT_CHAIN(rootchain, chains[i], ifname);
>> + virBufferAsprintf(buf,
>> + "%s -t %s -F %s\n",
>> + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE,
>> + rootchain);
>> + }
>> + virBufferAddLit(buf, "rm_chains \"$a\"\n");
> This results in:
>
> rm_chains "$a"
>
> See my earlier comments where using $* instead of $1 would let you write
> this as:
>
> rm_chains $a
>
> instead.
>
Using the 'rm_chains $a' now.
>> +static int
>> +ebtablesRenameTmpSubAndRootChains(virBufferPtr buf,
>> + const char *ifname)
>> +{
>> + char rootchain[MAX_CHAINNAME_LENGTH];
>> + unsigned i;
>> + char chains[3] = {
>> + CHAINPREFIX_HOST_IN_TEMP,
>> + CHAINPREFIX_HOST_OUT_TEMP,
>> + 0};
>> +
>> + virBufferAsprintf(buf, NWFILTER_FUNC_COLLECT_CHAINS,
>> + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, chains);
> This means you are outputting the function definition from two different
> call paths. It doesn't hurt to (re-)define a function to the same
> value, while avoiding the function if it isn't used; on the other hand,
> I'm wondering if it is worth outputting the functions as part of the
> file header rather than risking having the function definitions appear
> multiple times throughout the temporary script. I can live with your
> current approach, so don't bother refactoring things unless you want to.
Then I don't refactor this one.
> [I speak from experience - I'm one of the autoconf maintainers, and
> solving the problem of how to generate shell script output based on m4
> input, where pre-requisite shell functions are emitted at most once, up
> front, but used multiple times through the rest of the script, all in
> order to reduce the total size of the overall script, proved to be an
> interesting challenge in writing a code generator.]
>
>> + virBufferAsprintf(buf, NWFILTER_FUNC_RENAME_CHAINS,
>> + CHAINPREFIX_HOST_IN_TEMP,
>> + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE,
>> + CHAINPREFIX_HOST_IN,
>> + CHAINPREFIX_HOST_OUT_TEMP,
>> + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE,
>> + CHAINPREFIX_HOST_OUT);
>> +
>> + virBufferAsprintf(buf, NWFILTER_FUNC_SET_IFS, gawk_cmd_path);
>> + virBufferAddLit(buf, "a=\"");
>> + for (i = 0; chains[i] != 0; i++) {
>> + PRINT_ROOT_CHAIN(rootchain, chains[i], ifname);
>> + virBufferAsprintf(buf, "$(collect_chains %s) ", rootchain);
>> }
> Similar comments about collect_chains usage possibly being easier to
> understand if it includes a shell for loop over multiple arguments.
>
Converted.
> Looking forward to seeing what v5 looks like!
>
I did all conversions and libvirt-tck test cases pass without problems
or leaving any tables behind upon VM teardown.
Thanks a lot for your review and help. I'll send out v5 shortly.
Stefan
Use the name of the chain rather than its type index (enum). This pushes the later enablement of chains with user-given names into the XML parser. For now we still only allow those names that are well known ('root', 'arp', 'rarp', 'ipv4' and 'ipv6'). Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/conf/nwfilter_conf.c | 16 ++++++++++++---- src/conf/nwfilter_conf.h | 2 +- src/nwfilter/nwfilter_ebiptables_driver.c | 13 +++++++------ src/nwfilter/nwfilter_ebiptables_driver.h | 2 +- 4 files changed, 21 insertions(+), 12 deletions(-) Index: libvirt-acl/src/conf/nwfilter_conf.c =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_conf.c +++ libvirt-acl/src/conf/nwfilter_conf.c @@ -309,6 +309,7 @@ virNWFilterDefFree(virNWFilterDefPtr def virNWFilterEntryFree(def->filterEntries[i]); VIR_FREE(def->filterEntries); + VIR_FREE(def->chainsuffix); VIR_FREE(def); } @@ -2029,21 +2030,28 @@ virNWFilterDefParseXML(xmlXPathContextPt goto cleanup; } - ret->chainsuffix = VIR_NWFILTER_CHAINSUFFIX_ROOT; chain = virXPathString("string(./@chain)", ctxt); if (chain) { - if ((ret->chainsuffix = - virNWFilterChainSuffixTypeFromString(chain)) < 0) { + if (virNWFilterChainSuffixTypeFromString(chain) < 0) { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, _("unknown chain suffix '%s'"), chain); goto cleanup; } + ret->chainsuffix = chain; /* assign an implicit priority -- support XML attribute later */ if (intMapGetByString(chain_priorities, chain, 0, &ret->chainPriority) == false) { ret->chainPriority = (NWFILTER_MAX_FILTER_PRIORITY + NWFILTER_MIN_FILTER_PRIORITY) / 2; } + chain = NULL; + } else { + ret->chainsuffix = strdup(virNWFilterChainSuffixTypeToString( + VIR_NWFILTER_CHAINSUFFIX_ROOT)); + if (ret->chainsuffix == NULL) { + virReportOOMError(); + goto cleanup; + } } uuid = virXPathString("string(./uuid)", ctxt); @@ -2909,7 +2917,7 @@ virNWFilterDefFormat(virNWFilterDefPtr d virBufferAsprintf(&buf, "<filter name='%s' chain='%s'", def->name, - virNWFilterChainSuffixTypeToString(def->chainsuffix)); + def->chainsuffix); virBufferAddLit(&buf, ">\n"); virUUIDFormat(def->uuid, uuid); Index: libvirt-acl/src/conf/nwfilter_conf.h =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_conf.h +++ libvirt-acl/src/conf/nwfilter_conf.h @@ -455,7 +455,7 @@ struct _virNWFilterDef { char *name; unsigned char uuid[VIR_UUID_BUFLEN]; - int chainsuffix; /*enum virNWFilterChainSuffixType */ + char *chainsuffix; virNWFilterChainPriority chainPriority; int nentries; Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c @@ -369,7 +369,7 @@ ebiptablesRuleInstFree(ebiptablesRuleIns static int ebiptablesAddRuleInst(virNWFilterRuleInstPtr res, char *commandTemplate, - enum virNWFilterChainSuffixType neededChain, + const char *neededChain, virNWFilterChainPriority chainPriority, char chainprefix, unsigned int priority, @@ -1945,11 +1945,13 @@ ebtablesCreateRuleInstance(char chainPre goto err_exit; } - if (nwfilter->chainsuffix == VIR_NWFILTER_CHAINSUFFIX_ROOT) + if (STREQ(nwfilter->chainsuffix, + virNWFilterChainSuffixTypeToString( + VIR_NWFILTER_CHAINSUFFIX_ROOT))) PRINT_ROOT_CHAIN(chain, chainPrefix, ifname); else PRINT_CHAIN(chain, chainPrefix, ifname, - virNWFilterChainSuffixTypeToString(nwfilter->chainsuffix)); + nwfilter->chainsuffix); switch (rule->prtclType) { @@ -2516,7 +2518,7 @@ ebiptablesDisplayRuleInstance(virConnect ebiptablesRuleInstPtr inst = (ebiptablesRuleInstPtr)_inst; VIR_INFO("Command Template: '%s', Needed protocol: '%s'", inst->commandTemplate, - virNWFilterChainSuffixTypeToString(inst->neededProtocolChain)); + inst->neededProtocolChain); return 0; } @@ -3447,8 +3449,7 @@ ebiptablesApplyNewRules(virConnectPtr co for (i = 0; i < nruleInstances; i++) { sa_assert (inst); if (inst[i]->ruleType == RT_EBTABLES) { - const char *name = virNWFilterChainSuffixTypeToString( - inst[i]->neededProtocolChain); + const char *name = inst[i]->neededProtocolChain; if (inst[i]->chainprefix == CHAINPREFIX_HOST_IN_TEMP) { if (virHashUpdateEntry(chains_in_set, name, &inst[i]->chainPriority)) { Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.h =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.h +++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.h @@ -35,7 +35,7 @@ typedef struct _ebiptablesRuleInst ebipt typedef ebiptablesRuleInst *ebiptablesRuleInstPtr; struct _ebiptablesRuleInst { char *commandTemplate; - enum virNWFilterChainSuffixType neededProtocolChain; + const char *neededProtocolChain; virNWFilterChainPriority chainPriority; char chainprefix; /* I for incoming, O for outgoing */ unsigned int priority;
This patch extends the filter XML to support priorities of chains in the XML. An example would be: <filter name='allow-arpxyz' chain='arp-xyz' priority='200'> [...] </filter> The permitted values for priorities are [-1000, 1000]. By setting the pririty of a chain the order in which it is accessed from the interface root chain can be influenced. Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- docs/schemas/nwfilter.rng | 7 ++++++- src/conf/nwfilter_conf.c | 42 +++++++++++++++++++++++++++++++++++++----- 2 files changed, 43 insertions(+), 6 deletions(-) Index: libvirt-acl/src/conf/nwfilter_conf.c =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_conf.c +++ libvirt-acl/src/conf/nwfilter_conf.c @@ -2016,7 +2016,9 @@ virNWFilterDefParseXML(xmlXPathContextPt xmlNodePtr curr = ctxt->node; char *uuid = NULL; char *chain = NULL; + char *chain_pri_s = NULL; virNWFilterEntryPtr entry; + int chain_priority; if (VIR_ALLOC(ret) < 0) { virReportOOMError(); @@ -2030,6 +2032,26 @@ virNWFilterDefParseXML(xmlXPathContextPt goto cleanup; } + chain_pri_s = virXPathString("string(./@priority)", ctxt); + if (chain_pri_s) { + if (sscanf(chain_pri_s, "%d", &chain_priority) != 1) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not parse chain priority '%s'"), + chain_pri_s); + goto cleanup; + } + if (chain_priority < NWFILTER_MIN_FILTER_PRIORITY || + chain_priority > NWFILTER_MAX_FILTER_PRIORITY) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("Priority '%d' is outside valid " + "range of [%d,%d]"), + chain_priority, + NWFILTER_MIN_FILTER_PRIORITY, + NWFILTER_MAX_FILTER_PRIORITY); + goto cleanup; + } + } + chain = virXPathString("string(./@chain)", ctxt); if (chain) { if (virNWFilterChainSuffixTypeFromString(chain) < 0) { @@ -2038,11 +2060,16 @@ virNWFilterDefParseXML(xmlXPathContextPt goto cleanup; } ret->chainsuffix = chain; - /* assign an implicit priority -- support XML attribute later */ - if (intMapGetByString(chain_priorities, chain, 0, - &ret->chainPriority) == false) { - ret->chainPriority = (NWFILTER_MAX_FILTER_PRIORITY + - NWFILTER_MIN_FILTER_PRIORITY) / 2; + + if (chain_pri_s) { + ret->chainPriority = chain_priority; + } else { + /* assign an implicit priority -- support XML attribute later */ + if (intMapGetByString(chain_priorities, chain, 0, + &ret->chainPriority) == false) { + ret->chainPriority = (NWFILTER_MAX_FILTER_PRIORITY + + NWFILTER_MIN_FILTER_PRIORITY) / 2; + } } chain = NULL; } else { @@ -2099,6 +2126,7 @@ virNWFilterDefParseXML(xmlXPathContextPt } VIR_FREE(chain); + VIR_FREE(chain_pri_s); return ret; @@ -2106,6 +2134,7 @@ virNWFilterDefParseXML(xmlXPathContextPt virNWFilterDefFree(ret); VIR_FREE(chain); VIR_FREE(uuid); + VIR_FREE(chain_pri_s); return NULL; } @@ -2918,6 +2947,9 @@ virNWFilterDefFormat(virNWFilterDefPtr d virBufferAsprintf(&buf, "<filter name='%s' chain='%s'", def->name, def->chainsuffix); + if (def->chainPriority != 0) + virBufferAsprintf(&buf, " priority='%d'", + def->chainPriority); virBufferAddLit(&buf, ">\n"); virUUIDFormat(def->uuid, uuid); Index: libvirt-acl/docs/schemas/nwfilter.rng =================================================================== --- libvirt-acl.orig/docs/schemas/nwfilter.rng +++ libvirt-acl/docs/schemas/nwfilter.rng @@ -293,6 +293,11 @@ </choice> </attribute> </optional> + <optional> + <attribute name="priority"> + <ref name='priority-type'/> + </attribute> + </optional> </define> <define name="filterref-node-attributes"> @@ -881,7 +886,7 @@ <define name='priority-type'> <data type="int"> - <param name="minInclusive">0</param> + <param name="minInclusive">-1000</param> <param name="maxInclusive">1000</param> </data> </define>
This patch enables chains that have a known prefix in their name. Known prefixes are: 'ipv4', 'ipv6', 'arp', 'rarp'. All prefixes are also protocols that can be evaluated on the ebtables level. Following the prefix they will be automatically connected to an interface's 'root' chain and jumped into following the protocol they evalute, i.e., a table 'arp-xyz' will be accessed from the root table using ebtables -t nat -A <iface root table> -p arp -j I-<ifname>-arp-xyz thus generating a 'root' chain like this one here: Bridge chain: libvirt-O-vnet0, entries: 5, policy: ACCEPT -p IPv4 -j O-vnet0-ipv4 -p ARP -j O-vnet0-arp -p 0x8035 -j O-vnet0-rarp -p ARP -j O-vnet0-arp-xyz -j DROP where the chain 'arp-xyz' is accessed for filtering of ARP packets. v3: - assign a priority to filters that have an allowed prefix, e.g., assign the arp chain priority to a filter arp-xyz unless user provided a priority in the XML Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- docs/schemas/nwfilter.rng | 16 ++++++-- src/conf/nwfilter_conf.c | 87 ++++++++++++++++++++++++++++++++++++++++++---- src/conf/nwfilter_conf.h | 3 + 3 files changed, 96 insertions(+), 10 deletions(-) Index: libvirt-acl/src/conf/nwfilter_conf.c =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_conf.c +++ libvirt-acl/src/conf/nwfilter_conf.c @@ -2009,6 +2009,80 @@ err_exit: goto cleanup; } +static bool +virNWFilterIsValidChainName(const char *chainname) +{ + return chainname[strspn(chainname, VALID_CHAINNAME)] == 0; +} + +/* + * Test whether the name of the chain is supported. + * It current has to have a prefix of either one of the strings found in + * virNWFilterChainSuffixTypeToString(). + */ +static const char * +virNWFilterIsAllowedChain(const char *chainname) +{ + enum virNWFilterChainSuffixType i; + const char *name, *msg; + virBuffer buf = VIR_BUFFER_INITIALIZER; + bool printed = false; + + if (!virNWFilterIsValidChainName(chainname)) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("Chain name contains illegal characters")); + return NULL; + } + + if (strlen(chainname) > MAX_CHAIN_SUFFIX_SIZE) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("name of chain is longer than %u characters"), + MAX_CHAIN_SUFFIX_SIZE); + return NULL; + } + + for (i = 0; i < VIR_NWFILTER_CHAINSUFFIX_LAST; i++) { + name = virNWFilterChainSuffixTypeToString(i); + if (i == VIR_NWFILTER_CHAINSUFFIX_ROOT) { + /* allow 'root' as a complete name but not as a prefix */ + if (STREQ(chainname, name)) + return name; + if (STRPREFIX(chainname, name)) + return NULL; + } + if (STRPREFIX(chainname, name)) + return name; + } + + virBufferAsprintf(&buf, + _("Illegal chain name '%s'. Please use a chain name " + "called '%s' or either one of the following prefixes: "), + virNWFilterChainSuffixTypeToString( + VIR_NWFILTER_CHAINSUFFIX_ROOT), + chainname); + for (i = 0; i < VIR_NWFILTER_CHAINSUFFIX_LAST; i++) { + if (i == VIR_NWFILTER_CHAINSUFFIX_ROOT) + continue; + if (printed) + virBufferAddLit(&buf, ", "); + virBufferAdd(&buf, virNWFilterChainSuffixTypeToString(i), -1); + printed = true; + } + + if (virBufferError(&buf)) { + virReportOOMError(); + virBufferFreeAndReset(&buf); + goto err_exit; + } + + msg = virBufferContentAndReset(&buf); + + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, _("%s"), msg); + VIR_FREE(msg); + +err_exit: + return NULL; +} static virNWFilterDefPtr virNWFilterDefParseXML(xmlXPathContextPtr ctxt) { @@ -2019,6 +2093,7 @@ virNWFilterDefParseXML(xmlXPathContextPt char *chain_pri_s = NULL; virNWFilterEntryPtr entry; int chain_priority; + const char *name_prefix; if (VIR_ALLOC(ret) < 0) { virReportOOMError(); @@ -2054,19 +2129,19 @@ virNWFilterDefParseXML(xmlXPathContextPt chain = virXPathString("string(./@chain)", ctxt); if (chain) { - if (virNWFilterChainSuffixTypeFromString(chain) < 0) { - virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown chain suffix '%s'"), chain); + name_prefix = virNWFilterIsAllowedChain(chain); + if (name_prefix == NULL) goto cleanup; - } ret->chainsuffix = chain; if (chain_pri_s) { ret->chainPriority = chain_priority; } else { - /* assign an implicit priority -- support XML attribute later */ - if (intMapGetByString(chain_priorities, chain, 0, + /* assign an implicit priority */ + if (!name_prefix || + intMapGetByString(chain_priorities, name_prefix, 0, &ret->chainPriority) == false) { + /* assign default chain priority */ ret->chainPriority = (NWFILTER_MAX_FILTER_PRIORITY + NWFILTER_MIN_FILTER_PRIORITY) / 2; } Index: libvirt-acl/src/conf/nwfilter_conf.h =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_conf.h +++ libvirt-acl/src/conf/nwfilter_conf.h @@ -446,6 +446,9 @@ enum virNWFilterChainSuffixType { VIR_NWFILTER_CHAINSUFFIX_LAST, }; +# define VALID_CHAINNAME \ + "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_.:-" + typedef int32_t virNWFilterChainPriority; typedef struct _virNWFilterDef virNWFilterDef; Index: libvirt-acl/docs/schemas/nwfilter.rng =================================================================== --- libvirt-acl.orig/docs/schemas/nwfilter.rng +++ libvirt-acl/docs/schemas/nwfilter.rng @@ -286,10 +286,18 @@ <attribute name="chain"> <choice> <value>root</value> - <value>arp</value> - <value>rarp</value> - <value>ipv4</value> - <value>ipv6</value> + <data type="string"> + <param name="pattern">arp[a-zA-Z0-9_\.:-]*</param> + </data> + <data type="string"> + <param name="pattern">rarp[a-zA-Z0-9_\.:-]*</param> + </data> + <data type="string"> + <param name="pattern">ipv4[a-zA-Z0-9_\.:-]*</param> + </data> + <data type="string"> + <param name="pattern">ipv6[a-zA-Z0-9_\.:-]*</param> + </data> </choice> </attribute> </optional>
So far rules' priorities have only been valid in the range [0,1000]. Now I am extending their priority into the range [-1000, 1000] for subsequently being able to sort rules and the access of (jumps into) chains following priorities. Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/conf/nwfilter_conf.c | 7 ++++--- src/conf/nwfilter_conf.h | 6 ++++-- src/nwfilter/nwfilter_ebiptables_driver.c | 2 +- src/nwfilter/nwfilter_ebiptables_driver.h | 2 +- 4 files changed, 10 insertions(+), 7 deletions(-) Index: libvirt-acl/src/conf/nwfilter_conf.c =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_conf.c +++ libvirt-acl/src/conf/nwfilter_conf.c @@ -1899,7 +1899,7 @@ virNWFilterRuleParse(xmlNodePtr node) char *statematch; int found; int found_i = 0; - unsigned int priority; + int priority; xmlNodePtr cur; virNWFilterRuleDefPtr ret; @@ -1945,8 +1945,9 @@ virNWFilterRuleParse(xmlNodePtr node) ret->priority = MAX_RULE_PRIORITY / 2; if (prio) { - if (virStrToLong_ui(prio, NULL, 10, &priority) >= 0) { - if (priority <= MAX_RULE_PRIORITY) + if (virStrToLong_i(prio, NULL, 10, &priority) >= 0) { + if (priority <= MAX_RULE_PRIORITY && + priority >= MIN_RULE_PRIORITY) ret->priority = priority; } } Index: libvirt-acl/src/conf/nwfilter_conf.h =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_conf.h +++ libvirt-acl/src/conf/nwfilter_conf.h @@ -357,7 +357,7 @@ enum virNWFilterEbtablesTableType { }; -# define MIN_RULE_PRIORITY 0 +# define MIN_RULE_PRIORITY -1000 # define MAX_RULE_PRIORITY 1000 # define NWFILTER_MIN_FILTER_PRIORITY -1000 @@ -389,10 +389,12 @@ enum virNWFilterRuleFlags { void virNWFilterPrintStateMatchFlags(virBufferPtr buf, const char *prefix, int32_t flags, bool disp_none); +typedef int32_t virNWFilterRulePriority; + typedef struct _virNWFilterRuleDef virNWFilterRuleDef; typedef virNWFilterRuleDef *virNWFilterRuleDefPtr; struct _virNWFilterRuleDef { - unsigned int priority; + virNWFilterRulePriority priority; enum virNWFilterRuleFlags flags; int action; /*enum virNWFilterRuleActionType*/ int tt; /*enum virNWFilterRuleDirectionType*/ Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.h =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.h +++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.h @@ -38,7 +38,7 @@ struct _ebiptablesRuleInst { const char *neededProtocolChain; virNWFilterChainPriority chainPriority; char chainprefix; /* I for incoming, O for outgoing */ - unsigned int priority; + virNWFilterRulePriority priority; enum RuleType ruleType; }; Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c @@ -372,7 +372,7 @@ ebiptablesAddRuleInst(virNWFilterRuleIns const char *neededChain, virNWFilterChainPriority chainPriority, char chainprefix, - unsigned int priority, + virNWFilterRulePriority priority, enum RuleType ruleType) { ebiptablesRuleInstPtr inst;
The previous patch extends the priority of filtering rules into negative numbers. We now use this possibility to interleave the jumping into chains with filtering rules to for example create the 'root' table of an interface with the following sequence of rules: Bridge chain: libvirt-I-vnet0, entries: 6, policy: ACCEPT -p IPv4 -j I-vnet0-ipv4 -p ARP -j I-vnet0-arp -p ARP -j ACCEPT -p 0x8035 -j I-vnet0-rarp -p 0x835 -j ACCEPT -j DROP The '-p ARP -j ACCEPT' rule now appears between the jumps. Since the 'arp' chain has been assigned priority -700 and the 'rarp' chain -600, the above ordering can now be achieved with the following rule: <rule action='accept' direction='out' priority='-650'> <mac protocolid='arp'/> </rule> This patch now sorts the commands generating the above shown jumps into chains and interleaves their execution with those for generating rules. v3: - fix memory leak Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/nwfilter/nwfilter_ebiptables_driver.c | 120 ++++++++++++++++++++++++++---- 1 file changed, 107 insertions(+), 13 deletions(-) Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c @@ -2815,13 +2815,17 @@ ebtablesUnlinkTmpRootChain(virBufferPtr static int -ebtablesCreateTmpSubChain(virBufferPtr buf, +ebtablesCreateTmpSubChain(ebiptablesRuleInstPtr *inst, + int *nRuleInstances, int incoming, const char *ifname, enum l3_proto_idx protoidx, const char *filtername, - int stopOnError) + int stopOnError, + virNWFilterChainPriority priority) { + virBuffer buf = VIR_BUFFER_INITIALIZER; + ebiptablesRuleInstPtr tmp = *inst; char rootchain[MAX_CHAINNAME_LENGTH], chain[MAX_CHAINNAME_LENGTH]; char chainPrefix = (incoming) ? CHAINPREFIX_HOST_IN_TEMP : CHAINPREFIX_HOST_OUT_TEMP; @@ -2830,15 +2834,22 @@ ebtablesCreateTmpSubChain(virBufferPtr b PRINT_CHAIN(chain, chainPrefix, ifname, (filtername) ? filtername : l3_protocols[protoidx].val); - virBufferAsprintf(buf, + virBufferAsprintf(&buf, + CMD_DEF("%s -t %s -F %s") CMD_SEPARATOR + CMD_EXEC + CMD_DEF("%s -t %s -X %s") CMD_SEPARATOR + CMD_EXEC CMD_DEF("%s -t %s -N %s") CMD_SEPARATOR CMD_EXEC "%s" - CMD_DEF("%s -t %s -A %s -p 0x%x -j %s") CMD_SEPARATOR + CMD_DEF("%s -t %s -%%c %s %%s -p 0x%x -j %s") + CMD_SEPARATOR CMD_EXEC "%s", ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, chain, + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, chain, + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, chain, CMD_STOPONERR(stopOnError), @@ -2847,6 +2858,24 @@ ebtablesCreateTmpSubChain(virBufferPtr b CMD_STOPONERR(stopOnError)); + if (virBufferError(&buf) || + VIR_REALLOC_N(tmp, (*nRuleInstances)+1) < 0) { + virReportOOMError(); + virBufferFreeAndReset(&buf); + return 1; + } + + *inst = tmp; + + memset(&tmp[*nRuleInstances], 0, sizeof(tmp[0])); + tmp[*nRuleInstances].priority = priority; + tmp[*nRuleInstances].commandTemplate = + virBufferContentAndReset(&buf); + tmp[*nRuleInstances].neededProtocolChain = + virNWFilterChainSuffixTypeToString(VIR_NWFILTER_CHAINSUFFIX_ROOT); + + (*nRuleInstances)++; + return 0; } @@ -3320,9 +3349,34 @@ static int ebtablesCleanAll(const char * static int ebiptablesRuleOrderSort(const void *a, const void *b) { + const ebiptablesRuleInstPtr insta = (const ebiptablesRuleInstPtr)a; + const ebiptablesRuleInstPtr instb = (const ebiptablesRuleInstPtr)b; + const char *root = virNWFilterChainSuffixTypeToString( + VIR_NWFILTER_CHAINSUFFIX_ROOT); + bool root_a = STREQ(insta->neededProtocolChain, root); + bool root_b = STREQ(instb->neededProtocolChain, root); + + /* ensure root chain commands appear before all others since + we will need them to create the child chains */ + if (root_a) { + if (root_b) { + goto normal; + } + return -1; /* a before b */ + } + if (root_b) { + return 1; /* b before a */ + } +normal: + return (insta->priority - instb->priority); +} + +static int +ebiptablesRuleOrderSortPtr(const void *a, const void *b) +{ const ebiptablesRuleInstPtr *insta = a; const ebiptablesRuleInstPtr *instb = b; - return ((*insta)->priority - (*instb)->priority); + return ebiptablesRuleOrderSort(*insta, *instb); } static int @@ -3392,9 +3446,12 @@ ebtablesGetProtoIdxByFiltername(const ch static int ebtablesCreateTmpRootAndSubChains(virBufferPtr buf, const char *ifname, - virHashTablePtr chains, int direction) + virHashTablePtr chains, int direction, + ebiptablesRuleInstPtr *inst, + int *nRuleInstances) { int rc = 0, i; + virNWFilterChainPriority *priority; if (virHashSize(chains) != 0) { char **filter_names; @@ -3412,13 +3469,19 @@ ebtablesCreateTmpRootAndSubChains(virBuf filter_names[i]); if ((int)idx < 0) continue; - ebtablesCreateTmpSubChain(buf, direction, ifname, idx, - filter_names[i], 1); + priority = virHashLookup(chains, filter_names[i]); + if (ebtablesCreateTmpSubChain(inst, nRuleInstances, + direction, ifname, idx, + filter_names[i], 1, + *priority)) { + goto exit_free_keys; + } } +exit_free_keys: virHashFreeKeys(chains, (void **)filter_names); } - err_exit: +err_exit: return rc; } @@ -3428,7 +3491,7 @@ ebiptablesApplyNewRules(virConnectPtr co int nruleInstances, void **_inst) { - int i; + int i, j; int cli_status; ebiptablesRuleInstPtr *inst = (ebiptablesRuleInstPtr *)_inst; virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -3436,6 +3499,8 @@ ebiptablesApplyNewRules(virConnectPtr co chains_out_set = virHashCreate(10, NULL); bool haveIptables = false; bool haveIp6tables = false; + ebiptablesRuleInstPtr ebtChains = NULL; + int nEbtChains = 0; if (!chains_in_set || !chains_out_set) { virReportOOMError(); @@ -3443,7 +3508,8 @@ ebiptablesApplyNewRules(virConnectPtr co } if (nruleInstances > 1 && inst) - qsort(inst, nruleInstances, sizeof(inst[0]), ebiptablesRuleOrderSort); + qsort(inst, nruleInstances, sizeof(inst[0]), + ebiptablesRuleOrderSortPtr); /* scan the rules to see which chains need to be created */ for (i = 0; i < nruleInstances; i++) { @@ -3477,18 +3543,33 @@ ebiptablesApplyNewRules(virConnectPtr co } /* create needed chains */ - if (ebtablesCreateTmpRootAndSubChains(&buf, ifname, chains_in_set , 1) || - ebtablesCreateTmpRootAndSubChains(&buf, ifname, chains_out_set, 0)) { + if (ebtablesCreateTmpRootAndSubChains(&buf, ifname, chains_in_set , 1, + &ebtChains, &nEbtChains) || + ebtablesCreateTmpRootAndSubChains(&buf, ifname, chains_out_set, 0, + &ebtChains, &nEbtChains)) { goto tear_down_tmpebchains; } + if (nEbtChains > 0) + qsort(&ebtChains[0], nEbtChains, sizeof(ebtChains[0]), + ebiptablesRuleOrderSort); + if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0) goto tear_down_tmpebchains; + /* process ebtables commands; interleave commands from filters with + commands for creating and connecting ebtables chains */ + j = 0; for (i = 0; i < nruleInstances; i++) { sa_assert (inst); switch (inst[i]->ruleType) { case RT_EBTABLES: + while (j < nEbtChains && + ebtChains[j].priority <= inst[i]->priority) { + ebiptablesInstCommand(&buf, + ebtChains[j++].commandTemplate, + 'A', -1, 1); + } ebiptablesInstCommand(&buf, inst[i]->commandTemplate, 'A', -1, 1); @@ -3502,6 +3583,11 @@ ebiptablesApplyNewRules(virConnectPtr co } } + while (j < nEbtChains) + ebiptablesInstCommand(&buf, + ebtChains[j++].commandTemplate, + 'A', -1, 1); + if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0) goto tear_down_tmpebchains; @@ -3581,6 +3667,10 @@ ebiptablesApplyNewRules(virConnectPtr co virHashFree(chains_in_set); virHashFree(chains_out_set); + for (i = 0; i < nEbtChains; i++) + VIR_FREE(ebtChains[i].commandTemplate); + VIR_FREE(ebtChains); + return 0; tear_down_ebsubchains_and_unlink: @@ -3619,6 +3709,10 @@ exit_free_sets: virHashFree(chains_in_set); virHashFree(chains_out_set); + for (i = 0; i < nEbtChains; i++) + VIR_FREE(ebtChains[i].commandTemplate); + VIR_FREE(ebtChains); + return 1; }
Add test case for the chain names with known prefixes and the chain priority. Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- tests/nwfilterxml2xmlin/chain_prefixtest1.xml | 37 +++++++++++++++++++++++++ tests/nwfilterxml2xmlout/chain_prefixtest1.xml | 21 ++++++++++++++ tests/nwfilterxml2xmltest.c | 2 + 3 files changed, 60 insertions(+) Index: libvirt-acl/tests/nwfilterxml2xmlin/chain_prefixtest1.xml =================================================================== --- /dev/null +++ libvirt-acl/tests/nwfilterxml2xmlin/chain_prefixtest1.xml @@ -0,0 +1,37 @@ +<filter name='testcase' chain='arp-testme' priority='-650'> + <uuid>e5700920-a333-4c05-8016-b669e46b7599</uuid> + <rule action='accept' direction='out'> + <arp srcmacaddr='1:2:3:4:5:6' srcmacmask='ff:ff:ff:ff:ff:ff' + protocolid='arp' + dstmacaddr='aa:bb:cc:dd:ee:ff' dstmacmask='ff:ff:ff:ff:ff:ff' + hwtype='12' + protocoltype='34' + opcode='Request' + arpsrcmacaddr='1:2:3:4:5:6' + arpdstmacaddr='a:b:c:d:e:f'/> + </rule> + + <rule action='accept' direction='out'> + <arp srcmacaddr='1:2:3:4:5:6' srcmacmask='ff:ff:ff:ff:ff:ff' + opcode='1' hwtype='255' protocoltype='255'/> + </rule> + + <rule action='accept' direction='out'> + <arp srcmacaddr='1:2:3:4:5:6' srcmacmask='ff:ff:ff:ff:ff:ff' + opcode='11' hwtype='256' protocoltype='256'/> + </rule> + + <rule action='accept' direction='out'> + <arp srcmacaddr='1:2:3:4:5:6' srcmacmask='ff:ff:ff:ff:ff:ff' + opcode='65535' hwtype='65535' protocoltype='65535' /> + </rule> + + <rule action='accept' direction='out'> + <arp srcmacaddr='1:2:3:4:5:6' srcmacmask='ff:ff:ff:ff:ff:ff' + opcode='65536' hwtype='65536' protocoltype='65536' /> + </rule> + + <rule action='accept' direction='in'> + <arp gratuitous='true'/> + </rule> +</filter> Index: libvirt-acl/tests/nwfilterxml2xmlout/chain_prefixtest1.xml =================================================================== --- /dev/null +++ libvirt-acl/tests/nwfilterxml2xmlout/chain_prefixtest1.xml @@ -0,0 +1,21 @@ +<filter name='testcase' chain='arp-testme' priority='-650'> + <uuid>e5700920-a333-4c05-8016-b669e46b7599</uuid> + <rule action='accept' direction='out' priority='500'> + <arp srcmacaddr='01:02:03:04:05:06' srcmacmask='ff:ff:ff:ff:ff:ff' dstmacaddr='aa:bb:cc:dd:ee:ff' dstmacmask='ff:ff:ff:ff:ff:ff' hwtype='12' protocoltype='34' opcode='Request' arpsrcmacaddr='01:02:03:04:05:06' arpdstmacaddr='0a:0b:0c:0d:0e:0f'/> + </rule> + <rule action='accept' direction='out' priority='500'> + <arp srcmacaddr='01:02:03:04:05:06' srcmacmask='ff:ff:ff:ff:ff:ff' hwtype='255' protocoltype='255' opcode='Request'/> + </rule> + <rule action='accept' direction='out' priority='500'> + <arp srcmacaddr='01:02:03:04:05:06' srcmacmask='ff:ff:ff:ff:ff:ff' hwtype='256' protocoltype='256' opcode='11'/> + </rule> + <rule action='accept' direction='out' priority='500'> + <arp srcmacaddr='01:02:03:04:05:06' srcmacmask='ff:ff:ff:ff:ff:ff' hwtype='65535' protocoltype='65535' opcode='65535'/> + </rule> + <rule action='accept' direction='out' priority='500'> + <arp srcmacaddr='01:02:03:04:05:06' srcmacmask='ff:ff:ff:ff:ff:ff'/> + </rule> + <rule action='accept' direction='in' priority='500'> + <arp gratuitous='true'/> + </rule> +</filter> Index: libvirt-acl/tests/nwfilterxml2xmltest.c =================================================================== --- libvirt-acl.orig/tests/nwfilterxml2xmltest.c +++ libvirt-acl/tests/nwfilterxml2xmltest.c @@ -148,6 +148,8 @@ mymain(void) DO_TEST("example-1", false); DO_TEST("example-2", false); + DO_TEST("chain_prefixtest1", true); /* derived from arp-test */ + return (ret==0 ? EXIT_SUCCESS : EXIT_FAILURE); }
participants (2)
-
Eric Blake -
Stefan Berger