<div dir="ltr">Thanks very much, Ben for the comments,<div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Jun 18, 2013 at 1:10 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">Thanks again for working on this.<br>
<br>
It looks like your test builds are not configured to build a kernel<br>
module: this patch triggers tons of errors in the kernel build due to<br>
the change to OVSP_LOCAL, because OVS_FORCE is not defined in the<br>
kernel. I am not sure of that is the best possible fix (we are<br>
constrained by the kernel API here), but the following incremental<br>
does work:</blockquote><div><br></div><div><br></div><div>Sorry, I didn't test it with kernel module enabled. Thanks for pointing this out.</div><div><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">
diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h<br>
index add1287..bde3ba2 100644<br>
<div>--- a/include/linux/openvswitch.h<br>
+++ b/include/linux/openvswitch.h<br>
@@ -116,7 +116,7 @@ struct ovs_vport_stats {<br>
};<br>
<br>
/* Fixed logical ports. */<br>
</div>-#define OVSP_LOCAL ((OVS_FORCE odp_port_t) 0)<br>
+#define OVSP_LOCAL ((__u32)0)<br></blockquote><div><br></div><div><br></div><div>I think I will keep OVSP_LOCAL and OVSP_NONE u32. And create another macro ODPP_NONE of type odp_port_t.</div><div><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">
/* Packet transfer. */<br>
<br>
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c<br>
index 58435d7..823e5b3 100644<br>
--- a/lib/dpif-netdev.c<br>
+++ b/lib/dpif-netdev.c<br>
@@ -277,7 +277,8 @@ create_dp_netdev(const char *name, const struct dpif_class *class,<br>
hmap_init(&dp->flow_table);<br>
list_init(&dp->port_list);<br>
<br>
- error = do_add_port(dp, name, "internal", OVSP_LOCAL);<br>
+ error = do_add_port(dp, name, "internal",<br>
+ (OVS_FORCE odp_port_t) OVSP_LOCAL);<br>
if (error) {<br>
dp_netdev_free(dp);<br>
return error;<br>
@@ -468,8 +469,8 @@ static int<br>
<div> dpif_netdev_port_del(struct dpif *dpif, odp_port_t port_no)<br>
{<br>
struct dp_netdev *dp = get_dp_netdev(dpif);<br>
</div>- return (port_no == OVSP_LOCAL ?<br>
- EINVAL : do_del_port(dp, port_no));<br>
+ return (odp_to_u32(port_no) == OVSP_LOCAL ?<br>
<div>+ EINVAL : do_del_port(dp, port_no));<br>
}<br>
<br>
static bool<br>
<br>
<br>
</div>I find the new #defines at the top of openflow-1.0.h harder to read<br>
than the previous enum. How about this, instead:<br>
<br>
/* Ranges. */<br>
#define OFPP_MAX OFP_PORT_C(0xff00) /* Max # of switch ports. */<br>
#define OFPP_FIRST_RESV OFP_PORT_C(0xfff8) /* First assigned reserved port. */<br>
#define OFPP_LAST_RESV OFP_PORT_C(0xffff) /* Last assigned reserved port. */<br>
<div><br>
/* Reserved output "ports". */<br>
</div>#define OFPP_IN_PORT OFP_PORT_C(0xfff8) /* Where the packet came in. */<br>
#define OFPP_TABLE OFP_PORT_C(0xfff9) /* Perfor actions in flow table. */<br>
#define OFPP_NORMAL OFP_PORT_C(0xfffa) /* Process with normal L2/L3. */<br>
#define OFPP_FLOOD OFP_PORT_C(0xfffb) /* All ports except input port and<br>
* ports disabled by STP. */<br>
#define OFPP_ALL OFP_PORT_C(0xfffc) /* All ports except input port. */<br>
#define OFPP_CONTROLLER OFP_PORT_C(0xfffd) /* Send to controller. */<br>
#define OFPP_LOCAL OFP_PORT_C(0xfffe) /* Local openflow "port". */<br>
#define OFPP_NONE OFP_PORT_C(0xffff) /* Not associated with any port. */<br>
<br>
also adding macros for constants in types.h:<br>
<br>
diff --git a/include/openvswitch/types.h b/include/openvswitch/types.h<br>
index 7e1e59d..b292769 100644<br>
--- a/include/openvswitch/types.h<br>
+++ b/include/openvswitch/types.h<br>
@@ -66,4 +66,8 @@ typedef uint16_t OVS_BITWISE ofp_port_t;<br>
typedef uint32_t OVS_BITWISE odp_port_t;<br>
typedef uint32_t OVS_BITWISE ofp11_port_t;<br>
<br>
+#define OFP_PORT_C(X) ((OVS_FORCE ofp_port_t) X)<br>
+#define ODP_PORT_C(X) ((OVS_FORCE odp_port_t) X)<br>
+#define OFP11_PORT_C(X) ((OVS_FORCE ofp11_port_t) X)<br>
<div>+<br>
#endif /* openvswitch/types.h */<br></div></blockquote><div><br></div><div><br></div><div>This is cleaner. I'll adopt it. Thanks</div><div><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">
<div>
<br>
</div>Many of the new functions in flow.h are defined to take 'const'<br>
parameters that don't make much sense. For example:<br>
<div> static inline uint16_t ofp_to_u16(const ofp_port_t ofp_port);<br>
</div>'const' doesn't make sense on plain pass-by-value parameters like this<br>
because they do not communicate any useful information to the reader:<br>
the reader already knows that the function cannot modify the variable<br>
that it passes in, because it is passed by value. (In fact, this is<br>
true enough that C ignores the presence of qualifiers here for the<br>
purpose of considering compatibility of functions. Try compiling<br>
this:<br>
int x(const int);<br>
int x(int);<br>
int y(const int *);<br>
int y(int *);<br>
and you will see that the compiler does not complain about the change<br>
in the prototype for x(), but it does for y().)<br></blockquote><div><br></div><div>Thanks. I will remove it.</div><div><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">
I don't really like ofp_htons() and the other _htons() and _ntohs()<br>
functions. They make the reader have to know, or be willing to look<br>
up, what another set of functions does, and writing ofp_htons(port)<br>
instead of htons(ofp_to_u16(port)) does not make code much shorter.<br>
Same for hash_*_port() and *_port_increment().<br></blockquote><div><br></div><div><br></div><div>Sounds reasonable, I'd like to make the changes.</div><div><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">
Nothing uses the ofp11_port member of union flow_in_port. I don't<br>
think anything should, so please remove it. There is only one use of<br>
the port32 member; I think it could use the odp_port member instead,<br>
with odp_to_u32(), and then we could remove the member.<br></blockquote><div><br></div><div><br></div><div>This makes sense.</div><div><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">
The change to meta-flow.h is unnecessary.<br></blockquote><div><br></div><div><br></div><div>Okay, I'll remove it.</div><div><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">
<br>
In ofp-print.c, I think that compare_ports() would be easier to<br>
understand as:<br>
<br>
static int<br>
<div>compare_ports(const void *a_, const void *b_)<br>
{<br>
const struct ofputil_phy_port *a = a_;<br>
const struct ofputil_phy_port *b = b_;<br>
</div> uint16_t ap = ofp_to_u16(a->port_no);<br>
uint16_t bp = ofp_to_u16(b->port_no);<br>
<div><br>
return ap < bp ? -1 : ap > bp;<br>
}<br></div></blockquote><div><br></div><div><br></div><div>This is definitely more clear. Thanks.</div><div><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">
<div>
</div>I am not sure why ofp11_port_to_ofp_port() is in flow.h or why it is<br>
inline. It has only one user, ofputil_port_from_ofp11(). I would be<br>
inclined to just write it out as part of the latter function.<br></blockquote><div><br></div><div><br></div><div>I originally thought that the Openflow-1.1 port related functions would be useful.</div><div>But now, I think that is too early, right?</div>
<div><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">
Similarly for ofp_port_to_ofp11_port() and ofputil_port_to_ofp11().<br></blockquote><div><br></div><div><br></div><div>Agree, I will fix it.</div><div><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">
Please don't change lib/sflow*.h. It is copied from the sflow<br>
distribution and I do not want it to diverge.</blockquote><div><br></div><div> <br></div><div>No problem, I will leave it intact and adjust the code accordingly.</div><div><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">
nl_attr_get_odp_port() and nl_msg_put_odp_port() functions might be<br>
handy.<br></blockquote><div><br></div><div><br></div><div>Sure! honestly, it seems that I could not make the right decision on what to provide and</div><div>what not to. Especially after adding so many utility functions (*htons,*hash, odp/ofp_to_u),</div>
<div>more to learn ;D</div><div><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">
There seems to be some incomplete conversion in ovs-ofctl. I see a<br>
number of "uint32_t port_no" where I'd expect ofp_port_t, plus<br>
suspicious OVS_FORCE casts in fetch_port_by_features() and<br>
fetch_port_by_stats().<br></blockquote><div><br></div><div><br></div><div>I'll fix that.</div><div><br></div><div><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">
iface_create() in bridge.c adds a stray VLOG_INFO. <br></blockquote><div><br></div><div><br></div><div>
<p><span style="font-family:Arial">Something left out after
debugging. I will fix it. Thanks.</span></p>
</div><div><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>