[libvirt] [PATCH V5 06/10] Extend the filter XML to support priorities of chains

[Stefan Berger]

On 11/17/2011 06:15 PM, Eric Blake wrote:
> On 11/17/2011 01:11 PM, Stefan Berger wrote:
>> 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 at linux.vnet.ibm.com>
>>
>> ---
>>   docs/schemas/nwfilter.rng |    7 ++++++-
> Missing documentation in docs/formatnwfilter.html.in.  I'll live up to
> my hard-line reputation on this one, and request a v6 with documentation
> (for example, it's worth documenting whether priority 100 is accessed
> before or after priority 200).
>
I have such a patch much further down the queue. I'll pull the relevant 
parts into v6.
> But as to the code...
>
>> @@ -2028,6 +2030,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) {
> Let's use virStrToLong_i() instead of sscanf(); much nicer on the error
> handling aspect.
>
Done.
>> @@ -2036,11 +2058,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 */
> Is this comment still accurate, now that you have an XML attribute?
>
Fixed.
>> @@ -2852,6 +2881,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);
> That means an explicit pririoty='0' by the user is eaten and does not
> appear on the output.  But that's not too bad, and as long as we
> document that priority is 0 unless explicitly specified, we're covered
> (hence my plea for documentation...)
>
> Everything else looks okay.
>