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

[Ben Pfaff]

On Tue, Jun 11, 2013 at 04:10:34PM -0700, Alex Wang wrote:
> On Tue, Jun 11, 2013 at 3:59 PM, Ben Pfaff <blp at nicira.com> wrote:
> 
> > On Fri, Jun 07, 2013 at 11:11:43AM -0700, Alex Wang wrote:
> > > Currently datapath ports and openflow ports are both represented by
> > unsigned
> > > integers of various sizes. With implicit casts, etc. it is easy to mix
> > them
> > > up and use one where the other is expected. This commit creates two
> > typedefs
> > > ofp_port_t and odp_port_t. Both of these two types are marked by
> > > "__attribute__((bitwise))" so that Sparse can be used to detect any
> > misuse.
> > >
> > > Signed-off-by: Alex Wang <alexw at nicira.com>
> >
> > I am really glad to see this implemented.  Thank you.
> >
> > > REMAINING ISSUE
> > >
> > > When doing comparisions like '<', '<=', etc, both ofp_port_t and
> > odp_port_t
> > > will be converted to int, even though the L- and R- operands have same
> > type.
> > > This seems to be caused by "usual arithmetic conversion" and I have not
> > found
> > > a satisfying solution yet.
> > >
> > > As a workaround, I defined a macro function "PORT_COMPARE()" in
> > "lib/flow.h",
> > > which uses OVS_FORCE to cast both operands to uint32_t and conducts the
> > > comparision. It also makes it easy to check all such comparisons. Just
> > > grep "PORT_COMPARE".
> >
> > How about, instead, implementing a pair of functions something like
> > this in appropriate headers:
> >
> >         static inline uint32_t
> >         odp_to_u32(odp_port_t odp_port)
> >         {
> >             return (OVS_FORCE uint32_t) odp_port;
> >         }
> >
> >         static inline uint16_t
> >         ofp_to_u16(ofp_port_t ofp_port)
> >         {
> >             return (OVS_FORCE uint16_t) ofp_port:
> >         }
> >
> > Then comparisons can be written inline in a straightforward way,
> > e.g. in dpif_netdev_flow_from_nlattrs() in dpif-netdev.c:
> >     if (odp_to_u32(flow->in_port.odp_port) >= MAX_PORTS) {
> >
> > Does that make sense?
> 
> This makes sense. But one thing is that if we compare to "ofp_port_t"
> variables, e.g. "flow->in_port.ofp_port >= OFPP_MAX", we must call this
> function for both operands. this seems to make coding harder, really want
> to know what do you think?

I think it's OK in that case too.

> Also want to ask for your comments on "ofp_is_less_than()" solution, I
> provided in previous email.

I'll take another look.