Re: [ovs-dev] [PATCH] datapath-windows: Fix parsing of split buffers in OvsGetTcpHeader.

[Alin Serdean]

Hi Ilya,

Thanks for the patch. Will try to get a setup and test it.

In theory if we have a new analyze target of a newer visual studio it should 
flag those types of issues.

Will try to send a patch for the aforementioned target.

Wenying can you help in testing the patch?

Thank you,
Alin.

From: Ilya Maximets 
Sent: Thursday, June 6, 2024 12:52 PM
To: ovs-dev@openvswitch.org 
Cc: Alin Serdean ; Wenying Dong ; Ilya 
Maximets 
Subject: [PATCH] datapath-windows: Fix parsing of split buffers in 
OvsGetTcpHeader.

NdisGetDataBuffer() is called without providing a buffer to copy packet
data in case it is not contiguous.  So, it fails in some scenarios
where the packet is handled by the general network stack before OVS
and headers become split in multiple buffers.

Use existing helpers to retrieve the headers instead, they are using
OvsGetPacketBytes() which should be able to handle split data.

It might be a bit slower than getting direct pointers that may be
provided by NdisGetDataBuffer(), but it's better to optimize commonly
used OvsGetPacketBytes() helper in the future instead of optimizing
every single caller separately.  And we're still copying the TCP
header anyway.

Fixes: 9726a016d9d6 ("datapath-windows: Implement locking in conntrack NAT.")
Reported-at: https://github.com/openvswitch/ovs-issues/issues/323
Signed-off-by: Ilya Maximets 
---

WARNING: I beleive this code is correct, but I did not test it with real
 traffic, I only verified that it compiles.  Should not be applied
 unless someone tests it in an actual Windows setup.

 datapath-windows/ovsext/Conntrack.c | 45 ++---
 1 file changed, 21 insertions(+), 24 deletions(-)

diff --git a/datapath-windows/ovsext/Conntrack.c 
b/datapath-windows/ovsext/Conntrack.c
index 39ba5cc10..4649805dd 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -678,46 +678,43 @@ OvsGetTcpHeader(PNET_BUFFER_LIST nbl,
 VOID *storage,
 UINT32 *tcpPayloadLen)
 {
-IPHdr *ipHdr;
-IPv6Hdr *ipv6Hdr;
-TCPHdr *tcp;
+IPv6Hdr ipv6HdrStorage;
+IPHdr ipHdrStorage;
+const IPv6Hdr *ipv6Hdr;
+const IPHdr *ipHdr;
+const TCPHdr *tcp;
 VOID *dest = storage;
 uint16_t ipv6ExtLength = 0;

 if (layers->isIPv6) {
-ipv6Hdr = NdisGetDataBuffer(NET_BUFFER_LIST_FIRST_NB(nbl),
-layers->l4Offset + sizeof(TCPHdr),
-NULL, 1, 0);
+ipv6Hdr = OvsGetPacketBytes(nbl, sizeof *ipv6Hdr,
+layers->l3Offset, );
 if (ipv6Hdr == NULL) {
 return NULL;
 }

-tcp = (TCPHdr *)((PCHAR)ipv6Hdr + layers->l4Offset);
-ipv6Hdr = (IPv6Hdr *)((PCHAR)ipv6Hdr + layers->l3Offset);
-if (tcp->doff * 4 >= sizeof *tcp) {
-NdisMoveMemory(dest, tcp, sizeof(TCPHdr));
-ipv6ExtLength = layers->l4Offset - layers->l3Offset - 
sizeof(IPv6Hdr);
-*tcpPayloadLen = (ntohs(ipv6Hdr->payload_len) - ipv6ExtLength - 
TCP_HDR_LEN(tcp));
-return storage;
+tcp = OvsGetTcp(nbl, layers->l4Offset, dest);
+if (tcp == NULL) {
+return NULL;
 }
+
+ipv6ExtLength = layers->l4Offset - layers->l3Offset - sizeof(IPv6Hdr);
+*tcpPayloadLen = (ntohs(ipv6Hdr->payload_len) - ipv6ExtLength - 
TCP_HDR_LEN(tcp));
 } else {
-ipHdr = NdisGetDataBuffer(NET_BUFFER_LIST_FIRST_NB(nbl),
-  layers->l4Offset + sizeof(TCPHdr),
-  NULL, 1 /*no align*/, 0);
+ipHdr = OvsGetIp(nbl, layers->l3Offset, );
 if (ipHdr == NULL) {
 return NULL;
 }

-ipHdr = (IPHdr *)((PCHAR)ipHdr + layers->l3Offset);
-tcp = (TCPHdr *)((PCHAR)ipHdr + ipHdr->ihl * 4);
-
-if (tcp->doff * 4 >= sizeof *tcp) {
-NdisMoveMemory(dest, tcp, sizeof(TCPHdr));
-*tcpPayloadLen = TCP_DATA_LENGTH(ipHdr, tcp);
-return storage;
+tcp = OvsGetTcp(nbl, layers->l4Offset, dest);
+if (tcp == NULL) {
+return NULL;
 }
+
+*tcpPayloadLen = TCP_DATA_LENGTH(ipHdr, tcp);
 }
-return NULL;
+
+return storage;
 }

 static UINT8
--
2.45.0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] socket: Increase listen backlog to 128 everywhere.

[Alin Serdean]

Does it make sense to make this configurable via automake in order to avoid
future patches if we need to bump the value further?

On Tue, Jun 4, 2024 at 11:05 AM Simon Horman  wrote:

> + Ihar
>
> On Fri, May 31, 2024 at 03:40:08PM -0400, Brian Haley wrote:
> > An earlier patch [1] increased the size of the listen
> > backlog to 64. While that was a huge improvement over
> > 10, further testing in large deployments showed 128
> > was even better.
>
> nit: I would slightly prefer if a commit was referenced like this:
>
>   commit 2b7efee031c3 ("socket: Increase listen backlog to 64 everywhere.")
>
> > Looking at 'ss -lmt' output over more than one week for
> > port 6641, captured across three different controllers,
> > the average was:
> >
> > listen(s, 10) : 1213 drops/day
> > listen(s, 64) : 791 drops/day
> > listen(s, 128): 657 drops/day
> >
> > Looking at 'netstat -s | egrep -i 'listen|drop|SYN_RECV''
> > output over one week for port 6641, again captured across
> > three different controllers, the average was:
> >
> > listen(s, 10) : 741 drops/day
> > listen(s, 64) : 200 drops/day
> > listen(s, 128): 22 drops/day
> >
> > While having this value configurable would be the
> > best solution, changing to 128 is a quick fix that
> > should be good for all deployments. A link to the
> > original discussion is at [2].
> >
> > [1]
> https://github.com/openvswitch/ovs/commit/2b7efee031c3a2205ad2ee999275893edd083c1c
> > [2]
> https://github.com/openvswitch/ovs/commit/2b7efee031c3a2205ad2ee999275893edd083c1c
>
> nit: These two references are the same?
>
> > Signed-off-by: Brian Haley 
>
> I'd value input on this from Ihar (CCed) who worked on the cited commit.
>
> > ---
> >  lib/socket-util.c| 2 +-
> >  python/ovs/stream.py | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/socket-util.c b/lib/socket-util.c
> > index c569b7d16..552310266 100644
> > --- a/lib/socket-util.c
> > +++ b/lib/socket-util.c
> > @@ -769,7 +769,7 @@ inet_open_passive(int style, const char *target, int
> default_port,
> >  }
> >
> >  /* Listen. */
> > -if (style == SOCK_STREAM && listen(fd, 64) < 0) {
> > +if (style == SOCK_STREAM && listen(fd, 128) < 0) {
> >  error = sock_errno();
> >  VLOG_ERR("%s: listen: %s", target, sock_strerror(error));
> >  goto error;
> > diff --git a/python/ovs/stream.py b/python/ovs/stream.py
> > index dbb6b2e1f..874fe0bd5 100644
> > --- a/python/ovs/stream.py
> > +++ b/python/ovs/stream.py
> > @@ -620,7 +620,7 @@ class PassiveStream(object):
> >  raise Exception('Unknown connection string')
> >
> >  try:
> > -sock.listen(64)
> > +sock.listen(128)
> >  except socket.error as e:
> >  vlog.err("%s: listen: %s" % (name, os.strerror(e.error)))
> >  sock.close()
> > --
> > 2.34.1
> >
> > ___
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/2] appveyor: Remove reference to master branch.

[Alin Serdean]

Acked-by: Alin Gabriel Serdean 


> 
> On 10 Apr 2024, at 16:09, Simon Horman  wrote:
> 
> The OvS primary development branch has been renamed main
> so there is no longer any need for this CI configuration
> to refer to master.
> 
> Signed-off-by: Simon Horman 
> ---
> appveyor.yml | 1 -
> 1 file changed, 1 deletion(-)
> 
> diff --git a/appveyor.yml b/appveyor.yml
> index 050c7dead786..baa844753962 100644
> --- a/appveyor.yml
> +++ b/appveyor.yml
> @@ -3,7 +3,6 @@ image: Visual Studio 2019
> branches:
>   only:
>   - main
> -  - master
> configuration:
>   - Debug
>   - Release
> 
> --
> 2.43.0
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] treewide: Remove remaining XenServer references.

[Alin Serdean]

Ooops , my bad :)
Sent from phone 

> On 8 Apr 2024, at 15:39, Ilya Maximets  wrote:
> 
> On 4/8/24 15:29, Alin Serdean wrote:
>> 
>> The spellchecker one: 
>> https://github.com/openvswitch/ovs/blob/master/utilities/checkpatch.py#L115 
>> <https://github.com/openvswitch/ovs/blob/master/utilities/checkpatch.py#L115>
>> 
>> I don’t assume so but wanted to verify.
> 
> Ah, this one was just added today by Eelco. :)
> And it's also in OVS repo, not in OVN.
> 
>> 
>> 
>> 
>>>> On 8 Apr 2024, at 13:30, Ilya Maximets  wrote:
>>> 
>>> On 4/8/24 13:26, Alin Serdean wrote:
>>>> 
>>>> Just one question: do we have to remove the xenserver reference in the 
>>>> checkpatch.py?
>>> 
>>> Hmm.  Which one?
>>> 
>>>> 
>>>> Otherwise looks good.
>>>> 
>>>> Acked-by: Alin Gabriel Serdean 
>>>> 
>>>>> On 8 Apr 2024, at 13:06, Ilya Maximets  wrote:
>>>>> 
>>>>> Remaining bits from the OVS/OVN split.
>>>>> 
>>>>> Fixes: 1af37d11be73 ("Remove XenServer Code")
>>>>> Signed-off-by: Ilya Maximets 
>>>>> ---
>>>>> Documentation/tutorials/index.rst |  6 --
>>>>> README.rst|  3 ---
>>>>> acinclude.m4  | 26 ---
>>>>> build-aux/initial-tab-whitelist   |  1 -
>>>>> debian/copyright.in   | 34 ---
>>>>> 5 files changed, 70 deletions(-)
>>>>> 
>>>>> diff --git a/Documentation/tutorials/index.rst 
>>>>> b/Documentation/tutorials/index.rst
>>>>> index 0b7f52d0b..865a64018 100644
>>>>> --- a/Documentation/tutorials/index.rst
>>>>> +++ b/Documentation/tutorials/index.rst
>>>>> @@ -30,12 +30,6 @@ Tutorials
>>>>> Getting started with Open vSwitch (OVS) and Open Virtual Network (OVN) 
>>>>> for Open
>>>>> vSwitch.
>>>>> 
>>>>> -.. TODO(stephenfin): We could really do with a few basic tutorials, 
>>>>> along with
>>>>> -   some more specialized ones (DPDK, XenServer, Windows). The latter 
>>>>> could
>>>>> -   probably be formed from the install guides, but the former will need 
>>>>> to be
>>>>> -   produced from scratch or reproduced from blogs (with permission of the
>>>>> -   author)
>>>>> -
>>>>> .. toctree::
>>>>>   :maxdepth: 2
>>>>> 
>>>>> diff --git a/README.rst b/README.rst
>>>>> index 428cd8ee8..dce8c5bc8 100644
>>>>> --- a/README.rst
>>>>> +++ b/README.rst
>>>>> @@ -91,9 +91,6 @@ license applies to the file in question.
>>>>> 
>>>>> File build-aux/cccl is licensed under the GNU General Public License, 
>>>>> version 2.
>>>>> 
>>>>> -Files under the xenserver directory are licensed on a file-by-file basis.
>>>>> -Refer to each file for details.
>>>>> -
>>>>> Contact
>>>>> ---
>>>>> 
>>>>> diff --git a/acinclude.m4 b/acinclude.m4
>>>>> index 1a80dfaa7..73a2a51be 100644
>>>>> --- a/acinclude.m4
>>>>> +++ b/acinclude.m4
>>>>> @@ -177,32 +177,6 @@ AC_DEFUN([OVS_CONDITIONAL_CC_OPTION],
>>>>>   AM_CONDITIONAL([$2], [test $ovs_have_cc_option = yes])])
>>>>> dnl --
>>>>> 
>>>>> -dnl Check for too-old XenServer.
>>>>> -AC_DEFUN([OVS_CHECK_XENSERVER_VERSION],
>>>>> -  [AC_CACHE_CHECK([XenServer release], [ovs_cv_xsversion],
>>>>> -[if test -e /etc/redhat-release; then
>>>>> -   ovs_cv_xsversion=`sed -n 's/^XenServer DDK release 
>>>>> \([[^-]]*\)-.*/\1/p' /etc/redhat-release`
>>>>> - fi
>>>>> - if test -z "$ovs_cv_xsversion"; then
>>>>> -   ovs_cv_xsversion=none
>>>>> - fi])
>>>>> -  case $ovs_cv_xsversion in
>>>>> -none)
>>>>> -  ;;
>>>>> -
>>>>> -[[1-9]][[0-9]]* |dnl XenServer 10 or later
>>>>> -[[6-9]]* |   dnl XenServer 6 

Re: [ovs-dev] [PATCH ovn] treewide: Remove remaining XenServer references.

[Alin Serdean]

The spellchecker one: 
https://github.com/openvswitch/ovs/blob/master/utilities/checkpatch.py#L115

I don’t assume so but wanted to verify.



> On 8 Apr 2024, at 13:30, Ilya Maximets  wrote:
> 
> On 4/8/24 13:26, Alin Serdean wrote:
>> 
>> Just one question: do we have to remove the xenserver reference in the 
>> checkpatch.py?
> 
> Hmm.  Which one?
> 
>> 
>> Otherwise looks good.
>> 
>> Acked-by: Alin Gabriel Serdean 
>> 
>>>> On 8 Apr 2024, at 13:06, Ilya Maximets  wrote:
>>> 
>>> Remaining bits from the OVS/OVN split.
>>> 
>>> Fixes: 1af37d11be73 ("Remove XenServer Code")
>>> Signed-off-by: Ilya Maximets 
>>> ---
>>> Documentation/tutorials/index.rst |  6 --
>>> README.rst|  3 ---
>>> acinclude.m4  | 26 ---
>>> build-aux/initial-tab-whitelist   |  1 -
>>> debian/copyright.in   | 34 ---
>>> 5 files changed, 70 deletions(-)
>>> 
>>> diff --git a/Documentation/tutorials/index.rst 
>>> b/Documentation/tutorials/index.rst
>>> index 0b7f52d0b..865a64018 100644
>>> --- a/Documentation/tutorials/index.rst
>>> +++ b/Documentation/tutorials/index.rst
>>> @@ -30,12 +30,6 @@ Tutorials
>>> Getting started with Open vSwitch (OVS) and Open Virtual Network (OVN) for 
>>> Open
>>> vSwitch.
>>> 
>>> -.. TODO(stephenfin): We could really do with a few basic tutorials, along 
>>> with
>>> -   some more specialized ones (DPDK, XenServer, Windows). The latter could
>>> -   probably be formed from the install guides, but the former will need to 
>>> be
>>> -   produced from scratch or reproduced from blogs (with permission of the
>>> -   author)
>>> -
>>> .. toctree::
>>>   :maxdepth: 2
>>> 
>>> diff --git a/README.rst b/README.rst
>>> index 428cd8ee8..dce8c5bc8 100644
>>> --- a/README.rst
>>> +++ b/README.rst
>>> @@ -91,9 +91,6 @@ license applies to the file in question.
>>> 
>>> File build-aux/cccl is licensed under the GNU General Public License, 
>>> version 2.
>>> 
>>> -Files under the xenserver directory are licensed on a file-by-file basis.
>>> -Refer to each file for details.
>>> -
>>> Contact
>>> ---
>>> 
>>> diff --git a/acinclude.m4 b/acinclude.m4
>>> index 1a80dfaa7..73a2a51be 100644
>>> --- a/acinclude.m4
>>> +++ b/acinclude.m4
>>> @@ -177,32 +177,6 @@ AC_DEFUN([OVS_CONDITIONAL_CC_OPTION],
>>>   AM_CONDITIONAL([$2], [test $ovs_have_cc_option = yes])])
>>> dnl --
>>> 
>>> -dnl Check for too-old XenServer.
>>> -AC_DEFUN([OVS_CHECK_XENSERVER_VERSION],
>>> -  [AC_CACHE_CHECK([XenServer release], [ovs_cv_xsversion],
>>> -[if test -e /etc/redhat-release; then
>>> -   ovs_cv_xsversion=`sed -n 's/^XenServer DDK release 
>>> \([[^-]]*\)-.*/\1/p' /etc/redhat-release`
>>> - fi
>>> - if test -z "$ovs_cv_xsversion"; then
>>> -   ovs_cv_xsversion=none
>>> - fi])
>>> -  case $ovs_cv_xsversion in
>>> -none)
>>> -  ;;
>>> -
>>> -[[1-9]][[0-9]]* |dnl XenServer 10 or later
>>> -[[6-9]]* |   dnl XenServer 6 or later
>>> -5.[[7-9]]* | dnl XenServer 5.7 or later
>>> -5.6.[[1-9]][[0-9]][[0-9]][[0-9]]* |  dnl XenServer 5.6.1000 or later
>>> -5.6.[[2-9]][[0-9]][[0-9]]* | dnl XenServer 5.6.200 or later
>>> -5.6.1[[0-9]][[0-9]]) dnl Xenserver 5.6.100 or later
>>> -  ;;
>>> -
>>> -*)
>>> -  AC_MSG_ERROR([This appears to be XenServer $ovs_cv_xsversion, but 
>>> only XenServer 5.6.100 or later is supported.  (If you are really using a 
>>> supported version of XenServer, you may override this error message by 
>>> specifying 'ovs_cv_xsversion=5.6.100' on the "configure" command line.)])
>>> -  ;;
>>> -  esac])
>>> -
>>> dnl OVS_CHECK_SPARSE_TARGET
>>> dnl
>>> dnl The "cgcc" script from "sparse" isn't very good at detecting the
>>> diff --git a/build-aux/initial-tab-whitelist 
>>> b/build-aux/initial-tab-whitelist
>>> index 8ad43d616..eb5c1a23a 100644

Re: [ovs-dev] [PATCH ovn] treewide: Remove remaining XenServer references.

[Alin Serdean]

Just one question: do we have to remove the xenserver reference in the 
checkpatch.py?

Otherwise looks good.

Acked-by: Alin Gabriel Serdean 

> On 8 Apr 2024, at 13:06, Ilya Maximets  wrote:
> 
> Remaining bits from the OVS/OVN split.
> 
> Fixes: 1af37d11be73 ("Remove XenServer Code")
> Signed-off-by: Ilya Maximets 
> ---
> Documentation/tutorials/index.rst |  6 --
> README.rst|  3 ---
> acinclude.m4  | 26 ---
> build-aux/initial-tab-whitelist   |  1 -
> debian/copyright.in   | 34 ---
> 5 files changed, 70 deletions(-)
> 
> diff --git a/Documentation/tutorials/index.rst 
> b/Documentation/tutorials/index.rst
> index 0b7f52d0b..865a64018 100644
> --- a/Documentation/tutorials/index.rst
> +++ b/Documentation/tutorials/index.rst
> @@ -30,12 +30,6 @@ Tutorials
> Getting started with Open vSwitch (OVS) and Open Virtual Network (OVN) for 
> Open
> vSwitch.
> 
> -.. TODO(stephenfin): We could really do with a few basic tutorials, along 
> with
> -   some more specialized ones (DPDK, XenServer, Windows). The latter could
> -   probably be formed from the install guides, but the former will need to be
> -   produced from scratch or reproduced from blogs (with permission of the
> -   author)
> -
> .. toctree::
>:maxdepth: 2
> 
> diff --git a/README.rst b/README.rst
> index 428cd8ee8..dce8c5bc8 100644
> --- a/README.rst
> +++ b/README.rst
> @@ -91,9 +91,6 @@ license applies to the file in question.
> 
> File build-aux/cccl is licensed under the GNU General Public License, version 
> 2.
> 
> -Files under the xenserver directory are licensed on a file-by-file basis.
> -Refer to each file for details.
> -
> Contact
> ---
> 
> diff --git a/acinclude.m4 b/acinclude.m4
> index 1a80dfaa7..73a2a51be 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -177,32 +177,6 @@ AC_DEFUN([OVS_CONDITIONAL_CC_OPTION],
>AM_CONDITIONAL([$2], [test $ovs_have_cc_option = yes])])
> dnl --
> 
> -dnl Check for too-old XenServer.
> -AC_DEFUN([OVS_CHECK_XENSERVER_VERSION],
> -  [AC_CACHE_CHECK([XenServer release], [ovs_cv_xsversion],
> -[if test -e /etc/redhat-release; then
> -   ovs_cv_xsversion=`sed -n 's/^XenServer DDK release 
> \([[^-]]*\)-.*/\1/p' /etc/redhat-release`
> - fi
> - if test -z "$ovs_cv_xsversion"; then
> -   ovs_cv_xsversion=none
> - fi])
> -  case $ovs_cv_xsversion in
> -none)
> -  ;;
> -
> -[[1-9]][[0-9]]* |dnl XenServer 10 or later
> -[[6-9]]* |   dnl XenServer 6 or later
> -5.[[7-9]]* | dnl XenServer 5.7 or later
> -5.6.[[1-9]][[0-9]][[0-9]][[0-9]]* |  dnl XenServer 5.6.1000 or later
> -5.6.[[2-9]][[0-9]][[0-9]]* | dnl XenServer 5.6.200 or later
> -5.6.1[[0-9]][[0-9]]) dnl Xenserver 5.6.100 or later
> -  ;;
> -
> -*)
> -  AC_MSG_ERROR([This appears to be XenServer $ovs_cv_xsversion, but only 
> XenServer 5.6.100 or later is supported.  (If you are really using a 
> supported version of XenServer, you may override this error message by 
> specifying 'ovs_cv_xsversion=5.6.100' on the "configure" command line.)])
> -  ;;
> -  esac])
> -
> dnl OVS_CHECK_SPARSE_TARGET
> dnl
> dnl The "cgcc" script from "sparse" isn't very good at detecting the
> diff --git a/build-aux/initial-tab-whitelist b/build-aux/initial-tab-whitelist
> index 8ad43d616..eb5c1a23a 100644
> --- a/build-aux/initial-tab-whitelist
> +++ b/build-aux/initial-tab-whitelist
> @@ -5,7 +5,6 @@
> \.sln$
> ^ovs/
> ^third-party/
> -^xenserver/
> ^debian/rules.modules$
> ^debian/rules$
> ^\.gitmodules$
> diff --git a/debian/copyright.in b/debian/copyright.in
> index 9ad00340f..335fd6af8 100644
> --- a/debian/copyright.in
> +++ b/debian/copyright.in
> @@ -27,40 +27,6 @@ Upstream Copyright Holders:
> 
> License:
> 
> -* The following components are licensed under the
> -  GNU Lesser General Public License version 2.1 only
> -  with the exception clause below as a pre-amble.
> -
> -xenserver/etc_xensource_scripts_vif
> -xenserver/opt_xensource_libexec_InterfaceReconfigure.py
> -xenserver/opt_xensource_libexec_InterfaceReconfigureBridge.py
> -xenserver/opt_xensource_libexec_InterfaceReconfigureVswitch.py
> -xenserver/opt_xensource_libexec_interface-reconfigure
> -xenserver/usr_lib_xsconsole_plugins-base_XSFeatureVSwitch.py
> -
> -* These components are only distributed in the source package.
> -  They do not appear in any binary packages.
> -
> -  On Debian systems, the complete text of the
> -  GNU Lesser General Public License version 2.1 can be found in
> -  `/usr/share/common-licenses/LGPL-2.1'
> -
> -  The exception clause pre-amble reads:
> -
> -  As a special exception to the GNU Lesser General Public License, you
> -  may link, statically or 

Re: [ovs-dev] [PATCH v2 4/4] appveyor: Build with OpenSSL 3.0.

[Alin Serdean]

That’s understandable, I think we should be fine for now and if someone asks 
for a backport we can address it then.

Thank you again for all the work you put into this!

Alin

> 
> On 4 Mar 2024, at 23:44, Ilya Maximets  wrote:
> 
> On 3/1/24 22:29, Alin Serdean wrote:
>> Lgtm.  Thank you so much Ilya
>> 
>> Acked-by: Alin-Gabriel Serdean 
> 
> Thanks, Alin and Simon!
> 
> I applied the set now.
> 
> Also backported patches 2 and 3 to branch-3.3, since they are bug fixes
> and 3.3 is planned to be our next LTS.  We may backport further, but
> I'm a little hesitant to do so since we don't have CI for stable branches
> and no-one complained so far.  Let me know if you think we should backport
> further, I can do that.
> 
> Best regards, Ilya Maximets.
> 
>> 
>>> 
>>>> On 1 Mar 2024, at 22:10, Ilya Maximets  wrote:
>>> 
>>> OpenSSL 1.0.2u is long deprecated and not available for download.
>>> So, our CI never actually downloads it and uses whatever is in the
>>> OpenSSL-Win64 folder provided by AppVeyor.  Luckily, it happens to
>>> be OpenSSL 1.0.2u today.
>>> 
>>> The oldest supported version of OpenSSL upstream today is 3.0.
>>> And it is an LTS version.  3.1 and 3.2 are not LTS.
>>> 
>>> Use OpenSSL 3.0 for testing instead.
>>> 
>>> This commit does a few things to achieve that:
>>> 
>>> 1. Removes the folder provided by AppVeyor.  This way we will fail
>>>  the build if something goes wrong instead of silently using
>>>  OpenSSL version provided by AppVeyor.
>>> 
>>> 2. Obtains the JSON description of available releases and downloads
>>>  the latest minor version of OpenSSL 3.0 64-bit.  With this approach
>>>  we should not need to update the download link that frequently.
>>>  New minor releases will be picked up automatically.  They should
>>>  not have any breaking changes, so should be fine to use in CI.
>>>  OpenSSL 3.0 is supported until at least Sep 2026.
>>> 
>>>  The JSON file is an official file referenced on the:
>>>   https://slproweb.com/products/Win32OpenSSL.html
>>>  So, it should be safe to use.
>>> 
>>> 3. Executes the downloaded installer with 'Start-Process -Wait' to
>>>  properly wait for installation to finish instead of just sleeping
>>>  for 30 seconds.
>>> 
>>> 4. Caches the downloaded installer, so we're not downloading 300 MB
>>>  on each CI run as that is not nice to do.  We know the hash of the
>>>  latest version, so we will re-download only when the binary changes,
>>>  i.e. on a new minor release.
>>> 
>>>  For the cache to work we need to introduce the 'install' phase,
>>>  because caches are populated after 'init', but before 'install'.
>>>  Alternatively, we could have just renamed 'init' to 'install',
>>>  but I think it's a little nicer to have separate phases, and we
>>>  can also move 'windows-prepare.sh' to the install phase.
>>> 
>>>  Cache is also invalidated whenever appveyor.yml changes.
>>> 
>>> Acked-by: Simon Horman 
>>> Signed-off-by: Ilya Maximets 
>>> ---
>>> appveyor.yml | 52 ++--
>>> 1 file changed, 42 insertions(+), 10 deletions(-)
>>> 
>>> diff --git a/appveyor.yml b/appveyor.yml
>>> index 373f01a43..29cc44d6c 100644
>>> --- a/appveyor.yml
>>> +++ b/appveyor.yml
>>> @@ -8,28 +8,60 @@ configuration:
>>>  - Release
>>> clone_folder: C:\openvswitch_compile
>>> shallow_clone: true
>>> +
>>> init:
>>> - ps: $env:PATH ="C:\Python312-x64;"+$env:PATH
>>> - ps: New-Item -Type HardLink -Path  "C:\Python312-x64\python3.exe"
>>>  -Value "C:\Python312-x64\python.exe"
>>> +
>>> +cache:
>>> +- C:\ovs-build-downloads -> appveyor.yml
>>> +
>>> +install:
>>> - ps: |
>>> -mkdir C:\ovs-build-downloads
>>> +Remove-Item -Recurse -Force -Path C:/OpenSSL-Win64
>>> +New-Item -ItemType Directory -Force -Path C:\ovs-build-downloads
>>> +
>>> +# Find and download the latest stable OpenSSl 3.0.
>>> +$URL = 
>>> "https://raw.githubusercontent.com/slproweb/opensslhashes/master/win32_openssl_hashes.json;
>>> +$webData = (Invoke-WebRequest -Uri $URL).content | ConvertFrom-Json
>>> +$source = ($webData.files.PSObject.Properties | Where-O

Re: [ovs-dev] [PATCH v2 4/4] appveyor: Build with OpenSSL 3.0.

[Alin Serdean]

Lgtm.  Thank you so much Ilya

Acked-by: Alin-Gabriel Serdean 

> 
> On 1 Mar 2024, at 22:10, Ilya Maximets  wrote:
> 
> OpenSSL 1.0.2u is long deprecated and not available for download.
> So, our CI never actually downloads it and uses whatever is in the
> OpenSSL-Win64 folder provided by AppVeyor.  Luckily, it happens to
> be OpenSSL 1.0.2u today.
> 
> The oldest supported version of OpenSSL upstream today is 3.0.
> And it is an LTS version.  3.1 and 3.2 are not LTS.
> 
> Use OpenSSL 3.0 for testing instead.
> 
> This commit does a few things to achieve that:
> 
> 1. Removes the folder provided by AppVeyor.  This way we will fail
>   the build if something goes wrong instead of silently using
>   OpenSSL version provided by AppVeyor.
> 
> 2. Obtains the JSON description of available releases and downloads
>   the latest minor version of OpenSSL 3.0 64-bit.  With this approach
>   we should not need to update the download link that frequently.
>   New minor releases will be picked up automatically.  They should
>   not have any breaking changes, so should be fine to use in CI.
>   OpenSSL 3.0 is supported until at least Sep 2026.
> 
>   The JSON file is an official file referenced on the:
>https://slproweb.com/products/Win32OpenSSL.html
>   So, it should be safe to use.
> 
> 3. Executes the downloaded installer with 'Start-Process -Wait' to
>   properly wait for installation to finish instead of just sleeping
>   for 30 seconds.
> 
> 4. Caches the downloaded installer, so we're not downloading 300 MB
>   on each CI run as that is not nice to do.  We know the hash of the
>   latest version, so we will re-download only when the binary changes,
>   i.e. on a new minor release.
> 
>   For the cache to work we need to introduce the 'install' phase,
>   because caches are populated after 'init', but before 'install'.
>   Alternatively, we could have just renamed 'init' to 'install',
>   but I think it's a little nicer to have separate phases, and we
>   can also move 'windows-prepare.sh' to the install phase.
> 
>   Cache is also invalidated whenever appveyor.yml changes.
> 
> Acked-by: Simon Horman 
> Signed-off-by: Ilya Maximets 
> ---
> appveyor.yml | 52 ++--
> 1 file changed, 42 insertions(+), 10 deletions(-)
> 
> diff --git a/appveyor.yml b/appveyor.yml
> index 373f01a43..29cc44d6c 100644
> --- a/appveyor.yml
> +++ b/appveyor.yml
> @@ -8,28 +8,60 @@ configuration:
>   - Release
> clone_folder: C:\openvswitch_compile
> shallow_clone: true
> +
> init:
> - ps: $env:PATH ="C:\Python312-x64;"+$env:PATH
> - ps: New-Item -Type HardLink -Path  "C:\Python312-x64\python3.exe"
>   -Value "C:\Python312-x64\python.exe"
> +
> +cache:
> +- C:\ovs-build-downloads -> appveyor.yml
> +
> +install:
> - ps: |
> -mkdir C:\ovs-build-downloads
> +Remove-Item -Recurse -Force -Path C:/OpenSSL-Win64
> +New-Item -ItemType Directory -Force -Path C:\ovs-build-downloads
> +
> +# Find and download the latest stable OpenSSl 3.0.
> +$URL = 
> "https://raw.githubusercontent.com/slproweb/opensslhashes/master/win32_openssl_hashes.json;
> +$webData = (Invoke-WebRequest -Uri $URL).content | ConvertFrom-Json
> +$source = ($webData.files.PSObject.Properties | Where-Object {
> +$_.Value.basever   -match "3.0.*" -and
> +$_.Value.bits  -eq"64"-and
> +$_.Value.arch  -eq"INTEL" -and
> +$_.Value.installer -eq"exe"   -and
> +-not $_.Value.light
> +} | Select-Object Value).PSObject.Properties.Value
> +
> +Write-Host "Latest OpenSSL 3.0:" ($source | Format-List | Out-String)
> +
> +$destination = "C:\ovs-build-downloads\Win64OpenSSL.exe"
> +if (Test-Path $destination) {
> +$fileHash = (Get-FileHash $destination -Algorithm 
> SHA256).Hash.ToLower()
> +if ($fileHash -ne $source.sha256) {
> +Write-Host "Cache miss:" $fileHash "!=" $source.sha256
> +Remove-Item -Path $destination
> +}
> +}
> 
> -$source = "https://slproweb.com/download/Win64OpenSSL-1_0_2u.exe;
> -$destination = "C:\ovs-build-downloads\Win64OpenSSL-1_0_2u.exe"
> -Invoke-WebRequest $source -OutFile $destination
> +if (Test-Path $destination) {
> +Write-Host "Using cached:" $destination
> +} else {
> +Write-Host "Downloading:" $source.url
> +Invoke-WebRequest $source.url -OutFile $destination
> +}
> +
> +Write-Host "Installing:" $destination
> +Start-Process -FilePath $destination `
> +-ArgumentList "/silent /verysilent /sp- /suppressmsgboxes" -Wait
> 
> -cd C:\ovs-build-downloads
> -.\Win64OpenSSL-1_0_2u.exe /silent /verysilent /sp- /suppressmsgboxes
> -Start-Sleep -s 30
> -cd C:\openvswitch_compile
> - ps: git clone -q https://git.code.sf.net/p/pthreads4w/code 
> c:\pthreads4w-code
> - ps: python3 -m pip install pypiwin32 --disable-pip-version-check
> -
> -build_script:
> - 

