Thanks for review, Alin. Please see the comments in inline [Jinjun].
-- Jinjun From: Alin Serdean <[email protected]> Date: Tuesday, June 30, 2020 at 5:34 PM To: Jinjun Gao <[email protected]>, "[email protected]" <[email protected]> Cc: Anand Kumar <[email protected]> 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. [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: [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, 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? [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. -- Alin. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
