[libvirt] [PATCH V5 07/10] Enable chains with names having a known prefix

[Eric Blake]

On 11/17/2011 01:11 PM, Stefan Berger wrote:
> This patch enables chains that have a known prefix in their name.
> Known prefixes are: 'ipv4', 'ipv6', 'arp', 'rarp'. All prefixes
> are also protocols that can be evaluated on the ebtables level.
> 
> Following the prefix they will be automatically connected to an interface's
> 'root' chain and jumped into following the protocol they evalute, i.e.,

s/evalute/evaluate/

> a table 'arp-xyz' will be accessed from the root table using
> 
> ebtables -t nat -A <iface root table> -p arp -j I-<ifname>-arp-xyz
> 
> thus generating a 'root' chain like this one here:
> 
> Bridge chain: libvirt-O-vnet0, entries: 5, policy: ACCEPT
> -p IPv4 -j O-vnet0-ipv4
> -p ARP -j O-vnet0-arp
> -p 0x8035 -j O-vnet0-rarp
> -p ARP -j O-vnet0-arp-xyz
> -j DROP 
> 
> where the chain 'arp-xyz' is accessed for filtering of ARP packets.
> 
> v3:
>  - assign a priority to filters that have an allowed prefix, e.g., assign
>    the arp chain priority to a filter arp-xyz unless user provided a 
>    priority in the XML
> 
> Signed-off-by: Stefan Berger <stefanb at linux.vnet.ibm.com>
> 
> ---
>  docs/schemas/nwfilter.rng |   16 ++++++--
>  src/conf/nwfilter_conf.c  |   87 ++++++++++++++++++++++++++++++++++++++++++----
>  src/conf/nwfilter_conf.h  |    3 +
>  3 files changed, 96 insertions(+), 10 deletions(-)

Another patch without docs.  (It's okay if the docs comes as a separate
patch, and if a single doc patch covers the XML additions in both 6 and
7; my main concern is only that the docs get patched by the time the
series is completed, and I didn't see it when glancing ahead to 10/10).

> 
> Index: libvirt-acl/src/conf/nwfilter_conf.c
> ===================================================================
> --- libvirt-acl.orig/src/conf/nwfilter_conf.c
> +++ libvirt-acl/src/conf/nwfilter_conf.c
> @@ -2007,6 +2007,80 @@ err_exit:
>      goto cleanup;
>  }
>  
> +static bool
> +virNWFilterIsValidChainName(const char *chainname)
> +{
> +    return chainname[strspn(chainname, VALID_CHAINNAME)] == 0;
> +}

Thanks; this validation is essential since your shell scripting was
pretty loose on quoting (that is, if a user could name a chain with an
embedded space, or worse a semicolon, your script would probably blow up).

> +
> +    if (!virNWFilterIsValidChainName(chainname)) {
> +        virNWFilterReportError(VIR_ERR_INVALID_ARG,
> +                               _("Chain name contains illegal characters"));
> +        return NULL;
> +    }
> +
> +    if (strlen(chainname) > MAX_CHAIN_SUFFIX_SIZE) {
> +        virNWFilterReportError(VIR_ERR_INVALID_ARG,
> +                               _("Name of chain is longer than %u characters"),
> +                               MAX_CHAIN_SUFFIX_SIZE);
> +        return NULL;
> +    }

I think it would be worth inlining this second check (the strlen) into
the first one.  That is, it would make more sense if
virNWFilterIsValidChainName checks both for valid characters and for
valid length; and all other callers only have to make one call for
validation purposes instead of two.

> +    virBufferAsprintf(&buf,
> +                      _("Illegal chain name '%s'. Please use a chain name "
> +                      "called '%s' or either one of the following prefixes: "),

Illegal implies breaking a law; I prefer the term "Invalid".

s/either one of/any of/

In English, "either" is only appropriate for a choice among 2 items, but
you are listing more than 2.

> +                      virNWFilterChainSuffixTypeToString(
> +                          VIR_NWFILTER_CHAINSUFFIX_ROOT),
> +                      chainname);
> +    for (i = 0; i < VIR_NWFILTER_CHAINSUFFIX_LAST; i++) {
> +        if (i == VIR_NWFILTER_CHAINSUFFIX_ROOT)
> +            continue;
> +        if (printed)
> +            virBufferAddLit(&buf, ", ");
> +        virBufferAdd(&buf, virNWFilterChainSuffixTypeToString(i), -1);
> +        printed = true;
> +    }

[Hmm, wondering if this would be any simpler if we added the
virBufferTruncate that was mentioned as a possibility in another thread;
then you wouldn't need a 'printed' flag in your loop.  But that's a
potential cleanup for another day.]

> +
> +    if (virBufferError(&buf)) {
> +        virReportOOMError();
> +        virBufferFreeAndReset(&buf);
> +        goto err_exit;
> +    }
> +
> +    msg = virBufferContentAndReset(&buf);
> +
> +    virNWFilterReportError(VIR_ERR_INVALID_ARG, _("%s"), msg);

Don't mark "%s" for translation.  'make syntax-check' should be just
fine with unadorned virNWFilterReportError(VIR_ERR_INVALID_ARG, "%s", msg).

>          if (chain_pri_s) {
>              ret->chainPriority = chain_priority;
>          } else {
> -            /* assign an implicit priority -- support XML attribute later */
> -            if (intMapGetByString(chain_priorities, chain, 0,
> +            /* assign an implicit priority */

Ah, the comment line I mentioned in patch 6 got changed here anyways.
Oh well, sorry if I'm causing you needless churn in conflict resolution
when you rebase.

-- 
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/a9ffe23e/attachment-0001.sig>