On Sat, Jul 26, 2014 at 10:56:52AM -0700, Ben Pfaff wrote:
> On Sat, Jul 26, 2014 at 09:01:07AM -0700, Jarno Rajahalme wrote:
> > On Jul 25, 2014, at 10:25 PM, Ben Pfaff <[email protected]> wrote:
> > > @@ -195,7 +196,9 @@ enum oxm12_ofb_match_fields {
> > > #define OXM_OF_IPV6_EXTHDR_W  OXM_HEADER_W (OFPXMT13_OFB_IPV6_EXTHDR, 2)
> > > #define OXM_OF_PBB_UCA        OXM_HEADER   (OFPXMT14_OFB_PBB_UCA, 1)
> > > #define OXM_OF_TCP_FLAGS      OXM_HEADER   (OFPXMT15_OFB_TCP_FLAGS, 2)
> > > -#define OXM_OF_TCP_FLAGS_W    OXM_HEADER_W (OFPXMT15_OFB_TCP_FLAGS, 2)
> > 
> > This seems an unrelated change.
> 
> Oops.  Dropped.
> 
> > Did you consider modifying the struct flow instead, e.g.:
> > 
> > struct flow {
> >     /* L1 */
> >     struct flow_tnl tunnel;     /* Encapsulating tunnel parameters. */
> >     ovs_be64 metadata;          /* OpenFlow Metadata. */
> >     union {
> >         uint32_t regs[FLOW_N_REGS]; /* Registers. */
> >         uint64_t xregs[FLOW_N_REGS / 2]; /* 64-bit registers overlaid with 
> > 32-bit registers. */
> >     };
> 
> That didn't occur to me.  It's a little worrisome from a standard C
> aliasing perspective; I believe that in standard C only a single member
> of a union may be used at once.  ("Everyone" violates that rule though.)
> 
> The uintN_t members would probably need to become ovs_beN members so
> that the overlaying is consistent across little- and big-endian
> machines.
> 
> > >         #define REGNAME_LEN 20
> > > -        char regname[REGNAME_LEN];
> > > +        char regname[20];
> > 
> > Why this change?
> 
> I don't remember.  Dropped.
> 
> > > --- a/utilities/ovs-ofctl.8.in
> > > +++ b/utilities/ovs-ofctl.8.in
> > > @@ -1084,7 +1084,22 @@ indicates that the corresponding bit in 
> > > \fIvalue\fR must match
> > > exactly, and a 0-bit wildcards that bit.
> > > .IP
> > > When a packet enters an OpenFlow switch, all of the registers are set
> > > -to 0.  Only explicit Nicira extension actions change register values.
> > > +to 0.  Only explicit actions change register values.
> > > +.
> > > +.IP "\fBregx\fIidx\fB=\fIvalue\fR[\fB/\fImask\fR]?
> > 
> > ?regx? -> ?xreg? ?
> 
> Thanks, changed.

I decided to push this to master (since you did ack it).  We can
adjust how struct flow is built later, if we come to consensus on
that.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev

Reply via email to