Re: [ovs-dev] [PATCH 3/4] m4: Fix linking with OpenSSL 1.1.0+ and 3+ on Windows.

[Alin Serdean]

Thank you so much for the patches and dealing with this, Ilya!

Regarding the MD / MT linking for a given binary, this can work,  usually, 
without any issues, although it will increase the memory used by the process 
because both runtimes have to be loaded.

We are linking the rest of the binaries with MT / MTd inside OVS: 
https://github.com/openvswitch/ovs/blob/master/build-aux/cccl#L92-L98 .
Can you please change the OpenSSL to link with MT instead of MD?
This will ensure that we test with what will be built in the end.

Because we are changing the defaults to msys2 / openssl3 we need to also update 
the documentation.

I'm okay with changing it to a later date. If you prefer, I can pick this one 
up.

I will insert the snippets where the documentation has to be changed for 
OpenSSL:
https://github.com/openvswitch/ovs/blob/master/Documentation/intro/install/windows.rst?plain=1#L109-L115
https://github.com/openvswitch/ovs/blob/master/Documentation/intro/install/windows.rst?plain=1#L179-L185
https://github.com/openvswitch/ovs/blob/master/Documentation/intro/install/windows.rst?plain=1#L191-L199


Alin.

From: dev  on behalf of Simon Horman 

Sent: Thursday, February 15, 2024 2:08 PM
To: Ilya Maximets 
Cc: ovs-dev@openvswitch.org 
Subject: Re: [ovs-dev] [PATCH 3/4] m4: Fix linking with OpenSSL 1.1.0+ and 3+ 
on Windows.

On Tue, Feb 13, 2024 at 08:40:17PM +0100, Ilya Maximets wrote:
> OpenSSL 1.1.0 changed the library names from libeay32 and ssleay32 to
> standard libssl and libcrypto.  All the versions of OpenSSL that used
> old names reached their official EoL, so it should be safe to just
> migrate to new names.  They can still be supported via premium support
> option, but I don't think that is important for us.
>
> Also, OpenSSL installers for older versions had the following folder
> structure:

...

> Basically, instead of one generic library in the lib folder and a bunch
> of differently named versions of it for different type of linkage, we
> now have multiple instances of the library located in different folders
> based on the linkage type.  So, we have to provide an exact path in
> order to find the library.
>
> 'lib/VC/x64/MD' was chosen in this patch and it seems to work fine.
> MD stands for dynamic linking, MT is static, 'd' stands for debug
> versions of the libraries.
>
> Signed-off-by: Ilya Maximets 

Acked-by: Simon Horman 

...
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] appveyor: Move from MinGW 32bit to msys64.

[Alin Serdean]

Looking forward to that patch :)
Thank you for migrating the CI to msys!

Acked-by: Alin Gabriel Serdean 
Signed-off-by: Alin Gabriel Serdean 

> 
> On 12 Feb 2024, at 13:43, Ilya Maximets  wrote:
> 
> On 2/9/24 21:47, Ilya Maximets wrote:
>> AppVeyor is planning to remove support for MinGW 32bit soon.  And we
>> had a couple of incidents where it wasn't available already, so we
>> moved to a 'Previous' image.
>> 
>> Move to msys64 instead.
>> 
>> While at it making the CI scripts a little nicer, moving the non-Windows
>> parts of the preparation and build to separate files.
>> 
>> MSYS2 has its own version of python.  However, we do not support
>> building on Windows with non-Windows python build.  The main issue is
>> the delimiter symbol in PYTHONPATH.  In Windows version it has to be
>> ';', while the python supplied with MSYS2 uses ':' as on Linux, while
>> we detect Windows and pass ';' during the build.  Renaming the binary,
>> so the Windows version is used.
>> 
>> Additionally switched to Python 3.12, 3.7 reached EoL some time back,
>> though it's still available in AppVeyor.
>> 
>> The stderr has to be redirected to stdout for scripts, because any
>> message on stderr is treated as fatal failure by powershell.
>> 
>> Scripts are running with 'set -e', so a failure of individual
>> commands will fail the script.
>> 
>> The OpenSSL download is still failing, but it is out of scope for
>> this change.
> 
> For the record, I have a fix for OpenSSL ready, including the
> permission fixes with icacls, but it depends on changes made
> in this patch.
> 
> Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ofproto-dpif: Fix removal of renamed datapath ports.

[Alin Serdean]

Thanks for the quick patch!

Tested-by: Alin-Gabriel Serdean 
Acked-by: Alin-Gabriel Serdean 


From: Ilya Maximets 
Sent: Wednesday, July 19, 2023 6:14 PM
To: ovs-dev@openvswitch.org 
Cc: Alin Serdean ; Eelco Chaudron ; Ilya 
Maximets 
Subject: [PATCH] ofproto-dpif: Fix removal of renamed datapath ports.

OVS configuration is based on port names and OpenFlow port numbers.
Names are stored in the database and translated later to OF ports.
On the datapath level, each port has a name and a datapath port number.
Port name in the database has to match datapath port name, unless it's
a tunnel port.

If a datapath port is renamed with 'ip link set DEV name NAME',
ovs-vswitchd will wake up, destroy all the OpenFlow-related structures
and clean other things up.  This is because the port no longer
represents the port from a database due to a name difference.

However, ovs-vswitch will not actually remove the port from the
datapath, because it thinks that this port is no longer there.  This
is happening because lookup is performed by name and the name have
changed.  As a result we have a port in a datapath that is not related
to any port known to ovs-vswitchd and ovs-vswitchd can't remove it.
This port also occupies a datapath port number and prevents the port
to be added back with a new name.

Fix that by performing lookup by a datapath port number during the port
destruction.  The name was used only to avoid spurious warnings in a
normal case where the port was successfully deleted by other parts of
OVS.  Adding an extra flag to avoid these warnings instead.

Fixes: 02f8d6460afd ("ofproto-dpif: Query port existence by name to prevent 
warnings.")
Reported-at: https://github.com/openvswitch/ovs-issues/issues/284
Signed-off-by: Ilya Maximets 
---
 lib/dpctl.c   |  2 +-
 lib/dpif.c| 13 +
 lib/dpif.h|  2 +-
 ofproto/ofproto-dpif.c| 14 ++
 tests/system-interface.at | 57 +++
 5 files changed, 76 insertions(+), 12 deletions(-)

diff --git a/lib/dpctl.c b/lib/dpctl.c
index 4394653ab..79b82a176 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -673,7 +673,7 @@ show_dpif(struct dpif *dpif, struct dpctl_params *dpctl_p)
 }

 for (int i = 0; i < n_port_nos; i++) {
-if (dpif_port_query_by_number(dpif, port_nos[i], _port)) {
+if (dpif_port_query_by_number(dpif, port_nos[i], _port, true)) {
 continue;
 }

diff --git a/lib/dpif.c b/lib/dpif.c
index b1cbf39c4..29041ab91 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -705,13 +705,14 @@ dpif_port_set_config(struct dpif *dpif, odp_port_t 
port_no,
  * initializes '*port' appropriately; on failure, returns a positive errno
  * value.
  *
- * Retuns ENODEV if the port doesn't exist.
+ * Retuns ENODEV if the port doesn't exist.  Will not log a warning in this
+ * case unless 'warn_if_not_found' is true.
  *
  * The caller owns the data in 'port' and must free it with
  * dpif_port_destroy() when it is no longer needed. */
 int
 dpif_port_query_by_number(const struct dpif *dpif, odp_port_t port_no,
-  struct dpif_port *port)
+  struct dpif_port *port, bool warn_if_not_found)
 {
 int error = dpif->dpif_class->port_query_by_number(dpif, port_no, port);
 if (!error) {
@@ -719,8 +720,10 @@ dpif_port_query_by_number(const struct dpif *dpif, 
odp_port_t port_no,
 dpif_name(dpif), port_no, port->name);
 } else {
 memset(port, 0, sizeof *port);
-VLOG_WARN_RL(_rl, "%s: failed to query port %"PRIu32": %s",
- dpif_name(dpif), port_no, ovs_strerror(error));
+VLOG_RL(_rl,
+(error == ENODEV && !warn_if_not_found) ? VLL_DBG : VLL_WARN,
+"%s: failed to query port %"PRIu32": %s",
+dpif_name(dpif), port_no, ovs_strerror(error));
 }
 return error;
 }
@@ -788,7 +791,7 @@ dpif_port_get_name(struct dpif *dpif, odp_port_t port_no,

 ovs_assert(name_size > 0);

-error = dpif_port_query_by_number(dpif, port_no, );
+error = dpif_port_query_by_number(dpif, port_no, , true);
 if (!error) {
 ovs_strlcpy(name, port.name, name_size);
 dpif_port_destroy();
diff --git a/lib/dpif.h b/lib/dpif.h
index 9e9d0aa1b..0f2dc2ef3 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -463,7 +463,7 @@ void dpif_port_clone(struct dpif_port *, const struct 
dpif_port *);
 void dpif_port_destroy(struct dpif_port *);
 bool dpif_port_exists(const struct dpif *dpif, const char *devname);
 int dpif_port_query_by_number(const struct dpif *, odp_port_t port_no,
-  struct dpif_port *);
+  struct dpif_port *, bool warn_if_not_found);
 int dpif_port_query_by_name(const struct dpif *, const char *devname,
 struct dpif_port 

Re: [ovs-dev] [PATCH 1/2] appveyor: Don't download OpenSSL.

[Alin Serdean]

Thanks for taking care of it Ilya!

Alin.

Sent from phone 

> On 3 Jul 2023, at 14:59, Ilya Maximets  wrote:
> 
> On 7/1/23 01:55, Ilya Maximets wrote:
>>> On 7/1/23 01:43, Ilya Maximets wrote:
>>> On 6/8/23 12:07, Alin Serdean wrote:
>>>> Ack, I will try to see if I can get GH actions.
>>>> It would also be better to have everything integrated in one place.
>>>> 
>>>> I remember the discussion and the PR… I had some comments which never got 
>>>> addressed.
>>>> 
>>>> Windows is pretty slow when starting processes ,especially if the builtin 
>>>> AV is not disabled for the directory you are building in. That is the main 
>>>> issue for the slow compile and test times.
>>>> 
>>>> Using cmake/meson is helpful because unlike the automake tools, they will 
>>>> generate a visual studio solution and that in turn can be built by 
>>>> invoking a single msbuild process. Since meson does not have something 
>>>> builtin for testing, I was wondering if it would make more sense switching 
>>>> to cmake/ctest… 
>>> 
>>> We could just invoke autotest from meson as a workaround...
>>> I didn't work much with cmake, but personally I don't like it.
>>> 
>>> FWIW, appveyor started to fail not being able to find OpenSSL,
>>> even though it still claims that it's should be in the same
>>> location:
>>> 
>>> checking for openssl/ssl.h in C:/OpenSSL-Win64... no
>>> checking whether compiling and linking against OpenSSL works... no
>>> configure: error: Cannot find openssl (use --disable-ssl to configure 
>>> without SSL support)
>>> Command exited with code 1
>>> 
>>> That's annoying.
>> 
>> I opened an issue: https://github.com/appveyor/ci/issues/3883
> 
> FWIW, they fixed the image and the issue is resolved now.
> 
> Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] MAINTAINERS: Add Eelco Chaudron

[Alin Serdean]

Welcome Eelco!
Acked-by: Alin Gabriel Serdean 

> 
> On 21 Jun 2023, at 14:50, Simon Horman  wrote:
> 
> Eelco Chaudron was elected by the Open vSwitch committers yesterday.
> This formalises his status as an Open vSwitch committer.
> 
> Welcome Eelco!
> 
> Signed-off-by: Simon Horman 
> ---
> MAINTAINERS.rst | 2 ++
> 1 file changed, 2 insertions(+)
> 
> diff --git a/MAINTAINERS.rst b/MAINTAINERS.rst
> index 85b8e6416584..2fc2517177e1 100644
> --- a/MAINTAINERS.rst
> +++ b/MAINTAINERS.rst
> @@ -45,6 +45,8 @@ This is the current list of active Open vSwitch committers:
>  - aserd...@ovn.org
>* - Ansis Atteka
>  - ansisatt...@gmail.com
> +   * - Eelco Chaudron
> + - echau...@redhat.com
>* - Ian Stokes
>  - isto...@ovn.org
>* - Ilya Maximets
> -- 
> 2.30.2
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/2] appveyor: Don't download OpenSSL.

[Alin Serdean]

Ack, I will try to see if I can get GH actions.
It would also be better to have everything integrated in one place.

I remember the discussion and the PR… I had some comments which never got 
addressed.

Windows is pretty slow when starting processes ,especially if the builtin AV is 
not disabled for the directory you are building in. That is the main issue for 
the slow compile and test times.

Using cmake/meson is helpful because unlike the automake tools, they will 
generate a visual studio solution and that in turn can be built by invoking a 
single msbuild process. Since meson does not have something builtin for 
testing, I was wondering if it would make more sense switching to cmake/ctest… 

—
Alin

> On 8 Jun 2023, at 11:43, Ilya Maximets  wrote:
> 
> On 6/8/23 11:01, Alin Serdean wrote:
>> 
>> Ack. I will try to see if I can address it by the end of the week.
>> 
>> Thanks for clarifying the questions.
>> 
>> Would GH actions be better than appveyor?
> 
> Functionally, I don't think they are that much different.
> But GHA is better integrated into our CI, i.e. ovsrobot
> reports GHA status per patch on a patchwork, but it doesn't
> do the same for appveyor.  So, having GHA job is probably
> not a bad idea.  We discussed most of the pros and cons
> about 2 years ago here:
>  https://github.com/openvswitch/ovs-issues/issues/209
> The main problem is that windows build is slow in general.
> But that shouldn't be a blocker for CI.
> 
> Best regards, Ilya Maximets.
> 
>> 
>> —
>> Alin.
>> 
>>> On 2 Jun 2023, at 12:25, Ilya Maximets  wrote:
>>> 
>>> On 6/1/23 13:50, Ilya Maximets wrote:
>>>>> On 5/31/23 23:05, Alin Serdean wrote:
>>>>> 
>>>>> That makes sense.
>>>>> 
>>>>> We can leverage the  following commit:
>>>>> 
>>>>> https://patchwork.ozlabs.org/project/openvswitch/patch/20201013124655.1408-1-aserd...@cloudbasesolutions.com/
>>>>>  
>>>>> <https://patchwork.ozlabs.org/project/openvswitch/patch/20201013124655.1408-1-aserd...@cloudbasesolutions.com/>
>>>>> 
>>>>> But I still need to fix the permissions. I’ll try to find some time and 
>>>>> address it.
>>>> 
>>>> Thanks.  I actually forgot about this patch.  Would be great if you can
>>>> send an updated version.  Would also be great to migrate to OpenSSL 3.0
>>>> instead of 1.1.1 to not migrate again in some not so distant future.
>>>> OpenSSL 3.0 should work fine, unless there are some other library changes
>>>> (not OpenSSL 3.1, because OpenSSL 3.1 is not an LTS release).
>>> 
>>> I'll mark  the current patch as 'rejected' for now in favor of the
>>> future OpenSSL update with your suggested change.
>>> 
>>>> 
>>>>> I remember there were some discussions to modernize the build system to 
>>>>> meson or cmake. Was that effort fruitful in the end?
>>>> 
>>>> The main issue with meson is that we still need automake/autotools for
>>>> our testsuite.  We might invoke autotest from meson, but it sounds a
>>>> bit strange to do that.  I agree that it is still beneficial in some
>>>> cases to use meson, e.g. for a windows build, so might make sense to
>>>> migrate anyway, but an attempt from 2021 didn't receive any follow ups.
>>>> 
>>>> There was also a PR to add Windows build to GitHub Actions, but it
>>>> didn't move since your request to Sign-off the changes.
>>>> 
>>>> Best regards, Ilya Maximets.
>>>> 
>>>>> 
>>>>> Alin.
>>>>> 
>>>>>> On 31 May 2023, at 22:41, Ilya Maximets  wrote:
>>>>>> 
>>>>>> On 5/31/23 22:36, Alin Serdean wrote:
>>>>>>> 
>>>>>>> It would be best to change the link with the latest version of OpenSSL. 
>>>>>>> That will ensure there are no mishaps .
>>>>>> 
>>>>>> I think the problem here is that slproweb.com only provides
>>>>>> OpenSSL 1.1.1+ right now and our build system doesn't work
>>>>>> with that version.  And I don't have enough experience with
>>>>>> windows build in order to fix it...
>>>>>> 
>>>>>> Best regards, Ilya Maximets.
>>>>>> 
>>>>>>> 
>>>>>>> Alin.
>>>>>>> 
>>>>>>>> 
>>>>>>>> On 31 May 2023,

Re: [ovs-dev] [PATCH 2/2] appveyor: Silence the git clone of pthreads4w.

[Alin Serdean]

Thank you for the patch!

—
Alin.

> On 2 Jun 2023, at 12:24, Ilya Maximets  wrote:
> 
> On 5/31/23 22:29, Alin Serdean wrote:
>> Acked-by: Alin Gabriel Serdean 
> 
> Thanks!  I applied this one patch.
> 
> Best regards, Ilya Maximets.
> 
>> 
>>> 
>>>> On 31 May 2023, at 21:23, Ilya Maximets  wrote:
>>> 
>>> Git by default reports progress on stderr.  This doesn't fail
>>> the build, but upsets the powershell:
>>> 
>>> git : Cloning into 'c:\pthreads4w-code'...
>>> At line:3 char:1
>>> + git clone https://git.code.sf.net/p/pthreads4w/code c:\pthreads4w-cod ...
>>> + ~
>>>+ CategoryInfo  : NotSpecified:
>>>   (Cloning into 'c:\pthreads4w-code'...:String) [], RemoteException
>>>+ FullyQualifiedErrorId : NativeCommandError
>>> 
>>> Silence the git clone to avoid the warning.
>>> 
>>> Signed-off-by: Ilya Maximets 
>>> ---
>>> appveyor.yml | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/appveyor.yml b/appveyor.yml
>>> index 3287733b2..6bf5db8fd 100644
>>> --- a/appveyor.yml
>>> +++ b/appveyor.yml
>>> @@ -15,7 +15,7 @@ init:
>>> 
>>>cd C:\openvswitch
>>> 
>>> -git clone https://git.code.sf.net/p/pthreads4w/code c:\pthreads4w-code
>>> +git clone -q https://git.code.sf.net/p/pthreads4w/code 
>>> c:\pthreads4w-code
>>> 
>>>python3 -m pip install pypiwin32 --disable-pip-version-check
>>> 
>>> -- 
>>> 2.40.1
>>> 
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/2] appveyor: Don't download OpenSSL.

[Alin Serdean]

Ack. I will try to see if I can address it by the end of the week.

Thanks for clarifying the questions.

Would GH actions be better than appveyor?

—
Alin.

On 2 Jun 2023, at 12:25, Ilya Maximets  wrote:
> 
> On 6/1/23 13:50, Ilya Maximets wrote:
>>> On 5/31/23 23:05, Alin Serdean wrote:
>>> 
>>> That makes sense.
>>> 
>>> We can leverage the  following commit:
>>> 
>>> https://patchwork.ozlabs.org/project/openvswitch/patch/20201013124655.1408-1-aserd...@cloudbasesolutions.com/
>>>  
>>> <https://patchwork.ozlabs.org/project/openvswitch/patch/20201013124655.1408-1-aserd...@cloudbasesolutions.com/>
>>> 
>>> But I still need to fix the permissions. I’ll try to find some time and 
>>> address it.
>> 
>> Thanks.  I actually forgot about this patch.  Would be great if you can
>> send an updated version.  Would also be great to migrate to OpenSSL 3.0
>> instead of 1.1.1 to not migrate again in some not so distant future.
>> OpenSSL 3.0 should work fine, unless there are some other library changes
>> (not OpenSSL 3.1, because OpenSSL 3.1 is not an LTS release).
> 
> I'll mark  the current patch as 'rejected' for now in favor of the
> future OpenSSL update with your suggested change.
> 
>> 
>>> I remember there were some discussions to modernize the build system to 
>>> meson or cmake. Was that effort fruitful in the end?
>> 
>> The main issue with meson is that we still need automake/autotools for
>> our testsuite.  We might invoke autotest from meson, but it sounds a
>> bit strange to do that.  I agree that it is still beneficial in some
>> cases to use meson, e.g. for a windows build, so might make sense to
>> migrate anyway, but an attempt from 2021 didn't receive any follow ups.
>> 
>> There was also a PR to add Windows build to GitHub Actions, but it
>> didn't move since your request to Sign-off the changes.
>> 
>> Best regards, Ilya Maximets.
>> 
>>> 
>>> Alin.
>>> 
>>>> On 31 May 2023, at 22:41, Ilya Maximets  wrote:
>>>> 
>>>> On 5/31/23 22:36, Alin Serdean wrote:
>>>>> 
>>>>> It would be best to change the link with the latest version of OpenSSL. 
>>>>> That will ensure there are no mishaps .
>>>> 
>>>> I think the problem here is that slproweb.com only provides
>>>> OpenSSL 1.1.1+ right now and our build system doesn't work
>>>> with that version.  And I don't have enough experience with
>>>> windows build in order to fix it...
>>>> 
>>>> Best regards, Ilya Maximets.
>>>> 
>>>>> 
>>>>> Alin.
>>>>> 
>>>>>> 
>>>>>> On 31 May 2023, at 21:23, Ilya Maximets  wrote:
>>>>>> 
>>>>>> OpenSSL is already available in the exact location we need it [1].
>>>>>> Also, the download itself fails for a long time already, because
>>>>>> the version we're trying to download is not available.
>>>>>> 
>>>>>> [1] https://www.appveyor.com/docs/windows-images-software/#tools
>>>>>> 
>>>>>> Signed-off-by: Ilya Maximets 
>>>>>> ---
>>>>>> appveyor.yml | 14 --
>>>>>> 1 file changed, 14 deletions(-)
>>>>>> 
>>>>>> diff --git a/appveyor.yml b/appveyor.yml
>>>>>> index 25c3f69fb..3287733b2 100644
>>>>>> --- a/appveyor.yml
>>>>>> +++ b/appveyor.yml
>>>>>> @@ -11,22 +11,8 @@ init:
>>>>>> - ps: $env:PATH ="C:\Python37;"+$env:PATH
>>>>>> - ps: New-Item -Type HardLink -Path "C:\Python37\python3.exe" -Value 
>>>>>> "C:\Python37\python.exe"
>>>>>> - ps: >-
>>>>>> -mkdir C:\ovs-build-downloads
>>>>>> -
>>>>>>mkdir C:\openvswitch\driver
>>>>>> 
>>>>>> -$source = "https://slproweb.com/download/Win64OpenSSL-1_0_2u.exe;
>>>>>> -
>>>>>> -$destination = "C:\ovs-build-downloads\Win64OpenSSL-1_0_2u.exe"
>>>>>> -
>>>>>> -Invoke-WebRequest $source -OutFile $destination
>>>>>> -
>>>>>> -cd C:\ovs-build-downloads
>>>>>> -
>>>>>> -.\Win64OpenSSL-1_0_2u.exe /silent /verysilent /sp- /suppressmsgboxes
>>>>>> -
>>>>>> -Start-Sleep -s 30
>>>>>> -
>>>>>>cd C:\openvswitch
>>>>>> 
>>>>>>git clone https://git.code.sf.net/p/pthreads4w/code c:\pthreads4w-code
>>>>>> -- 
>>>>>> 2.40.1
>>>>>> 
>>>> 
>> 
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/2] appveyor: Don't download OpenSSL.

[Alin Serdean]

That makes sense.

We can leverage the  following commit:

https://patchwork.ozlabs.org/project/openvswitch/patch/20201013124655.1408-1-aserd...@cloudbasesolutions.com/

But I still need to fix the permissions. I’ll try to find some time and address 
it.

I remember there were some discussions to modernize the build system to meson 
or cmake. Was that effort fruitful in the end?

Alin.

> On 31 May 2023, at 22:41, Ilya Maximets  wrote:
> 
> On 5/31/23 22:36, Alin Serdean wrote:
>> 
>> It would be best to change the link with the latest version of OpenSSL. That 
>> will ensure there are no mishaps .
> 
> I think the problem here is that slproweb.com only provides
> OpenSSL 1.1.1+ right now and our build system doesn't work
> with that version.  And I don't have enough experience with
> windows build in order to fix it...
> 
> Best regards, Ilya Maximets.
> 
>> 
>> Alin.
>> 
>>> 
>>>> On 31 May 2023, at 21:23, Ilya Maximets  wrote:
>>> 
>>> OpenSSL is already available in the exact location we need it [1].
>>> Also, the download itself fails for a long time already, because
>>> the version we're trying to download is not available.
>>> 
>>> [1] https://www.appveyor.com/docs/windows-images-software/#tools
>>> 
>>> Signed-off-by: Ilya Maximets 
>>> ---
>>> appveyor.yml | 14 --
>>> 1 file changed, 14 deletions(-)
>>> 
>>> diff --git a/appveyor.yml b/appveyor.yml
>>> index 25c3f69fb..3287733b2 100644
>>> --- a/appveyor.yml
>>> +++ b/appveyor.yml
>>> @@ -11,22 +11,8 @@ init:
>>> - ps: $env:PATH ="C:\Python37;"+$env:PATH
>>> - ps: New-Item -Type HardLink -Path "C:\Python37\python3.exe" -Value 
>>> "C:\Python37\python.exe"
>>> - ps: >-
>>> -mkdir C:\ovs-build-downloads
>>> -
>>>mkdir C:\openvswitch\driver
>>> 
>>> -$source = "https://slproweb.com/download/Win64OpenSSL-1_0_2u.exe;
>>> -
>>> -$destination = "C:\ovs-build-downloads\Win64OpenSSL-1_0_2u.exe"
>>> -
>>> -Invoke-WebRequest $source -OutFile $destination
>>> -
>>> -cd C:\ovs-build-downloads
>>> -
>>> -.\Win64OpenSSL-1_0_2u.exe /silent /verysilent /sp- /suppressmsgboxes
>>> -
>>> -Start-Sleep -s 30
>>> -
>>>cd C:\openvswitch
>>> 
>>>git clone https://git.code.sf.net/p/pthreads4w/code c:\pthreads4w-code
>>> -- 
>>> 2.40.1
>>> 
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/2] appveyor: Don't download OpenSSL.

[Alin Serdean]

It would be best to change the link with the latest version of OpenSSL. That 
will ensure there are no mishaps .

Alin.

