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

[Alex Wang]

Thanks very much, Ben for the comments,


On Tue, Jun 18, 2013 at 1:10 PM, Ben Pfaff <blp at nicira.com> wrote:

> 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:



Sorry, I didn't test it with kernel module enabled. Thanks for pointing
this out.



> 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)
>


I think I will keep OVSP_LOCAL and OVSP_NONE u32. And create another macro
ODPP_NONE of type odp_port_t.



>  /* 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 */
>


This is cleaner. I'll adopt it. Thanks



>
> 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().)
>

Thanks. I will remove it.



> 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().
>


Sounds reasonable, I'd like to make the changes.



> 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.
>


This makes sense.



> The change to meta-flow.h is unnecessary.
>


Okay, I'll remove it.



>
> 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;
> }
>


This is definitely more clear. Thanks.



>  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.
>


I originally thought that the Openflow-1.1 port related functions would be
useful.
But now, I think that is too early, right?



> Similarly for ofp_port_to_ofp11_port() and ofputil_port_to_ofp11().
>


Agree, I will fix it.



> Please don't change lib/sflow*.h.  It is copied from the sflow
> distribution and I do not want it to diverge.



No problem, I will leave it intact and adjust the code accordingly.



> nl_attr_get_odp_port() and nl_msg_put_odp_port() functions might be
> handy.
>


Sure! honestly, it seems that I could not make the right decision on what
to provide and
what not to. Especially after adding so many utility functions
(*htons,*hash, odp/ofp_to_u),
more to learn ;D



> 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().
>


I'll fix that.




> iface_create() in bridge.c adds a stray VLOG_INFO.
>


Something left out after debugging. I will fix it. Thanks.



> Thanks,
>
> Ben.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130619/3bb6c3b6/attachment-0003.html>