On Tue, Nov 08, 2011 at 11:06:21PM -0800, Justin Pettit wrote:
> On Nov 8, 2011, at 4:24 PM, Ben Pfaff wrote:
>
> > I think that these |=s can become =s:
> >
> >> - key->ip.tos_frag |= OVS_FRAG_TYPE_LATER;
> >> + key->ip.frag |= OVS_FRAG_TYPE_LATER;
> >>
> >> - key->ip.tos_frag |= OVS_FRAG_TYPE_FIRST;
> >> + key->ip.frag |= OVS_FRAG_TYPE_FIRST;
> >>
> >> @@ -764,10 +762,10 @@ int flow_extract(struct sk_buff *skb, u16 in_port,
> >> struct sw_flow_key *key,
> >>
> >> - key->ip.tos_frag |= OVS_FRAG_TYPE_FIRST;
> >> + key->ip.frag |= OVS_FRAG_TYPE_FIRST;
>
> Since they're flags, I was planning to leave them as "or"s, but I see
> I didn't do it consistently, so I've switched them to straight
> assignment.
They aren't flags. You can only have one of OVS_FRAG_TYPE_NONE,
OVS_FRAG_TYPE_FIRST, or OVS_FRAG_TYPE_LATER. Combinations don't make
sense.
> > Did you mean to update the comment on 'frag' below?
> >
> >> + u8 frag; /* OVS_FRAG_TYPE_* in low 2 bits. */
>
> I've replaced "in low 2 bits" with "flags".
>
> > In struct, could we put the 'frag' byte after arp_tha so that arp_sha
> > and arp_tha don't start on odd addresses:
> >
> >> @@ -70,21 +70,23 @@ struct flow {
> >> ?
> >> };
>
> Okay.
>
> > In nx-match.c, I think that it might be a little better to keep the
> > nxm_put_tos_frag() function together just to reduce code duplication.
>
> It seems odd to me to process these unrelated fields in the same
> function. nxm_put_frag() is there, which centralizes the most
> interesting code. If you really feel it makes it cleaner, though,
> I'll make the change.
I don't care that much, if you think it's better as-is then that's fine.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