> 
> On 31 May 2023, at 21:23, Ilya Maximets  wrote:
> 
> OpenSSL is already available in the exact location we need it [1].
> Also, the download itself fails for a long time already, because
> the version we're trying to download is not available.
> 
> [1] https://www.appveyor.com/docs/windows-images-software/#tools
> 
> Signed-off-by: Ilya Maximets 
> ---
> appveyor.yml | 14 --
> 1 file changed, 14 deletions(-)
> 
> diff --git a/appveyor.yml b/appveyor.yml
> index 25c3f69fb..3287733b2 100644
> --- a/appveyor.yml
> +++ b/appveyor.yml
> @@ -11,22 +11,8 @@ init:
> - ps: $env:PATH ="C:\Python37;"+$env:PATH
> - ps: New-Item -Type HardLink -Path "C:\Python37\python3.exe" -Value 
> "C:\Python37\python.exe"
> - ps: >-
> -mkdir C:\ovs-build-downloads
> -
> mkdir C:\openvswitch\driver
> 
> -$source = "https://slproweb.com/download/Win64OpenSSL-1_0_2u.exe;
> -
> -$destination = "C:\ovs-build-downloads\Win64OpenSSL-1_0_2u.exe"
> -
> -Invoke-WebRequest $source -OutFile $destination
> -
> -cd C:\ovs-build-downloads
> -
> -.\Win64OpenSSL-1_0_2u.exe /silent /verysilent /sp- /suppressmsgboxes
> -
> -Start-Sleep -s 30
> -
> cd C:\openvswitch
> 
> git clone https://git.code.sf.net/p/pthreads4w/code c:\pthreads4w-code
> -- 
> 2.40.1
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/2] appveyor: Silence the git clone of pthreads4w.

[Alin Serdean]

Acked-by: Alin Gabriel Serdean 

> 
> On 31 May 2023, at 21:23, Ilya Maximets  wrote:
> 
> Git by default reports progress on stderr.  This doesn't fail
> the build, but upsets the powershell:
> 
> git : Cloning into 'c:\pthreads4w-code'...
> At line:3 char:1
> + git clone https://git.code.sf.net/p/pthreads4w/code c:\pthreads4w-cod ...
> + ~
> + CategoryInfo  : NotSpecified:
>(Cloning into 'c:\pthreads4w-code'...:String) [], RemoteException
> + FullyQualifiedErrorId : NativeCommandError
> 
> Silence the git clone to avoid the warning.
> 
> Signed-off-by: Ilya Maximets 
> ---
> appveyor.yml | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/appveyor.yml b/appveyor.yml
> index 3287733b2..6bf5db8fd 100644
> --- a/appveyor.yml
> +++ b/appveyor.yml
> @@ -15,7 +15,7 @@ init:
> 
> cd C:\openvswitch
> 
> -git clone https://git.code.sf.net/p/pthreads4w/code c:\pthreads4w-code
> +git clone -q https://git.code.sf.net/p/pthreads4w/code c:\pthreads4w-code
> 
> python3 -m pip install pypiwin32 --disable-pip-version-check
> 
> -- 
> 2.40.1
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] MAINTAINERS.rst: Move several people to emeritus status

[Alin Serdean]

Acked-by: Alin Gabriel Serdean 


> 
> On 19 May 2023, at 16:53, Russell Bryant  wrote:
> 
> The following document discusses emeritus committer status:
> 
> https://docs.openvswitch.org/en/latest/internals/committer-emeritus-status/
> 
> There are several people who I would guess consider themselves
> emeritus committers but have not formally declared it. Those moved to
> emeritus status in this commit have either explicitly communicated
> their desire to move or have both not been active in the last year and
> have not yet replied to this patch.
> 
> It is easy to re-add people in the future should any emeritus
> committer desire to become active again.
> 
> Per our policies, a vote of the majority of current committers (or
> the list of maintainers prior to this change) is required to move a
> committer to emeritus status.
> 
> Signed-off-by: Russell Bryant 
> CC: Alin Serdean 
> CC: Andy Zhou 
> CC: Ansis Atteka 
> CC: Daniele Di Proietto 
> CC: Gurucharan Shetty 
> CC: Ian Stokes 
> CC: Ilya Maximets 
> CC: Jarno Rajahalme 
> CC: Jesse Gross 
> CC: Justin Pettit 
> CC: Pravin B Shelar 
> CC: Simon Horman 
> CC: Thomas Graf 
> CC: William Tu 
> CC: YAMAMOTO Takashi 
> ---
> MAINTAINERS.rst | 42 +-
> 1 file changed, 21 insertions(+), 21 deletions(-)
> 
> diff --git a/MAINTAINERS.rst b/MAINTAINERS.rst
> index 1dc406170..85b8e6416 100644
> --- a/MAINTAINERS.rst
> +++ b/MAINTAINERS.rst
> @@ -41,40 +41,20 @@ This is the current list of active Open vSwitch 
> committers:
> 
>* - Name
>  - Email
> -   * - Alex Wang
> - - ee07b...@gmail.com
>* - Alin Serdean
>  - aserd...@ovn.org
> -   * - Andy Zhou
> - - az...@ovn.org
>* - Ansis Atteka
> - - aatt...@nicira.com
> -   * - Daniele Di Proietto
> - - daniele.di.proie...@gmail.com
> -   * - Gurucharan Shetty
> - - g...@ovn.org
> + - ansisatt...@gmail.com
>* - Ian Stokes
>  - isto...@ovn.org
>* - Ilya Maximets
>  - i.maxim...@ovn.org
> -   * - Jarno Rajahalme
> - - ja...@ovn.org
> -   * - Jesse Gross
> - - je...@kernel.org
> -   * - Justin Pettit
> - - jpet...@ovn.org
> -   * - Pravin B Shelar
> - - pshe...@ovn.org
>* - Russell Bryant
>  - russ...@ovn.org
>* - Simon Horman
>  - ho...@ovn.org
> -   * - Thomas Graf
> - - tg...@noironetworks.com
>* - William Tu
>  - u9012...@gmail.com
> -   * - YAMAMOTO Takashi
> - - yamam...@midokura.com
> 
> The project also maintains a list of Emeritus Committers (or Maintainers).
> More information about Emeritus Committers can be found here:
> @@ -85,12 +65,32 @@ More information about Emeritus Committers can be found 
> here:
> 
>* - Name
>  - Email
> +   * - Alex Wang
> + - ee07b...@gmail.com
> +   * - Andy Zhou
> + - az...@ovn.org
>* - Ben Pfaff
>  - b...@ovn.org
> +   * - Daniele Di Proietto
> + - daniele.di.proie...@gmail.com
>* - Ethan J. Jackson
>  - e...@eecs.berkeley.edu
> +   * - Gurucharan Shetty
> + - g...@ovn.org
> +   * - Jarno Rajahalme
> + - ja...@ovn.org
> +   * - Jesse Gross
> + - je...@kernel.org
>* - Joe Stringer
>  - j...@ovn.org
> +   * - Justin Pettit
> + - jpet...@ovn.org
> +   * - Pravin B Shelar
> + - pshe...@ovn.org
> +   * - Thomas Graf
> + - tg...@tgraf.ch
> +   * - YAMAMOTO Takashi
> + - yamam...@midokura.com
> 
> .. Cut here for the Documentation/internals/maintainers.rst
> 
> -- 
> 2.40.1
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] MAINTAINERS.rst: Make myself an active maintainer

[Alin Serdean]

Acked-by: Alin Gabriel Serdean 


> 
> On 18 May 2023, at 15:40, Russell Bryant  wrote:
> 
> I am currently an emeritus committer, but I would like to become
> active again for a short period of time to work through some
> governance issues preventing us from updating our committers list
> following our approved policies for doing so.
> 
> Signed-off-by: Russell Bryant 
> ---
> MAINTAINERS.rst | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/MAINTAINERS.rst b/MAINTAINERS.rst
> index 5df9aab78..1dc406170 100644
> --- a/MAINTAINERS.rst
> +++ b/MAINTAINERS.rst
> @@ -65,6 +65,8 @@ This is the current list of active Open vSwitch committers:
>  - jpet...@ovn.org
>* - Pravin B Shelar
>  - pshe...@ovn.org
> +   * - Russell Bryant
> + - russ...@ovn.org
>* - Simon Horman
>  - ho...@ovn.org
>* - Thomas Graf
> @@ -89,8 +91,6 @@ More information about Emeritus Committers can be found 
> here:
>  - e...@eecs.berkeley.edu
>* - Joe Stringer
>  - j...@ovn.org
> -   * - Russell Bryant
> - - russ...@ovn.org
> 
> .. Cut here for the Documentation/internals/maintainers.rst
> 
> -- 
> 2.40.1
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v1 1/1] netdev-windows: Add checking when creating netdev with system type on Windows

[Alin Serdean]

Applied on master and backported until 2.9

Thank you!

> On 9 Nov 2022, at 04:32, Wilson Peng  wrote:
> 
> From: Wilson Peng 
> 
> In the recent Antrea project testing, some port could not be created
> on Windows.
> 
> When doing debug, our team found there is one case happening when multiple
> ports are waiting for be created with correct port number.
> 
> Some system type port will be created netdev successfully and it will cause
> conflict as in the dpif side it will be internal type. So finally the port
> will be created failed and it could not be easily recovered.
> 
> With the patch, on Windows the netdev creating will be blocked for system
> type when the ovs_tyep got on dpif is internal. More detailed case description
> is in the reported issue No.262 with link below.
> 
> Reported-at:https://github.com/openvswitch/ovs-issues/issues/262
> Signed-off-by: Wilson Peng 
> ---
> lib/netdev-windows.c | 11 +++
> 1 file changed, 11 insertions(+)
> 
> diff --git a/lib/netdev-windows.c b/lib/netdev-windows.c
> index 4ad45ffa1..3fad501e3 100644
> --- a/lib/netdev-windows.c
> +++ b/lib/netdev-windows.c
> @@ -156,6 +156,7 @@ netdev_windows_system_construct(struct netdev *netdev_)
> struct netdev_windows_netdev_info info;
> struct ofpbuf *buf;
> int ret;
> +const char*type = NULL;
> 
> /* Query the attributes and runtime status of the netdev. */
> ret = query_netdev(netdev_get_name(>up), , );
> @@ -167,6 +168,16 @@ netdev_windows_system_construct(struct netdev *netdev_)
> }
> ofpbuf_delete(buf);
> 
> +/* Don't create netdev if ovs-type is "internal"
> + * but the type of netdev->up is "system". */
> +type = netdev_get_type(>up);
> +if (type && !strcmp(type, "system") &&
> +(info.ovs_type == OVS_VPORT_TYPE_INTERNAL)) {
> +VLOG_DBG("construct device %s, ovs_type: %u failed",
> + netdev_get_name(>up), info.ovs_type);
> +return 1;
> +}
> +
> netdev->change_seq = 1;
> netdev->dev_type = info.ovs_type;
> netdev->port_no = info.port_no;
> -- 
> 2.32.1 (Apple Git-133)
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] [RFC] [ovn] northd: Fix IGMP external subscriber subscribing to internal feed

[Alin Serdean]

> On 24 May 2022, at 00:15, Dumitru Ceara  wrote:
> 
> On 5/3/22 17:15, Alin-Gabriel Serdean wrote:
>> Add localnet ports to their corresponding southbound multicast group.
>> 
>> Update unit tests.
>> 
>> Reported-at: https://github.com/ovn-org/ovn/issues/125
>> Reported-by: Diko Parvanov 
>> Suggested-by: Dumitru Ceara 
>> Signed-off-by: Alin-Gabriel Serdean 
>> ---
> 
> Hi Alin,
> 
> Thanks for the patch!

Hi Dumitru,

Thanks for the review!
> 
>> northd/northd.c | 17 ++---
>> tests/ovn.at| 15 ++-
>> 2 files changed, 12 insertions(+), 20 deletions(-)
>> 
>> diff --git a/northd/northd.c b/northd/northd.c
>> index a5297..4441ef631 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -4591,13 +4591,6 @@ ovn_igmp_group_aggregate_ports(struct ovn_igmp_group 
>> *igmp_group,
>> ovn_igmp_group_destroy_entry(entry);
>> free(entry);
>> }
>> -
>> -if (igmp_group->datapath->n_localnet_ports) {
>> -ovn_multicast_add_ports(mcast_groups, igmp_group->datapath,
>> -_group->mcgroup,
>> -igmp_group->datapath->localnet_ports,
>> -igmp_group->datapath->n_localnet_ports);
>> -}
>> }
>> 
>> static void
>> @@ -15066,6 +15059,16 @@ build_mcast_groups(struct lflow_input *input_data,
>> 
>> /* Add the extracted ports to the IGMP group. */
>> ovn_igmp_group_add_entry(igmp_group, igmp_ports, n_igmp_ports);
>> +if (!ovn_igmp_group_allocate_id(igmp_group)) {
>> +ovn_igmp_group_destroy(igmp_groups, igmp_group);
>> +continue;
>> +}
>> +if (igmp_group->datapath->n_localnet_ports) {
>> +ovn_multicast_add_ports(mcast_groups, igmp_group->datapath,
>> +_group->mcgroup,
>> +igmp_group->datapath->localnet_ports,
>> +igmp_group->datapath->n_localnet_ports);
>> +}
> 
> I'm not sure I understand how this changes things.  After looking at the
> main branch code more closely, we already include all localnet ports in
> the multicast_group generated for an IGMP group.  We just do it at the
> end of build_mcast_groups(), in ovn_igmp_group_aggregate_ports() which
> is called once for every IGMP Group learnt in a logical datapath.

Indeed I got confused between branches and I didn’t saw the 
ovn_igmp_group_aggregate_ports.

>> }
>> 
>> /* Build IGMP groups for multicast routers with relay enabled. The router
>> diff --git a/tests/ovn.at b/tests/ovn.at
>> index 34b6abfc0..5440639a8 100644
>> --- a/tests/ovn.at
>> +++ b/tests/ovn.at
>> @@ -19570,7 +19570,8 @@ ovn-nbctl lsp-add sw3 sw3-p1
>> ovn-nbctl lsp-add sw3 sw3-p2
>> ovn-nbctl lsp-add sw3 sw3-ln\
>> -- lsp-set-type sw3-ln localnet \
>> --- lsp-set-options sw3-ln network_name=phys
>> +-- lsp-set-options sw3-ln network_name=phys \
>> +-- lsp-set-options sw3-ln mcast_flood=true
> 
> This threw me off track for a bit.. "lsp-set-options  "
> will actually overwrite all options of  with .  Which
> most likely is not what we want here because we would be removing the
> network_name, essentially disconnecting the localnet port.
> 
> If I understand the intention correctly, you should probably use:
> 
> -- lsp-set-options sw3-ln network_name=phys mcast_flood=true

You are correct. This actually made me think I was changing things.

> 
>> 
>> ovn-nbctl lr-add rtr
>> ovn-nbctl lrp-add rtr rtr-sw1 00:00:00:00:01:00 10.0.0.254/24
>> @@ -19985,18 +19986,6 @@ store_ip_multicast_pkt \
>> $(ip_to_hex 10 0 0 42) $(ip_to_hex 239 0 1 68) 1e 20 ca70 11 \
>> e518e518000a3b3a expected_switched
>> 
>> -# TODO: IGMP Relay duplicates IP multicast packets in some conditions, for
>> -# more details see TODO.rst, section "IP Multicast Relay". Once that issue 
>> is
>> -# fixed the duplicated packets should not appear anymore.
>> -store_ip_multicast_pkt \
>> -0100 01005e000144 \
>> -$(ip_to_hex 10 0 0 42) $(ip_to_hex 239 0 1 68) 1e 1f cb70 11 \
>> -e518e518000a3b3a expected_routed_sw1
>> -store_ip_multicast_pkt \
>> -0200 01005e000144 \
>> -$(ip_to_hex 10 0 0 42) $(ip_to_hex 239 0 1 68) 1e 1f cb70 11 \
>> -e518e518000a3b3a expected_routed_sw2
>> -
> 
> After changing the test to properly set mcast_flood=true on the localnet
> port, it starts failing due to the duplicated routed packets (the TODO
> above).  There was no logical change in northd.c as far as I can tell,
> so that makes sense.
> 
>> OVS_WAIT_UNTIL(
>>   [check_packets 'hv1/vif1-tx.pcap expected_routed_sw1' \
>>  'hv2/vif3-tx.pcap expected_routed_sw2' \
> 
> Then I tried to replicate the issue Diko reported but I couldn't.  The
> databases attached to the GitHub issue are truncated, I get:
> 
> ovsdb-tool: syntax error: ovnnb_db.db: 731669 bytes starting at offset
> 62 have 

Re: [ovs-dev] [PATCH v2] windows: Bump OpenSSL version

[Alin Serdean]

From: Ilya Maximets<mailto:i.maxim...@ovn.org>
Sent: Wednesday, October 21, 2020 6:50 PM
To: Alin Serdean<mailto:aserd...@cloudbasesolutions.com>; 
d...@openvswitch.org<mailto:d...@openvswitch.org>
Cc: i.maxim...@ovn.org<mailto:i.maxim...@ovn.org>; Mark 
Gray<mailto:mark.d.g...@redhat.com>
Subject: Re: [PATCH v2] windows: Bump OpenSSL version

On 10/13/20 2:46 PM, Alin Gabriel Serdean wrote:
> Switch from OpenSSL 1.0.2 to 1.1.1.
>
> `mkdir` does not support permission arguments on Windows. Create a wrapper
> for it that uses only the last argument and uses `-p` option.

Does chmod work fine on windows?
Maybe it's better to replace 'mkdir -m' invocations with pairs
of 'mkdir'+'chmod'?  It seems important to have correct permissions
on certificates and keys.

[Alin] Thank you for the review!
Unfortunately chmod doesn’t work as expected on Windows.
Thank you for raising the concern, I will switch it to `mkdir`+`icacls`
to make sure only the right permissions are set.
--
Alin.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v4 1/2] conntrack: Rename "master" connection to "parent" connection.

[Alin Serdean]

From: Ben Pfaff<mailto:b...@ovn.org>
Sent: Saturday, October 17, 2020 7:36 AM
To: d...@openvswitch.org<mailto:d...@openvswitch.org>
Cc: Ben Pfaff<mailto:b...@ovn.org>; Alin 
Serdean<mailto:aserd...@cloudbasesolutions.com>
Subject: [PATCH v4 1/2] conntrack: Rename "master" connection to "parent" 
connection.

Signed-off-by: Ben Pfaff 
---
Acked-by: Alin Gabriel Serdean 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v4 2/2] Eliminate use of term "slave" in bond, LACP, and bundle contexts.

[Alin Serdean]

From: Ben Pfaff<mailto:b...@ovn.org>
Sent: Saturday, October 17, 2020 7:36 AM
To: d...@openvswitch.org<mailto:d...@openvswitch.org>
Cc: Ben Pfaff<mailto:b...@ovn.org>; Alin 
Serdean<mailto:aserd...@cloudbasesolutions.com>
Subject: [PATCH v4 2/2] Eliminate use of term "slave" in bond, LACP, and bundle 
contexts.

Most of these changes should not change user-visible behavior.  One
place where they do is in "ovs-ofctl dump-flows", which will now output
"subs:..." inside "bundle" actions instead of "slaves:...".  I don't
expect this to cause real problems in most systems.  The old syntax
is still supported on input for backward compatibility.

Signed-off-by: Ben Pfaff 
---
Acked-by: Alin Gabriel Serdean 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] bond: Fix using uninitialized 'lacp_fallback_ab_cfg' for 'bond-primary'.

[Alin Serdean]

Acked-by: Alin Gabriel Serdean 
aserd...@cloudbasesolutions.com


From: Ilya Maximets
Sent: Tuesday, October 13, 2020 1:02 PM
To: ovs-dev@openvswitch.org; Jeff 
Squyres
Cc: Flavio Leitner; Ilya 
Maximets
Subject: [ovs-dev] [PATCH] bond: Fix using uninitialized 'lacp_fallback_ab_cfg' 
for 'bond-primary'.

's->lacp_fallback_ab_cfg' initialized down below in the code, so
we're using it uninitialized to detect if we need to get 'bond-primary'
configuration.

Found by valgrind:

 Conditional jump or move depends on uninitialised value(s)
at 0x409114: port_configure_bond (bridge.c:4569)
by 0x409114: port_configure (bridge.c:1284)
by 0x40F6E6: bridge_reconfigure (bridge.c:917)
by 0x411425: bridge_run (bridge.c:3330)
by 0x406D84: main (ovs-vswitchd.c:127)
  Uninitialised value was created by a stack allocation
at 0x408C53: port_configure (bridge.c:1190)

Fix that by moving this code to the point where 'lacp_fallback_ab_cfg'
already initialized.  Additionally clarified behavior of 'bond-primary'
in manpages for the fallback to AB case.

Fixes: b4e50218a0f8 ("bond: Add 'primary' interface concept for active-backup 
mode.")
Signed-off-by: Ilya Maximets 
---
 vswitchd/bridge.c| 9 -
 vswitchd/vswitch.xml | 5 -
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index a3e7facd3..a332517bc 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -4565,11 +4565,6 @@ port_configure_bond(struct port *port, struct 
bond_settings *s)
   port->name);
 }

-s->primary = NULL;
-if (s->balance == BM_AB || s->lacp_fallback_ab_cfg) {
-s->primary = smap_get(>cfg->other_config, "bond-primary");
-}
-
 miimon_interval = smap_get_int(>cfg->other_config,
"bond-miimon-interval", 0);
 if (miimon_interval <= 0) {
@@ -4596,6 +4591,10 @@ port_configure_bond(struct port *port, struct 
bond_settings *s)

 s->lacp_fallback_ab_cfg = smap_get_bool(>cfg->other_config,
"lacp-fallback-ab", false);
+s->primary = NULL;
+if (s->balance == BM_AB || s->lacp_fallback_ab_cfg) {
+s->primary = smap_get(>cfg->other_config, "bond-primary");
+}

 LIST_FOR_EACH (iface, port_elem, >ifaces) {
 netdev_set_miimon_interval(iface->netdev, miimon_interval);
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index 07da2ee8c..20decb2db 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -2008,7 +2008,10 @@
 If a slave interface with this name exists in the bond and
 is up, it will be made active.  Relevant only when  is
-active-backup.
+active-backup or if balance-tcp falls back
+to active-backup (e.g. LACP negotiation fails and
+ is
+true).
   

   
--
2.25.4

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2 2/8] windows: Add default value for VSTUDIO_CONFIG

[Alin Serdean]

On 9/21/20 3:32 AM, Alin Gabriel Serdean wrote:
> VSTUDIO_CONFIG is used when generating the windows installer.
>
> If the parameter passed to configure `--with-vstudiotarget` is not specified
> to configure we default it to `Default`.
>
> Fixes bug: vstudiotarget/vstudiotargetver should be available only on Windows.
>
> Signed-off-by: Alin Gabriel Serdean 
> ---
>  m4/openvswitch.m4 | 93 ---
>  1 file changed, 47 insertions(+), 46 deletions(-)


Hi, Alin.  Thanks for the series!

However it still breaks the linux build with the following error:

  configure: error: conditional "VSTUDIO_WIN8" was never defined.

  Usually this means the macro was only invoked conditionally.

Details here: https://travis-ci.org/github/ovsrobot/ovs/builds/728856195
[Alin] Ooops. Thanks a lot for letting me know.
I sent a  new series here:
https://patchwork.ozlabs.org/project/openvswitch/list/?series=203845

Also I sent a PR to check if CIs are ok:
https://github.com/openvswitch/ovs/pull/338
Hopefully everything is ok this time.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] windows, tests: Strip EOL characters when passing them to tasklist

[Alin Serdean]

On 9/23/20 12:39 PM, Alin Gabriel Serdean wrote:
> When running OVSDB cluster tests on Windows not all the ovsdb
> processes are terminated.
> Strip carriage return and newline of the arguments passed to the kill
> command because they will cause problems when passing them to tasklist
> and taskkill.
>
> Signed-off-by: Alin Gabriel Serdean 
> ---

Thanks!  I didn't test this patch, but it looks fine.

If it fixes the issue for you:
Acked-by: Ilya Maximets 

[Alin] Thank you. I applied it on master.

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] windows, tests: Modify service test

[Alin Serdean]

Thanks a lot for the review!

It’s been some time since I proposed the patch, I think I wanted to make sure
the service was deleted, but I agree with your suggestions.
I sent an incremental here:
https://patchwork.ozlabs.org/project/openvswitch/patch/20200923112247.738-1-aserd...@ovn.org/


Alin.

From: Ilya Maximets
Sent: Thursday, September 10, 2020 3:07 PM
To: Alin Gabriel Serdean; 
d...@openvswitch.org
Cc: i.maxim...@ovn.org
Subject: Re: [ovs-dev] [PATCH] windows, tests: Modify service test

On 7/24/19 4:42 PM, Alin Gabriel Serdean wrote:
> The database is now called "_Server" so look for that instead of 
> "Open_vSwitch".
>
> Modify the test so it always stops and deletes the service.
>
> Signed-off-by: Alin Gabriel Serdean 
> ---

Hi.  Thanks for the fix.

It looks OK to me in general, but it seems like it was never accepted or even 
reviewed.

See one comment inline.

Best regards, Ilya Maximets.

>  tests/daemon.at | 12 
>  1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/tests/daemon.at b/tests/daemon.at
> index bdc8910f9..ff1953633 100644
> --- a/tests/daemon.at
> +++ b/tests/daemon.at
> @@ -212,17 +212,13 @@ abs_path="$(cd $(dirname `which ovsdb-server`); pwd -W; 
> cd $OLDPWD)"
>  AT_CHECK([sc create ovsdb-server binpath="$abs_path/ovsdb-server --no-db 
> --log-file=`pwd`/ovsdb-server.log --pidfile=`pwd`/ovsdb-server.pid 
> --unixctl=`pwd`/ovsdb-server.ctl --remote=punix:`pwd`/socket --service"],
>  [0], [[[SC]] CreateService SUCCESS
>  ])
> -
> -AT_CHECK([sc start ovsdb-server], [0], [ignore], [ignore], [sc delete 
> ovsdb-server])
> -OVS_WAIT_UNTIL([test -s ovsdb-server.pid])
> +on_exit 'sc delete ovsdb-server'
> +on_exit 'sc stop ovsdb-server'
> +AT_CHECK([sc start ovsdb-server], [0], [ignore])
>  OVS_WAIT_UNTIL([sc query ovsdb-server | grep STATE | grep RUNNING > 
> /dev/null 2>&1])
> -AT_CHECK([kill -0 `cat ovsdb-server.pid`], [0], [ignore])
>  AT_CHECK([ovs-appctl -t ovsdb-server ovsdb-server/list-dbs], [0],
> -[Open_vSwitch
> +[_Server
>  ])
>  AT_CHECK([sc stop ovsdb-server], [0], [ignore])
>  OVS_WAIT_UNTIL([test ! -s ovsdb-server.pid])
> -AT_CHECK([sc query ovsdb-server | grep STATE | grep STOPPED], [0], [ignore])
> -AT_CHECK([sc delete ovsdb-server], [0], [[[SC]] DeleteService SUCCESS
> -])

I'd like to keep these explicit checks for 'STOPPED' state and deletion success.
I see that we have 'on_exit' now, but this test was designed to test graceful
service start/stop/delete and code executed during cleanup will not check for
any issues.  So, I think, that we need to test graseful termination and have
'on_exit' hooks just in case some part of the test failed early.
BTW, It might make sense to replace AT_CHECK with OVS_WAIT_UNTIL for checking
the 'STOPPED' state, the same way as it done for 'RUNNING'.

What do you think?

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] tests: Queue for termination all OVSDB IDL pids

[Alin Serdean]

>
> I posted a patch to strip the line endings here:
>
> https://patchwork.ozlabs.org/project/openvswitch/patch/20200923103955.1694-1-aserd...@ovn.org/

Thanks!  I'll take a look.

But anyway, patch below seems still valid (with my comments addressed) just 
because
if one of ovsdb-servers will fail to start for any reason other servers will not
be killed.  So, If you could update the patch and send v2 it would be good.

Best regards, Ilya Maximets.

[Alin] Of course! I sent an incremental here:
https://patchwork.ozlabs.org/project/openvswitch/patch/20200923112247.738-1-aserd...@ovn.org/
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] tests: Queue for termination all OVSDB IDL pids

[Alin Serdean]

Hi Ilya,

Thank you so much for the review!

You are right the PID in double quotes was not being propagated properly.

I digged deeper to problem and the issue was actually a carriage return
which is passed to the kill wrapper we use on Windows.

This will mess up the search string we use here:
https://github.com/openvswitch/ovs/blob/master/tests/ovs-macros.at#L114

I posted a patch to strip the line endings here:
https://patchwork.ozlabs.org/project/openvswitch/patch/20200923103955.1694-1-aserd...@ovn.org/


Thanks,
Alin.

From: Ilya Maximets
Sent: Thursday, September 10, 2020 2:55 PM
To: Alin Gabriel Serdean; 
d...@openvswitch.org
Cc: i.maxim...@ovn.org
Subject: Re: [ovs-dev] [PATCH] tests: Queue for termination all OVSDB IDL pids

On 7/24/19 4:32 PM, Alin Gabriel Serdean wrote:
> When running OVSDB cluster tests on Windows not all the ovsdb processes are 
> terminated.
> Queue up the pids of the started processes for termination when the test 
> stops.
>
> Signed-off-by: Alin Gabriel Serdean 
> ---

Hi, Alin.  Thanks for the fix!
I'm looking through old patches in patchwork and this one seems to be never
reviewed, but it targets a valid issue.

See my review comments inline.

>  tests/ovsdb-idl.at | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
> index 7c937f742..fd9e973cd 100644
> --- a/tests/ovsdb-idl.at
> +++ b/tests/ovsdb-idl.at
> @@ -27,8 +27,8 @@ ovsdb_cluster_start_idltest () {
> done
> for i in `seq $n`; do
>   ovsdb-server -vraft -vconsole:warn --detach --no-chdir 
> --log-file=s$i.log --pidfile=s$i.pid --unixctl=s$i --remote=punix:s$i.ovsdb 
> ${2:+--remote=$2} s$i.db || return $?
> + on_exit "kill `cat s$i.pid`"

There is one issue with this solution.  Previously expression in a single
quotes was evaluated at the cleanup time, but changing it to double quotes
makes the shell to process expression right away.  It's possible that
pidfile is not exist yet at this point or not filled with correct pid and
resulted cleanup code will not kill the process in this case.

2 options here:
1. Wait for the pidfile.
2. Move previous "on_exit 'kill `cat s*.pid`'" before the loop.

Option #2 is preferred since it will not add any extra time cost on waiting
for the pidfile and actually covers all possible cases.  It will work just
fine because actual expansion of the * and actual 'cat' are executed during
cleanup.

Also, this patch needs a rebase.

Would you mind implementing option #2 on top of current master and send v2?

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/2] Documentation, windows: Document how to generate the Windows installer

[Alin Serdean]

Thank you for the review!

I addressed the comments and sent a new patchset:
https://patchwork.ozlabs.org/project/openvswitch/list/?series=203406

On 9/8/20 4:04 AM, Alin Gabriel Serdean wrote:
> This patch adds information on how to generate the Windows installer
> which can be used to easily deploy the userspace binaries, kernel module
> and create services on new environments.
>
> Signed-off-by: Alin Gabriel Serdean 
> ---
> v2: Change line endings at 79 characters
> ---
>  Documentation/intro/install/windows.rst | 31 +++--
>  1 file changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/intro/install/windows.rst 
> b/Documentation/intro/install/windows.rst
> index 394572f00..caa8dd062 100644
> --- a/Documentation/intro/install/windows.rst
> +++ b/Documentation/intro/install/windows.rst
> @@ -71,7 +71,10 @@ The following explains the steps in some detail.
>
>You will need at least Visual Studio 2013 (update 4) to compile userspace
>binaries.  In addition to that, if you want to compile the kernel module 
> you
> -  will also need to install Windows Driver Kit (WDK) 8.1 Update.
> +  will also need to install Windows Driver Kit (WDK) 8.1 Update or later.
> +  To generate the Windows installer you need
> +  `WiX Toolset `__ and also be able to build the
> +  kernel module.
>
>It is important to get the Visual Studio related environment variables and 
> to
>have the $PATH inside the bash to point to the proper compiler and linker.
> @@ -319,6 +322,21 @@ An alternative way to do the same is to run the 
> following command:
> seconds has been observed for the change to be reflected in the UI.  This 
> is
> not a bug in Open vSwitch.
>
> +Generate the Windows installer
> +~~
> +
> +To generate the Windows installler run the following command from the top
> +source directory:
> +
> +::
> +
> +   $ make windows_installer
> +
> +.. note::

It might be good to have an empty line here.

> +   This will generate the Windows installer in the following location 
> (relative
> +   to the top source directory):
> +   windows/ovs-windows-installer/bin/Release/OpenvSwitch.msi
> +
>  Starting
>  
>
> @@ -785,10 +803,10 @@ Windows CI Service
>  --
>
>  `AppVeyor http://www.appveyor.com>>`__ provides a free 
> Windows autobuild service for
> -open source projects.  Open vSwitch has integration with AppVeyor for 
> continuous
> -build.  A developer can build test his changes for Windows by logging into
> -appveyor.com using a github account, creating a new project by linking it to
> -his development repository in github and triggering a new build.
> +open source projects.  Open vSwitch has integration with AppVeyor for
> +continuous build.  A developer can build test his changes for Windows by
> +logging into appveyor.com using a github account, creating a new project by
> +linking it to his development repository in github and triggering a new 
> build.

This should be part of the next patch, right?

>
>  TODO
>  
> @@ -797,5 +815,4 @@ TODO
>
>  * Investigate and add the feature to provide QoS.
>
> -* Sign the driver & create an MSI for installing the different Open vSwitch
> -  components on Windows.
> +* Sign the driver.
>

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] Remove duplicate include file

[Alin Serdean]

Thank you for the patch. Applied on master!

Alin.

From: Yi Li
Sent: Tuesday, September 22, 2020 5:26 AM
To: d...@openvswitch.org
Cc: Yi Li
Subject: [ovs-dev] [PATCH] Remove duplicate include file

Found by checkincludes.pl

Signed-off-by: Yi Li 
---
 AUTHORS.rst | 1 +
 datapath-windows/ovsext/Vxlan.c | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/AUTHORS.rst b/AUTHORS.rst
index ba47c9c2c..b47806bf7 100644
--- a/AUTHORS.rst
+++ b/AUTHORS.rst
@@ -411,6 +411,7 @@ xu rongxu.r...@zte.com.cn
 YAMAMOTO Takashi   yamam...@midokura.com
 Yanqin Wei yanqin@arm.com
 Yasuhito Takamiya  yasuh...@gmail.com
+Yi Li  y...@winhong.com
 Yi Yangyangy...@inspur.com
 Yi-Hung Weiyihung@gmail.com
 Yifeng Sun pkusunyif...@gmail.com
diff --git a/datapath-windows/ovsext/Vxlan.c b/datapath-windows/ovsext/Vxlan.c
index 09809d397..04df9f6c9 100644
--- a/datapath-windows/ovsext/Vxlan.c
+++ b/datapath-windows/ovsext/Vxlan.c
@@ -19,7 +19,6 @@
 #include "Atomic.h"
 #include "Debug.h"
 #include "Flow.h"
-#include "Flow.h"
 #include "IpHelper.h"
 #include "NetProto.h"
 #include "Offload.h"
--
2.25.3



___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] AUTHORS: update email for Mark Gray

[Alin Serdean]

Applied on master. Thank you!

From: Mark Gray
Sent: Monday, September 7, 2020 11:45 AM
To: ovs-dev@openvswitch.org
Subject: [ovs-dev] [PATCH] AUTHORS: update email for Mark Gray

Update email address for Mark Gray

Signed-off-by: Mark Gray 
---
 .mailmap| 1 +
 AUTHORS.rst | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/.mailmap b/.mailmap
index 894062d48..9175d23a4 100644
--- a/.mailmap
+++ b/.mailmap
@@ -54,6 +54,7 @@ Justin Pettit  
 Kmindg 
 Kyle Mestery  
 Lance Richardson  
+Mark Gray  
 Mauricio Vasquez  

 Miguel Angel Ajo  
 Neil McKee 
diff --git a/AUTHORS.rst b/AUTHORS.rst
index 4d8eaa3bd..cb26e0197 100644
--- a/AUTHORS.rst
+++ b/AUTHORS.rst
@@ -255,7 +255,7 @@ Madhu Challa   cha...@noironetworks.com
 Manohar K Cman...@gmail.com
 Marcin Mirecki mmire...@redhat.com
 Mario Cabrera  mario.cabr...@hpe.com
-Mark D. Gray   mark.d.g...@intel.com
+Mark D. Gray   mark.d.g...@redhat.com
 Mark Hamilton
 Mark Kavanagh  mark.b.kavanag...@gmail.com
 Mark Maglana   mmagl...@gmail.com
--
2.26.2

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] Bond support on Windows OVS

[Alin Serdean]

Hi Jinjun,

Basically we need to use `New-NetSwitchTeam` to make OVS aware of multiple 
adapters:
[1] 
https://docs.openvswitch.org/en/latest/intro/install/windows/#add-multiple-nics-to-be-managed-by-ovs

For bonding in particular we need to add some(or all) of those interfaces 
inside OVS bonds:
[2] 
https://blog.scottlowe.org/2012/10/19/link-aggregation-and-lacp-with-open-vswitch/

Please let me know if you want me to send a snippet that combines [1] and [2].

In particular use cases, we can also create a switch over LBFO team NIC as per:
https://docs.microsoft.com/en-us/powershell/module/netlbfo/new-netlbfoteam?view=win10-ps
but in that case from the OVS side we see one single external NIC.

Thanks,
Alin.

From: Jinjun Gao<mailto:jinj...@vmware.com>
Sent: Monday, August 24, 2020 9:09 AM
To: Alin Serdean<mailto:aserd...@cloudbasesolutions.com>; Anand 
Kumar<mailto:kumaran...@vmware.com>
Cc: ovs-dev@openvswitch.org<mailto:ovs-dev@openvswitch.org>
Subject: Bond support on Windows OVS

Hi Alin and Anand,

I'm investigating how to support bond with dual or more physical NICs on 
Windows OVS.  I heard from Anand that you ever discussed it with community guys 
and had a good idea about how to implement it. Can you please provide some 
input on it to us?

Thanks,
-Jinjun

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] datapath-windows: Update flow key in SET action

[Alin Serdean]

Applied on master. Thank you for the fixes and testing!


Alin.

From: Jinjun Gao<mailto:jinj...@vmware.com>
Sent: Wednesday, July 29, 2020 6:29 AM
To: d...@openvswitch.org<mailto:d...@openvswitch.org>; Alin 
Serdean<mailto:aserd...@cloudbasesolutions.com>; 
kumaran...@vmware.com<mailto:kumaran...@vmware.com>
Cc: r...@vmware.com<mailto:r...@vmware.com>; Jinjun 
Gao<mailto:jinj...@vmware.com>
Subject: [PATCH v2] datapath-windows: Update flow key in SET action

The flow key is not updated when process OVS_ACTION_ATTR_SET action.
It will impact follow-up actions, such as, conntrack module cannot
find created conntrack entry if passing old flow key to it.

Reported-by: Rui Cao 
Signed-off-by: Jinjun Gao 
---
v2: Separated assignment to happy MSFT SDV tool
---
 datapath-windows/ovsext/Actions.c | 31 ---
 datapath-windows/ovsext/Actions.h |  3 +++
 2 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/datapath-windows/ovsext/Actions.c 
b/datapath-windows/ovsext/Actions.c
index 4a11cea..4f43369 100644
--- a/datapath-windows/ovsext/Actions.c
+++ b/datapath-windows/ovsext/Actions.c
@@ -1259,6 +1259,7 @@ OvsActionMplsPush(OvsForwardingContext *ovsFwdCtx,
  */
 static __inline NDIS_STATUS
 OvsUpdateEthHeader(OvsForwardingContext *ovsFwdCtx,
+   OvsFlowKey *key,
const struct ovs_key_ethernet *ethAttr)
 {
 PNET_BUFFER curNb;
@@ -1285,9 +1286,11 @@ OvsUpdateEthHeader(OvsForwardingContext *ovsFwdCtx,
 }
 ethHdr = (EthHdr *)(bufferStart + NET_BUFFER_CURRENT_MDL_OFFSET(curNb));

-RtlCopyMemory(ethHdr->Destination, ethAttr->eth_dst,
-   sizeof ethHdr->Destination);
-RtlCopyMemory(ethHdr->Source, ethAttr->eth_src, sizeof ethHdr->Source);
+RtlCopyMemory(ethHdr->Destination, ethAttr->eth_dst, ETH_ADDR_LENGTH);
+RtlCopyMemory(ethHdr->Source, ethAttr->eth_src, ETH_ADDR_LENGTH);
+/* Update l2 flow key */
+RtlCopyMemory(key->l2.dlDst, ethAttr->eth_dst, ETH_ADDR_LENGTH);
+RtlCopyMemory(key->l2.dlSrc, ethAttr->eth_src, ETH_ADDR_LENGTH);

 return NDIS_STATUS_SUCCESS;
 }
