[libvirt] [PATCH v2] nwfilters: support for TCP flags evaluation

[Stefan Berger]

On 04/07/2011 05:47 PM, Eric Blake wrote:
> On 04/07/2011 01:47 PM, Stefan Berger wrote:
>> This patch adds support for the evaluation of TCP flags in nwfilters.
>>
>> It adds documentation to the web page and extends the tests as well.
>> Also, the nwfilter schema is extended.
>>
>> The following are some example for rules using the tcp flags:
>>
>> <rule action='accept' direction='in'>
>> <tcp state='NONE' flags='SYN/ALL' dsptportstart='80'/>
>> </rule>
>> <rule action='drop' direction='in'>
>> <tcp state='NONE' flags='SYN/ALL'/>
>> </rule>
>>
>>
>> Signed-off-by: Stefan Berger<stefanb at linux.vnet.ibm.com>
>>
>> ---
>>   docs/formatnwfilter.html.in               |   10 ++
>>   docs/schemas/nwfilter.rng                 |   16 ++++
>>   src/conf/nwfilter_conf.c                  |  115
>> +++++++++++++++++++++++++++---
>>   src/conf/nwfilter_conf.h                  |    9 ++
>>   src/libvirt_private.syms                  |    1
>>   src/nwfilter/nwfilter_ebiptables_driver.c |    9 ++
>>   tests/nwfilterxml2xmlin/tcp-test.xml      |   12 +++
>>   tests/nwfilterxml2xmlout/tcp-test.xml     |   12 +++
>>   8 files changed, 174 insertions(+), 10 deletions(-)
> ACK, looks reasonable to me, with one optimization nit fixed:
>
>> +    int32_t mask = 0x1;
>>
>> -    for (i = 0; int_map[i].val; i++) {
>> -        if (last_attr != int_map[i].attr&&
>> -            flags&  int_map[i].attr) {
>> -            if (c>= 1)
>> -                virBufferVSprintf(buf, "%s", sep);
>> -            virBufferVSprintf(buf, "%s", int_map[i].val);
>> -            c++;
>> +    while (mask) {
>> +        if ((mask&  flags)) {
>> +            for (i = 0; int_map[i].val; i++) {
>> +                if (mask == int_map[i].attr) {
>> +                    if (c>= 1)
>> +                        virBufferVSprintf(buf, "%s", sep);
>> +                    virBufferVSprintf(buf, "%s", int_map[i].val);
>> +                    c++;
>> +                }
>> +            }
>> +            flags ^= mask;
>> +            if (!flags)
>> +                break;
> This if condition should be after...
>
>>           }
> this brace; otherwise, if flags == 0 on entry, then you will
> (needlessly) iterate through all 32 bits of mask, rather than breaking
> on the first iteration.
>
Excellent! I'll modify and push.

    Stefan