17 Nov
2011
17 Nov
'11
10:03 a.m.
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