@@ -1376,6 +1379,7 @@ PUINT8 OvsGetHeaderBySize(OvsForwardingContext *ovsFwdCtx,
  */
 NDIS_STATUS
 OvsUpdateUdpPorts(OvsForwardingContext *ovsFwdCtx,
+  OvsFlowKey *key,
   const struct ovs_key_udp *udpAttr)
 {
 PUINT8 bufferStart;
@@ -1400,15 +1404,19 @@ OvsUpdateUdpPorts(OvsForwardingContext *ovsFwdCtx,
 udpHdr->check = ChecksumUpdate16(udpHdr->check, udpHdr->source,
  udpAttr->udp_src);
 udpHdr->source = udpAttr->udp_src;
+key->ipKey.l4.tpSrc = udpAttr->udp_src;
 }
 if (udpHdr->dest != udpAttr->udp_dst) {
 udpHdr->check = ChecksumUpdate16(udpHdr->check, udpHdr->dest,
  udpAttr->udp_dst);
 udpHdr->dest = udpAttr->udp_dst;
+key->ipKey.l4.tpDst = udpAttr->udp_dst;
 }
 } else {
 udpHdr->source = udpAttr->udp_src;
+key->ipKey.l4.tpSrc = udpAttr->udp_src;
 udpHdr->dest = udpAttr->udp_dst;
+key->ipKey.l4.tpDst = udpAttr->udp_dst;
 }

 return NDIS_STATUS_SUCCESS;
@@ -1423,6 +1431,7 @@ OvsUpdateUdpPorts(OvsForwardingContext *ovsFwdCtx,
  */
 NDIS_STATUS
 OvsUpdateTcpPorts(OvsForwardingContext *ovsFwdCtx,
+  OvsFlowKey *key,
   const struct ovs_key_tcp *tcpAttr)
 {
 PUINT8 bufferStart;
@@ -1447,11 +1456,13 @@ OvsUpdateTcpPorts(OvsForwardingContext *ovsFwdCtx,
 tcpHdr->check = ChecksumUpdate16(tcpHdr->check, tcpHdr->source,
  tcpAttr->tcp_src);
 tcpHdr->source = tcpAttr->tcp_src;
+key->ipKey.l4.tpSrc = tcpAttr->tcp_src;
 }
 if (tcpHdr->dest != tcpAttr->tcp_dst) {
 tcpHdr->check = ChecksumUpdate16(tcpHdr->check, tcpHdr->dest,
  tcpAttr->tcp_dst);
 tcpHdr->dest = tcpAttr->tcp_dst;
+key->ipKey.l4.tpDst = tcpAttr->tcp_dst;
 }

 return NDIS_STATUS_SUCCESS;
@@ -1579,6 +1590,7 @@ OvsUpdateAddressAndPort(OvsForwardingContext *ovsFwdCtx,
  */
 NDIS_STATUS
 OvsUpdateIPv4Header(OvsForwardingContext *ovsFwdCtx,
+OvsFlowKey *key,
 const struct ovs_key_ipv4 *ipAttr)
 {
 PUINT8 bufferStart;
@@ -1632,6 +1644,7 @@ OvsUpdateIPv4Header(OvsForwardingContext *ovsFwdCtx,
 ipAttr->ipv4_src);
 }
 ipHdr->saddr = ipAttr->ipv4_src;
+key->ipKey.nwSrc = ipAttr->ipv4_src;
 }
 if (ipHdr->daddr != ipAttr->ipv4_dst) {
 if (tcpHdr) {
@@ -1647,6 +1660,7 @@ OvsUpdateIPv4Header(OvsFor

Re: [ovs-dev] [PATCH] datapath-windows: Update flow key in SET action

[Alin Serdean]

Thanks a lot for testing and fixing this as discussed offline.

Can you please split the assignment via the form:
“key->ipKey.nwDst = ipHdr->daddr = ipAttr->ipv4_dst”

It confuses the SDV tool from msft for some reason:
https://docs.microsoft.com/en-us/windows-hardware/drivers/develop/static-driver-verifier-known-issues

Thanks,
Alin.

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3] datapath-windows: Reset ct_mark/ct_label to support ALG

[Alin Serdean]

Acked-by: Alin Gabriel Serdean 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [ovs-dev v2] datapath-windows: Reset ct_mark/ct_label to support ALG

[Alin Serdean]

Thanks a lot for the patch.

In general please prefer to use RtlCopyMemory instead of memcpy: 
https://community.osr.com/discussion/242667/rtlcopymemory-vs-memcpy

Just a few more nits inlined.

Alin.

From: Jinjun Gao<mailto:jinj...@vmware.com>
Sent: Monday, July 20, 2020 6:00 PM
To: d...@openvswitch.org<mailto:d...@openvswitch.org>; Alin 
Serdean<mailto:aserd...@cloudbasesolutions.com>; 
kumaran...@vmware.com<mailto:kumaran...@vmware.com>
Cc: Jinjun Gao<mailto:jinj...@vmware.com>
Subject: [ovs-dev v2] datapath-windows: Reset ct_mark/ct_label to support ALG

The ct_mark/ct_label setting on related connection keep the same
behavior with Linux datapath. If one CT entry has parent/master entry,
its ct_mark and ct_label should inherit from the corresponding part
of parent/master entry at initialization.

Signed-off-by: Jinjun Gao 
---
v2: Resolve Anand's comment to add braces for if-block.
---
 datapath-windows/ovsext/Conntrack.c | 88 +++--
 1 file changed, 56 insertions(+), 32 deletions(-)

diff --git a/datapath-windows/ovsext/Conntrack.c 
b/datapath-windows/ovsext/Conntrack.c
index d065591..cda6d8b 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -789,60 +789,84 @@ OvsProcessConntrackEntry(OvsForwardingContext *fwdCtx,
 static __inline VOID
 OvsConntrackSetMark(OvsFlowKey *key,
 POVS_CT_ENTRY entry,
-UINT32 value,
-UINT32 mask,
+MD_MARK *mark,
 BOOLEAN *markChanged)
 {
-UINT32 newMark;
-newMark = value | (entry->mark & ~(mask));
-if (entry->mark != newMark) {
+POVS_CT_ENTRY parent = entry->parent;
+BOOLEAN changed = FALSE;
+UINT32 newMark = 0;
+
+if (parent && parent->mark) {
+newMark = parent->mark;
+changed = TRUE;
+} else if (mark) {
+newMark = mark->value | (entry->mark & ~(mark->mask));
+changed = TRUE;
+}
+
+if (changed && entry->mark != newMark) {
 entry->mark = newMark;
 key->ct.mark = newMark;
 *markChanged = TRUE;
 }
 }

+static __inline BOOLEAN
+OvsConntrackIsLabelsNonZero(const struct ovs_key_ct_labels *labels)
+{
+UINT8 i;
+
+for (i = 0; i < OVS_CT_LABELS_LEN_32; i++) {
+if (labels->ct_labels_32[i]) {
+return TRUE;
+}
+}
+
+return FALSE;
+}
+
 static __inline void
 OvsConntrackSetLabels(OvsFlowKey *key,
   POVS_CT_ENTRY entry,
-  struct ovs_key_ct_labels *val,
-  struct ovs_key_ct_labels *mask,
+  MD_LABELS *labels,
   BOOLEAN *labelChanged)
 {
-ovs_u128 v, m, pktMdLabel = {0};
-memcpy(, val, sizeof v);
-memcpy(, mask, sizeof m);
-memcpy(, >labels, sizeof(struct ovs_key_ct_labels));
+POVS_CT_ENTRY parent = entry->parent;

-pktMdLabel.u64.lo = v.u64.lo | (pktMdLabel.u64.lo & ~(m.u64.lo));
-pktMdLabel.u64.hi = v.u64.hi | (pktMdLabel.u64.hi & ~(m.u64.hi));
+/* Inherit master's labels at labels initialization, if any. */
+if (!OvsConntrackIsLabelsNonZero(>labels) &&
+parent && OvsConntrackIsLabelsNonZero(>labels)) {
+memcpy(>labels, >labels, OVS_CT_LABELS_LEN);
+*labelChanged = TRUE;
+}
+
+/* Use the same computing method with Linux kernel datapath.
+ * It is more clean and easy understanding.
+ */
[Alin]Nit: You should drop the comment above. It is more useful to
say what you are actually computing.
+if (labels && OvsConntrackIsLabelsNonZero(>mask)) {
+UINT8 i;
+UINT32 * dst = entry->labels.ct_labels_32;
[Alin] Nit: Remove the ‘ ‘. UINT32 *dst
+for (i = 0; i < OVS_CT_LABELS_LEN_32; i++) {
+dst[i] = (dst[i] & ~(labels->mask.ct_labels_32[i])) |
+ (labels->value.ct_labels_32[i] & 
labels->mask.ct_labels_32[i]);
+}

-if (!NdisEqualMemory(>labels, ,
- sizeof(struct ovs_key_ct_labels))) {
 *labelChanged = TRUE;
 }
-NdisMoveMemory(>labels, ,
-   sizeof(struct ovs_key_ct_labels));
-NdisMoveMemory(>ct.labels, ,
-   sizeof(struct ovs_key_ct_labels));
+
+/* Update flow key's ct labels */
+NdisMoveMemory(>ct.labels, >labels, OVS_CT_LABELS_LEN);
 }

 static void
 OvsCtSetMarkLabel(OvsFlowKey *key,
-   POVS_CT_ENTRY entry,
-   MD_MARK *mark,
-   MD_LABELS *labels,
-   BOOLEAN *triggerUpdateEvent)
+  POVS_CT_ENTRY entry,
+  MD_MARK *mark,
+  MD_LABELS *labels,
+  BOOLEAN *triggerUpdateEvent)
 {
-if (mark) {
-OvsConn

Re: [ovs-dev] [PATCH v3 1/4] Eliminate use of term "slave" in bond, LACP, and bundle contexts.

[Alin Serdean]

From: Ben Pfaff
Sent: Tuesday, July 7, 2020 7:29 PM
To: d...@openvswitch.org
Cc: Ben Pfaff
Subject: [ovs-dev] [PATCH v3 1/4] Eliminate use of term "slave" in bond, LACP, 
and bundle contexts.

Most of these changes should not change user-visible behavior.  One
place where they do is in "ovs-ofctl dump-flows", which will now output
"subs:..." inside "bundle" actions instead of "slaves:...".  I don't
expect this to cause real problems in most systems.  The old syntax
is still supported on input for backward compatibility.

Signed-off-by: Ben Pfaff 
---
As discussed offline instead of the term “sub-interface” maybe use “member”.

Leaving the link for the MSFT naming convention:
https://docs.microsoft.com/en-us/powershell/module/netlbfo/add-netlbfoteammember?view=win10-ps

--
Alin.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3 3/4] conntrack: Rename "master" connection to "primary" connection.

[Alin Serdean]

From: Ben Pfaff
Sent: Tuesday, July 7, 2020 7:26 PM
To: d...@openvswitch.org
Cc: Ben Pfaff
Subject: [ovs-dev] [PATCH v3 3/4] conntrack: Rename "master" connection to 
"primary" connection.

Signed-off-by: Ben Pfaff 
---

As discussed offline, your suggestion of switching to “parent” connection 
instead of “primary”, sounds better
in my opinion.


--
Alin
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3 2/4] Use primary/secondary, not master/slave, as names for OpenFlow roles.

[Alin Serdean]

From: Ben Pfaff
Sent: Tuesday, July 7, 2020 7:26 PM
To: d...@openvswitch.org
Cc: Ben Pfaff
Subject: [ovs-dev] [PATCH v3 2/4] Use primary/secondary, not master/slave, as 
names for OpenFlow roles.

Signed-off-by: Ben Pfaff 
---
Acked-by: Alin Gabriel Serdean aserd...@ovn.org
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3 4/4] Eliminate "whitelist" and "blacklist" terms.

[Alin Serdean]

From: Ben Pfaff
Sent: Tuesday, July 7, 2020 7:25 PM
To: d...@openvswitch.org
Cc: Ben Pfaff
Subject: [ovs-dev] [PATCH v3 4/4] Eliminate "whitelist" and "blacklist" terms.

There is one remaining use under datapath.  That change should happen
upstream in Linux first according to our usual policy.

Signed-off-by: Ben Pfaff 
---

Acked-by: Alin Gabriel Serdean aserd...@ovn.org
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [ovs-dev v2] datapath-windows: Add CTA_HELP and CTA_TUPLE_MASTER

[Alin Serdean]

From: Jinjun Gao<mailto:jinj...@vmware.com>
Sent: Tuesday, June 30, 2020 2:45 PM
To: d...@openvswitch.org<mailto:d...@openvswitch.org>; Alin 
Serdean<mailto:aserd...@cloudbasesolutions.com>
Cc: kumaran...@vmware.com<mailto:kumaran...@vmware.com>; Jinjun 
Gao<mailto:jinj...@vmware.com>
Subject: [ovs-dev v2] datapath-windows: Add CTA_HELP and CTA_TUPLE_MASTER

Add helper and master if existing to a conntrack entry:
1, For CTA_HELP, only support FTP/TFTP;
2, For CTA_TUPLE_MASTER, only support FTP.

Signed-off-by: Jinjun Gao 
---
v2:
  1, Add CTA_TUPLE_MASTER support in CT event subscribe/report system.
  2, Release CT entry lock in fail path.
  3, Add more comments.
---
Thank you for addressing the comments, applied on master!

--
Alin.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] datapath-windows: Add CTA_HELP and CTA_TUPLE_MASTER

[Alin Serdean]

Thanks for sending the patch.

Running the command:
make datapath_windows_analyze
resulted in the following:
ovsext\Conntrack.c(873): warning C28167: The function 'OvsCtExecute_' changes 
the IRQL and does not
restore the IRQL before it exits. It should be annotated to reflect the change 
or the IRQL should
be restored. IRQL was last set to 2 at line 955.
[datapath-windows\ovsext\ovsext.vcxproj]

There is a lock acquired but it is not released on the failure path.

You can fold in following incremental:

$ git diff datapath-windows/ovsext/Conntrack.c
diff --git a/datapath-windows/ovsext/Conntrack.c 
b/datapath-windows/ovsext/Conntrack.c
index 414b3f4b2..717d04f49 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -974,6 +974,7 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx,
   OVS_CT_POOL_TAG);
 if (!entry->helper_name) {
 OVS_LOG_ERROR("Error while allocating memory");
+OVS_RELEASE_SPIN_LOCK(&(entry->lock), irql);
 return NDIS_STATUS_RESOURCES;
 }

The rest looks good, but I have the following question:

 /* FTP ACTIVE - Server initiates the connection */
 if ((incomingKey.src.addr.ipv4 == entryKey.src.addr.ipv4) &&
-(incomingKey.src.port == entryKey.src.port) &&
Why is the line above removed?


--
Alin.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] Windows OVS issue

[Alin Serdean]

Hello Wenying,

Trimming the email a bit.

Please see my comments inlined.

> Hi,
> 
> I am using OVS on Windows Server 2019 to provide container networking.
> OpenFlow is used to support both container and host networking, and OVS
> conntrack feature is also used on my setup. I find a lot of error or warning 
> in
> ovs-vswitchd log, are there any thoughts about the errors?
> 
> e.g.,
> 2020-04-27T19:54:27.142Z|34004|odp_util(handler7)|WARN|Dropped 56
> log messages in last 59 seconds (most recently, 1 seconds ago) due to 
> excessive
> rate 2020-04-27T19:54:27.142Z|34005|odp_util(handler7)|WARN|the flow
> key in error is:
> recirc_id(0),ct_state(0),ct_zone(0),ct_mark(0),ct_label(0),ct_tuple4(src=0.0.0.
> 0,dst=0.0.0.0,proto=0,tp_src=0,tp_dst=0),eth(src=68:4f:64:15:61:9a,dst=01:0
> 0:0c:cc:cc:cd),in_port(1),eth_type(0x05ff)
> 2020-04-27T19:55:27.535Z|34006|odp_util(handler7)|WARN|Dropped 56
> log messages in last 59 seconds (most recently, 2 seconds ago) due to 
> excessive
> rate 2020-04-27T19:55:27.535Z|34007|odp_util(handler7)|WARN|invalid
> Ethertype 1535 in flow key 2020-04-
> 27T19:55:27.535Z|34008|odp_util(handler7)|WARN|Dropped 56 log
> messages in last 59 seconds (most recently, 2 seconds ago) due to excessive 
> rate
> 2020-04-27T19:55:27.535Z|34009|odp_util(handler7)|WARN|the flow key in
> error is:
> recirc_id(0),ct_state(0),ct_zone(0),ct_mark(0),ct_label(0),ct_tuple4(src=0.0.0.
> 0,dst=0.0.0.0,proto=0,tp_src=0,tp_dst=0),eth(src=68:4f:64:15:61:9a,dst=01:0
> 0:0c:cc:cc:cd),in_port(1),eth_type(0x05ff)

[Alin Serdean] From what I can see those are packets which are missing a valid
Ethernet type. 

> 
> Except for above error logs, there are also some igmp warnings printed:
> 
...
> 2020-04-27T19:45:53.412Z|33960|dpif(handler7)|WARN|system@ovs-
> system: execute ct(zone=65520,nat),recirc(0x59b7) failed (Unknown error) on
> packet
> igmp,vlan_tci=0x,dl_src=00:15:5d:49:62:bd,dl_dst=01:00:5e:00:00:16,nw
> _src=172.16.6.17,nw_dst=224.0.0.22,nw_tos=0,nw_ecn=0,nw_ttl=1,igmp_ty
> pe=34,igmp_code=0
> with metadata skb_priority(0),skb_mark(0),in_port(5) mtu 0

[Alin Serdean] The windows datapath does not support IGMP, only ICMP, TCP,
UDP and minimal support for FTP.

> 
> Thanks,
> Wenying
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] doc: document the nointegritychecks setting.

[Alin Serdean]

> -Original Message-
> From: William Tu 
> Sent: Monday, March 2, 2020 8:07 PM
> To: d...@openvswitch.org
> Cc: aserd...@ovn.org
> Subject: [PATCH] doc: document the nointegritychecks setting.
> 
> OVS kernel module requires to turn on the 'nointegritychecks'. It has 
> following
> limitations:
 [Alin Serdean] Actually the generated driver from our project is testsigned, 
which
implies the host only needs the /testsigning enabled, which again is not valid
under secure boot.

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [RFC PATCH v1 1/3] datapath-windows: Introduce HNS OVS calls

[Alin Serdean]

> -Original Message-
> From: ovs-dev-boun...@openvswitch.org  boun...@openvswitch.org> On Behalf Of Alin Gabriel Serdean
> Sent: Tuesday, October 9, 2018 2:26 AM
> To: d...@openvswitch.org
> Cc: Alin Gabriel Serdean 
> Subject: [ovs-dev] [RFC PATCH v1 1/3] datapath-windows: Introduce HNS OVS
> calls
> 
> We introduce a new powershell module for interacting with HNS (host network
> service):
> https://github.com/Microsoft/SDN/blob/master/Kubernetes/windows/hns.psm
> 1
> 
> In our repository this shall be named HNSHelper.psm1.
> 
> We also add five new commandlets in our OVS powershell module:
> Disable-OVSOnHNSNetwork - disable OVS extension on a given HNS network.
> Enable-OVSOnHNSNetwork - enable OVS extension on a given HNS network.
> Get-OVSEnabledHNSNetworks - return the HNS network on which OVS is
> enabled.
> Add-OVSHNSInternalPort - will add a new internal port with he name: `name`, on
>  the HNS network, which has OVS enabled. This port
>  shall use the host compartment.
> Delete-OVSHNSInternalPort - will delete an internal port with name: `name` on 
> a
> HNS network which has OVS enabled.
> 
> Signed-off-by: Alin Gabriel Serdean 
> ---

[Alin Serdean] For future references:
HNS API has been renamed to HCN (host compute networking).

An API schema has been released and documented under:
https://docs.microsoft.com/en-us/windows-server/networking/technologies/hcn/hcn-top#features-of-the-hcn-service-api

The patchset was using an undocumented version of the V1 HNS API
via the official Microsoft repository:
https://github.com/microsoft/SDN/blob/710cad6fc9025c86e04ef5daa6eb53f577802448/Kubernetes/windows/hns.psm1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH RFC]: windows: cross-compile using mingw-w64 gcc.

[Alin Serdean]

> -Original Message-
> From: dev  On Behalf Of Ben Pfaff
> Sent: Friday, February 14, 2020 2:20 AM
> To: William Tu 
> Cc:  ; Alin Gabriel Serdean
> 
> Subject: Re: [ovs-dev] [PATCH RFC]: windows: cross-compile using mingw-w64
> gcc.
> 
> On Thu, Feb 13, 2020 at 04:08:20PM -0800, William Tu wrote:
> > On Thu, Feb 13, 2020 at 3:50 PM Ben Pfaff  wrote:
> > >
> > > On Tue, Feb 11, 2020 at 11:13:35AM -0800, William Tu wrote:
> > > > Currently we use MSVC to compile OVS on Windows. The patch tries
> > > > to cross-compile OVS for windows using gcc from mingw-w64.
> > > > The patch still shows lots of warnings I haven't fixed, but now it
> > > > can generate all the .exe and I tested on windows 10 (no kernel
> > > > module).
[Alin Serdean] The patch as is will break MSVC though.
> > >
> > > I'm enthusiastic about supporting fewer families of compilers.  MSVC
> > > is very different from Clang and GCC.
> >
> > Yes, it also makes the build process simpler.
> >
> > One problem is that we have to compile the Windows kernel module, and
> > it has to use MSVC.
> 
> That should be solvable, since the Windows kernel module is quite separate
> from the rest of the OVS code.

[Alin Serdean] Thanks a lot for the patch, but I'm not sure about this direction
to be honest.

It might be better to create a list of `pros` and `cons`. 

 [Pros]
Can be used to cross compile.

[Cons]
- It would create additional include issues, i.e. `unistd.h` is defined for 
mingw but
  not for MSVC. 

- Support/Security - what happens if we hit an issue with the compiler or 
libraries.

I agree with Ben, it would be ideal to move to Clang at some point. This will 
help us in the long
term and it will us also to cross compile.

> > Yes, it also makes the build process simpler.
Beside the need to have Windows to compile. Why is the current build process 
complex?


--
Alin.

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [patch v3] datapath-windows: Append tunnel info to upcall for correct template

[Alin Serdean]

Thank you for the patch, applied on master!

> -Original Message-
> From: Alin Serdean
> Sent: Tuesday, February 4, 2020 8:41 PM
> To: Amber Hu ; d...@openvswitch.org
> Cc: Qiong Wang ; Wenyu Zhang
> ; Jinjun Gao 
> Subject: RE: [patch v3] datapath-windows: Append tunnel info to upcall for
> correct template
> 
> > -Original Message-
> > From: Amber Hu 
> > Sent: Tuesday, February 4, 2020 7:03 AM
> > To: Alin Serdean ;
> > d...@openvswitch.org
> > Cc: Qiong Wang ; Wenyu Zhang
> ;
> > Jinjun Gao 
> > Subject: [patch v3] datapath-windows: Append tunnel info to upcall for
> > correct template
> >
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [patch v3] datapath-windows: Append tunnel info to upcall for correct template

[Alin Serdean]

> -Original Message-
> From: Amber Hu 
> Sent: Tuesday, February 4, 2020 7:03 AM
> To: Alin Serdean ; d...@openvswitch.org
> Cc: Qiong Wang ; Wenyu Zhang
> ; Jinjun Gao 
> Subject: [patch v3] datapath-windows: Append tunnel info to upcall for correct
> template
> 
> Formerly, there is no tunnel information appended in the upcall’s packet data,
> which is expected by IPFIX in userspace to calculate the template for 
> exporting
> the sampled flow record of on egress tunnel port.
> To fix this, during performing OvsOutputUserspaceAction(), we would check
> whether it is initiated by the sampling on egress tunnel which would be 
> indicated
> by the attribute as OVS_USERSPACE_ATTR_EGRESS_TUN_PORT in the nested
> attribute list. If so, we would append the tunKey in OvsForwardingContext
> indexed by OVS_PACKET_ATTR_EGRESS_TUN_KEY to the upcall.
> Besides, at this point, the source transport port and  source ip address are 
> not
> available in the structure, so we have to fill it in the way how the packet 
> would
> be capsulated during performing OvsEncapGeneve(), which is following the
> OvsOutputUserspaceAction() unfortunately.
> I have tested the IPFIX functionality with the change, we could see the 
> template
> is correct and the expected tunnel information could be packed in the IPFIX
> packet finally. The traffic for test is generated by PING utility.
> 
> From d727d051c9a44a4a93e5ee5f3da3ca9b125aad29 Mon Sep 17 00:00:00
> 2001
> From: Amber Hu 
> Date: Thu, 30 Jan 2020 18:01:32 -0800
> Subject: [PATCH v3] datapath-windows: Append tunnel info to upcall for correct
> template
> 

Thank you for addressing the comments!

Acked-by: Alin Gabriel Serdean 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [patch] datapath-windows: Append tunnel info to upcall for correct template

[Alin Serdean]

> 
> On 1/31/20, 10:42, "Amber Hu"  wrote:
> 
> 
> Formerly, there is no tunnel information appended in the upcall’s
> packet data, which is expected by IPFIX in userspace to calculate
> the template for exporting the sampled flow record of on egress
> tunnel port.
> To fix this, during performing OvsOutputUserspaceAction(), we
> would check whether it is initiated by the sampling on egress
> tunnel which would be indicated by the attribute as
> OVS_USERSPACE_ATTR_EGRESS_TUN_PORT in the nested attribute
> list. If so, we would append the tunKey in OvsForwardingContext
> indexed by OVS_PACKET_ATTR_EGRESS_TUN_KEY to the upcall.
> Besides, at this point, the source transport port and  source ip
> address are not available in the structure, so we have to fill it in 
> the
> way how the packet would be capsulated during performing
> OvsEncapGeneve(), which is following the
> OvsOutputUserspaceAction() unfortunately.
> I have tested the IPFIX functionality with the change, we could see 
> the
> template is correct and the expected tunnel information could be
> packed in the IPFIX packet finally. The traffic for test is generated 
> by
> PING utility.
> 
> Signed-off-by: Amber Hu 
> 
> From 31e3baa833d4d37686c3402b95db018ad132d3b7 Mon Sep 17
> 00:00:00 2001
> From: Amber Hu 
> Date: Thu, 30 Jan 2020 18:01:32 -0800
> Subject: [PATCH] datapath-windows: Append tunnel info to upcall for
> correct
>  template
> 
> Signed-off-by: Amber Hu 
[Alin Serdean] The patch will break compilation. You probably want the 
following incremental:
$ git diff datapath-windows/ovsext/Tunnel.c
diff --git a/datapath-windows/ovsext/Tunnel.c b/datapath-windows/ovsext/Tunnel.c
index ad2c254f5..5d1be80f4 100644
--- a/datapath-windows/ovsext/Tunnel.c
+++ b/datapath-windows/ovsext/Tunnel.c
@@ -308,7 +308,7 @@ OvsInjectPacketThroughActions(PNET_BUFFER_LIST pNbl,

 datapath->misses++;
 elem = OvsCreateQueueNlPacket(NULL, 0, OVS_PACKET_CMD_MISS,
-  vport, , pNbl, curNb,
+  vport, , NULL, pNbl, curNb,
   TRUE, );
 if (elem) {
 /* Complete the packet since it was copied to user buffer. */

Also mark the patch as a new version, it is easier for me to see it.

Alin.


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH]lib/stream-windows.c: Grant Access Privilege of Named Pipe to Creator

[Alin Serdean]

Thank you Ning and Anand.

Applied on master!

Alin.

> -Original Message-
> From: Anand Kumar 
> Sent: Friday, January 24, 2020 7:22 AM
> To: Ning Wu ; Alin Serdean
> ; d...@openvswitch.org
> Cc: Lina Li ; Roy Luo 
> Subject: Re: [PATCH]lib/stream-windows.c: Grant Access Privilege of Named
> Pipe to Creator
> 
> Acked-by: Anand Kumar 
> 
> Thanks,
> Anand Kumar
> 
> On 1/22/20, 12:19 AM, "Ning Wu"  wrote:
> 
> From e42950665acee9aab941b26ebdd067ca0de908a3 Mon Sep 17 00:00:00
> 2001
> From: Ning Wu 
> Date: Tue, 21 Jan 2020 23:46:58 -0800
> Subject: [PATCH]lib/stream-windows.c: Grant Access Privilege of Named Pipe
> to Creator
> 
> Current implementation of ovs on windows only allows LocalSystem and
> Administrators to access the named pipe created with API of ovs.
> Thus any service that needs to invoke the API to create named pipe
> has to run as System account to interactive with ovs. It causes the
> system more vulnerable if one of those services was break into.
> The patch adds the creator owner account to allowed ACLs.
> 
> Signed-off-by: Ning Wu 
> ---
>  Documentation/ref/ovsdb.7.rst |  3 ++-
>  lib/stream-windows.c  | 33 -
>  2 files changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/ref/ovsdb.7.rst b/Documentation/ref/ovsdb.7.rst
> index b1f3f5d..da4dbed 100644
> --- a/Documentation/ref/ovsdb.7.rst
> +++ b/Documentation/ref/ovsdb.7.rst
> @@ -422,7 +422,8 @@ punix:
>  named .
> 
>  On Windows, listens on a local named pipe, creating a named pipe
> - to mimic the behavior of a Unix domain socket.
> + to mimic the behavior of a Unix domain socket. The ACLs of the
> named
> +pipe include LocalSystem, Administrators, and Creator Owner.
> 
>  All IP-based connection methods accept IPv4 and IPv6 addresses.  To 
> specify
> an
>  IPv6 address, wrap it in square brackets, e.g.  ``ssl:[::1]:6640``.  
> Passive
> diff --git a/lib/stream-windows.c b/lib/stream-windows.c
> index 34bc610..5c4c55e 100644
> --- a/lib/stream-windows.c
> +++ b/lib/stream-windows.c
> @@ -41,7 +41,7 @@ static void maybe_unlink_and_free(char *path);
>  #define LOCAL_PREFIX ".\\pipe\\"
> 
>  /* Size of the allowed PSIDs for securing Named Pipe. */
> -#define ALLOWED_PSIDS_SIZE 2
> +#define ALLOWED_PSIDS_SIZE 3
> 
>  /* This function has the purpose to remove all the slashes received in 
> s. */
>  static char *
> @@ -412,6 +412,9 @@ create_pnpipe(char *name)
>  PACL acl = NULL;
>  PSECURITY_DESCRIPTOR psd = NULL;
>  HANDLE npipe;
> +HANDLE hToken = NULL;
> +DWORD dwBufSize = 0;
> +PTOKEN_USER pTokenUsr = NULL;
> 
>  /* Disable access over network. */
>  if (!AllocateAndInitializeSid(, 1, SECURITY_NETWORK_RID,
> @@ -438,6 +441,32 @@ create_pnpipe(char *name)
>  goto handle_error;
>  }
> 
> +/* Open the access token of calling process */
> +if (!OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, )) {
> +VLOG_ERR_RL(, "Error opening access token of calling 
> process.");
> +goto handle_error;
> +}
> +
> +/* get the buffer size buffer needed for SID */
> +GetTokenInformation(hToken, TokenUser, NULL, 0, );
> +
> +pTokenUsr = xmalloc(dwBufSize);
> +memset(pTokenUsr, 0, dwBufSize);
> +
> +/* Retrieve the token information in a TOKEN_USER structure. */
> +if (!GetTokenInformation(hToken, TokenUser, pTokenUsr, dwBufSize,
> +)) {
> +VLOG_ERR_RL(, "Error retrieving token information.");
> +goto handle_error;
> +}
> +CloseHandle(hToken);
> +
> +if (!IsValidSid(pTokenUsr->User.Sid)) {
> +VLOG_ERR_RL(, "Invalid SID.");
> +goto handle_error;
> +}
> +allowedPsid[2] = pTokenUsr->User.Sid;
> +
>  for (int i = 0; i < ALLOWED_PSIDS_SIZE; i++) {
>  aclSize += sizeof(ACCESS_ALLOWED_ACE) +
> GetLengthSid(allowedPsid[i]) -
> @@ -490,11 +519,13 @@ create_pnpipe(char *name)
>  npipe = CreateNamedPipe(name, PIPE_ACCESS_DUPLEX |
> FILE_FLAG_OVERLAPPED,
>  PIPE_TYPE_MESSAGE | PIPE_READMODE_BYTE | 
> PIPE_WAIT,
>  64, BUFSIZE, BUFSIZE, 0, );
> +free(pTokenUsr);
>  free(ac

Re: [ovs-dev] [PATCH]lib/stream-windows.c: Grant Access Privilege of Named Pipe to Creator

[Alin Serdean]

> -Original Message-
> From: Ning Wu 
> Sent: Wednesday, January 22, 2020 10:19 AM
> To: Alin Serdean ; d...@openvswitch.org;
> Anand Kumar 
> Cc: Lina Li ; Roy Luo 
> Subject: [PATCH]lib/stream-windows.c: Grant Access Privilege of Named Pipe to
> Creator
> 
[Alin Serdean] In the future include [PATCH v##], where ## is the number
of the revision.

Acked-by: Alin Gabriel Serdean 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] Grant Access Privilege of Named Pipe to Creator

[Alin Serdean]

Hi,

Sorry I missed the email.

The direction sounds ok with me. It will surely help with unit tests, since 
right now
they require elevated permissions.

Adding also Anand in the loop.
Anand do you like the idea?

Please also add a few lines to the documentation so users are aware of the
change.

The patch as is, fails to apply. Rebase on master.

Also please change the title so it is in compliance with:
http://docs.openvswitch.org/en/latest/internals/contributing/submitting-patches/#email-subject
 

Thanks,
Alin.

> -Original Message-
> From: Ning Wu 
> Sent: Monday, January 20, 2020 4:16 AM
> To: d...@openvswitch.org; Alin Serdean 
> Cc: Lina Li ; Roy Luo 
> Subject: RE: [PATCH] Grant Access Privilege of Named Pipe to Creator
> 
> Hi Alin,
> 
> 
> 
> Could you please help to give some comment?
> 
> Thanks.
> 
> 
> 
> From: Ning Wu
> Sent: Friday, January 17, 2020 15:15
> To: d...@openvswitch.org; Alin Serdean 
> Cc: Lina Li ; Roy Luo 
> Subject: [PATCH] Grant Access Privilege of Named Pipe to Creator
> 
> 
> 
> Current implementation of ovs on windows only allows LocalSystem and
> 
> Administrators to access the named pipe created with API of ovs.
> 
> Thus any service that needs to invoke the API to create named pipe
> 
> has to run as System account to interactive with ovs. It causes the
> 
> system more vulnerable if one of those services was break into.
> 
> The patch adds the creator owner account to allowed ACLs.
> 
> 
> 
> Signed-off-by: Ning Wu mailto:n...@vmware.com> >
> 
> ---
> 
> lib/stream-windows.c | 33 -
> 
> 1 file changed, 32 insertions(+), 1 deletion(-)
> 
> 
> 
> diff --git a/lib/stream-windows.c b/lib/stream-windows.c
> 
> index 34bc610..0cad927 100644
> 
> --- a/lib/stream-windows.c
> 
> +++ b/lib/stream-windows.c
> 
> @@ -41,7 +41,7 @@ static void maybe_unlink_and_free(char *path);
> 
> #define LOCAL_PREFIX ".\\pipe\\  "
> 
> 
> 
> /* Size of the allowed PSIDs for securing Named Pipe. */
> 
> -#define ALLOWED_PSIDS_SIZE 2
> 
> +#define ALLOWED_PSIDS_SIZE 3
> 
> 
> 
> /* This function has the purpose to remove all the slashes received in s. */
> 
> static char *
> 
> @@ -412,6 +412,9 @@ create_pnpipe(char *name)
> 
>  PACL acl = NULL;
> 
>  PSECURITY_DESCRIPTOR psd = NULL;
> 
>  HANDLE npipe;
> 
> +HANDLE hToken = NULL;
> 
> +DWORD dwBufSize = 0;
> 
> +PTOKEN_USER pTokenUsr = NULL;
> 
> 
> 
>  /* Disable access over network. */
> 
>  if (!AllocateAndInitializeSid(, 1, SECURITY_NETWORK_RID,
> 
> @@ -438,6 +441,32 @@ create_pnpipe(char *name)
> 
>  goto handle_error;
> 
>  }
> 
> 
> 
> +/* Open the access token of calling process */
> 
> +if (!OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, )) {
> 
> +   VLOG_ERR_RL(, "Error opening access token of calling process.");
> 
> +goto handle_error;
> 
> +}
> 
> +
> 
> +/* get the buffer size buffer needed for SID */
> 
> +GetTokenInformation(hToken, TokenUser, NULL, 0, );
> 
> +
> 
> +pTokenUsr = xmalloc(dwBufSize);
> 
> +memset(pTokenUsr, 0, dwBufSize);
> 
> +
> 
> +/* Retrieve the token information in a TOKEN_USER structure. */
> 
> +if (!GetTokenInformation(hToken, TokenUser, pTokenUsr, dwBufSize,
> 
> +)) {
> 
> +VLOG_ERR_RL(, "Error retrieving token information.");
> 
> +goto handle_error;
> 
> +}
> 
> +CloseHandle(hToken);
> 
> +
> 
> +if (!IsValidSid(pTokenUsr->User.Sid)) {
> 
> +VLOG_ERR_RL(, "Invalid SID.");
> 
> +goto handle_error;
> 
> +}
> 
> +allowedPsid[2] = pTokenUsr->User.Sid;
> 
> +
> 
>  for (int i = 0; i < ALLOWED_PSIDS_SIZE; i++) {
> 
>  aclSize += sizeof(ACCESS_ALLOWED_ACE) +
> 
> GetLengthSid(allowedPsid[i]) -
> 
> @@ -490,11 +519,13 @@ create_pnpipe(char *name)
> 
>  npipe = CreateNamedPipe(name, PIPE_ACCESS_DUPLEX |
> FILE_FLAG_OVERLAPPED,
> 
>  PIPE_TYPE_MESSAGE | PIPE_READMODE_BYTE | 
> PIPE_WAIT,
> 
>  64, BUFSIZE, BUFSIZE, 0, );
> 
> +free(pTokenUsr);
> 
>  free(acl);
> 
>  free(psd);
> 
>  return npipe;
> 
> 
> 
> handle_error:
> 
> +free(pTokenUsr);
> 
>  free(acl);
> 
>  free(psd);
> 
>  return INVALID_HANDLE_VALUE;
> 
> --
> 
> 2.6.2
> 
> 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [patch] datapath-windows: Append tunnel info to upcall for correct template

[Alin Serdean]

Hi Amber,

The patch fails to apply:
"error: corrupt patch at line 198"

Please rebase on master.

Trimming the patch a bit and comments inlined.
> -Original Message-
> From: Amber Hu 
> Sent: Tuesday, January 14, 2020 7:50 AM
> To: d...@openvswitch.org; Alin Serdean 
> Cc: Qiong Wang ; Wenyu Zhang
> ; Jinjun Gao 
> Subject: [patch] datapath-windows: Append tunnel info to upcall for correct
> template
> 

> diff --git a/datapath-windows/ovsext/Actions.c b/datapath-
> windows/ovsext/Actions.c
> 
> index 0c4d64b..936e68b 100644
> 
> --- a/datapath-windows/ovsext/Actions.c
> 
> +++ b/datapath-windows/ovsext/Actions.c
> 
> @@ -1839,11 +1839,16 @@ OvsOutputUserspaceAction(OvsForwardingContext
> *ovsFwdCtx,
> 
>   const PNL_ATTR attr)
> 
> {
> 
>  NTSTATUS status = NDIS_STATUS_SUCCESS;
> 
> -PNL_ATTR userdataAttr;
> 
> -PNL_ATTR queueAttr;
 [Alin Serdean] queueAttr can be safely removed since it's not actually used.
> 
> +PNL_ATTR userdataAttr = NULL;
> 
> +PNL_ATTR queueAttr = NULL;
> 
> +PNL_ATTR egrTunAttr = NULL;
> 
> +PNL_ATTR a = NULL;
[Alin Serdean] Not sure why did is needed. NlAttrFindNested does the safe 
version
of NL_ATTR_FOR_EACH_UNSAFE. Any reason to change it?
> 
>  POVS_PACKET_QUEUE_ELEM elem;
> 
>  POVS_PACKET_HDR_INFO layers = >layers;
> 
>  BOOLEAN isRecv = FALSE;
> 
> +INT rem = 0;
> 
> +OVS_FWD_INFO fwdInfo;
> 
> +OvsIPv4TunnelKey tunKey;
> 
>  POVS_VPORT_ENTRY vport = OvsFindVportByPortNo(ovsFwdCtx-
> >switchContext,
> 
>ovsFwdCtx->srcVportNo);
> 
> @@ -1855,13 +1860,41 @@ OvsOutputUserspaceAction(OvsForwardingContext
> *ovsFwdCtx,
> 
>  }
> 
>  }
> 
> -queueAttr = NlAttrFindNested(attr, OVS_USERSPACE_ATTR_PID);
> 
> -userdataAttr = NlAttrFindNested(attr, OVS_USERSPACE_ATTR_USERDATA);
> 
> +NL_ATTR_FOR_EACH_UNSAFE(a, rem, NlAttrData(attr), NlAttrGetSize(attr)) {
[Alin Serdean] NlAttrFindNested does the safe version of 
NL_ATTR_FOR_EACH_UNSAFE.
Any reason to change it?
> 
> +switch (NlAttrType(a)) {
> 
> +case OVS_USERSPACE_ATTR_PID:
> 
> +queueAttr = a;
> 
> +break;
> 
> +case OVS_USERSPACE_ATTR_USERDATA:
> 
> +userdataAttr = a;
> 
> +break;
> 
> +case OVS_USERSPACE_ATTR_EGRESS_TUN_PORT:
> 
> +/* Indicate the packet is from egress tunnel port */
> 
> +    egrTunAttr = a;
> 
> +break;
> 
> +default:
> 
> +break;
> 
> +}
> 
> +}
> 
> +
> 
> +/* Fill tunnel key to export to usersspace to calculate the template id 
> */
> 
> +if (egrTunAttr) {
> 
[Alin Serdean] It would be safer to zero out tunKey before this.
> +RtlCopyMemory(, >tunKey, sizeof tunKey);
> 
> +if (!tunKey.src) {
> 
> +status = OvsLookupIPFwdInfo(tunKey.src, tunKey.dst, );
> 
> +if (status == NDIS_STATUS_SUCCESS && tunKey.dst ==
> fwdInfo.dstIpAddr) {
> 
> +tunKey.src = fwdInfo.srcIpAddr;
> 
> +}
> 
> +}
> 
> +tunKey.flow_hash = tunKey.flow_hash?tunKey.flow_hash:MAXINT16;
[Alin Serdean] Missing a whitespace before and after '?'
> 
> +}
> 
>  elem = OvsCreateQueueNlPacket(NlAttrData(userdataAttr),
> 
>NlAttrGetSize(userdataAttr),
> 
>OVS_PACKET_CMD_ACTION,
> 
> -  vport, key, ovsFwdCtx->curNbl,
> 
> +  vport, key,
> 
> +  egrTunAttr?&(tunKey):NULL,
[Alin Serdean] Missing whitespace before and after '?'
> 
> +  ovsFwdCtx->curNbl,
> 
>
> NET_BUFFER_LIST_FIRST_NB(ovsFwdCtx->curNbl),
> 
>isRecv,
> 
>layers);
> 
> diff --git a/datapath-windows/ovsext/Flow.c b/datapath-
> windows/ovsext/Flow.c
> 
> index fdb1010..ac0582c 100644
> 
> --- a/datapath-windows/ovsext/Flow.c
> 
> +++ b/datapath-windows/ovsext/Flow.c
> 
> @@ -1094,6 +1094,18 @@ MapFlowTunKeyToNlKey(PNL_BUFFER nlBuf,
> 
>  goto done;
> 
>  }
> 
> +if (!NlMsgPutTailU16(nlBuf, OVS_TUNNEL_KEY_ATTR_TP_SRC,
> 
> + tunKey->flow_hash)) {
> 
> +rc = STA

Re: [ovs-dev] [patch] datapath-windows: Append tunnel info to upcall for correct template

[Alin Serdean]

Sure. I will review it tonight.

Thanks,
Alin.

-Original Message-
From: Ben Pfaff  
Sent: Tuesday, January 21, 2020 7:59 PM
To: Amber Hu 
Cc: d...@openvswitch.org; Alin Serdean ; Qiong 
Wang 
Subject: Re: [ovs-dev] [patch] datapath-windows: Append tunnel info to upcall 
for correct template

I expect that, of the OVS maintainers, Alin is most qualified to review it.  
Alin, are you planning to take a look at it?

On Tue, Jan 21, 2020 at 02:52:54AM +, Amber Hu via dev wrote:
> Hi,
> 
> May I ask the process of reviewing for the below patch 
> https://patchwork.ozlabs.org/patch/1222463/
> Thanks for your time.
> 
> BR/Amber
> 
> From: Amber Hu 
> Date: Tuesday, January 14, 2020 at 13:49
> To: "d...@openvswitch.org" , Alin Serdean 
> 
> Cc: Qiong Wang , Wenyu Zhang , 
> Jinjun Gao 
> Subject: [patch] datapath-windows: Append tunnel info to upcall for 
> correct template
> 
> Formerly, there is no tunnel information appended in the upcall’s 
> packet data, which is expected by IPFIX in userspace to calculate the 
> template for exporting the sampled flow record of on egress tunnel 
> port.
> To fix this, during performing OvsOutputUserspaceAction(), we would 
> check whether it is initiated by the sampling on egress tunnel which 
> would be indicated by the attribute as 
> OVS_USERSPACE_ATTR_EGRESS_TUN_PORT in the nested attribute list. If 
> so, we would append the tunKey in OvsForwardingContext indexed by 
> OVS_PACKET_ATTR_EGRESS_TUN_KEY to the upcall.
> Besides, at this point, the source transport port and  source ip 
> address are not available in the structure, so we have to fill it in 
> the way how the packet would be capsulated during performing 
> OvsEncapGeneve(), which is following the
> OvsOutputUserspaceAction() unfortunately.
> I have tested the IPFIX functionality with the change, we could see 
> the template is correct and the expected tunnel information could be 
> packed in the IPFIX packet finally. The traffic for test is generated 
> by PING utility.
> 
> Issue: 2449045
> Reported-by: Meng Yang mailto:ym...@vmware.com>>
> Reported-at: https://bugzilla.eng.vmware.com/show_bug.cgi?id=2449045
> Signed-off-by: Amber Hu 
> 
> From 8262494fb9a353c3adbe02cfc4c932231c34b3ed Mon Sep 17 00:00:00 2001
> From: Amber Hu 
> Date: Sun, 12 Jan 2020 22:40:05 -0800
> Subject: [PATCH] datapath-windows: Append tunnel info to upcall for 
> correct template
> 
> Issue: #2449045
> Signed-off-by: Amber Hu 
> ---
> datapath-windows/ovsext/Actions.c | 43 ++-
> datapath-windows/ovsext/Flow.c| 12 +++
> datapath-windows/ovsext/User.c| 10 +++--
> datapath-windows/ovsext/User.h|  1 +
> 4 files changed, 59 insertions(+), 7 deletions(-)
> 
> diff --git a/datapath-windows/ovsext/Actions.c 
> b/datapath-windows/ovsext/Actions.c
> index 0c4d64b..936e68b 100644
> --- a/datapath-windows/ovsext/Actions.c
> +++ b/datapath-windows/ovsext/Actions.c
> @@ -1839,11 +1839,16 @@ OvsOutputUserspaceAction(OvsForwardingContext 
> *ovsFwdCtx,
>   const PNL_ATTR attr) {
>  NTSTATUS status = NDIS_STATUS_SUCCESS;
> -PNL_ATTR userdataAttr;
> -PNL_ATTR queueAttr;
> +PNL_ATTR userdataAttr = NULL;
> +PNL_ATTR queueAttr = NULL;
> +PNL_ATTR egrTunAttr = NULL;
> +PNL_ATTR a = NULL;
>  POVS_PACKET_QUEUE_ELEM elem;
>  POVS_PACKET_HDR_INFO layers = >layers;
>  BOOLEAN isRecv = FALSE;
> +INT rem = 0;
> +OVS_FWD_INFO fwdInfo;
> +OvsIPv4TunnelKey tunKey;
>  POVS_VPORT_ENTRY vport = OvsFindVportByPortNo(ovsFwdCtx->switchContext,
>
> ovsFwdCtx->srcVportNo); @@ -1855,13 +1860,41 @@ 
> OvsOutputUserspaceAction(OvsForwardingContext *ovsFwdCtx,
>  }
>  }
> -queueAttr = NlAttrFindNested(attr, OVS_USERSPACE_ATTR_PID);
> -userdataAttr = NlAttrFindNested(attr, OVS_USERSPACE_ATTR_USERDATA);
> +NL_ATTR_FOR_EACH_UNSAFE(a, rem, NlAttrData(attr), NlAttrGetSize(attr)) {
> +switch (NlAttrType(a)) {
> +case OVS_USERSPACE_ATTR_PID:
> +queueAttr = a;
> +break;
> +case OVS_USERSPACE_ATTR_USERDATA:
> +userdataAttr = a;
> +break;
> +case OVS_USERSPACE_ATTR_EGRESS_TUN_PORT:
> +/* Indicate the packet is from egress tunnel port */
> +egrTunAttr = a;
> +break;
> +default:
> +break;
> +}
> +}
> +
> +/* Fill tunnel key to export to usersspace to calculate the template id 
> */
> +if (egrTunAttr) {
> + 

Re: [ovs-dev] datapath-windows: Don't delete internal port

[Alin Serdean]

Applied on master! Thank you!

On 6 Dec 2019, at 16:39, Alin Serdean 
mailto:aserd...@cloudbasesolutions.com>> wrote:



On 3 Dec 2019, at 06:38, Jinjun Gao 
mailto:jinj...@vmware.com>> wrote:

According to the microsoft doc:
https://docs.microsoft.com/en-us/windows-hardware/drivers/network/hyper-v-extensible-switch-port-and-network-adapter-states
Below OID request sequence is validation:
OID_SWITCH_NIC_CONNECT -> OID_SWITCH_NIC_DISCONNECT
 ^   |
 |   V
OID_SWITCH_NIC_CREATE  <- OID_SWITCH_NIC_DELETE

In above sequence, the windows extensible switch interface assumes the
OID_SWITCH_PORT_CREATE has issued and the port has been created
successfully. If delete the internal port in HvDisconnectNic(),
HvCreateNic() will fail when received OID_SWITCH_NIC_CREATE late because
there is no corresponding port.

Signed-off-by: Jinjun Gao 
---

Thanks for sending the patch out!

I tried to apply the patch from patchworks and it fails to apply on master:
$ git apply mbox
error: corrupt patch at line 183


Indeed we should not call OvsRemoveAndDeleteVport, however we should keep
OvsPostVportEvent and also call it for all the ports so that the userspace will 
be
notified of the changes.

Can you please fold in the following:

diff --git a/datapath-windows/ovsext/Vport.c b/datapath-windows/ovsext/Vport.c
index 57e75109d..9f1587f44 100644
--- a/datapath-windows/ovsext/Vport.c
+++ b/datapath-windows/ovsext/Vport.c
@@ -628,6 +628,7 @@ HvDisconnectNic(POVS_SWITCH_CONTEXT switchContext,
event.upcallPid = vport->upcallPid;
RtlCopyMemory(, >ovsName, sizeof event.ovsName);
event.type = OVS_EVENT_LINK_DOWN;
+OvsPostVportEvent();

/*
 * Delete the port from the hash tables accessible to userspace. After this
@@ -635,13 +636,18 @@ HvDisconnectNic(POVS_SWITCH_CONTEXT switchContext,
 */
if (OvsIsRealExternalVport(vport)) {
OvsRemoveAndDeleteVport(NULL, switchContext, vport, FALSE, TRUE);
-OvsPostVportEvent();
}

if (isInternalPort) {
OvsUnBindVportWithIpHelper(vport, switchContext);
-OvsRemoveAndDeleteVport(NULL, switchContext, vport, TRUE, TRUE);
-OvsPostVportEvent();
+/*
+ * Don't delete the port from the hash tables here for internal port
+ * because the internal port cannot be recreated in HvCreateNic(). It
+ * only can be created in HvCreatePort() by issuing
+ * OID_SWITCH_PORT_CREATE. We should wait extensible switch interface
+ * to issue OID_SWITCH_PORT_TEARDOWN and OID_SWITCH_PORT_DELETE to
+ * delete the internal port.
+ */
}
NdisReleaseRWLock(switchContext->dispatchLock, );




___
dev mailing list
d...@openvswitch.org<mailto:d...@openvswitch.org>
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] datapath-windows: Don't delete internal port

[Alin Serdean]

> On 3 Dec 2019, at 06:38, Jinjun Gao  wrote:
> 
> According to the microsoft doc:
> https://docs.microsoft.com/en-us/windows-hardware/drivers/network/hyper-v-extensible-switch-port-and-network-adapter-states
> Below OID request sequence is validation:
>  OID_SWITCH_NIC_CONNECT -> OID_SWITCH_NIC_DISCONNECT
>   ^   |
>   |   V
>  OID_SWITCH_NIC_CREATE  <- OID_SWITCH_NIC_DELETE
>  
> In above sequence, the windows extensible switch interface assumes the
> OID_SWITCH_PORT_CREATE has issued and the port has been created
> successfully. If delete the internal port in HvDisconnectNic(),
> HvCreateNic() will fail when received OID_SWITCH_NIC_CREATE late because
> there is no corresponding port.
>  
> Signed-off-by: Jinjun Gao 
> ---

Thanks for sending the patch out!

I tried to apply the patch from patchworks and it fails to apply on master:
$ git apply mbox
error: corrupt patch at line 183


Indeed we should not call OvsRemoveAndDeleteVport, however we should keep
OvsPostVportEvent and also call it for all the ports so that the userspace will 
be
notified of the changes.

Can you please fold in the following:

diff --git a/datapath-windows/ovsext/Vport.c b/datapath-windows/ovsext/Vport.c
index 57e75109d..9f1587f44 100644
--- a/datapath-windows/ovsext/Vport.c
+++ b/datapath-windows/ovsext/Vport.c
@@ -628,6 +628,7 @@ HvDisconnectNic(POVS_SWITCH_CONTEXT switchContext,
 event.upcallPid = vport->upcallPid;
 RtlCopyMemory(, >ovsName, sizeof event.ovsName);
 event.type = OVS_EVENT_LINK_DOWN;
+OvsPostVportEvent();

 /*
  * Delete the port from the hash tables accessible to userspace. After this
@@ -635,13 +636,18 @@ HvDisconnectNic(POVS_SWITCH_CONTEXT switchContext,
  */
 if (OvsIsRealExternalVport(vport)) {
 OvsRemoveAndDeleteVport(NULL, switchContext, vport, FALSE, TRUE);
-OvsPostVportEvent();
 }

 if (isInternalPort) {
 OvsUnBindVportWithIpHelper(vport, switchContext);
-OvsRemoveAndDeleteVport(NULL, switchContext, vport, TRUE, TRUE);
-OvsPostVportEvent();
+/*
+ * Don't delete the port from the hash tables here for internal port
+ * because the internal port cannot be recreated in HvCreateNic(). It
+ * only can be created in HvCreatePort() by issuing
+ * OID_SWITCH_PORT_CREATE. We should wait extensible switch interface
+ * to issue OID_SWITCH_PORT_TEARDOWN and OID_SWITCH_PORT_DELETE to
+ * delete the internal port.
+ */
 }
 NdisReleaseRWLock(switchContext->dispatchLock, );




___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/2] backtrace: Fix 32-bit libunwind build.

[Alin Serdean]

On 8 Oct 2019, at 20:51, Ben Pfaff mailto:b...@ovn.org>> wrote:

On Tue, Oct 08, 2019 at 06:33:10PM +0200, Ilya Maximets wrote:
On 08.10.2019 18:13, William Tu wrote:
On Mon, Oct 07, 2019 at 07:10:26PM +0200, Ilya Maximets wrote:
On 07.10.2019 18:11, Ilya Maximets wrote:
On 07.10.2019 18:07, William Tu wrote:
On Mon, Oct 7, 2019 at 5:06 AM Ilya Maximets 
mailto:i.maxim...@ovn.org>> wrote:

On 04.10.2019 23:21, William Tu wrote:
The libunwind unw_word_t type is defined as uint32_t for 32-bit
system and uint64_t for 64-bit system.  The patch fixes the
compile error using PRIxPTR to print this value.

Fixes: e2ed6fbeb18c ("fatal-signal: Catch SIGSEGV and print backtrace.")
Signed-off-by: William Tu mailto:u9012...@gmail.com>>
Acked-by: Ilya Maximets mailto:i.maxim...@ovn.org>>
---

Thanks! I applied both patches to master, but in the reverse order.

Best regards, Ilya Maximets.

Hi Ilya,

Thanks...
Now the cirrus CI and appveyor seem to be failing. I will work on it.

Not sure about appveyor, but cirrus is failing because of this:
https://mail.openvswitch.org/pipermail/ovs-dev/2019-October/363182.html

It's not related to current patch, freebsd just updated to use gcc > 9.
And it's not possible to build OVS with -Werror and gcc 9 right now.

Thanks, I'm reviewing this patch.

And this somehow FreeBSD specific. netinet/icmp6.h from glibc development
package doesn't mark the structure as 'packed' on linux.

Best regards, Ilya Maximets.


The appveyor failure is not due to other issues.

appveyor failure is due to missing python3 binary.
Caused by the patch:
1ca0323e7c29 ("Require Python 3 and remove support for Python 2.")

Need to add something like this to the build script:

SET PYTHON="C:\\Python37-x64"
SET PATH="%PYTHON%;%PYTHON%\\Scripts;%PATH%"
python --version

And revert back 's/python3/python/'.

Best regards, Ilya Maximets.

It looks like Alin is working on this:
https://github.com/openvswitch/ovs/pull/296

Sorry for the lack of communication on my side.

I sent a patch to update appveyor here: 
https://patchwork.ozlabs.org/patch/1173786/

Alin.


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v1] datapath-windows: Fix updating ct label when mask is specified

[Alin Serdean]

Thanks a lot for the patch!

I applied it on master and backported as far as I could.

Alin.

> On 15 Aug 2019, at 19:39, Anand Kumar via dev  wrote:
> 
> From: kumaranand 
> 
> When an existing label needs to be changed by specifing bits to be
> updated using mask, instead of updating only the masked bits,
> new label was getting overridden. This patch fixes this issue.
> 
> Signed-off-by: Anand Kumar 
> ---
> datapath-windows/ovsext/Conntrack.c | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/datapath-windows/ovsext/Conntrack.c 
> b/datapath-windows/ovsext/Conntrack.c
> index bc00b60..ba56116 100644
> --- a/datapath-windows/ovsext/Conntrack.c
> +++ b/datapath-windows/ovsext/Conntrack.c
> @@ -807,6 +807,7 @@ OvsConntrackSetLabels(OvsFlowKey *key,
> ovs_u128 v, m, pktMdLabel = {0};
> memcpy(, val, sizeof v);
> memcpy(, mask, sizeof m);
> +memcpy(, >labels, sizeof(struct ovs_key_ct_labels));
> 
> pktMdLabel.u64.lo = v.u64.lo | (pktMdLabel.u64.lo & ~(m.u64.lo));
> pktMdLabel.u64.hi = v.u64.hi | (pktMdLabel.u64.hi & ~(m.u64.hi));
> -- 
> 2.9.3.windows.1
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] faq: Explain why select groups don't sort out packets evenly.

[Alin Serdean]

On 8 Mar 2019, at 03:47, Ben Pfaff mailto:b...@ovn.org>> wrote:

This keeps coming up.

Signed-off-by: Ben Pfaff mailto:b...@ovn.org>>
—

Acked-by: Alin Gabriel Serdean mailto:aserd...@ovn.org>>


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] datapath-windows: Add guards around IpHelper adapter binding calls

[Alin Serdean]

Applied on master! 

> On 25 Mar 2019, at 19:26, Alin Serdean  
> wrote:
> 
> -Original Message-
> From: ovs-dev-boun...@openvswitch.org  On 
> Behalf Of Sairam Venugopal via dev
> Sent: Thursday, March 14, 2019 12:37 AM
> To: ovs-dev@openvswitch.org
> Subject: [ovs-dev] [PATCH] datapath-windows: Add guards around IpHelper 
> adapter binding calls
> 
> Protect internal adapter up/down calls with a dispatch lock. It was observed 
> that the InternalAdapter bind calls could happen out of order thereby causing 
> encap packets to not be sent properly.
> 
> Add assert around the IpHelper bind calls to ensure Up/Down gets called only 
> for the appropriate vports.
> 
> Signed-off-by: Sairam Venugopal 
> ---
> 
> Acked-by: Alin Gabriel Serdean 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] compiler: Disable BUILD_MESSAGE() when processing with sparse.

[Alin Serdean]

On 28 Mar 2019, at 01:11, Ben Pfaff mailto:b...@ovn.org>> wrote:

sparse doesn't support _Pragma(message(x)), even though GCC does, so
HAVE_PRAGMA_MESSAGE is deceptive in that case and causes pointless errors.

Signed-off-by: Ben Pfaff mailto:b...@ovn.org>>
---
lib/compiler.h | 4 ++—

Acked-by: Alin Gabriel Serdean mailto:aserd...@ovn.org>>


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] OVS mailing lists *sometimes* replace the From: address by the list address

[Alin Serdean]

> On 13 Mar 2019, at 22:47, Ben Pfaff  wrote:
> 
> On Wed, Mar 13, 2019 at 01:46:18PM -0400, Aaron Conole wrote:
>> Ben Pfaff  writes:
>> 
>>> On Tue, Mar 12, 2019 at 09:03:19PM -0700, Ben Pfaff wrote:
 On Mon, Mar 11, 2019 at 12:13:23PM -0400, Aaron Conole wrote:
> Ben Pfaff  writes:
> 
>> Hi Linux Foundation folks,
>> 
>> We're seeing a recurring problem on the ovs-dev mailing list where, for
>> some people, mailman is replacing the poster's own email address in
>> mailing list messages by the list address, in the From: header.  Can you
>> help us figure out what's going on?  Here is the latest example:
> 
> Most recent example is archived here:
> https://mail.openvswitch.org/pipermail/ovs-dev/2019-March/357100.html
> 
> Seems this does still happen.  The message id is:
> 
> <20190308212233.35688-1-vsai...@vmware.com>
> 
> Hopefully you can help us figure out what is happening :)
 
 Hi LF folks, anyone there?  I first wrote to you about 2 weeks ago and
 it's been only crickets since then.  Help appreciated, thanks.
>>> 
>>> I received an autoreply from LF saying that they don't use email for
>>> support anymore and to file a ticket.  I did:
>>> 
>>>https://jira.linuxfoundation.org/servicedesk/customer/portal/2/IT-49
>>> 
>>> I have no idea whether the ticket is public.
>> 
>> Thanks, Ben.
>> 
>> I tried to view it, but I don't have permissions.
> 
> The LF staff immediately closed my ticket as a duplicate of another
> ticket that I do not have permission to view.

:(
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] datapath-windows: Fix race condition during port creation

[Alin Serdean]

> -Mesaj original-
> De la: ovs-dev-boun...@openvswitch.org  boun...@openvswitch.org> În numele aserd...@ovn.org
> Trimis: Wednesday, February 27, 2019 2:27 PM
> Către: 'Sairam Venugopal' ; d...@openvswitch.org
> Subiect: Re: [ovs-dev] [PATCH] datapath-windows: Fix race condition during
> port creation
> 
> >
> > Hold the dispatch lock until port-add operations are completed.
> >
> > Found by inspection.
> >
> > Signed-off-by: Sairam Venugopal 
> [Alin Serdean] Thanks a lot for the patch!
> 
> For some reason the author seems to be ovs-dev@openvswitch.org, can you
> try to respin the patch?
> 
> Acked-by: Alin Gabriel Serdean 
> 
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[Alin Serdean] Applied on master an branch-2.11
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] Windows: Fix broken kernel userspace communication

[Alin Serdean]

Thanks Ben! Applied on master and backported.

--
Alin

> -Mesaj original-
> De la: ovs-dev-boun...@openvswitch.org  boun...@openvswitch.org> În numele Ben Pfaff
> Trimis: Friday, November 16, 2018 4:36 PM
> Către: Alin Gabriel Serdean 
> Cc: d...@openvswitch.org
> Subiect: Re: [ovs-dev] [PATCH v2] Windows: Fix broken kernel userspace
> communication
> 
> On Fri, Nov 16, 2018 at 03:32:58PM +0200, Alin Gabriel Serdean wrote:
> > Patch:
> >
> https://github.com/openvswitch/ovs/commit/69c51582ff786a68fc325c1c506
> 2
> > 4715482bc460 broke Windows userpace - kernel communication.
> >
> > On windows we create netlink sockets when the handlers are initiated
> > and reuse them.
> > This patch remaps the usage of the netlink socket pool.
> >
> > Fixes:
> > https://github.com/openvswitch/ovs-issues/issues/164
> >
> > Signed-off-by: Alin Gabriel Serdean 
> > Acked-by: Shashank Ram 
> > Tested-by: Shashank Ram 
> > Signed-off-by: Ben Pfaff 
> > Co-authored-by: Ben Pfaff 
> > ---
> > v1 -> v2: Fold in nice incremental from Ben RFC -> v1: Improve
> > wording, change function name and add ifdef guard for them
> 
> Looks good to me, thank you!
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] Windows: Fix broken kernel userspace communication

[Alin Serdean]

Yup in a couple of hours.

Sent from phone 

> On 15 Nov 2018, at 20:27, 0-day Robot  wrote:
> 
> Bleep bloop.  Greetings Alin Gabriel Serdean, I am a robot and I have tried 
> out your patch.
> Thanks for your contribution.
> 
> I encountered some error that I wasn't expecting.  See the details below.
> 
> 
> git-am:
> Failed to merge in the changes.
> Patch failed at 0001 Windows: Fix broken kernel userspace communication
> The copy of the patch that failed is found in:
>   
> /var/lib/jenkins/jobs/upstream_build_from_pw/workspace/.git/rebase-apply/patch
> When you have resolved this problem, run "git am --resolved".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
> 
> 
> Please check this out.  If you feel there has been an error, please email 
> acon...@bytheb.org
> 
> Thanks,
> 0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] Windows: Fix broken kernel userspace communication

[Alin Serdean]

> -Mesaj original-
> De la: ovs-dev-boun...@openvswitch.org  boun...@openvswitch.org> În numele Ben Pfaff
> Trimis: Thursday, November 15, 2018 7:12 PM
> Către: Alin Gabriel Serdean 
> Cc: d...@openvswitch.org
> Subiect: Re: [ovs-dev] [PATCH] Windows: Fix broken kernel userspace
> communication
> 
> On Thu, Nov 15, 2018 at 06:42:38PM +0200, Alin Gabriel Serdean wrote:
> > Patch:
> >
> https://github.com/openvswitch/ovs/commit/69c51582ff786a68fc325c1c506
> 2
> > 4715482bc460 broke Windows userpace - kernel communication.
> >
> > On windows we create netlink sockets when the handlers are initiated
> > and reuse them.
> > This patch remaps the usage of the netlink socket pool.
> >
> > Fixes:
> > https://github.com/openvswitch/ovs-issues/issues/164
> >
> > Signed-off-by: Alin Gabriel Serdean 
> > Acked-by: Shashank Ram 
> > Tested-by: Shashank Ram 
> 
> Sorry we broke Windows.  I apologize.
[Alin Serdean] No worries. Maybe this increases the awareness about the
need of a CI.
> 
> Usually I prefer to put all the #ifdefs in a single place when I can.
> In this case, I think that means putting them inside
> create_socksp_windows() and del_socksp_windows().  It's easy enough.
> 
> But del_socksp_windows() is a really weird function.  It does nothing:
> it sets one of its parameters to NULL, which is not visible outside the
> function, and it ignores its other parameter.  That would make sense if there
> were multiple implementations, or if it were referred to via function pointer,
> etc., but I don't understand it here.  Why call it at all?
[Alin Serdean] No reason, I forgot to remove when I sent the v1.
> 
> Anyhow, here is one variation that I suggest.  I built it on Linux but not on
> Windows, so maybe I screwed some things up.
> 
> Actually, looking at it again, I think that the first change in
> dpif_netlink_port_add__() fixes a bug: it looks like, previously, an error
> return from nl_sock_create() was passed up to the caller as a success.  I
> guess that should be fixed in a separate patch, first:
[Alin Serdean] Yup I folded it in my patch when I should've sent a different
patch.
I acked your patch.
> https://mail.openvswitch.org/pipermail/ovs-dev/2018-
> November/353952.html
[Alin Serdean] Thanks a lot for the review and incremental patch!
> 
> --8<--cut here-->8--
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] Tests: Fix testing bridge - add port after stopping controller on Windows

[Alin Serdean]

Thanks for the suggestion Ben.

I was thinking about changing the regular open to a CreateFile on
Windows with the flag:
`
FILE_FLAG_DELETE_ON_CLOSE
0x0400

The file is to be deleted immediately after all of its handles are closed,
which includes the specified handle and any other open or
duplicated handles.

If there are existing open handles to a file, the call fails unless
they were all opened with the FILE_SHARE_DELETE share mode.

Subsequent open requests for the file fail, unless the
FILE_SHARE_DELETE share mode is specified.`

https://docs.microsoft.com/en-us/windows/desktop/api/FileAPI/nf-fileapi-createfilea

But both can work.

> -Mesaj original-
> De la: ovs-dev-boun...@openvswitch.org  boun...@openvswitch.org> În numele Ben Pfaff
> Trimis: Thursday, November 15, 2018 6:11 PM
> Către: aserd...@ovn.org
> Cc: d...@openvswitch.org
> Subiect: Re: [ovs-dev] [PATCH] Tests: Fix testing bridge - add port after
> stopping controller on Windows
> 
> On Thu, Nov 15, 2018 at 03:23:28PM +0200, aserd...@ovn.org wrote:
> > I have opened an issue for it:
> > https://github.com/openvswitch/ovs-issues/issues/165
> >
> > It's not test specific unfortunately. The file is put up for unlink on
> > exit, but I'm guessing someone still has an opened handle at that point.
> >
> > I'm applying this patch for the moment and following up with the
> > actual fix for the userspace.
> 
> Unix has similar-sounding issues with dangling Unix domain sockets.  On
> Unix, the customary way to solve it is to delete an existing socket before
> trying to create one with the same name.  You can see that in
> make_unix_socket() in socket-util-unix.c:
> 
> if (bind_path) {
> char linkname[MAX_UN_LEN + 1];
> struct sockaddr_un un;
> socklen_t un_len;
> int dirfd;
> 
> if (unlink(bind_path) && errno != ENOENT) {
> VLOG_WARN("unlinking \"%s\": %s\n",
>   bind_path, ovs_strerror(errno));
> }
> fatal_signal_add_file_to_unlink(bind_path);
> 
> If that approach is appropriate under Windows as well, it might be
> implemented something like this:
> diff --git a/lib/stream-windows.c b/lib/stream-windows.c index
> 34bc610b6f49..b027e48b4f8d 100644
> --- a/lib/stream-windows.c
> +++ b/lib/stream-windows.c
> @@ -616,6 +616,11 @@ pwindows_open(const char *name OVS_UNUSED,
> char *suffix,
>  path = xstrdup(suffix);
>  }
> 
> +/* Remove any existing named pipe. */
> +if (remove(bind_path) && errno != ENOENT) {
> +VLOG_WARN("removing \"%s\": %s\n", bind_path,
> ovs_strerror(errno));
> +}
> +
>  /* Try to create a file under the path location. */
>  FILE *file = fopen(path, "w");
>  if (!file) {
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] datapath-windows: Fix invalid reference in Buffermgmt.c

[Alin Serdean]

I applied the patch on master and as far as it goes without conflicts.

--
Alin.

> -Mesaj original-
> De la: ovs-dev-boun...@openvswitch.org  boun...@openvswitch.org> În numele aserd...@ovn.org
> Trimis: Thursday, November 15, 2018 4:52 PM
> Către: 'Sairam Venugopal' ; d...@openvswitch.org
> Subiect: Re: [ovs-dev] [PATCH] datapath-windows: Fix invalid reference in
> Buffermgmt.c
> 
> Thanks for the fix!
> 
> Acked-by: Alin Gabriel Serdean 
> 
> > -Mesaj original-
> > De la: ovs-dev-boun...@openvswitch.org  > boun...@openvswitch.org> În numele Sairam Venugopal
> > Trimis: Wednesday, November 14, 2018 10:07 PM
> > Către: d...@openvswitch.org
> > Subiect: [ovs-dev] [PATCH] datapath-windows: Fix invalid reference in
> > Buffermgmt.c
> >
> > OVS_BUFFER_CONTEXT gets cleared as part of
> > NdisFreeNetBufferListContext function call. This causes an invalid
> reference
> > error.
> >
> > Found while testing with driver verifier enabled.
> >
> > Signed-off-by: Sairam Venugopal 
> > ---
> >  datapath-windows/ovsext/BufferMgmt.c | 6 --
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/datapath-windows/ovsext/BufferMgmt.c b/datapath-
> > windows/ovsext/BufferMgmt.c index 448cd76..da5c04a 100644
> > --- a/datapath-windows/ovsext/BufferMgmt.c
> > +++ b/datapath-windows/ovsext/BufferMgmt.c
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] treewide: Fix spelling of "receive".

[Alin Serdean]

Also applied on branch-2.10.

—
Alin.

> On 27 Sep 2018, at 20:22, Ben Pfaff  wrote:
> 
> On Wed, Sep 26, 2018 at 11:03:26PM -0700, Justin Pettit wrote:
>> 
>>> On Sep 26, 2018, at 4:12 PM, Ben Pfaff  wrote:
>>> 
>>> Signed-off-by: Ben Pfaff 
>> 
>> Ha.
>> 
>> Acked-by: Justin Pettit 
> 
> Thanks, applied to master.
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/4] util: Fix abs_file_name() bugs on Windows.

