On Thu, Oct 18, 2012 at 4:42 PM, Ben Pfaff <b...@nicira.com> wrote:
> On Thu, Oct 18, 2012 at 12:51:49PM -0700, Justin Pettit wrote:
> > The current code has a simple mapping between datapath and OpenFlow port
> > numbers (1-to-1 other than the local port).
>
> The important fact is not really that the mapping is one-to-one, it's
> that the port numbers are actually *the same* except OFPP_LOCAL <-> 0,
> and that that is statically known at compile time.
>
Okay, it should be clearer now.
> I think something is inconsistent in dpif_netdev_flow_from_nlattrs().
> The function odp_flow_key_to_flow() that it calls is documented to put
> a datapath port number in the flow that it returns, but
> dpif_netdev_flow_from_nlattrs() then compares the returned port number
> against OFPP_MAX, OFPP_LOCAL, and OFPP_NONE.
>
> I think that the comments could be improved. Suggestion for
> odp_flow_key_from_flow():
> * 'flow->in_port' is ignored (since it is likely to be an OpenFlow port
> number
> * rather than a datapath port number). Instead, if 'odp_in_port' is
> anything
> * other than OVSP_NONE, it is included in 'buf' as the input port.
>
> Suggestion for struct flow:
>
> /*
> * A flow in the network.
> *
> * The meaning of 'in_port' is context-dependent. In most cases, it is a
> * 16-bit OpenFlow 1.0 port number. In the software datapath interface
> (dpif)
> * layer and its implementations (e.g. dpif-linux, dpif-netdev), it is
> instead
> * a 32-bit datapath port number.
> */
> struct flow {
> struct flow_tnl tunnel; /* Encapsulating tunnel parameters. */
> ovs_be64 metadata; /* OpenFlow Metadata. */
> struct in6_addr ipv6_src; /* IPv6 source address. */
> struct in6_addr ipv6_dst; /* IPv6 destination address. */
> struct in6_addr nd_target; /* IPv6 neighbor discovery (ND) target. */
> uint32_t skb_priority; /* Packet priority for QoS. */
> uint32_t regs[FLOW_N_REGS]; /* Registers. */
> ovs_be32 nw_src; /* IPv4 source address. */
> ovs_be32 nw_dst; /* IPv4 destination address. */
> ovs_be32 ipv6_label; /* IPv6 flow label. */
> uint32_t in_port; /* Input port (see above). */
>
Great suggestions. Done.
> In ofproto_unixctl_trace(), it looks very much to me like the conversion
> to an ODP port in the argc == 6 case should not be happening, that is,
> that the ofp_port_to_odp_port call should be omitted here:
> uint32_t in_port = ofp_port_to_odp_port(atoi(in_port_s));
>
Fixed. Isn't this a bug in the existing code? Should I do a separate
patch to fix this in existing branches?
> Are there any callers to ofp_port_to_odp_port() that couldn't handle the
> extra semantics that ofp_port_to_odp_in_port() provides? I think that
> we could just fold the latter into the former.
>
Good point. I made that change.
Thanks.
--Justin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
