| Message ID | 1499899627-49634-1-git-send-email-jpettit@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
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@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); Thanks, Ben.
> On Jul 12, 2017, at 5:42 PM, Ben Pfaff <blp@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@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? --Justin
On Wed, Jul 12, 2017 at 07:04:26PM -0700, Justin Pettit wrote: > > > On Jul 12, 2017, at 5:42 PM, Ben Pfaff <blp@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@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@ovn.org>
> On Jul 12, 2017, at 7:13 PM, Ben Pfaff <blp@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@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@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
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); + } nxm_put_16m(&ctx, MFF_CT_TP_SRC, oxm, flow->ct_tp_src, match->wc.masks.ct_tp_src); nxm_put_16m(&ctx, MFF_CT_TP_DST, oxm,
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@ovn.org> --- lib/nx-match.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)