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

[Alex Wang]

Thanks for the explanation,


On Thu, Jun 20, 2013 at 12:47 PM, Ben Pfaff <blp at nicira.com> wrote:

> On Thu, Jun 20, 2013 at 12:35:57PM -0700, Alex Wang wrote:
> > And sorry, I should have put you as co-author in the last commit.
>
> I don't think it was warranted, you did most of the work.
>
> > On Thu, Jun 20, 2013 at 10:53 AM, Ben Pfaff <blp at nicira.com> wrote:
> >
> > > On Wed, Jun 19, 2013 at 04:58:44PM -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 folded in some incrementals, below, and pushed this to master.
> > >
> > > The incrementals:
> > >
> > >         * Better protect the *_PORT_C macros.  I'd meant them only for
> > >           integer literals (like UINT32_C, etc.) so I didn't
> > >           originally put enough parentheses in, but I can see that it
> > >           is tempting to use them in other places.
> > >
> >
> >
> > I really want to learn the difference here. Could you please explain more
> > why the additional parentheses matter?
>
> Casts have higher precedence that most other operators, so (type)x and
> (type)(x) can be different.  For example, (uint32_t)UINT64_MAX >> 1 has
> value UINT32_MAX/2, but (uint32_t)(UINT64_MAX >> 1) has value
> UINT32_MAX, if I haven't screwed anything up.



This makes sense. Thanks,



>  > >         * Fix some places where UINT16_MAX was used for a mask but it
> > >           had been replaced by OFPP_MAX.  It's important for a mask
> > >           that it be all-1-bits, so UINT16_MAX is appropriate there.
> >>
> > Actually, I think you mean OFPP_NONE here, right?
>
> Oops, yes.
>
> > Is it to be more clear that we use "u16_to_ofp(UINT16_MAX)" for
> > "masks.in_port.ofp_port" assignment ?
>
> Yes, thanks for the correction.
>

Then, there is one leftover in tests/test-classifier.c, I'll patch it
quickly,
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130620/bc2e2215/attachment-0003.html>