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

[Eric Blake]

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).

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.

> @@ -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?

> @@ -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.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20111117/a1658ad8/attachment-0001.sig>