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()?
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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