[Alin Serdean]

On 3 Aug 2018, at 18:52, Ben Pfaff mailto:b...@ovn.org>> wrote:

On Fri, Aug 03, 2018 at 03:41:55PM +, Alin Serdean wrote:

abs_file_name() believed that a file name that begins with / or contains :
is absolute and that any other file name is relative.  On Windows, this is
wrong in at least the following ways:

  * / and \ are interchangeable on Windows.

  * A name that begins with \\ or // is also absolute.

  * A name that begins with X: but not X:\ is not absolute.

  * A name with : in some position other than the second position is
not absolute (although it might not be valid either?).

Furthermore, Windows has more than one current working directory (one
per volume letter), so trying to make a file name absolute by just prefixing
the current working directory for the current volume results in silliness.

This patch attempts to fix the problem.

Found by inspection.

CC: Alin Gabriel Serdean mailto:aserd...@ovn.org>>
Signed-off-by: Ben Pfaff mailto:b...@ovn.org>>
[Alin Serdean] Thanks a lot for the change Ben!
I was wondering if you can fold in the following so we can remove even more 
confusion:

That is nicer.  Thanks, I'll do that.

Are the changes to linker flags because PathIsRelative is in some
library not previously linked?

The changes are needed because PathIsRelative is included in shlwapi library 
which wasn’t previously included in the link step.

