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

[Eric Blake]

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;
default:
    virAsprintf(&protostr, "-p 0x%04x ", l3_protocols[protoidx].attr);
    break;
...
   CMD_DEF("%s -t %s -%%c %s %%s %s-j %s")

ACK with those tweaks.

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