On Nov 7, 2011, at 11:42 AM, Ben Pfaff wrote:
> On Mon, Nov 07, 2011 at 10:33:07AM -0800, Justin Pettit wrote:
>> Signed-off-by: Justin Pettit <jpet...@nicira.com>
>
> Would you mind humoring me by writing htonl(0) below:
>> + if (wc & FWW_IPV6_LABEL) {
>> + flow->ipv6_label = 0;
>> + }
>
> and here too:
> + case MFF_IPV6_LABEL:
> + rule->wc.wildcards |= FWW_IPV6_LABEL;
> + rule->flow.ipv6_label = 0;
> + break;
> +
Yes. That's my preferred style, too. Other places in those function don't do
that conversion either. I'll write up a patch to get those, too.
> I'm pretty sure that the added "3" here should be a "4" (and the "7"
> an "8") since you defined NXM_OF_IPV6_LABEL as a 32-bit field in the
> NXM protocol:
>
>> @@ -106,6 +106,7 @@ nxm_decode_n_bits(ovs_be16 ofs_nbits)
>> * NXM_OF_IP_PROTO 4 2 -- 6
>> * NXM_OF_IPV6_SRC_W 4 16 16 36
>> * NXM_OF_IPV6_DST_W 4 16 16 36
>> + * NXM_OF_IPV6_LABEL 4 3 -- 7
>> * NXM_OF_ICMP_TYPE 4 1 -- 5
>> * NXM_OF_ICMP_CODE 4 1 -- 5
>> * NXM_NX_ND_TARGET 4 16 -- 20
Good catch. It was originally three bytes, but it made some of the endian
issues weird, so I switched it to four. Clearly, I missed this part.
> In ofputil_normalize_rule(), I think that the new MAY_IPV6_LABEL is
> redundant with the existing MAY_IPV6_ADDR so you could fold it in.
Good point. Changed.
> In packets.h the new IPV6_LABEL_MASK might deserve a comment.
Okay.
> I didn't really look at the kernel code.
I'll wait for Jesse's review for that part.
Thanks,
--Justin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
