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

Jinjun Gao jinjung at vmware.com
Tue Jun 30 10:12:34 UTC 2020

Thanks for review, Alin.

Please see the comments in inline [Jinjun].


From: Alin Serdean <aserdean at cloudbasesolutions.com>
Date: Tuesday, June 30, 2020 at 5:34 PM
To: Jinjun Gao <jinjung at vmware.com>, "dev at openvswitch.org" <dev at openvswitch.org>
Cc: Anand Kumar <kumaranand at vmware.com>
Subject: RE: [ovs-dev] datapath-windows: Add CTA_HELP and CTA_TUPLE_MASTER

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.

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

You can fold in following incremental:

[Jinjun]: Will add it, thanks.

$ 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,
         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?
 [Jinjun]: I used pyftpdlib as ftp server to test this patch. Pyftpdlib doesn’t use 20 as data port and it chooses a random port as data port in active mode. It causes the incomingKey cannot match any entry in related table. In this case, the data flow cannot find control flow and add it as tuple master. After deleting above line, it works! If we don’t consider to support random data port in active mode, we need not to remove above line. It should be better to support random data port case in active mode. How do you think about it? If you agree, I can add some comments here to clear it.


More information about the dev mailing list