Chandra Sekhar Vejendla/San Jose/IBM@IBMUS wrote on 09/08/2016 11:31:54 PM:
> From: Chandra Sekhar Vejendla/San Jose/IBM@IBMUS
> To: dev@openvswitch.org
> Cc: Chandra Sekhar Vejendla/San Jose/IBM@IBMUS, Ryan
Moats/Omaha/IBM@IBMUS
> Date: 09/08/2016 11:32 PM
> Subject: [PATCH v2] ovn-controller: Fix match crieria for dynamic
> mac binding flows
>
> match struct is not initialized before adding flows for each entry in
> mac_bindings table. This results in incorrect match criteria.
>
> Signed-off-by: Chandra Sekhar Vejendla <csvej...@us.ibm.com>
> Signed-off-by: Ryan Moats <rmo...@us.ibm.com>
> Co-authored-by: Ryan Moats <rmo...@us.ibm.com>
> ---
> ovn/controller/lflow.c | 31 ++++++++++++++-----------------
> 1 file changed, 14 insertions(+), 17 deletions(-)
>
> diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
> index efac5b3..3ea7fc7 100644
> --- a/ovn/controller/lflow.c
> +++ b/ovn/controller/lflow.c
> @@ -343,10 +343,13 @@ put_load(const uint8_t *data, size_t len,
> static void
> consider_neighbor_flow(const struct lport_index *lports,
> const struct sbrec_mac_binding *b,
> - struct ofpbuf *ofpacts_p,
> - struct match *match_p,
> struct hmap *flow_table)
> {
> + struct ofpbuf ofpacts;
> + struct match match;
> + match_init_catchall(&match);
> + ofpbuf_init(&ofpacts, 0);
> +
> const struct sbrec_port_binding *pb
> = lport_lookup_by_name(lports, b->logical_port);
> if (!pb) {
> @@ -368,7 +371,7 @@ consider_neighbor_flow(const struct lport_index
*lports,
> VLOG_WARN_RL(&rl, "bad 'ip' %s", b->ip);
> return;
> }
> - match_set_reg(match_p, 0, ntohl(ip));
> + match_set_reg(&match, 0, ntohl(ip));
> } else {
> struct in6_addr ip6;
> if (!ipv6_parse(b->ip, &ip6)) {
> @@ -378,16 +381,17 @@ consider_neighbor_flow(const struct lport_index
*lports,
> }
> ovs_be128 value;
> memcpy(&value, &ip6, sizeof(value));
> - match_set_xxreg(match_p, 0, ntoh128(value));
> + match_set_xxreg(&match, 0, ntoh128(value));
> }
>
> - match_set_metadata(match_p, htonll(pb->datapath->tunnel_key));
> - match_set_reg(match_p, MFF_LOG_OUTPORT - MFF_REG0, pb->tunnel_key);
> + match_set_metadata(&match, htonll(pb->datapath->tunnel_key));
> + match_set_reg(&match, MFF_LOG_OUTPORT - MFF_REG0, pb->tunnel_key);
>
> - ofpbuf_clear(ofpacts_p);
> - put_load(mac.ea, sizeof mac.ea, MFF_ETH_DST, 0, 48, ofpacts_p);
> + ofpbuf_clear(&ofpacts);
> + put_load(mac.ea, sizeof mac.ea, MFF_ETH_DST, 0, 48, &ofpacts);
>
> - ofctrl_add_flow(flow_table, OFTABLE_MAC_BINDING, 100, match_p,
> ofpacts_p);
> + ofctrl_add_flow(flow_table, OFTABLE_MAC_BINDING, 100, &match,
&ofpacts);
> + ofpbuf_uninit(&ofpacts);
> }
>
> /* Adds an OpenFlow flow to flow tables for each MAC binding in the OVN
> @@ -398,17 +402,10 @@ add_neighbor_flows(struct controller_ctx *ctx,
> const struct lport_index *lports,
> struct hmap *flow_table)
> {
> - struct ofpbuf ofpacts;
> - struct match match;
> - match_init_catchall(&match);
> - ofpbuf_init(&ofpacts, 0);
> -
> const struct sbrec_mac_binding *b;
> SBREC_MAC_BINDING_FOR_EACH (b, ctx->ovnsb_idl) {
> - consider_neighbor_flow(lports, b, &ofpacts, &match, flow_table);
> + consider_neighbor_flow(lports, b, flow_table);
> }
> -
> - ofpbuf_uninit(&ofpacts);
> }
>
> /* Translates logical flows in the Logical_Flow table in the OVN_SB
database
> --
> 1.9.1
>
I haven't been able to come up with a valid test case that would fail
without
this patch. For the unit test, I tried to add multiple mac_bindings with
ovn-sbctl command and then verify that the flows generated should have only
reg0 and reg15 in the match criteria. When i tested this in a real setup,
without the patch, reg1, reg2 and reg3 will sometimes show up in the match
criteria along with reg0 and reg15. This is not 100% reproducible, though.
Thanks,
Chandra
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