(https://docs.microsoft.com/en-us/windows/desktop/api/shlwapi/nf-shlwapi-pathisrelativea)

 Or are they independent changes?

Thanks,

Ben.

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/4] util: Fix abs_file_name() bugs on Windows.

[Alin Serdean]

> 
> abs_file_name() believed that a file name that begins with / or contains :
> is absolute and that any other file name is relative.  On Windows, this is
> wrong in at least the following ways:
> 
>* / and \ are interchangeable on Windows.
> 
>* A name that begins with \\ or // is also absolute.
> 
>* A name that begins with X: but not X:\ is not absolute.
> 
>* A name with : in some position other than the second position is
>  not absolute (although it might not be valid either?).
> 
> Furthermore, Windows has more than one current working directory (one
> per volume letter), so trying to make a file name absolute by just prefixing
> the current working directory for the current volume results in silliness.
> 
> This patch attempts to fix the problem.
> 
> Found by inspection.
> 
> CC: Alin Gabriel Serdean 
> Signed-off-by: Ben Pfaff 
[Alin Serdean] Thanks a lot for the change Ben!
I was wondering if you can fold in the following so we can remove even more 
confusion:
diff --git a/lib/util.c b/lib/util.c
index d3c62988b..082bc7bb0 100644
--- a/lib/util.c
+++ b/lib/util.c
@@ -38,6 +38,9 @@
 #ifdef HAVE_PTHREAD_SET_NAME_NP
 #include 
 #endif
+#ifdef _WIN32
+#include 
+#endif

 VLOG_DEFINE_THIS_MODULE(util);

