<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Jun 11, 2013 at 3:59 PM, Ben Pfaff <span dir="ltr"><<a href="mailto:blp@nicira.com" target="_blank">blp@nicira.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">On Fri, Jun 07, 2013 at 11:11:43AM -0700, Alex Wang wrote:<br>
> Currently datapath ports and openflow ports are both represented by unsigned<br>
> integers of various sizes. With implicit casts, etc. it is easy to mix them<br>
> up and use one where the other is expected. This commit creates two typedefs<br>
> ofp_port_t and odp_port_t. Both of these two types are marked by<br>
> "__attribute__((bitwise))" so that Sparse can be used to detect any misuse.<br>
><br>
> Signed-off-by: Alex Wang <<a href="mailto:alexw@nicira.com">alexw@nicira.com</a>><br>
<br>
</div>I am really glad to see this implemented. Thank you.<br>
<div class="im"><br>
> REMAINING ISSUE<br>
><br>
> When doing comparisions like '<', '<=', etc, both ofp_port_t and odp_port_t<br>
> will be converted to int, even though the L- and R- operands have same type.<br>
> This seems to be caused by "usual arithmetic conversion" and I have not found<br>
> a satisfying solution yet.<br>
><br>
> As a workaround, I defined a macro function "PORT_COMPARE()" in "lib/flow.h",<br>
> which uses OVS_FORCE to cast both operands to uint32_t and conducts the<br>
> comparision. It also makes it easy to check all such comparisons. Just<br>
> grep "PORT_COMPARE".<br>
<br>
</div>How about, instead, implementing a pair of functions something like<br>
this in appropriate headers:<br>
<br>
static inline uint32_t<br>
odp_to_u32(odp_port_t odp_port)<br>
{<br>
return (OVS_FORCE uint32_t) odp_port;<br>
}<br>
<br>
static inline uint16_t<br>
ofp_to_u16(ofp_port_t ofp_port)<br>
{<br>
return (OVS_FORCE uint16_t) ofp_port:<br>
}<br>
<br>
Then comparisons can be written inline in a straightforward way,<br>
e.g. in dpif_netdev_flow_from_nlattrs() in dpif-netdev.c:<br>
if (odp_to_u32(flow->in_port.odp_port) >= MAX_PORTS) {<br>
<br>
Does that make sense?<br>
<br></blockquote><div><br></div><div><br></div><div style>Thanks Ben,</div><div style><br></div><div style>This makes sense. But one thing is that if we compare to "ofp_port_t" variables, e.g. "flow->in_port.ofp_port >= OFPP_MAX", we must call this function for both operands. this seems to make coding harder, really want to know what do you think?</div>
<div><br></div><div style>Also want to ask for your comments on "<span style="white-space:pre-wrap">ofp_is_less_than()" solution, I provided in previous email.</span></div><div><br></div><div style>Kind Regards,</div>
<div style>Alex Wang,</div><div style><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Thanks,<br>
<br>
Ben.<br>
</blockquote></div><br></div></div>