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