[libvirt] [PATCH V2 1/5] Add a mac chain

[Stefan Berger]

On 11/22/2011 01:47 PM, Eric Blake wrote:
> On 11/22/2011 08:51 AM, Stefan Berger wrote:
>> With hunks borrowed from one of David Steven's previous patches, we now
>> add the capability of having a 'mac' chain which is useful to filter
>> for multiple valid MAC addresses.
>>
>> Signed-off-by: David L Stevens<dlstevens at us.ibm.com>
>> Signed-off-by: Stefan Berger<stefanb at linux.vnet.ibm.com>
>>
>> ---
>>   docs/schemas/nwfilter.rng                 |    3 +++
>>   src/conf/nwfilter_conf.c                  |    2 ++
>>   src/conf/nwfilter_conf.h                  |    2 ++
>>   src/nwfilter/nwfilter_ebiptables_driver.c |   22 ++++++++++++++++++++--
>>   4 files changed, 27 insertions(+), 2 deletions(-)
> Almost - in addressing my v1 comments, you introduced some other problems.
>
>> @@ -2806,11 +2808,25 @@ ebtablesCreateTmpSubChain(ebiptablesRule
>>       char rootchain[MAX_CHAINNAME_LENGTH], chain[MAX_CHAINNAME_LENGTH];
>>       char chainPrefix = (incoming) ? CHAINPREFIX_HOST_IN_TEMP
>>                                     : CHAINPREFIX_HOST_OUT_TEMP;
>> +    char *protostr = NULL;
>>
>>       PRINT_ROOT_CHAIN(rootchain, chainPrefix, ifname);
>>       PRINT_CHAIN(chain, chainPrefix, ifname,
>>                   (filtername) ? filtername : l3_protocols[protoidx].val);
>>
>> +    switch (protoidx) {
>> +    case L2_PROTO_MAC_IDX:
>> +        break;
>> +    default:
>> +        virAsprintf(&protostr, "-p 0x%04x", l3_protocols[protoidx].attr);
>> +        break;
>> +    }
>> +
>> +    if (!protostr) {
>> +        virReportOOMError();
> Oops.  This gives a spurious OOM failure if protoidx is L2_PROTO_MAC_IDX.
>
>> @@ -2819,7 +2835,7 @@ ebtablesCreateTmpSubChain(ebiptablesRule
>>                         CMD_DEF("%s -t %s -N %s") CMD_SEPARATOR
>>                         CMD_EXEC
>>                         "%s"
>> -                      CMD_DEF("%s -t %s -%%c %s %%s -p 0x%x -j %s")
>> +                      CMD_DEF("%s -t %s -%%c %s %%s %s -j %s")
>                                                         ^^
>
> While you fixed the double space problem for a non-empty protostr, you
> still have the double space for L2_PROTO_MAC_IDX.  To completely avoid a
> double space, as well as the spurious OOM, you'd need:
>
> case L2_PROTO_MAC_IDX:
>      protostr = strdup("");
>      break;
My further testing also pointed that out to me in the meantime... :-/
> default:
>      virAsprintf(&protostr, "-p 0x%04x ", l3_protocols[protoidx].attr);
I removed the trailing whitespace above...
>      break;
> ...
>     CMD_DEF("%s -t %s -%%c %s %%s %s-j %s")
>
> ACK with those tweaks.
>
    Stefan