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.
> 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'll send out a revised version of this patch once I've incorporated Jesse's
suggestions.
--Justin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
