Hi Nithin, Thanks for your reviews. Please see my answers inline.
-- Sorin -----Original Message----- From: Nithin Raju [mailto:nit...@vmware.com] Sent: Wednesday, 31 December, 2014 21:41 To: Sorin Vinturis Cc: dev@openvswitch.org Subject: Re: [ovs-dev] [PATCH v2 3/3] datapath-windows: Add a WFP system provider On Dec 22, 2014, at 7:06 AM, Sorin Vinturis <svintu...@cloudbasesolutions.com> wrote: > This patch was enforced by the WHCK logo testing. In order to pass the > Windows Filtering Platform tests we need to add a persistent system > provider. > > Signed-off-by: Sorin Vinturis <svintu...@cloudbasesolutions.com> hi Sorin, Sorry the delay in the review. The code mostly looks good. I had the following minor comments: 1) The new functions added is mostly additional functionality that gets called during driver load/unload. In terms of code organization, can you add that at the end of the file, or at the beginning of the file, and not in between OvsTunnelRemoveFilter() and OvsTunnelRegisterDatagramDataCallouts()? SV: OK, I'll add the functions at the beginning of the file. 2) Can you add a log if FwpmEngineOpen() fails in OvsTunnelAddSystemProvider()? The caller does not seem to care about return values. So, a log would be useful. SV: I will add an error log in case an error occurs. 3) Since OvsTunnelRemoveSystemProvider() can close the 'gEngineHandle' that OvsTunnelAddSystemProvider() opened, should we not set 'engineOpened = TRUE' only if OvsTunnelRegisterCallouts() does really open the handle? SV: OK, I'll set the 'engineOpened' only if the session was opened in the OvsTunnelRegisterCallouts function. ie. OvsTunnelRegisterCallouts() { [...] if (NULL == gEngineHandle) { [...] engineOpened = TRUE; } [...] Exit: if (inTransaction) { FwpmTransactionAbort(gEngineHandle); } } The reason is, if we called FwpmEngineClose(), then the system provider created by OvsTunnelAddSystemProvider() might get automatically deleted, since 'session.flags' is set to FWPM_SESSION_FLAG_DYNAMIC while initializing 'gEngineHandle'. I understand this is a corner case. Pls. look at the following documentation: http://msdn.microsoft.com/en-us/library/windows/desktop/aa364040%28v=vs.85%29.aspx It seems to say the following: "The session is automatically closed when the program ends. To explicitly close a session, call FwpmEngineClose0." "If session.flags is set to FWPM_SESSION_FLAG_DYNAMIC, any WFP objects added during the session are automatically deleted when the session ends. If the session is not dynamic, the caller needs to explicitly delete all WFP objects added during the session." SV: The provider object created by the OvsTunnelAddSystemProvider function is persistent and is not deleted when the session is closed. For this the FWPM_PROVIDER_FLAG_PERSISTENT flag is set. 4) The code to initialize 'gEngineHandle' is repeated 3 times. It could be refactored into an small/inline function. SV: OK, I'll make the changes. thanks, -- Nithin _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
