On Tue, Aug 27, 2013 at 04:03:18PM -0700, Jarno Rajahalme wrote:
>
> On Aug 27, 2013, at 11:12 AM, Ben Pfaff <b...@nicira.com> wrote:
>
> > Suggested-by: pritesh <pritesh.koth...@cisco.com>
> > Signed-off-by: Ben Pfaff <b...@nicira.com>
> > ---
> > ofproto/tunnel.c | 100
> > +++++++++++++++++++++++++-----------------------------
> > 1 file changed, 46 insertions(+), 54 deletions(-)
> >
> > diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c
> > index 202358b..4729f5f 100644
> > --- a/ofproto/tunnel.c
> > +++ b/ofproto/tunnel.c
> ...
> > +/* Returns the tnl_port that is the best match for the tunnel data in
> > 'flow',
> > + * or NULL if no tnl_port matches 'flow'. */
> > static struct tnl_port *
> > -tnl_find(struct tnl_match *match_) OVS_REQ_RDLOCK(rwlock)
> > +tnl_find(const struct flow *flow) OVS_REQ_RDLOCK(rwlock)
> > {
> > - struct tnl_match match = *match_;
> > - struct tnl_port *tnl_port;
> > + enum ip_src_type {
> > + IP_SRC_EXACT, /* ip_src must match exactly. */
> > + IP_SRC_ANY, /* Any ip_src is acceptable. */
> > + IP_SRC_FLOW /* ip_src is handled in flow table. */
> > + };
> > +
> > + struct tnl_match_pattern {
> > + bool in_key_flow;
> > + bool ip_dst_flow;
> > + enum ip_src_type ip_src;
> > + };
> > +
> > + static const struct tnl_match_pattern patterns[] = {
> > + { false, false, IP_SRC_EXACT }, /* remote_ip, local_ip, in_key. */
> > + { false, false, IP_SRC_ANY }, /* remote_ip, in_key. */
> > + { true, false, IP_SRC_EXACT }, /* remote_ip, local_ip. */
> > + { true, false, IP_SRC_ANY }, /* remote_ip. */
> > + { true, true, IP_SRC_ANY }, /* Flow-based remote. */
> > + { true, true, IP_SRC_FLOW }, /* Flow-based everything. */
> > + };
> > +
>
> Sorry to mention this now rather than sooner, but the above would be even
> more self explanatory if the booleans were replaced with enums, like this:
>
> + static const struct tnl_match_pattern patterns[] = {
> + { KEY_CFG, IP_DST_CFG, IP_SRC_CFG }, /* remote_ip, local_ip, in_key.
> */
> + { KEY_CFG, IP_DST_CFG, IP_SRC_ANY }, /* remote_ip, in_key. */
> + { KEY_FLOW, IP_DST_CFG, IP_SRC_CFG }, /* remote_ip, local_ip. */
> + { KEY_FLOW, IP_DST_CFG, IP_SRC_ANY }, /* remote_ip. */
> + { KEY_FLOW, IP_DST_FLOW, IP_SRC_ANY }, /* Flow-based remote. */
> + { KEY_FLOW, IP_DST_FLOW, IP_SRC_FLOW }, /* Flow-based everything.
> */
> + };
>
> The key and dst fields could still be used like booleans, so no code needs to
> change because of this.
>
> (Note the renamed IP_SRC_EXACT -> IP_SRC_CFG also.)
I'm not entirely comfortable with these further directions. I think
that what I had was still clearer than what was there before. I renamed
IP_SRC_EXACT to IP_SRC_CFG as you suggested but left the rest as I
proposed, and applied this. If you think it's worthwhile to continue
refining this then let's do it with additional patches.
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
