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

Ben Pfaff blp at ovn.org
Thu Jul 13 02:13:12 UTC 2017


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.

Acked-by: Ben Pfaff <blp at ovn.org>


More information about the dev mailing list