On Wed, Nov 13, 2013 at 03:28:39PM +0900, Simon Horman wrote:
> On Tue, Nov 12, 2013 at 09:58:09PM -0800, Ben Pfaff wrote:
> > On Tue, Nov 12, 2013 at 05:52:37PM -0800, Jarno Rajahalme wrote:
> > >
> > >
> > > > On Nov 12, 2013, at 8:47 AM, Ben Pfaff <b...@nicira.com> wrote:
> > > >
> > > >> On Tue, Nov 12, 2013 at 04:45:51PM +0900, Simon Horman wrote:
> > > ...
> > > >> +enum ofp_table_config {
> > > >> + OFPTC_TABLE_MISS_CONTROLLER = 0, /* Send to controller. */
> > > >> + OFPTC_TABLE_MISS_CONTINUE = 1 << 0, /* Continue to the next table
> > > >> in the
> > > >> + pipeline (OpenFlow 1.0
> > > >> behavior). */
> > > >> + OFPTC_TABLE_MISS_DROP = 1 << 1, /* Drop the packet. */
> > > >> + OFPTC_TABLE_MISS_MASK = 3
> > > >> +};
> > > >
> > > > I know that at least some version of the standard writes it that way but
> > > > the constants don't make any sense in that form. 0 << 0, 1 << 0, 2 <<
> > > > 0, 3 << 0 are a lot more sensible.
> > >
> > > Why shift zero bits?
> >
> > Only to imply more strongly, at a glance, that these are bitwise fields
> > within a larger word. I'm not sure any other bitwise fields were ever
> > defined in that word, but that was the intent, at least.
>
> OF1.4 defines more bits. Perhaps I should send a follow-up patch
> to merge in the following. It didn't occur to me earlier as my patch-set
> does not use the new bits.
>
> OFPTC_EVICTION = 1 << 2, /* Authorise table to evict flows. */
> OFPTC_VACANCY_EVENTS = 1 << 3, /* Enable vacancy events. */
It might be a good idea to add those definitions just because it makes
the intent of the other definitions clearer.
> Also, as of OF1.3 the meaning of 3 has changed, though
> I don't think it is particularly important from the
> point of view of the Open vSwitch implementation.
>
> OFPTC_DEPRECATED_MASK = 3, /* Deprecated bits */
I'd stick with our current name.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
