[ovs-dev] [PATCH] nx-match: Don't append "ct_nw_proto" nx_match if mask not set.

[Justin Pettit]

> On Jul 12, 2017, at 7:13 PM, Ben Pfaff <blp at ovn.org> wrote:
> 
> On Wed, Jul 12, 2017 at 07:04:26PM -0700, Justin Pettit wrote:
>> 
>>> On Jul 12, 2017, at 5:42 PM, Ben Pfaff <blp at ovn.org> wrote:
>>> 
>>> On Wed, Jul 12, 2017 at 03:47:07PM -0700, Justin Pettit wrote:
>>>> The function nx_put_raw() shouldn't append "ct_nw_proto" to nx_match if
>>>> the corresponding mask isn't set.
>>>> 
>>>> Signed-off-by: Justin Pettit <jpettit at ovn.org>
>>>> ---
>>>> lib/nx-match.c | 4 +++-
>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/lib/nx-match.c b/lib/nx-match.c
>>>> index cb0cad8458b9..c64953b8892b 100644
>>>> --- a/lib/nx-match.c
>>>> +++ b/lib/nx-match.c
>>>> @@ -1190,7 +1190,9 @@ nx_put_raw(struct ofpbuf *b, enum ofp_version oxm, const struct match *match,
>>>>    nxm_put_ipv6(&ctx, MFF_CT_IPV6_DST, oxm,
>>>>                 &flow->ct_ipv6_dst, &match->wc.masks.ct_ipv6_dst);
>>>>    if (flow->ct_nw_proto) {
>>>> -        nxm_put_8(&ctx, MFF_CT_NW_PROTO, oxm, flow->ct_nw_proto);
>>>> +        if (match->wc.masks.ct_nw_proto) {
>>>> +            nxm_put_8(&ctx, MFF_CT_NW_PROTO, oxm, flow->ct_nw_proto);
>>>> +        }
>>> 
>>> Please use nxm_put_8m instead, e.g.:
>>> nxm_put_8m(&ctx, MFF_CT_NW_PROTO, oxm, flow->ct_nw_proto,
>>>          match->wc.masks.ct_nw_proto);
>> 
>> I thought about that, but I think nw_proto is normally not masked, and the other "nw_proto" fields in that file are similarly handled.  Do you think we should make them behave the same?
> 
> I see two uses of nw_proto in that function.  The first one uses an
> explicit "if" because there's other code that need to do different
> things based on the value of nw_proto, and checking the mask twice
> seemed weird:
> 
>    if (match->wc.masks.nw_proto) {
>        nxm_put_8(ctx, MFF_IP_PROTO, oxm, flow->nw_proto);
> 
>        if (flow->nw_proto == IPPROTO_TCP) {
>            ...
> 
> The second one is because of that super-weird special case where we take
> an 8-bit nw_proto field and expand it into a 16-bit arp_op field:
> 
>        if (match->wc.masks.nw_proto) {
>            nxm_put_16(&ctx, MFF_ARP_OP, oxm,
>                       htons(flow->nw_proto));
>        }
> 
> In the end it doesn't matter, both will have the same functionality, so
> it's fine either way.

I went ahead and changed it to nxm_put_8m and pushed it to master.

Thanks,

--Justin