On Wed, Dec 26, 2012 at 05:16:41PM -0800, Ethan Jackson wrote:
> Before translating a datapath flow key into actions, ofproto-dpif
> must parse it, tweak it, and figure out what ofproto_dpif it
> belongs to. This patch brings all this logic into one place where
> it will be easier to extend in the future.
>
> Signed-off-by: Ethan Jackson <et...@nicira.com>
I agree that this is an improvement.
The special cases for "trace" are odd. First:
> + port = odp_port_to_ofport(backer, flow->in_port);
> + if (!port) {
> + VLOG_INFO_RL(&rl, "received packet on an unassociated port %"PRIu32,
> + flow->in_port);
> + flow->in_port = OFPP_NONE;
> + return ofproto ? ODP_FIT_ERROR : fitness;
> + }
Why shouldn't we give up in trace, as we do in the packet receive
path, if the port can't be found? "trace" is supposed to match the
packet receive path (otherwise it isn't as useful). I guess the
reason is that we don't have to, since the user specified the ofproto
name, but I think that we should anyway.
Second: since the "trace" case doesn't report the ofproto that
matched, it might not be the same as the ofproto the user specified.
We should check that. Another alternative, which might be better,
would be to just not have the user specify the ofproto name at all in
the case where an ODP flow is being traced, since the information is
redundant.
The parentheses are not needed here, and look funny:
> + flow->in_port = (port)->up.ofp_port;
The return value is now doing double duty in kind of a funny way. It
reports not just the fitness but also gets used as a general-purpose
error return. It might be cleaner to make the fitness another output
parameter and just use the return value for success or failure. (This
is a very minor point.)
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
