[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