| Message ID | 1512459015-18138-1-git-send-email-jpettit@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] datapath-windows: Correct endianness for deleting zone. | expand |
> -----Original Message----- > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > bounces@openvswitch.org] On Behalf Of Justin Pettit > Sent: Tuesday, December 5, 2017 9:30 AM > To: dev@openvswitch.org; Alin Gabriel Serdean <aserdean@ovn.org>; > Sairam Venugopal <vsairam@vmware.com> > Subject: [ovs-dev] [PATCH] datapath-windows: Correct endianness for > deleting zone. > > The zone Netlink attribute is supposed to be in network-byte order, but the > Windows code for deleting conntrack entries was treating it as host-byte > order. > > Found by inspection. > > Signed-off-by: Justin Pettit <jpettit@ovn.org> Thanks for the patch, Justin. I tested the patch, and everything looks good. I want to hold on the acknowledge until all of us are on the same page. This will make the windows datapath consistent with future patches from the userspace. For consistency I would like to backport the patch all the way to branch-2.6. What do you think Sai? Thanks, Alin.
Thanks for the patch. Discussed about the patch offline. This is required since we have now introduced a newer api for deleting 5-tuples and this breaks the assumption of zone-id not being in the right byte order. Acked-by: Sairam Venugopal <vsairam@vmware.com> On 12/4/17, 11:30 PM, "Justin Pettit" <jpettit@ovn.org> wrote: >The zone Netlink attribute is supposed to be in network-byte order, but >the Windows code for deleting conntrack entries was treating it as >host-byte order. > >Found by inspection. > >Signed-off-by: Justin Pettit <jpettit@ovn.org> >--- > datapath-windows/ovsext/Conntrack.c | 2 +- > lib/netlink-conntrack.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > >diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c >index 3203411a8b7a..edc0ec9c5324 100644 >--- a/datapath-windows/ovsext/Conntrack.c >+++ b/datapath-windows/ovsext/Conntrack.c >@@ -1076,7 +1076,7 @@ OvsCtDeleteCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, > } > > if (ctAttrs[CTA_ZONE]) { >- zone = NlAttrGetU16(ctAttrs[CTA_ZONE]); >+ zone = ntohs(NlAttrGetU16(ctAttrs[CTA_ZONE])); > } > > status = OvsCtFlush(zone); >diff --git a/lib/netlink-conntrack.c b/lib/netlink-conntrack.c >index 1e1bb2f79d1d..3c651b6c856a 100644 >--- a/lib/netlink-conntrack.c >+++ b/lib/netlink-conntrack.c >@@ -251,7 +251,7 @@ nl_ct_flush_zone(uint16_t flush_zone) > > nl_msg_put_nfgenmsg(&buf, 0, AF_UNSPEC, NFNL_SUBSYS_CTNETLINK, > IPCTNL_MSG_CT_DELETE, NLM_F_REQUEST); >- nl_msg_put_be16(&buf, CTA_ZONE, flush_zone); >+ nl_msg_put_be16(&buf, CTA_ZONE, htons(flush_zone)); > > err = nl_transact(NETLINK_NETFILTER, &buf, NULL); > ofpbuf_uninit(&buf); >-- >2.7.4 >
> On Dec 7, 2017, at 2:57 PM, Sairam Venugopal <vsairam@vmware.com> wrote: > > Thanks for the patch. Discussed about the patch offline. > > This is required since we have now introduced a newer api for deleting 5-tuples > and this breaks the assumption of zone-id not being in the right byte order. > > Acked-by: Sairam Venugopal <vsairam@vmware.com> Thanks. I pushed this to master. --Justin
We can hold off on back-porting this. This change mainly affects the delete by 5-tuple API. https://github.com/openvswitch/ovs/commit/817a76577fec3f03310d7d3a5a10df01340ee8ad Unless we plan to backport that patch, we should hold off on backporting this as well. Thanks, Sairam On 12/6/17, 9:41 AM, "aserdean@ovn.org" <aserdean@ovn.org> wrote: >> -----Original Message----- >> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- >> bounces@openvswitch.org] On Behalf Of Justin Pettit >> Sent: Tuesday, December 5, 2017 9:30 AM >> To: dev@openvswitch.org; Alin Gabriel Serdean <aserdean@ovn.org>; >> Sairam Venugopal <vsairam@vmware.com> >> Subject: [ovs-dev] [PATCH] datapath-windows: Correct endianness for >> deleting zone. >> >> The zone Netlink attribute is supposed to be in network-byte order, but >the >> Windows code for deleting conntrack entries was treating it as host-byte >> order. >> >> Found by inspection. >> >> Signed-off-by: Justin Pettit <jpettit@ovn.org> > >Thanks for the patch, Justin. I tested the patch, and everything looks >good. >I want to hold on the acknowledge until all of us are on the same page. > >This will make the windows datapath consistent with future patches from the >userspace. >For consistency I would like to backport the patch all the way to >branch-2.6. > >What do you think Sai? > >Thanks, >Alin. >
I was concerned a bit about binary compatibility between the future userspace binaries and older kernel versions. But Windows users usually just use the MSI (because of the signing issues). Thanks both! Alin. > -----Original Message----- > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > bounces@openvswitch.org] On Behalf Of Sairam Venugopal > Sent: Friday, December 8, 2017 1:26 AM > To: aserdean@ovn.org; 'Justin Pettit' <jpettit@ovn.org>; > dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH] datapath-windows: Correct endianness for > deleting zone. > > We can hold off on back-porting this. This change mainly affects the delete > by 5-tuple API. > > https://github.com/openvswitch/ovs/commit/817a76577fec3f03310d7d3a5a > 10df01340ee8ad > > Unless we plan to backport that patch, we should hold off on backporting this > as well. > > Thanks, > Sairam > > > > On 12/6/17, 9:41 AM, "aserdean@ovn.org" <aserdean@ovn.org> wrote: > > >> -----Original Message----- > >> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > >> bounces@openvswitch.org] On Behalf Of Justin Pettit > >> Sent: Tuesday, December 5, 2017 9:30 AM > >> To: dev@openvswitch.org; Alin Gabriel Serdean <aserdean@ovn.org>; > >> Sairam Venugopal <vsairam@vmware.com> > >> Subject: [ovs-dev] [PATCH] datapath-windows: Correct endianness for > >> deleting zone. > >> > >> The zone Netlink attribute is supposed to be in network-byte order, > >> but > >the > >> Windows code for deleting conntrack entries was treating it as > >> host-byte order. > >> > >> Found by inspection. > >> > >> Signed-off-by: Justin Pettit <jpettit@ovn.org> > > > >Thanks for the patch, Justin. I tested the patch, and everything looks > >good. > >I want to hold on the acknowledge until all of us are on the same page. > > > >This will make the windows datapath consistent with future patches from > >the userspace. > >For consistency I would like to backport the patch all the way to > >branch-2.6. > > > >What do you think Sai? > > > >Thanks, > >Alin. > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c index 3203411a8b7a..edc0ec9c5324 100644 --- a/datapath-windows/ovsext/Conntrack.c +++ b/datapath-windows/ovsext/Conntrack.c @@ -1076,7 +1076,7 @@ OvsCtDeleteCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, } if (ctAttrs[CTA_ZONE]) { - zone = NlAttrGetU16(ctAttrs[CTA_ZONE]); + zone = ntohs(NlAttrGetU16(ctAttrs[CTA_ZONE])); } status = OvsCtFlush(zone); diff --git a/lib/netlink-conntrack.c b/lib/netlink-conntrack.c index 1e1bb2f79d1d..3c651b6c856a 100644 --- a/lib/netlink-conntrack.c +++ b/lib/netlink-conntrack.c @@ -251,7 +251,7 @@ nl_ct_flush_zone(uint16_t flush_zone) nl_msg_put_nfgenmsg(&buf, 0, AF_UNSPEC, NFNL_SUBSYS_CTNETLINK, IPCTNL_MSG_CT_DELETE, NLM_F_REQUEST); - nl_msg_put_be16(&buf, CTA_ZONE, flush_zone); + nl_msg_put_be16(&buf, CTA_ZONE, htons(flush_zone)); err = nl_transact(NETLINK_NETFILTER, &buf, NULL); ofpbuf_uninit(&buf);
The zone Netlink attribute is supposed to be in network-byte order, but the Windows code for deleting conntrack entries was treating it as host-byte order. Found by inspection. Signed-off-by: Justin Pettit <jpettit@ovn.org> --- datapath-windows/ovsext/Conntrack.c | 2 +- lib/netlink-conntrack.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)