@@ -1053,9 +1056,8 @@ bool
 is_file_name_absolute(const char *fn)
 {
 #ifdef _WIN32
-/* An absolute path begins with X:\ or \\. */
-return ((fn[0] && fn[1] == ':' && strchr("/\\", fn[2]))
-|| (strchr("/\\", fn[0]) && strchr("/\\", fn[1])));
+/* Use platform specific API */
+return !PathIsRelative(fn);
 #else
 /* An absolute path begins with /. */
 return fn[0] == '/';

diff --git a/Documentation/intro/install/windows.rst 
b/Documentation/intro/install/windows.rst
index 6d8c746c5..f696d2c9b 100644
--- a/Documentation/intro/install/windows.rst
+++ b/Documentation/intro/install/windows.rst
@@ -156,7 +156,7 @@ component installation directories, etc. For example:
 ::

$ ./configure CC=./build-aux/cccl LD="$(which link)" \
-   LIBS="-lws2_32 -liphlpapi -lwbemuuid -lole32 -loleaut32" \
+   LIBS="-lws2_32 -lShlwapi -liphlpapi -lwbemuuid -lole32 -loleaut32" \
--prefix="C:/openvswitch/usr" \
--localstatedir="C:/openvswitch/var" \
--sysconfdir="C:/openvswitch/etc" \
@@ -172,7 +172,7 @@ To configure with SSL support, add the requisite additional 
options:
 ::

$ ./configure CC=./build-aux/cccl LD="`which link`"  \
-   LIBS="-lws2_32 -liphlpapi -lwbemuuid -lole32 -loleaut32" \
+   LIBS="-lws2_32 -lShlwapi -liphlpapi -lwbemuuid -lole32 -loleaut32" \
--prefix="C:/openvswitch/usr" \
--localstatedir="C:/openvswitch/var"
--sysconfdir="C:/openvswitch/etc" \
@@ -184,7 +184,7 @@ Finally, to the kernel module also:
 ::

$ ./configure CC=./build-aux/cccl LD="`which link`" \
-   LIBS="-lws2_32 -liphlpapi -lwbemuuid -lole32 -loleaut32" \
+   LIBS="-lws2_32 -lShlwapi -liphlpapi -lwbemuuid -lole32 -loleaut32" \
--prefix="C:/openvswitch/usr" \
--localstatedir="C:/openvswitch/var" \
--sysconfdir="C:/openvswitch/etc" \

diff --git a/appveyor.yml b/appveyor.yml
index 49fcae4b5..2e5c37a37 100644
--- a/appveyor.yml
+++ b/appveyor.yml
@@ -41,6 +41,6 @@ build_script:
 - C:\MinGW\msys\1.0\bin\bash -lc "cp 
/c/pthreads-win32/Pre-built.2/dll/x86/*.dll /c/openvswitch/."
 - C:\MinGW\msys\1.0\bin\bash -lc "mv /bin/link.exe /bin/link_copy.exe"
 - C:\MinGW\msys\1.0\bin\bash -lc "cd /c/openvswitch && ./boot.sh"
-- C:\MinGW\msys\1.0\bin\bash -lc "cd /c/openvswitch && ./configure 
CC=build-aux/cccl LD=\"`which link`\" LIBS=\"-lws2_32 -liphlpapi -lwbemuuid 
-lole32 -loleaut32\" --with-pthread=C:/pthreads-win32/Pre-built.2 
--with-openssl=C:/OpenSSL-Win32 --with-vstudiotarget=\"Debug\"
+- C:\MinGW\msys\1.0\bin\bash -lc "cd /c/openvswitch && ./configure 
CC=build-aux/cccl LD=\"`which link`\" LIBS=\"-lws2_32 -lShlwapi -liphlpapi 
-lwbemuuid -lole32 -loleaut32\" --with-pthread=C:/pthreads-win32/Pre-built.2 
--with-openssl=C:/OpenSSL-Win32 --with-vstudiotarget=\"Debug\"
 - C:\MinGW\msys\1.0\bin\bash -lc "cd /c/openvswitch && make"
 - C:\MinGW\msys\1.0\bin\bash -lc "cd /c/openvswitch && make 
datapath_windows_analyze"

Either way:
Acked-by: Alin Gabriel Serdean 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/2] Datapath: Fix ovs_vport_init unreachable code and goto labels

[Alin Serdean]

> Subiect: Re: [ovs-dev] [PATCH 2/2] Datapath: Fix ovs_vport_init unreachable
> code and goto labels
> 
> I think the correct fix may be as follows, do you mind rechecking it?
> Thanks.
> 
> diff --git a/datapath/vport.c b/datapath/vport.c index
> 02f6b56d3243..fcf0fea0a245 100644
> --- a/datapath/vport.c
> +++ b/datapath/vport.c
> @@ -93,7 +93,6 @@ int ovs_vport_init(void)
> goto err_stt;
> 
> return 0;
> -   ovs_stt_cleanup_module();
>  err_stt:
> vxlan_cleanup_module();
>  err_vxlan:
> 
Thanks for the suggestion. I folded in the changes and sent out a v2:
https://patchwork.ozlabs.org/patch/946556/
Do you mind giving a Signed-off? 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCHv2] dpif-netlink: Add meter support.

[Alin Serdean]

The incremental looks good.

Thanks,
Alin.

Sent from phone 

> On 29 Jun 2018, at 20:32, Justin Pettit  wrote:
> 
> 
>>> On Jun 29, 2018, at 7:23 AM, Alin Serdean  
>>> wrote:
>>> 
>>> On 29 Jun 2018, at 07:02, Justin Pettit  wrote:
>>> 
>>> @@ -3040,6 +3297,9 @@ dpif_netlink_init(void)
>>>error = nl_lookup_genl_mcgroup(OVS_VPORT_FAMILY, 
>>> OVS_VPORT_MCGROUP,
>>>   _vport_mcgroup);
>>>}
>>> +if (!error) {
>>> +error = nl_lookup_genl_family(OVS_METER_FAMILY, 
>>> _meter_family);
>>> +}
>>> 
>>> 
>> It could be ifdef out since the the rest of the communication (get/set) will 
>> fail and the
>> userspace can compensate.
>> Please also add a to do comment so I can look on how to implement it later 
>> on.
> 
> That was my concern.  Does the incremental at the bottom handle both of those 
> concerns?  If not, can you give me an incremental patch that does what you 
> need?
> 
> Thanks,
> 
> --Justin
> 
> 
> -=-=-=-=-=-=-=-=-=-=-=-
> 
> diff --git a/Documentation/faq/releases.rst b/Documentation/faq/releases.rst
> index fab93b1888a6..d5f14e384155 100644
> --- a/Documentation/faq/releases.rst
> +++ b/Documentation/faq/releases.rst
> @@ -124,6 +124,7 @@ Q: Are all features available with all datapaths?
> Set actionYESYESYES   PARTIAL
> NIC Bonding   YESYESYES   YES
> Multiple VTEPsYESYESYES   YES
> +Meters4.15   YESYES   NO
> = == == = ===
> 
> Do note, however:
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index c79e78ef2ef8..fd176500849c 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -3297,9 +3297,11 @@ dpif_netlink_init(void)
> error = nl_lookup_genl_mcgroup(OVS_VPORT_FAMILY, 
> OVS_VPORT_MCGROUP,
>_vport_mcgroup);
> }
> +#ifndef _WIN32
> if (!error) {
> error = nl_lookup_genl_family(OVS_METER_FAMILY, 
> _meter_family);
> }
> +#endif
> 
> ovs_tunnels_out_of_tree = dpif_netlink_rtnl_probe_oot_tunnels();
> 
> 
> 
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCHv2] dpif-netlink: Add meter support.

[Alin Serdean]

On 29 Jun 2018, at 07:02, Justin Pettit 
mailto:jpet...@ovn.org>> wrote:

From: Andy Zhou mailto:az...@ovn.org>>

To work with kernel datapath that supports meter.

Signed-off-by: Andy Zhou mailto:az...@ovn.org>>
Signed-off-by: Justin Pettit mailto:jpet...@ovn.org>>
---
v1->v2: D'oh. Stupid typo.

This is Andy's patch.  I made a few fixes and cleanups, so I wouldn't
mind another set of eyes.

Also, this may have impact on the Windows port, since I don't believe it
has support for meters.  Unfortunately, I don't have a good way to test
Windows builds, so I would appreciate if someone from the Windows side
could look at this.
Unfortunately Windows does not have support for metering.
Note to self: I need to work on a way to make it easier for people to test this.
We still lack the ability to dynamically create families on Windows
(https://github.com/openvswitch/ovs/blob/master/datapath-windows/include/OvsDpInterfaceExt.h#L55-L60)
thus making the following change break datapath creation:

@@ -3040,6 +3297,9 @@ dpif_netlink_init(void)
error = nl_lookup_genl_mcgroup(OVS_VPORT_FAMILY, OVS_VPORT_MCGROUP,
   _vport_mcgroup);
}
+if (!error) {
+error = nl_lookup_genl_family(OVS_METER_FAMILY, _meter_family);
+}


It could be ifdef out since the the rest of the communication (get/set) will 
fail and the
userspace can compensate.
Please also add a to do comment so I can look on how to implement it later on.

Thanks,
Alin.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v4 2/3] datapath-windows: Implement locking in conntrack NAT.

[Alin Serdean]

> -Mesaj original-
> De la: ovs-dev-boun...@openvswitch.org  boun...@openvswitch.org> În numele Anand Kumar
> Trimis: Tuesday, June 19, 2018 8:33 PM
> Către: d...@openvswitch.org
> Subiect: [ovs-dev] [PATCH v4 2/3] datapath-windows: Implement locking in
> conntrack NAT.
> 
> This patch primarily replaces existing ndis RWlock based implementaion for
> NAT in conntrack with a spinlock based implementation inside NAT, module
> along with some conntrack optimization.
> 
> - The 'ovsNatTable' and 'ovsUnNatTable' tables are shared
>   between cleanup threads and packet processing thread.
>   In order to protect these two tables use a spinlock.
>   Also introduce counters to track number of nat entries.
> - Introduce a new function OvsGetTcpHeader() to retrieve TCP header
>   and payload length, to optimize for TCP traffic.
> - Optimize conntrack look up.
> - Remove 'bucketlockRef' member from conntrack entry structure.
> 
> Testing:
> Verified loading/unloading the driver with driver verified enabled.
> Ran TCP/UDP and ICMP traffic.
> 
> Signed-off-by: Anand Kumar 
> ---
> v1->v2: Merge patch 2 and 3 so that NAT locks related changes are in a
>   single patch.
> v2->v3: No change
> v3->v4: No change
> ---
>  datapath-windows/ovsext/Conntrack-ftp.c |   4 +-
>  datapath-windows/ovsext/Conntrack-nat.c |  27 +++-  datapath-
> windows/ovsext/Conntrack-tcp.c |  15 ++---
>  datapath-windows/ovsext/Conntrack.c | 110 +--
> -
>  datapath-windows/ovsext/Conntrack.h |  36 +++
>  5 files changed, 100 insertions(+), 92 deletions(-)
> 

Can you please fold in the following:
diff --git a/datapath-windows/ovsext/Conntrack-nat.c 
b/datapath-windows/ovsext/Conntrack-nat.c
index 11057e6ed..559a7f689 100644
--- a/datapath-windows/ovsext/Conntrack-nat.c
+++ b/datapath-windows/ovsext/Conntrack-nat.c
@@ -151,7 +151,8 @@ VOID OvsNatFlush(UINT16 zone)
 VOID OvsNatCleanup()
 {
 if (ovsNatTable == NULL) {
-   return;
+NdisFreeSpinLock();
+return;
 }

 NdisAcquireSpinLock();

The rest looks good.

Acked-by: Alin Gabriel Serdean 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2 0/3] Optimize conntrack performance

[Alin Serdean]

Thanks a lot for the series and the benchmarks .

This is not an actual review. I applied the patches and ran the code analysis
and I get the following:
ovs\datapath-windows\ovsext\conntrack-nat.c(151): warning C28167: The function
'OvsNatCleanup' changes the IRQL and does not restore the IRQL before it exits.
It should be annotated to reflect the change or the IRQL should be restored.
IRQL was last set at line 162.

ovs\datapath-windows\ovsext\conntrack-related.c(147): warning C28167:
The function 'OvsCtRelatedFlush' changes the IRQL and does not restore the IRQL
before it exits. It should be annotated to reflect the
change or the IRQL should be restored. IRQL was last set at line 163. 

ovs\datapath-windows\ovsext\conntrack-related.c(163): warning C6001:
Using uninitialized memory 'lockState'. 

ovs\datapath-windows\ovsext\conntrack-related.c(163): warning C26110: Caller
failing to hold lock 'ovsCtRelatedLockObj' before calling function
'NdisReleaseRWLock'.

ovs\datapath-windows\ovsext\conntrack-related.c(210): warning C28122: The
function 'NdisReleaseRWLock' is not permitted to be called at a low IRQ level.
Prior function calls are inconsistent with this constraint: 
It may be that the error is actually in some prior call that limited the range.
Maximum legal IRQL was last set to 1 at line 211. 

ovs\datapath-windows\ovsext\conntrack-related.c(176): warning C28166: The
function 'OvsCtRelatedEntryCleaner' does not restore the IRQL to the value that
was current at function entry and is required to do so.
IRQL was last set at line 210. 

ovs\datapath-windows\ovsext\conntrack-related.c(210): warning C6001: Using
uninitialized memory 'lockState'. 

ovs\datapath-windows\ovsext\conntrack-related.c(210): warning C26110: Caller
failing to hold lock 'ovsCtRelatedLockObj' before calling function
'NdisReleaseRWLock'.

Can you please add code annotations where needed?

Thanks,
Alin.

> -Mesaj original-
> De la: ovs-dev-boun...@openvswitch.org  boun...@openvswitch.org> În numele Anand Kumar
> Trimis: Tuesday, June 19, 2018 3:56 AM
> Către: d...@openvswitch.org
> Subiect: [ovs-dev] [PATCH v2 0/3] Optimize conntrack performance
> 
> This patch series is primarily to refactor conntrack code for better 
> throughput
> with conntrack.
> 
> With this patch series TCP throughput with conntrack increased by ~50%.
> 
> Anand Kumar (3):
>   datapath-windows: Use spinlock instead of RW lock for ct entry
>   datapath-windows: Implement locking in conntrack NAT.
>   datapath-windows: Compute ct hash based on 5-tuple and zone
> 
>  datapath-windows/ovsext/Conntrack-ftp.c |   4 +-
>  datapath-windows/ovsext/Conntrack-nat.c |  34 +-
>  datapath-windows/ovsext/Conntrack-related.c |  17 +-
>  datapath-windows/ovsext/Conntrack-tcp.c |  15 +-
>  datapath-windows/ovsext/Conntrack.c | 469 +
> ---
>  datapath-windows/ovsext/Conntrack.h |  40 ++-
>  datapath-windows/ovsext/Util.h  |  18 ++
>  7 files changed, 308 insertions(+), 289 deletions(-)
> 
> --
> 2.9.3.windows.1
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] datapath-windows: Add support for handling DEI bit of VLAN header

[Alin Serdean]

> > On 16 May 2018, at 02:38, Anand Kumar 
> wrote:
> >
> > The Drop eligible indicator(DEI) is 1 bit wide and it is part of Tag
> > control information (TCI) in VLAN header, which indicates that the
> > frame can be dropped during congestion.
> >
> > Signed-off-by: Anand Kumar 
> > ---
> > datapath-windows/ovsext/Actions.c |  1 +
> > datapath-windows/ovsext/User.c| 19 +--
> > 2 files changed, 14 insertions(+), 6 deletions(-)
> >
> > diff --git a/datapath-windows/ovsext/Actions.c
> > b/datapath-windows/ovsext/Actions.c
> > index 9bbc787..6922f05 100644
> > --- a/datapath-windows/ovsext/Actions.c
> > +++ b/datapath-windows/ovsext/Actions.c
> > @@ -2023,6 +2023,7 @@ OvsDoExecuteActions(POVS_SWITCH_CONTEXT
> switchContext,
> >  vlan = (struct ovs_action_push_vlan *)NlAttrGet((const
> PNL_ATTR)a);
> >  vlanTag->TagHeader.VlanId = ntohs(vlan->vlan_tci) & 0xfff;
> >  vlanTag->TagHeader.UserPriority =
> > ntohs(vlan->vlan_tci) >> 13;
> > + vlanTag->TagHeader.CanonicalFormatId =
> > + (ntohs(vlan->vlan_tci) >> 12) & 0x1;
> >
> >  NET_BUFFER_LIST_INFO(ovsFwdCtx.curNbl,
> >   Ieee8021QNetBufferListInfo) =
> > vlanTagValue; diff --git a/datapath-windows/ovsext/User.c
> > b/datapath-windows/ovsext/User.c index 4693a8b..509472f 100644
> > --- a/datapath-windows/ovsext/User.c
> > +++ b/datapath-windows/ovsext/User.c
> > @@ -1000,11 +1000,12 @@ OvsCreateQueueNlPacket(PVOID userData,
> >POVS_PACKET_HDR_INFO hdrInfo) { #define
> > VLAN_TAG_SIZE 4
> > -UINT32 allocLen, dataLen, extraLen;
> > +UINT32 allocLen, dataLen, extraLen = 0;
> > POVS_PACKET_QUEUE_ELEM elem;
> > UINT8 *src, *dst;
> > NDIS_TCP_IP_CHECKSUM_NET_BUFFER_LIST_INFO csumInfo;
> > -NDIS_NET_BUFFER_LIST_8021Q_INFO vlanInfo;
> > +PNDIS_NET_BUFFER_LIST_8021Q_INFO vlanInfo = NULL;
> > +PVOID vlanTag;
> > OvsIPv4TunnelKey *tunnelKey = (OvsIPv4TunnelKey *)>tunKey;
> > UINT32 pid;
> > UINT32 nlMsgSize;
> > @@ -1037,8 +1038,13 @@ OvsCreateQueueNlPacket(PVOID userData,
> > return NULL;
> > }
> >
> > -vlanInfo.Value = NET_BUFFER_LIST_INFO(nbl,
> Ieee8021QNetBufferListInfo);
> > -extraLen = vlanInfo.TagHeader.VlanId ? VLAN_TAG_SIZE : 0;
> > +vlanTag = NET_BUFFER_LIST_INFO(nbl, Ieee8021QNetBufferListInfo);
> > +if (vlanTag) {
> > +vlanInfo = (PNDIS_NET_BUFFER_LIST_8021Q_INFO)(PVOID
> *)
> > +if (vlanInfo->Value) {
> > +extraLen = VLAN_TAG_SIZE;
> > +}
> > +}
> >
> > dataLen = NET_BUFFER_DATA_LENGTH(nb);
> >
> > @@ -1148,8 +1154,9 @@ OvsCreateQueueNlPacket(PVOID userData,
> > ((UINT32 *)dst)[2] = ((UINT32 *)src)[2];
> > dst += 12;
> > ((UINT16 *)dst)[0] = htons(0x8100);
> > -((UINT16 *)dst)[1] = htons(vlanInfo.TagHeader.VlanId |
> > -(vlanInfo.TagHeader.UserPriority << 13));
> > +((UINT16 *)dst)[1] = htons(vlanInfo->TagHeader.VlanId |
> > +(vlanInfo->TagHeader.CanonicalFormatId << 12) |
> > +(vlanInfo->TagHeader.UserPriority << 13));
> > elem->hdrInfo.l3Offset += VLAN_TAG_SIZE;
> > elem->hdrInfo.l4Offset += VLAN_TAG_SIZE;
> > ovsUserStats.vlanInsert++;
> > --
> > 2.9.3.windows.1
> >
> > ___
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 
> Acked-by: Alin Gabriel Serdean 
> 
Thanks Anand, I applied this on master!
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] datapath-windows: Prevent ct-counters from getting redundantly incremented

[Alin Serdean]

> On 27 Apr 2018, at 20:00, Anand Kumar  wrote:
> 
> The conntrack-counters ought to be incremented only if it's a new lookup
> or if it's recirculated through a different zone for the first time.
> 
> Signed-off-by: Anand Kumar 

Applied on master. Thanks Anand!

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovsdb-client: Set binary on FDs when doing backup/restore

[Alin Serdean]

Please disregard this patch. Copy/paste error 

> -Mesaj original-
> De la: ovs-dev-boun...@openvswitch.org  boun...@openvswitch.org> În numele Alin Gabriel Serdean
> Trimis: Thursday, March 8, 2018 8:09 PM
> Către: d...@openvswitch.org
> Cc: Alin Gabriel Serdean 
> Subiect: [ovs-dev] [PATCH] ovsdb-client: Set binary on FDs when doing
> backup/restore
> 
> Add some needed consitentcy on Windows for STD_IN/OUT file descriptors
> when doing backup and restore.
> 
> Reported-at:https://mail.openvswitch.org/pipermail/ovs-dev/2018-
> January/343518.html
> Suggested-by: Ben Pfaff 
> Signed-off-by: Alin Gabriel Serdean 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] datapath-windows: Support to selectively compile targets

[Alin Serdean]

Let's have both .

@Shashank Ram can you please update the documentation
to accommodate for the new `vstudiotargetver` option.

Thanks,
Alin.

-Mesaj original-
De la: Shashank Ram [mailto:r...@vmware.com] 
Trimis: Tuesday, February 20, 2018 7:29 PM
Către: Alin Balutoiu <abalut...@cloudbasesolutions.com>; Anand Kumar 
<kumaran...@vmware.com>; Alin Serdean <aserd...@cloudbasesolutions.com>; 
d...@openvswitch.org
Subiect: Re: [ovs-dev] [PATCH] datapath-windows: Support to selectively compile 
targets

That is not consistent with how we specify the Debug/Release flag. There is no 
need to do the whole configure if you specify the targets correctly. Running 
multiple make commands is confusing for the user in terms of the current 
workflow.

Thanks,
Shashank


From: Alin Balutoiu <abalut...@cloudbasesolutions.com>
Sent: Tuesday, February 20, 2018 2:58:06 AM
To: Anand Kumar; Shashank Ram; Alin Serdean; d...@openvswitch.org
Subject: RE: [ovs-dev] [PATCH] datapath-windows: Support to selectively compile 
targets

I agree with having the target specified during make and not during configure.

There is no reason to re-do the whole configure process (as Alin mentioned that 
it is particularly slow on Windows) only to change the target version.



Thanks,

Alin Balutoiu



> -Original Message-

> From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev-

> boun...@openvswitch.org] On Behalf Of Anand Kumar

> Sent: Tuesday, February 13, 2018 6:34 AM

> To: Shashank Ram <r...@vmware.com>; Alin Serdean

> <aserd...@cloudbasesolutions.com>; d...@openvswitch.org

> Subject: Re: [ovs-dev] [PATCH] datapath-windows: Support to selectively

> compile targets

>

> Hi,

>

> My thoughts are with Shashank on this, it makes sense to have 1 configure and 
> 1

> make command to build a particular target, instead of having flexibility to

> specify multiple targets.

>

> Thanks,

> Anand Kumar

>

> On 2/8/18, 10:56 AM, "ovs-dev-boun...@openvswitch.org on behalf of

> Shashank Ram" <ovs-dev-boun...@openvswitch.org on behalf of

> r...@vmware.com> wrote:

>

>

>

>

>

> 

> From: aserd...@ovn.org <aserd...@ovn.org>

> Sent: Thursday, February 8, 2018 10:43 AM

> To: Shashank Ram; aserd...@ovn.org; d...@openvswitch.org

> Subject: RE: [ovs-dev] [PATCH] datapath-windows: Support to selectively

> compile targets

>

> Trimming the message a bit.

>

> -Mesaj original-

> De la: ovs-dev-boun...@openvswitch.org

> [mailto:ovs-dev-boun...@openvswitch.org] În numele Shashank Ram

> Trimis: Thursday, February 8, 2018 7:50 PM

> Către: aserd...@ovn.org; d...@openvswitch.org

> Subiect: Re: [ovs-dev] [PATCH] datapath-windows: Support to selectively

> compile targets

>

> Hi Alin, thanks for the review.

> I personally feel we should be consistent and run configure, and have a

> single make command to build both user space and kernel. What part did you

> find complicated?

> [Alin Serdean] I.e. if I configure to target 8. And after I need to target

> 10 I need to do a reconfigure (similar, for debug and or other platforms).

> [SR]: In an automated environment, this shouldn't happen.

> For local compilation, you should be able to manually compile the kernel.

>

> The configure part is particularly slow on Windows.

> For convenience the old part with selecting Debug/Release and

> trying to build for all the compilers found in the system is still there, 
> so

> building both

> userspace and kernel will still be in a single command.

> I don't see a huge issue to specify two or more make commands to build a

> particular target of the kernel

> via the shell.

> [SR]: I don't think its a big deal either, but its more convenient to run 
> 1

> configure and 1

> make command.

>

> I prefer to keep this as is for now and wait for more reviews.

>

> ___

> dev mailing list

> d...@openvswitch.org

> https://urldefense.proofpoint.com/v2/url?u=https-

> 3A__mail.openvswitch.org_mailman_listinfo_ovs-

> 2Ddev=DwIFBA=uilaK90D4TOVoH58JNXRgQ=Q5z9tBe-

> nAOpE7LIHSPV8uy5-

> 437agMXvkeHHMkR8Us=fKK6KZRD0tZEfwHzLhMsabCH5aXzzYiRP-

> pJR20Xj9o=nEl_7Q-LhJ74AdsiY85DjA-kWy0uESr5DyFrWDQYKjs=

>

>

> ___

> dev mailing list

> d...@openvswitch.org

> https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev=DwIGaQ=uilaK90D4TOVoH58JNXRgQ=6OuVHk-mnufSWzkKa74UkQ=0mrz2IZApdtvtjOTZbZRQCPxRCib7Tbkwj_B20vv1cI=NXY8REuxaZfNEMFuCbcesEoSMUjNza2Xyn8D6O7NamE=

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] datapath-windows: Add trace level logs in conntrack for invalid ct state.

[Alin Serdean]

Applied on master. Ty!

-Mesaj original-
De la: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev-boun...@openvswitch.org] 
În numele aserd...@ovn.org
Trimis: Saturday, February 3, 2018 12:48 AM
Către: 'Anand Kumar' ; d...@openvswitch.org
Subiect: Re: [ovs-dev] [PATCH v2] datapath-windows: Add trace level logs in 
conntrack for invalid ct state.

Much better 

Acked-by: Alin Gabriel Serdean 

-Mesaj original-
De la: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev-boun...@openvswitch.org] 
În numele Anand Kumar
Trimis: Saturday, February 3, 2018 12:43 AM
Către: d...@openvswitch.org
Subiect: [ovs-dev] [PATCH v2] datapath-windows: Add trace level logs in 
conntrack for invalid ct state.

Signed-off-by: Anand Kumar 
---
 datapath-windows/ovsext/Conntrack-icmp.c | 1 +  
datapath-windows/ovsext/Conntrack-tcp.c  | 4 
 datapath-windows/ovsext/Conntrack.c  | 6 ++
 3 files changed, 11 insertions(+)

diff --git a/datapath-windows/ovsext/Conntrack-icmp.c 
b/datapath-windows/ovsext/Conntrack-icmp.c
index 4da0665..28fe2bf 100644
--- a/datapath-windows/ovsext/Conntrack-icmp.c
+++ b/datapath-windows/ovsext/Conntrack-icmp.c
@@ -61,6 +61,7 @@ BOOLEAN
 OvsConntrackValidateIcmpPacket(const ICMPHdr *icmp)  {
 if (!icmp) {
+OVS_LOG_TRACE("Invalid ICMP packet detected, header cannot be 
+ NULL");
 return FALSE;
 }
 
diff --git a/datapath-windows/ovsext/Conntrack-tcp.c 
b/datapath-windows/ovsext/Conntrack-tcp.c
index f8e85a2..8cbab24 100644
--- a/datapath-windows/ovsext/Conntrack-tcp.c
+++ b/datapath-windows/ovsext/Conntrack-tcp.c
@@ -444,12 +444,14 @@ BOOLEAN
 OvsConntrackValidateTcpPacket(const TCPHdr *tcp)  {
 if (!tcp) {
+OVS_LOG_TRACE("Invalid TCP packet detected, header cannot be 
+ NULL");
 return FALSE;
 }
 
 UINT16 tcp_flags = ntohs(tcp->flags);
 
 if (OvsCtInvalidTcpFlags(tcp_flags)) {
+OVS_LOG_TRACE("Invalid TCP packet detected, tcp_flags %hu", 
+ tcp_flags);
 return FALSE;
 }
 
@@ -457,6 +459,8 @@ OvsConntrackValidateTcpPacket(const TCPHdr *tcp)
  * totally new connections (syn) or already established, not partially
  * open (syn+ack). */
 if ((tcp_flags & TCP_SYN) && (tcp_flags & TCP_ACK)) {
+OVS_LOG_TRACE("Invalid TCP packet detected, SYN+ACK flags not allowed,"
+  "tcp_flags %hu", tcp_flags);
 return FALSE;
 }
 
diff --git a/datapath-windows/ovsext/Conntrack.c 
b/datapath-windows/ovsext/Conntrack.c
index 43c9dd3..678bedb 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -317,6 +317,10 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx,
 const ICMPHdr *icmp;
 icmp = OvsGetIcmp(curNbl, l4Offset, );
 if (!OvsConntrackValidateIcmpPacket(icmp)) {
+if(icmp) {
+OVS_LOG_TRACE("Invalid ICMP packet detected, icmp->type %u",
+  icmp->type);
+}
 state = OVS_CS_F_INVALID;
 break;
 }
@@ -334,6 +338,8 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx,
 break;
 }
 default:
+OVS_LOG_TRACE("Invalid packet detected, protocol not supported"
+  " ipProto %u", ipProto);
 state = OVS_CS_F_INVALID;
 break;
 }
--
2.9.3.windows.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] datapath-windows: Add trace level logs in conntrack for invalid ct state.

[Alin Serdean]

Looks good just a small nit:
s/syn/SYN/g
s/ack/ACK/g

Also I would prefer if you drop the text `Invalid!`. Either just remove it or 
expand it, i.e.:
"Invalid! ICMPhdr cannot be NULL" => "Invalid ICMP packet detected the header 
cannot be NULL"

Thanks,
Alin.

-Mesaj original-
De la: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev-boun...@openvswitch.org] 
În numele Anand Kumar
Trimis: Friday, February 2, 2018 11:19 PM
Către: d...@openvswitch.org
Subiect: [ovs-dev] [PATCH] datapath-windows: Add trace level logs in conntrack 
for invalid ct state.

Signed-off-by: Anand Kumar 
---
 datapath-windows/ovsext/Conntrack-icmp.c | 1 +  
datapath-windows/ovsext/Conntrack-tcp.c  | 4 
 datapath-windows/ovsext/Conntrack.c  | 4 
 3 files changed, 9 insertions(+)

diff --git a/datapath-windows/ovsext/Conntrack-icmp.c 
b/datapath-windows/ovsext/Conntrack-icmp.c
index 4da0665..d86feed 100644
--- a/datapath-windows/ovsext/Conntrack-icmp.c
+++ b/datapath-windows/ovsext/Conntrack-icmp.c
@@ -61,6 +61,7 @@ BOOLEAN
 OvsConntrackValidateIcmpPacket(const ICMPHdr *icmp)  {
 if (!icmp) {
+OVS_LOG_TRACE("Invalid! ICMPhdr cannot be NULL");
 return FALSE;
 }
 
diff --git a/datapath-windows/ovsext/Conntrack-tcp.c 
b/datapath-windows/ovsext/Conntrack-tcp.c
index f8e85a2..65eaac5 100644
--- a/datapath-windows/ovsext/Conntrack-tcp.c
+++ b/datapath-windows/ovsext/Conntrack-tcp.c
@@ -444,12 +444,14 @@ BOOLEAN
 OvsConntrackValidateTcpPacket(const TCPHdr *tcp)  {
 if (!tcp) {
+OVS_LOG_TRACE("Invalid! TCPHdr cannot be NULL");
 return FALSE;
 }
 
 UINT16 tcp_flags = ntohs(tcp->flags);
 
 if (OvsCtInvalidTcpFlags(tcp_flags)) {
+OVS_LOG_TRACE("Invalid! tcp_flags %hu", tcp_flags);
 return FALSE;
 }
 
@@ -457,6 +459,8 @@ OvsConntrackValidateTcpPacket(const TCPHdr *tcp)
  * totally new connections (syn) or already established, not partially
  * open (syn+ack). */
 if ((tcp_flags & TCP_SYN) && (tcp_flags & TCP_ACK)) {
+OVS_LOG_TRACE("Invalid! syn+ack flags not allowed, tcp_flags %hu",
+  tcp_flags);
 return FALSE;
 }
 
diff --git a/datapath-windows/ovsext/Conntrack.c 
b/datapath-windows/ovsext/Conntrack.c
index 43c9dd3..7e413c6 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -317,6 +317,9 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx,
 const ICMPHdr *icmp;
 icmp = OvsGetIcmp(curNbl, l4Offset, );
 if (!OvsConntrackValidateIcmpPacket(icmp)) {
+if(icmp) {
+OVS_LOG_TRACE("Invalid! icmp->type %u", icmp->type);
+}
 state = OVS_CS_F_INVALID;
 break;
 }
@@ -334,6 +337,7 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx,
 break;
 }
 default:
+OVS_LOG_TRACE("Invalid! Not supported protocol, ipProto %u", 
+ ipProto);
 state = OVS_CS_F_INVALID;
 break;
 }
--
2.9.3.windows.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v4 3/3] datapath-windows: Optimize conntrack lock implementation.

[Alin Serdean]

Thanks for the series Anand.

I folded the following to suppress the warning:
diff --git a/datapath-windows/ovsext/Conntrack.c 
b/datapath-windows/ovsext/Conntrack.c
index c90c00074..43c9dd319 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -151,6 +151,9 @@ OvsCleanupConntrack(VOID)
 }

 for (UINT32 i = 0; i < CT_HASH_TABLE_SIZE; i++) {
+/* Disabling the uninitialized memory warning because it should
+ * always be initialized during OvsInitConntrack */
+#pragma warning(suppress: 6001)
 if (ovsCtBucketLock[i] != NULL) {
 NdisFreeRWLock(ovsCtBucketLock[i]);
 }
and applied it on master.

Alin.

-Mesaj original-
De la: Alin Serdean 
Trimis: Friday, February 2, 2018 10:52 PM
Către: Anand Kumar <kumaran...@vmware.com>; d...@openvswitch.org
Subiect: RE: [ovs-dev] [PATCH v4 3/3] datapath-windows: Optimize conntrack lock 
implementation.

Acked-by: Alin Gabriel Serdean <aserd...@ovn.org>

-Mesaj original-
De la: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev-boun...@openvswitch.org] 
În numele Anand Kumar
Trimis: Monday, January 29, 2018 8:28 PM
Către: d...@openvswitch.org
Subiect: [ovs-dev] [PATCH v4 3/3] datapath-windows: Optimize conntrack lock 
implementation.

Currently, there is one global lock for conntrack module, which protects 
conntrack entries and conntrack table. All the NAT operations are performed 
holding this lock.

This becomes inefficient, as the number of conntrack entries grow.
With new implementation, we will have two PNDIS_RW_LOCK_EX locks in conntrack.

1. ovsCtBucketLock - one rw lock per bucket of the conntrack table, which is 
shared by all the ct entries that belong to the same bucket.
2. lock -  a rw lock in OVS_CT_ENTRY structure that protects the members of 
conntrack entry.

Also, OVS_CT_ENTRY structure will have a lock reference(bucketLockRef) to the 
corresponding OvsCtBucketLock of conntrack table.
We need this reference to retrieve ovsCtBucketLock from ct entry for delete 
operation.

Signed-off-by: Anand Kumar <kumaran...@vmware.com>
---
v1->v2: Address potential memory leak in conntrack initialization.
v2->v3: Fix invalid memory access after deleting ct entry.
v3->v4: Address warning "uninitialized memory"
---
 datapath-windows/ovsext/Conntrack-nat.c |   6 +
 datapath-windows/ovsext/Conntrack.c | 233 
 datapath-windows/ovsext/Conntrack.h |   3 +
 3 files changed, 157 insertions(+), 85 deletions(-)

