[libvirt] [PATCH V4 03/10] Make filter creation in root table more flexible

[Stefan Berger]

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 at 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