Thanks very much, Ben for the comments,
On Tue, Jun 18, 2013 at 1:10 PM, Ben Pfaff <b...@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.
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
