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

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(a)us.ibm.com> > Signed-off-by: Stefan Berger<stefanb(a)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.