[ovs-dev] [PATCH v2 3/3] datapath-windows: Add a WFP system provider

[Nithin Raju]

On Dec 22, 2014, at 7:06 AM, Sorin Vinturis <svinturis at 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 <svinturis at 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()?

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.

3)
Since OvsTunnelRemoveSystemProvider() can close the 'gEngineHandle' that OvsTunnelAddSystemProvider() opened, should we not set 'engineOpened = TRUE' only if OvsTunnelRegisterCallouts() does really open the handle?
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."

4)
The code to initialize 'gEngineHandle' is repeated 3 times. It could be refactored into an small/inline function.

thanks,
-- Nithin