On Thu, Oct 18, 2012 at 4:42 PM, Ben Pfaff <span dir="ltr">&lt;<a href="mailto:blp@nicira.com" target="_blank">blp@nicira.com</a>&gt;</span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">On Thu, Oct 18, 2012 at 12:51:49PM -0700, Justin Pettit wrote:<br>
&gt; The current code has a simple mapping between datapath and OpenFlow port<br>
&gt; numbers (1-to-1 other than the local port).<br>
<br>
</div>The important fact is not really that the mapping is one-to-one, it&#39;s<br>
that the port numbers are actually *the same* except OFPP_LOCAL &lt;-&gt; 0,<br>
and that that is statically known at compile time.<br></blockquote><div><br></div><div>Okay, it should be clearer now.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I think something is inconsistent in dpif_netdev_flow_from_nlattrs().<br>
The function odp_flow_key_to_flow() that it calls is documented to put<br>
a datapath port number in the flow that it returns, but<br>
dpif_netdev_flow_from_nlattrs() then compares the returned port number<br>
against OFPP_MAX, OFPP_LOCAL, and OFPP_NONE.<br>
<br>
I think that the comments could be improved.  Suggestion for<br>
odp_flow_key_from_flow():<br>
 * &#39;flow-&gt;in_port&#39; is ignored (since it is likely to be an OpenFlow port number<br>
 * rather than a datapath port number).  Instead, if &#39;odp_in_port&#39; is anything<br>
 * other than OVSP_NONE, it is included in &#39;buf&#39; as the input port.<br>
<br>
Suggestion for struct flow:<br>
<br>
/*<br>
 * A flow in the network.<br>
 *<br>
 * The meaning of &#39;in_port&#39; is context-dependent.  In most cases, it is a<br>
 * 16-bit OpenFlow 1.0 port number.  In the software datapath interface (dpif)<br>
 * layer and its implementations (e.g. dpif-linux, dpif-netdev), it is instead<br>
 * a 32-bit datapath port number.<br>
 */<br>
struct flow {<br>
    struct flow_tnl tunnel;     /* Encapsulating tunnel parameters. */<br>
    ovs_be64 metadata;          /* OpenFlow Metadata. */<br>
    struct in6_addr ipv6_src;   /* IPv6 source address. */<br>
    struct in6_addr ipv6_dst;   /* IPv6 destination address. */<br>
    struct in6_addr nd_target;  /* IPv6 neighbor discovery (ND) target. */<br>
    uint32_t skb_priority;      /* Packet priority for QoS. */<br>
    uint32_t regs[FLOW_N_REGS]; /* Registers. */<br>
<div class="im">    ovs_be32 nw_src;            /* IPv4 source address. */<br>
    ovs_be32 nw_dst;            /* IPv4 destination address. */<br>
    ovs_be32 ipv6_label;        /* IPv6 flow label. */<br>
</div>    uint32_t in_port;           /* Input port (see above). */<br></blockquote><div><br></div><div>Great suggestions.  Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
In ofproto_unixctl_trace(), it looks very much to me like the conversion<br>
to an ODP port in the argc == 6 case should not be happening, that is,<br>
that the ofp_port_to_odp_port call should be omitted here:<br>
         uint32_t in_port = ofp_port_to_odp_port(atoi(in_port_s));<br></blockquote><div><br></div><div>







<p class="p1">Fixed.  Isn&#39;t this a bug in the existing code?  Should I do a separate patch to fix this in existing branches?</p></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Are there any callers to ofp_port_to_odp_port() that couldn&#39;t handle the<br>
extra semantics that ofp_port_to_odp_in_port() provides?  I think that<br>
we could just fold the latter into the former.<br></blockquote><div><br></div><div>Good point.  I made that change.</div><div><br></div><div>Thanks.</div><div><br></div><div>--Justin</div><div> </div><div><br></div></div>