diff --git a/datapath-windows/ovsext/Conntrack-nat.c 
b/datapath-windows/ovsext/Conntrack-nat.c
index 7975770..316c946 100644
--- a/datapath-windows/ovsext/Conntrack-nat.c
+++ b/datapath-windows/ovsext/Conntrack-nat.c
@@ -167,12 +167,16 @@ OvsNatPacket(OvsForwardingContext *ovsFwdCtx,  {
 UINT32 natFlag;
 const struct ct_endpoint* endpoint;
+LOCK_STATE_EX lockState;
+/* XXX: Move conntrack locks out of NAT after implementing lock in NAT. */
+NdisAcquireRWLockRead(entry->lock, , 0);
 /* When it is NAT, only entry->rev_key contains NATTED address;
When it is unNAT, only entry->key contains the UNNATTED address;*/
 const OVS_CT_KEY *ctKey = reverse ? >key : >rev_key;
 BOOLEAN isSrcNat;
 
 if (!(natAction & (NAT_ACTION_SRC | NAT_ACTION_DST))) {
+NdisReleaseRWLock(entry->lock, );
 return;
 }
 isSrcNat = (((natAction & NAT_ACTION_SRC) && !reverse) || @@ -202,6 +206,7 
@@ OvsNatPacket(OvsForwardingContext *ovsFwdCtx,
 }
 } else if (ctKey->dl_type == htons(ETH_TYPE_IPV6)){
 // XXX: IPv6 packet not supported yet.
+NdisReleaseRWLock(entry->lock, );
 return;
 }
 if (natAction & (NAT_ACTION_SRC_PORT | NAT_ACTION_DST_PORT)) { @@ -215,6 
+220,7 @@ OvsNatPacket(OvsForwardingContext *ovsFwdCtx,
 }
 }
 }
+NdisReleaseRWLock(entry->lock, );
 }
 
 
diff --git a/datapath-windows/ovsext/Conntrack.c 
b/datapath-windows/ovsext/Conntrack.c
index 7d56a50..c90c000 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -31,7 +31,7 @@
 KSTART_ROUTINE OvsConntrackEntryCleaner;  static PLIST_ENTRY 
ovsConntrackTable;  static OVS_CT_THREAD_CTX ctThreadCtx; -static 
PNDIS_RW_LOCK_EX ovsConntrackLockObj;
+static PNDIS_RW_LOCK_EX *ovsCtBucketLock = NULL;
 static PNDIS_RW_LOCK_EX ovsCtNatLockObj;  extern POVS_SWITCH_CONTEXT 
gOvsSwitchContext;  static LONG ctTotalEntries; @@ -49,20 +49,14 @@ 
MapNlToCtTuple(POVS_MESSAGE msgIn, PNL_ATTR attr,  NTSTATUS  
OvsInitConntrack(POVS_SWITCH_CONTEXT context)  {
-NTSTATUS status;
+NTSTATUS status = STATUS_SUCCESS;
 HANDLE threadHandle = NULL;
 ctTotalEntries = 0;
+UINT32 numBucketLocks = CT_HASH_TABLE_SIZE;
 
 /* Init the sync-lock */
-ovsConntrackLockObj = NdisAllocateRWL

Re: [ovs-dev] [PATCH v4 3/3] datapath-windows: Optimize conntrack lock implementation.

[Alin Serdean]

Acked-by: Alin Gabriel Serdean 

-Mesaj original-
De la: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev-boun...@openvswitch.org] 
În numele Anand Kumar
Trimis: Monday, January 29, 2018 8:28 PM
Către: d...@openvswitch.org
Subiect: [ovs-dev] [PATCH v4 3/3] datapath-windows: Optimize conntrack lock 
implementation.

Currently, there is one global lock for conntrack module, which protects 
conntrack entries and conntrack table. All the NAT operations are performed 
holding this lock.

This becomes inefficient, as the number of conntrack entries grow.
With new implementation, we will have two PNDIS_RW_LOCK_EX locks in conntrack.

1. ovsCtBucketLock - one rw lock per bucket of the conntrack table, which is 
shared by all the ct entries that belong to the same bucket.
2. lock -  a rw lock in OVS_CT_ENTRY structure that protects the members of 
conntrack entry.

Also, OVS_CT_ENTRY structure will have a lock reference(bucketLockRef) to the 
corresponding OvsCtBucketLock of conntrack table.
We need this reference to retrieve ovsCtBucketLock from ct entry for delete 
operation.

Signed-off-by: Anand Kumar 
---
v1->v2: Address potential memory leak in conntrack initialization.
v2->v3: Fix invalid memory access after deleting ct entry.
v3->v4: Address warning "uninitialized memory"
---
 datapath-windows/ovsext/Conntrack-nat.c |   6 +
 datapath-windows/ovsext/Conntrack.c | 233 
 datapath-windows/ovsext/Conntrack.h |   3 +
 3 files changed, 157 insertions(+), 85 deletions(-)

diff --git a/datapath-windows/ovsext/Conntrack-nat.c 
b/datapath-windows/ovsext/Conntrack-nat.c
index 7975770..316c946 100644
--- a/datapath-windows/ovsext/Conntrack-nat.c
+++ b/datapath-windows/ovsext/Conntrack-nat.c
@@ -167,12 +167,16 @@ OvsNatPacket(OvsForwardingContext *ovsFwdCtx,  {
 UINT32 natFlag;
 const struct ct_endpoint* endpoint;
+LOCK_STATE_EX lockState;
+/* XXX: Move conntrack locks out of NAT after implementing lock in NAT. */
+NdisAcquireRWLockRead(entry->lock, , 0);
 /* When it is NAT, only entry->rev_key contains NATTED address;
When it is unNAT, only entry->key contains the UNNATTED address;*/
 const OVS_CT_KEY *ctKey = reverse ? >key : >rev_key;
 BOOLEAN isSrcNat;
 
 if (!(natAction & (NAT_ACTION_SRC | NAT_ACTION_DST))) {
+NdisReleaseRWLock(entry->lock, );
 return;
 }
 isSrcNat = (((natAction & NAT_ACTION_SRC) && !reverse) || @@ -202,6 +206,7 
@@ OvsNatPacket(OvsForwardingContext *ovsFwdCtx,
 }
 } else if (ctKey->dl_type == htons(ETH_TYPE_IPV6)){
 // XXX: IPv6 packet not supported yet.
+NdisReleaseRWLock(entry->lock, );
 return;
 }
 if (natAction & (NAT_ACTION_SRC_PORT | NAT_ACTION_DST_PORT)) { @@ -215,6 
+220,7 @@ OvsNatPacket(OvsForwardingContext *ovsFwdCtx,
 }
 }
 }
+NdisReleaseRWLock(entry->lock, );
 }
 
 
diff --git a/datapath-windows/ovsext/Conntrack.c 
b/datapath-windows/ovsext/Conntrack.c
index 7d56a50..c90c000 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -31,7 +31,7 @@
 KSTART_ROUTINE OvsConntrackEntryCleaner;  static PLIST_ENTRY 
ovsConntrackTable;  static OVS_CT_THREAD_CTX ctThreadCtx; -static 
PNDIS_RW_LOCK_EX ovsConntrackLockObj;
+static PNDIS_RW_LOCK_EX *ovsCtBucketLock = NULL;
 static PNDIS_RW_LOCK_EX ovsCtNatLockObj;  extern POVS_SWITCH_CONTEXT 
gOvsSwitchContext;  static LONG ctTotalEntries; @@ -49,20 +49,14 @@ 
MapNlToCtTuple(POVS_MESSAGE msgIn, PNL_ATTR attr,  NTSTATUS  
OvsInitConntrack(POVS_SWITCH_CONTEXT context)  {
-NTSTATUS status;
+NTSTATUS status = STATUS_SUCCESS;
 HANDLE threadHandle = NULL;
 ctTotalEntries = 0;
+UINT32 numBucketLocks = CT_HASH_TABLE_SIZE;
 
 /* Init the sync-lock */
-ovsConntrackLockObj = NdisAllocateRWLock(context->NdisFilterHandle);
-if (ovsConntrackLockObj == NULL) {
-return STATUS_INSUFFICIENT_RESOURCES;
-}
-
 ovsCtNatLockObj = NdisAllocateRWLock(context->NdisFilterHandle);
 if (ovsCtNatLockObj == NULL) {
-NdisFreeRWLock(ovsConntrackLockObj);
-ovsConntrackLockObj = NULL;
 return STATUS_INSUFFICIENT_RESOURCES;
 }
 
@@ -71,15 +65,27 @@ OvsInitConntrack(POVS_SWITCH_CONTEXT context)
  * CT_HASH_TABLE_SIZE,
  OVS_CT_POOL_TAG);
 if (ovsConntrackTable == NULL) {
-NdisFreeRWLock(ovsConntrackLockObj);
-ovsConntrackLockObj = NULL;
 NdisFreeRWLock(ovsCtNatLockObj);
 ovsCtNatLockObj = NULL;
 return STATUS_INSUFFICIENT_RESOURCES;
 }
 
-for (int i = 0; i < CT_HASH_TABLE_SIZE; i++) {
+ovsCtBucketLock = OvsAllocateMemoryWithTag(sizeof(PNDIS_RW_LOCK_EX)
+   * CT_HASH_TABLE_SIZE,
+   OVS_CT_POOL_TAG);
+if 

Re: [ovs-dev] [PATCH v3 3/3] datapath-windows: Optimize conntrack lock implementation.

[Alin Serdean]

Trimming the patch a bit.

Just one small nit from the static analyzer inlined.

Rest looks good.

Acked-by: Alin Gabriel Serdean <aserd...@ovn.org>
<--8<-->
 /*
@@ -124,12 +135,9 @@ OvsInitConntrack(POVS_SWITCH_CONTEXT context)  VOID
 OvsCleanupConntrack(VOID)
 {
-LOCK_STATE_EX lockState, lockStateNat;
-NdisAcquireRWLockWrite(ovsConntrackLockObj, , 0);
+LOCK_STATE_EX lockStateNat;
 ctThreadCtx.exit = 1;
 KeSetEvent(, 0, FALSE);
-NdisReleaseRWLock(ovsConntrackLockObj, );
-
 KeWaitForSingleObject(ctThreadCtx.threadObject, Executive,
   KernelMode, FALSE, NULL);
 ObDereferenceObject(ctThreadCtx.threadObject);
@@ -142,8 +150,14 @@ OvsCleanupConntrack(VOID)
 ovsConntrackTable = NULL;
 }
 
-NdisFreeRWLock(ovsConntrackLockObj);
-ovsConntrackLockObj = NULL;
+for (UINT32 i = 0; i < CT_HASH_TABLE_SIZE; i++) {
+if (ovsCtBucketLock[i] != NULL) {
[Alin Serdean] datapath-windows\ovsext\conntrack.c(154): warning C6001: Using 
uninitialized memory '*ovsCtBucketLock'.
+NdisFreeRWLock(ovsCtBucketLock[i]);
+}
+}
+OvsFreeMemoryWithTag(ovsCtBucketLock, OVS_CT_POOL_TAG);
+ovsCtBucketLock = NULL;
+
 NdisAcquireRWLockWrite(ovsCtNatLockObj, , 0);
 OvsNatCleanup();
 NdisReleaseRWLock(ovsCtNatLockObj, ); @@ -179,11 +193,20 @@ 
OvsCtUpdateFlowKey(struct OvsFlowKey *key,
 }
 }
 
<--8<-->
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/5] ovsdb-idl: Improve comments.

[Alin Serdean]

> -Original Message-
> From: Ben Pfaff [mailto:b...@ovn.org]
> Sent: Tuesday, December 12, 2017 12:30 AM
> To: Alin Serdean <aserd...@cloudbasesolutions.com>
> Cc: d...@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH 1/5] ovsdb-idl: Improve comments.
> 
> On Mon, Dec 11, 2017 at 08:23:44PM +, Alin Serdean wrote:
> > LGTM. Just two nits.
> 
> > > +/* Terminal error state that indicates that nothing useful can be 
> > > done.
> > > + * The most likely reason is that the database server doesn't
> > > + actually have
> > [Alin Serdean] doesn't have, maybe?
> 
> OK, thanks.
> 
> > > @@ -1118,7 +1153,7 @@ ovsdb_idl_condition_clone(struct
> > > ovsdb_idl_condition *dst,
> > >   * arranges to send the new condition to the database server.
> > >   *
> > >   * Return the next conditional update sequence number. When this
> > > - * value and ovsdb_idl_get_condition_seqno() matchs, the 'idl'
> > > + * value and ovsdb_idl_get_condition_seqno() matches, the 'idl'
> > >   * contains rows that match the 'condition'.
> > >   */
> > >  unsigned int
> > > --
> > s/.  *\//. *\//
> 
> I don't understand that suggestion.  Maybe you were suggesting that */
> should be on the same line as the last word; if so, OK, sure.
[Alin Serdean] I think in some places there is `.  */` vs `. */` , but maybe my 
email client messed up the formatting.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 4/5] ovsdb-idl: Remove 'uuid' member of struct ovsdb_idl.

[Alin Serdean]

> -Original Message-
> From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev-
> boun...@openvswitch.org] On Behalf Of Ben Pfaff
> Sent: Friday, December 8, 2017 11:24 PM
> To: d...@openvswitch.org
> Cc: Ben Pfaff 
> Subject: [ovs-dev] [PATCH 4/5] ovsdb-idl: Remove 'uuid' member of struct
> ovsdb_idl.
> 
> This was used to uniquely identify the monitor, but there's no need for that.
> A fixed monitor name works fine.
> 
> Signed-off-by: Ben Pfaff 
> ---

Acked-by: Alin Gabriel Serdean 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/5] ovsdb-idl: Improve comments.

[Alin Serdean]

LGTM. Just two nits.

> -Original Message-
> From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev-
> boun...@openvswitch.org] On Behalf Of Ben Pfaff
> Sent: Friday, December 8, 2017 11:24 PM
> To: d...@openvswitch.org
> Cc: Ben Pfaff <b...@ovn.org>
> Subject: [ovs-dev] [PATCH 1/5] ovsdb-idl: Improve comments.
> 
> This change documents the IDL state machine, adds other comments, and
> fixes a spelling error in a comment.
> 
> Signed-off-by: Ben Pfaff <b...@ovn.org>
> ---
>  lib/ovsdb-idl.c | 57
> ++---
>  1 file changed, 46 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c index
> 26a19fe8d5e6..7e3abdee8e62 100644
> --- a/lib/ovsdb-idl.c
> +++ b/lib/ovsdb-idl.c
> @@ -79,29 +79,63 @@ struct ovsdb_idl_arc {
>  struct ovsdb_idl_row *dst;  /* Destination row. */  };
> 
> +/* Connection state machine.
> + *
> + * When a JSON-RPC session connects, the IDL sends a "get_schema"
> +request and
> + * transitions to IDL_S_SCHEMA_REQUESTED.  If the session drops and
> +reconnects,
> + * the IDL starts over again in the same way. */
>  enum ovsdb_idl_state {
> +/* Waits for "get_schema" reply, then sends a "monitor_cond" request
> whose
> + * details are informed by the schema and transitions to
> + * IDL_S_MONITOR_COND_REQUESTED. */
>  IDL_S_SCHEMA_REQUESTED,
> -IDL_S_MONITOR_REQUESTED,
> -IDL_S_MONITORING,
> +
> +/* Waits for "monitor_cond" reply:
> + *
> + *- If the reply indicates success, replaces the IDL contents by the
> + *  data carried in the reply and transitions to
> IDL_S_MONITORING_COND.
> + *
> + *- If the reply indicates failure because the database is too old to
> + *  support monitor_cond, sends a "monitor" request and transitions 
> to
> + *  IDl_S_MONITOR_REQUESTED.  */
>  IDL_S_MONITOR_COND_REQUESTED,
> +
> +/* Waits for "monitor" reply, then replaces the IDL contents by the data
> + * carried in the reply and transitions to IDL_S_MONITORING.  */
> +IDL_S_MONITOR_REQUESTED,
> +
> +/* Terminal states that process "update2" (IDL_S_MONITORING_COND)
> or
> + * "update" (IDL_S_MONITORING) notifications. */
>  IDL_S_MONITORING_COND,
> +IDL_S_MONITORING,
> +
> +/* Terminal error state that indicates that nothing useful can be done.
> + * The most likely reason is that the database server doesn't actually 
> have
[Alin Serdean] doesn't have, maybe?
> + * the desired database.  We maintain the session with the database
> server
> + * anyway.  If it starts serving the database that we want, then it will
> + * kill the session and we will automatically reconnect and try
> + again. */
>  IDL_S_NO_SCHEMA
>  };
> 
>  struct ovsdb_idl {
> +/* Data. */
>  const struct ovsdb_idl_class *class_;
> -struct jsonrpc_session *session;
> -struct uuid uuid;
>  struct shash table_by_name; /* Contains "struct ovsdb_idl_table *"s.*/
>  struct ovsdb_idl_table *tables; /* Array of ->class_->n_tables elements.
> */
>  unsigned int change_seqno;
> -bool verify_write_only;
> 
> -/* Session state. */
> -unsigned int state_seqno;
> -enum ovsdb_idl_state state;
> -struct json *request_id;
> -struct json *schema;
> +/* Session state.
> + *
> + *'state_seqno' is a snapshot of the session's sequence number as
> returned
> + * jsonrpc_session_get_seqno(session), so if it differs from the value 
> that
> + * function currently returns then the session has reconnected and the
> + * state machine must restart.  */
> +struct jsonrpc_session *session; /* Connection to the server. */
> +enum ovsdb_idl_state state;  /* Current session state. */
> +unsigned int state_seqno;/* See above. */
> +struct json *request_id; /* JSON ID for request awaiting reply. 
> */
> +struct json *schema; /* Temporary copy of database schema. */
> +struct uuid uuid;/* Identifier for monitor. */
> 
>  /* Database locking. */
>  char *lock_name;/* Name of lock we need, NULL if none. */
> @@ -112,6 +146,7 @@ struct ovsdb_idl {
>  /* Transaction support. */
>  struct ovsdb_idl_txn *txn;
>  struct hmap outstanding_txns;
> +bool verify_write_only;
> 
>  /* Conditional monitoring. */
>  bool cond_changed;
> @@ -1118,7 +1153,7 @@ ovsdb_idl_condition_clone(struct
> ovsdb_idl_condition *dst,
>   * arrange

Re: [ovs-dev] [PATCH 2/5] ovsdb-idl: Fix indentation in a couple of places.

[Alin Serdean]

> -Original Message-
> From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev-
> boun...@openvswitch.org] On Behalf Of Ben Pfaff
> Sent: Friday, December 8, 2017 11:24 PM
> To: d...@openvswitch.org
> Cc: Ben Pfaff 
> Subject: [ovs-dev] [PATCH 2/5] ovsdb-idl: Fix indentation in a couple of
> places.
> 
> White space changes only.
> 
> Signed-off-by: Ben Pfaff 
> ---

Acked-by: Alin Gabriel Serdean 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 3/5] ovsdb-idl: Fix assertion failure on error path parsing server reply.

[Alin Serdean]

> -Original Message-
> From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev-
> boun...@openvswitch.org] On Behalf Of Ben Pfaff
> Sent: Friday, December 8, 2017 11:24 PM
> To: d...@openvswitch.org
> Cc: Ben Pfaff 
> Subject: [ovs-dev] [PATCH 3/5] ovsdb-idl: Fix assertion failure on error path
> parsing server reply.
> 
> If the database server sent an error reply to a monitor_cond request, and
> the error was not a JSON string, then passing the error to json_string()
> caused an assertion failure.
> 
> Found by inspection.
> 
> Signed-off-by: Ben Pfaff 
> ---
Acked-by: Alin Gabriel Serdean  
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] datapath-windows: Account for VLAN tag in tunnel Decap

[Alin Serdean]

It makes sense, but unfortunately it would be a stretch. The patch adds a 
feature.

Can you wait until 2.9 is out, or do you have a use case for 2.8 which you want 
to fulfill?

Thanks,
Alin.

> -Original Message-
> From: Shashank Ram [mailto:r...@vmware.com]
> Sent: Tuesday, November 28, 2017 3:33 PM
> To: d...@openvswitch.org; Alin Serdean
> <aserd...@cloudbasesolutions.com>
> Subject: Re: [ovs-dev] [PATCH] datapath-windows: Account for VLAN tag in
> tunnel Decap
> 
> Alin, do you think it makes sense to also apply this on 2.8?
> 
> --
> Thanks,
> Shashank
> 
> On 11/28/17, 11:47 AM, "ovs-dev-boun...@openvswitch.org on behalf of
> Shashank Ram" <ovs-dev-boun...@openvswitch.org on behalf of
> r...@vmware.com> wrote:
> 
> Alin, can we apply this patch? It’s been sitting around in the mailing 
> list for a
> week without any reviews after an ACK.
> 
> 
> 
> --
> 
> Thanks,
> 
> Shashank
> 
> 
> 
> On 11/21/17, 11:18 PM, "Anand Kumar" <kumaran...@vmware.com>
> wrote:
> 
> 
> 
> Acked-by: Anand Kumar <kumaran...@vmware.com>
> 
> 
> 
> Thanks,
> 
> Anand Kumar
> 
> 
> 
> On 11/20/17, 3:06 PM, "ovs-dev-boun...@openvswitch.org on behalf of
> Shashank Ram" <ovs-dev-boun...@openvswitch.org on behalf of
> r...@vmware.com> wrote:
> 
> 
> 
> Decap functions for tunneling protocols do not compute
> 
> the packet header offsets correctly when there is a VLAN
> 
> tag in the L2 header. This results in incorrect checksum
> 
> computation causing the packet to be dropped.
> 
> 
> 
> This patch adds support to account for the VLAN tag in the
> 
> packet if its present, and makes use of the OvsExtractLayers()
> 
> function to correctly compute the header offsets for different
> 
> layers.
> 
> 
> 
> Testing done:
> 
> - Tested Geneve, STT, Vxlan and Gre and verified that there
> 
>   are no regressions.
> 
> - Verified that packets with VLAN tags are correctly handled
> 
>   in the decap code of all tunneling protocols. Previously,
> 
>   this would result in packet drops due to invalid checksums
> 
>   being computed.
> 
> - Verified that non-VLAN tagged packets are handled correctly.
> 
> 
> 
> Signed-off-by: Shashank Ram <r...@vmware.com>
> 
> ---
> 
>  datapath-windows/ovsext/Geneve.c  | 14 +
> 
>  datapath-windows/ovsext/Geneve.h  |  6 ++
> 
>  datapath-windows/ovsext/Gre.c | 29 --
> 
>  datapath-windows/ovsext/Gre.h | 16 ++
> 
>  datapath-windows/ovsext/Offload.c | 10 +
> 
>  datapath-windows/ovsext/Offload.h |  3 ++-
> 
>  datapath-windows/ovsext/Stt.c | 44
> +++
> 
>  datapath-windows/ovsext/Stt.h |  6 ++
> 
>  datapath-windows/ovsext/Vxlan.c   | 14 +
> 
>  datapath-windows/ovsext/Vxlan.h   |  6 ++
> 
>  10 files changed, 111 insertions(+), 37 deletions(-)
> 
> 
> 
> diff --git a/datapath-windows/ovsext/Geneve.c b/datapath-
> windows/ovsext/Geneve.c
> 
> index 6dca69b..210716d 100644
> 
> --- a/datapath-windows/ovsext/Geneve.c
> 
> +++ b/datapath-windows/ovsext/Geneve.c
> 
> @@ -262,10 +262,16 @@ NDIS_STATUS
> OvsDecapGeneve(POVS_SWITCH_CONTEXT switchContext,
> 
>  PUINT8 bufferStart;
> 
>  PVOID optStart;
> 
>  NDIS_STATUS status;
> 
> +OVS_PACKET_HDR_INFO layers = { 0 };
> 
> +
> 
> +status = OvsExtractLayers(curNbl, );
> 
> +if (status != NDIS_STATUS_SUCCESS) {
> 
> +return status;
> 
> +}
> 
> 
> 
>  /* Check the length of the UDP payload */
> 
>  curNb = NET_BUFFER_LIST_FIRST_NB(curNbl);
> 
> -tunnelSize = OvsGetGeneveTunHdrMinSize();
> 
> +tunnelSize = OvsGetGeneveTunHdrSizeFromLayers();
> 
>  packetLength = NET_BUFFER_DATA_LENGTH(curNb);
> 
>  if (packetLength <= tunnelSize) {
> 
>  return NDIS_STATUS_INVALI

Re: [ovs-dev] [PATCH v1 1/3] datapath-windows: Refactor conntrack code.

[Alin Serdean]

Acked-by: Alin Gabriel Serdean 

> -Original Message-
> From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev-
> boun...@openvswitch.org] On Behalf Of Anand Kumar
> Sent: Tuesday, November 14, 2017 8:48 PM
> To: d...@openvswitch.org
> Subject: [ovs-dev] [PATCH v1 1/3] datapath-windows: Refactor conntrack
> code.
> 
> Some of the functions and  code are refactored so that new conntrack lock
> can be implemented
> 
> Signed-off-by: Anand Kumar 
> ---
>  datapath-windows/ovsext/Conntrack-nat.c |  11 +--
>  datapath-windows/ovsext/Conntrack.c | 170 ++--
> 
>  datapath-windows/ovsext/Conntrack.h |   4 -
>  3 files changed, 101 insertions(+), 84 deletions(-)
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 0/7] Introduce high resolution sleep support.

[Alin Serdean]

> -Original Message-
> From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev-
> boun...@openvswitch.org] On Behalf Of Ben Pfaff
> Sent: Wednesday, November 8, 2017 11:46 PM
> To: Bodireddy, Bhanuprakash <bhanuprakash.bodire...@intel.com>
> Cc: d...@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH 0/7] Introduce high resolution sleep support.
> 
> On Wed, Nov 08, 2017 at 08:54:11PM +, Bodireddy, Bhanuprakash wrote:
> > HI Ben,
> > >On Wed, Nov 08, 2017 at 04:35:52PM +, Bhanuprakash Bodireddy
> wrote:
> > >> This patchset introduces high resolution sleep support for linux
> > >> and
> > >windows.
> > >> Also time macros are introduced to replace the numbers with
> > >> meaningful names.
> > >
> > >Thank you very much for the series.
> > >
> > >Did you test that the Windows version of the code compiles (e.g. via
> > >appveyor)?
> >
> > I cross checked with appveyor and the build was successful. I replied to
> another thread where we were discussing about the windows
> implementation.
> >
> > >
> > >I would normally squash patch 3 (the Windows version of xnanosleep)
> > >into patch 2 (the Linux version).
> >
> > I couldn't verify the functionality of windows implementation and hence
> posted it as a separate patch for now.
> > I will squash it once I receive feedback from Alin.
> 
> OK.  Once we have feedback from Alin, I think that v2 of the series will be
> ready to go.  Thanks again!
[Alin Serdean] Sorry for the delay.
I sent out my review on the patch: 
https://mail.openvswitch.org/pipermail/ovs-dev/2017-November/340889.html. 
I'll try to run some benchmarks on the accuracy when I have time.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/2] lib: Move lib/poll-loop.h to include/openvswitch

[Alin Serdean]

> -Original Message-
> From: Xiao Liang [mailto:shaw.l...@gmail.com]
> Sent: Thursday, November 2, 2017 4:32 AM
> To: Alin Serdean <aserd...@cloudbasesolutions.com>
> Cc: Ben Pfaff <b...@ovn.org>; d...@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH 2/2] lib: Move lib/poll-loop.h to
> include/openvswitch
> 
> On Thu, Nov 2, 2017 at 5:51 AM, Alin Serdean
> <aserd...@cloudbasesolutions.com> wrote:
> >
> >
> >> -Original Message-
> >> From: Ben Pfaff [mailto:b...@ovn.org]
> >> Sent: Tuesday, October 31, 2017 9:34 PM
> >> To: Xiao Liang <shaw.l...@gmail.com>; Alin Serdean
> >> <aserd...@cloudbasesolutions.com>
> >> Cc: d...@openvswitch.org
> >> Subject: Re: [ovs-dev] [PATCH 2/2] lib: Move lib/poll-loop.h to
> >> include/openvswitch
> >>
> >> On Thu, Aug 17, 2017 at 12:06:25AM +0800, Xiao Liang wrote:
> >> > Poll-loop is the core to implement main loop. It should be
> >> > available in libopenvswitch.
> >> >
> >> > Signed-off-by: Xiao Liang <shaw.l...@gmail.com>
> >>
> >> I'm concerned about the way that this adds a definition of HANDLE in
> >> a public header.  That seems unfriendly to code that might want to
> >> include both this header and Win32 headers that properly define HANDLE.
> >>
> >> Alin, what's the right thing to do here?
> > [Alin Serdean] First the type definition is wrong (HANDLE is VOID*).
> > I would avoid adding the definition to HANDLE. Maybe add an include to
>  or  to include/windows/poll.h .
> 
> Since include/windows is not installed as public header, is it safe to remove
> inclusion of poll.h and add windows.h in poll-loop.h?
That sounds reasonable to me. Ben do you agree?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH, RFC] tests: Add a default timeout for control utilities

[Alin Serdean]

> -Original Message-
> From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev-
> boun...@openvswitch.org] On Behalf Of Ben Pfaff
> Sent: Wednesday, November 1, 2017 12:32 AM
> To: Alin Gabriel Serdean <aserd...@ovn.org>
> Cc: d...@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH, RFC] tests: Add a default timeout for control
> utilities
> 
> On Sat, Sep 02, 2017 at 08:52:38AM -0700, Ben Pfaff wrote:
> > On Mon, Aug 28, 2017 at 08:14:59PM +0300, Alin Gabriel Serdean wrote:
> > > Let's suppose that ovsdb-server is running properly, but
> > > ovs-vswitchd is not responsive/crashed. We try to add a port via
> > > ovs-vsctl and it will hang.
> > > This patch aims at that scenario and tries to make life easier when
> > > debugging hanging tests.
> > >
> > > Some shells do not allow dashes in function names (default
> > > behavior), we shall try to define an alias to overcome dashes if the shell
> allows it.
> > >
> > > Signed-off-by: Alin Gabriel Serdean <aserd...@ovn.org>
> > > Suggested-by: Ben Pfaff <b...@ovn.org>
> >
> > Acked-by: Ben Pfaff <b...@ovn.org>
> 
> I expected that you'd apply this, but, anyway, I've done it just now.
[Alin Serdean] Thanks Ben! I was convinced I applied this patch before I went 
on vacation but apparently, I didn't. Sorry!
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/2] lib: Move lib/poll-loop.h to include/openvswitch

[Alin Serdean]

> -Original Message-
> From: Ben Pfaff [mailto:b...@ovn.org]
> Sent: Tuesday, October 31, 2017 9:34 PM
> To: Xiao Liang <shaw.l...@gmail.com>; Alin Serdean
> <aserd...@cloudbasesolutions.com>
> Cc: d...@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH 2/2] lib: Move lib/poll-loop.h to
> include/openvswitch
> 
> On Thu, Aug 17, 2017 at 12:06:25AM +0800, Xiao Liang wrote:
> > Poll-loop is the core to implement main loop. It should be available
> > in libopenvswitch.
> >
> > Signed-off-by: Xiao Liang <shaw.l...@gmail.com>
> 
> I'm concerned about the way that this adds a definition of HANDLE in a public
> header.  That seems unfriendly to code that might want to include both this
> header and Win32 headers that properly define HANDLE.
> 
> Alin, what's the right thing to do here?
[Alin Serdean] First the type definition is wrong (HANDLE is VOID*).
I would avoid adding the definition to HANDLE. Maybe add an include to 
 or  to include/windows/poll.h .
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev