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

[Stefan Berger]

On 11/17/2011 06:35 PM, Eric Blake wrote:
> 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).
>
Will cover this part also.
>> 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.
>
Done.
>> +    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).
>
Fixed.
>>           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.
>
Fixed it now.