[ovs-dev] [PATCH V2] feature: Create specific types for ofp and odp port

[Ben Pfaff]

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.