Hi Ben, I just checked and saw that my patch is already on master. From what I saw the patch is OK.
Thanks, Sorin -----Original Message----- From: Ben Pfaff [mailto:[email protected]] Sent: Tuesday, 24 March, 2015 19:06 To: Eitan Eliahu Cc: Sorin Vinturis; [email protected] Subject: Re: [ovs-dev] [PATCH] datapath-windows: Updated WFP system provider handling Thanks. I tried to apply this now but there are multiple rejects. Sorin, can you post a rebased version with Eitan's ack included? Thanks a lot. On Tue, Mar 24, 2015 at 05:02:28PM +0000, Eitan Eliahu wrote: > > I'm fine with this change. > Thanks Ben and Sorin. > Eitan > > -----Original Message----- > From: Ben Pfaff [mailto:[email protected]] > Sent: Tuesday, March 24, 2015 10:01 AM > To: Eitan Eliahu > Cc: Sorin Vinturis; [email protected] > Subject: Re: [ovs-dev] [PATCH] datapath-windows: Updated WFP system > provider handling > > It's been a long conversation here, so Eitan is this ack for the patch as > posted or should I wait for a revision? Thanks a lot to both of you for your > thorough checking. > > On Tue, Mar 24, 2015 at 12:50:47PM +0000, Eitan Eliahu wrote: > > Acked-by: Eitan Eliahu <[email protected]> > > > > Thanks Sorin. > > Eitan > > > > -----Original Message----- > > From: Sorin Vinturis [mailto:[email protected]] > > Sent: Tuesday, March 24, 2015 12:29 AM > > To: Eitan Eliahu > > Cc: [email protected] > > Subject: RE: [ovs-dev] [PATCH] datapath-windows: Updated WFP system > > provider handling > > > > Eitan, > > > > The BFE is not started when the machine is booted and OVS driver is > > initialized. This happens all the time. Due to this, the WFP system > > provider is not added. > > > > When the OS is started and the userspace tools are executed the BFE is > > running and all WFP objects are correctly added. At this point, if the > > commands 'net stop ovsext' + 'net start ovsext' are executed all OVS > > initializations are successfully performed, including the addition of the > > WFP system provider. > > > > So this patch is trying to address the scenario when the WFP system > > provider addition is performed at boot time. > > > > Thanks, > > Sorin > > > > -----Original Message----- > > From: Eitan Eliahu [mailto:[email protected]] > > Sent: Tuesday, 24 March, 2015 06:37 > > To: Sorin Vinturis > > Cc: [email protected] > > Subject: RE: [ovs-dev] [PATCH] datapath-windows: Updated WFP system > > provider handling > > > > Thanks Sorin, > > What would be the case where BFE is not started before the driver > > initializes? > > When BFE is not started does the driver continue with WFP rule setiings? > > Eitan > > > > > > > > -----Original Message----- > > From: Sorin Vinturis [mailto:[email protected]] > > Sent: Monday, March 23, 2015 4:40 PM > > To: Sorin Vinturis; Eitan Eliahu > > Cc: [email protected] > > Subject: RE: [ovs-dev] [PATCH] datapath-windows: Updated WFP system > > provider handling > > > > As I explained in the patch description, I have added this change because, > > at driver initialization phase, the system provider failed to be added due > > to the fact that the Base Filtering Engine (BFE) is not started and no > > session to the engine could be acquired, i.e. OvsTunnelEngineOpen() fails. > > > > > > > > -----Original Message----- > > > > From: dev [mailto:[email protected]] On Behalf Of Sorin > > Vinturis > > > > Sent: Tuesday, 24 March, 2015 01:01 > > > > To: Eitan Eliahu > > > > Cc: [email protected] > > > > Subject: Re: [ovs-dev] [PATCH] datapath-windows: Updated WFP system > > provider handling > > > > > > > > Hi Eitan, > > > > > > > > In this case the second call will enter this code path: > > > > > > > > status = FwpmProviderAdd(handle, > > > > &provider, > > > > NULL); > > > > if (!NT_SUCCESS(status)) { > > > > - OVS_LOG_ERROR("Fail to add WFP provider, status: %x.", status); > > > > - break; > > > > + if (STATUS_FWP_ALREADY_EXISTS != status) { > > > > + OVS_LOG_ERROR("Failed to add WFP provider, status: > > + %x.", > > > > + status); > > > > + break; > > > > + } > > > > } > > > > > > > > Thanks, > > > > Sorin > > > > > > > > -----Original Message----- > > > > From: Eitan Eliahu [mailto:[email protected]] > > > > Sent: Monday, 23 March, 2015 18:13 > > > > To: Sorin Vinturis > > > > Cc: [email protected] > > > > Subject: RE: [ovs-dev] [PATCH] datapath-windows: Updated WFP system > > provider handling > > > > > > > > Sorin, > > > > Ok on [1]. Thanks. > > > > > > > > On [2], I can see a scenario when the provider can added twice from > > OvsRegisterSystemProvider() as well as from the callback > > > > > > > > if (FWPM_SERVICE_RUNNING == FwpmBfeStateGet()) { ----> Callback > > is called from a different thread context ! > > > > OvsTunnelEngineOpen(&handle); > > > > > > > > > > > > How do you prevent it? > > > > > > > > Also, can you please explain which issue you ran into which triggered this > > change? > > > > Thank you, > > > > Eitan > > > > > > > > -----Original Message----- > > > > From: Sorin Vinturis [mailto:[email protected]] > > > > Sent: Monday, March 23, 2015 1:37 AM > > > > To: Eitan Eliahu > > > > Cc: [email protected] > > > > Subject: RE: [ovs-dev] [PATCH] datapath-windows: Updated WFP system > > provider handling > > > > > > > > Hi Eitan, > > > > > > > > Thanks for reviewing my patch. > > > > [1] The OvsTunnelEngineClose(HANDLE *handle) is verifying the handle it > > receives before closing it. > > > > [2] In order to retrieve the current state of the filter engine I am using > > the FwpmBfeStateGet function. But before calling FwpmBfeStateGet, the > > callout driver MUST call the FwpmBfeStateSubscribeChanges function. That is > > the reason I am registering the callback no mater of the state of the > > filter engine. > > > > Only if the engine is running the provider is added by opening the engine > > handle first and closing it after the provider is added, then unsubscribes > > the callback. > > > > If the engine is not running the callback is called and the provider is > > added only for the service running notification. > > > > > > > > Hope this answers your comments. > > > > > > > > Thanks, > > > > Sorin > > > > > > > > -----Original Message----- > > > > From: Eitan Eliahu [mailto:[email protected]] > > > > Sent: Monday, 23 March, 2015 02:32 > > > > To: Sorin Vinturis > > > > Cc: [email protected] > > > > Subject: RE: [ovs-dev] [PATCH] datapath-windows: Updated WFP system > > provider handling > > > > > > > > > > > > Hi Sorin, > > > > Thank you for posting this change. > > > > Here are few comments: > > > > [1] Please call OvsTunnelEngineClose(&handle) only if the handle is not > > zero. (you might want to add an assertion if it zero). > > > > [2] I'm somewhat confused about the registration for engine state change. > > As I understand you check the state of the engine and if it is running you > > open the engine handle (in the context of the foreground thread). However, > > you subscribe to state change anyway and you open the handle to the engine > > from the callback function too. > > > > Did I miss anything? > > > > Thanks, > > > > Eitan > > > > > > > > > > > > -----Original Message----- > > > > > > > > From: dev [mailto:[email protected]] On Behalf Of Sorin > > Vinturis > > > > > > > > Sent: Monday, 16 March, 2015 20:08 > > > > > > > > To: [email protected] > > > > > > > > Subject: [ovs-dev] [PATCH] datapath-windows: Updated WFP system > > provider handling > > > > > > > > > > > > > > > > If the Base Filtering Engine (BFE) is not started, the WFP system provider > > failed to be added because no session to the engine could be acquired. > > > > > > > > > > > > > > > > The solution for this was to registered a BFE notification callback that is > > called whenever the BFE's state changes. Only if the BFE's state is running > > the WFP system provider is added. > > > > > > > > > > > > > > > > Signed-off-by: Sorin Vinturis <[email protected]> > > > > > > > > Reported-by: Sorin Vinturis <[email protected]> > > > > > > > > Reported-at: > > https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_open > > vs > > witch_ovs-2Dissues_issues_65&d=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeA > > w- > > YihVMNtXt-uEs&r=CWsgHUxi6ExLXY798tmo3LJ4e3geGYp56lkcH-5cLCY&m=MVriWG > > Is > > ZGTQ5f_haLS0X5H8R3YdfikbqnWKRpKksKU&s=1iWqFul95i3o_mD41jKjZNdLnK-df_ > > QW > > RzSY1614RTw&e= > > > > > > > > --- > > > > > > > > datapath-windows/ovsext/Datapath.c | 20 ++----- > > > > > > > > datapath-windows/ovsext/TunnelFilter.c | 99 > > ++++++++++++++++++++++++++++++++-- > > > > > > > > datapath-windows/ovsext/TunnelIntf.h | 8 +-- > > > > > > > > 3 files changed, 100 insertions(+), 27 deletions(-) > > > > > > > > > > > > > > > > diff --git a/datapath-windows/ovsext/Datapath.c > > b/datapath-windows/ovsext/Datapath.c > > > > > > > > index 8eb13f1..c6fe89e 100644 > > > > > > > > --- a/datapath-windows/ovsext/Datapath.c > > > > > > > > +++ b/datapath-windows/ovsext/Datapath.c > > > > > > > > @@ -353,35 +353,19 @@ PNDIS_SPIN_LOCK gOvsCtrlLock; VOID > > > > > > > > OvsInit() > > > > > > > > { > > > > > > > > - HANDLE handle = NULL; > > > > > > > > - > > > > > > > > gOvsCtrlLock = &ovsCtrlLockObj; > > > > > > > > NdisAllocateSpinLock(gOvsCtrlLock); > > > > > > > > OvsInitEventQueue(); > > > > > > > > - > > > > > > > > - OvsTunnelEngineOpen(&handle); > > > > > > > > - if (handle) { > > > > > > > > - OvsTunnelAddSystemProvider(handle); > > > > > > > > - } > > > > > > > > - OvsTunnelEngineClose(&handle); > > > > > > > > } > > > > > > > > > > > > > > > > VOID > > > > > > > > OvsCleanup() > > > > > > > > { > > > > > > > > - HANDLE handle = NULL; > > > > > > > > - > > > > > > > > OvsCleanupEventQueue(); > > > > > > > > if (gOvsCtrlLock) { > > > > > > > > NdisFreeSpinLock(gOvsCtrlLock); > > > > > > > > gOvsCtrlLock = NULL; > > > > > > > > } > > > > > > > > - > > > > > > > > - OvsTunnelEngineOpen(&handle); > > > > > > > > - if (handle) { > > > > > > > > - OvsTunnelRemoveSystemProvider(handle); > > > > > > > > - } > > > > > > > > - OvsTunnelEngineClose(&handle); > > > > > > > > } > > > > > > > > > > > > > > > > VOID > > > > > > > > @@ -448,6 +432,8 @@ OvsCreateDeviceObject(NDIS_HANDLE > > ovsExtDriverHandle) > > > > > > > > if (ovsExt) { > > > > > > > > ovsExt->numberOpenInstance = 0; > > > > > > > > } > > > > > > > > + } else { > > > > > > > > + OvsRegisterSystemProvider((PVOID)gOvsDeviceObject); > > > > > > > > } > > > > > > > > > > > > > > > > OVS_LOG_TRACE("DeviceObject: %p", gOvsDeviceObject); @@ -471,6 > > +457,8 @@ OvsDeleteDeviceObject() > > > > > > > > NdisDeregisterDeviceEx(gOvsDeviceHandle); > > > > > > > > gOvsDeviceHandle = NULL; > > > > > > > > gOvsDeviceObject = NULL; > > > > > > > > + > > > > > > > > + OvsUnregisterSystemProvider(); > > > > > > > > } > > > > > > > > } > > > > > > > > > > > > > > > > diff --git a/datapath-windows/ovsext/TunnelFilter.c > > b/datapath-windows/ovsext/TunnelFilter.c > > > > > > > > index e0adc37..4b879c0 100644 > > > > > > > > --- a/datapath-windows/ovsext/TunnelFilter.c > > > > > > > > +++ b/datapath-windows/ovsext/TunnelFilter.c > > > > > > > > @@ -111,6 +111,7 @@ DEFINE_GUID( > > > > > > > > PDEVICE_OBJECT gDeviceObject; > > > > > > > > > > > > > > > > HANDLE gEngineHandle = NULL; > > > > > > > > +HANDLE gBfeSubscriptionHandle = NULL; > > > > > > > > UINT32 gCalloutIdV4; > > > > > > > > > > > > > > > > > > > > > > > > @@ -173,17 +174,20 @@ OvsTunnelAddSystemProvider(HANDLE handle) > > > > > > > > provider.displayData.name = OVS_TUNNEL_PROVIDER_NAME; > > > > > > > > provider.displayData.description = > > OVS_TUNNEL_PROVIDER_DESC; > > > > > > > > /* > > > > > > > > - * Since we always want the provider to be present, it's easiest to > > add > > > > > > > > - * it as persistent object during driver load. > > > > > > > > - */ > > > > > > > > + * Since we always want the provider to be present, it's > > + easiest to add > > > > > > > > + * it as persistent object during driver load. > > > > > > > > + */ > > > > > > > > provider.flags = FWPM_PROVIDER_FLAG_PERSISTENT; > > > > > > > > > > > > > > > > status = FwpmProviderAdd(handle, > > > > > > > > &provider, > > > > > > > > NULL); > > > > > > > > if (!NT_SUCCESS(status)) { > > > > > > > > - OVS_LOG_ERROR("Fail to add WFP provider, status: %x.", status); > > > > > > > > - break; > > > > > > > > + if (STATUS_FWP_ALREADY_EXISTS != status) { > > > > > > > > + OVS_LOG_ERROR("Failed to add WFP provider, status: > > + %x.", > > > > > > > > + status); > > > > > > > > + break; > > > > > > > > + } > > > > > > > > } > > > > > > > > > > > > > > > > status = FwpmTransactionCommit(handle); @@ -541,3 +545,88 @@ Exit: > > > > > > > > > > > > > > > > return status; > > > > > > > > } > > > > > > > > + > > > > > > > > +VOID NTAPI > > > > > > > > +OvsBfeStateChangeCallback(PVOID context, > > > > > > > > + FWPM_SERVICE_STATE bfeState) { > > > > > > > > + HANDLE handle = NULL; > > > > > > > > + > > > > > > > > + DBG_UNREFERENCED_PARAMETER(context); > > > > > > > > + > > > > > > > > + if (FWPM_SERVICE_RUNNING == bfeState) { > > > > > > > > + OvsTunnelEngineOpen(&handle); > > > > > > > > + if (handle) { > > > > > > > > + OvsTunnelAddSystemProvider(handle); > > > > > > > > + } > > > > > > > > + OvsTunnelEngineClose(&handle); > > > > > > > > + } > > > > > > > > +} > > > > > > > > + > > > > > > > > +NTSTATUS > > > > > > > > +OvsSubscribeBfeStateChanges(PVOID deviceObject) { > > > > > > > > + NTSTATUS status = STATUS_SUCCESS; > > > > > > > > + > > > > > > > > + if (!gBfeSubscriptionHandle) { > > > > > > > > + status = FwpmBfeStateSubscribeChanges(deviceObject, > > > > > > > > + > > + OvsBfeStateChangeCallback, > > > > > > > > + NULL, > > > > > > > > + > > + &gBfeSubscriptionHandle); > > > > > > > > + if (!NT_SUCCESS(status)) { > > > > > > > > + OVS_LOG_ERROR( > > > > > > > > + "Failed to open subscribe BFE state change > > + callback, > > + status: %x.", > > > > > > > > + status); > > > > > > > > + } > > > > > > > > + } > > > > > > > > + > > > > > > > > + return status; > > > > > > > > +} > > > > > > > > + > > > > > > > > +VOID > > > > > > > > +OvsUnsubscribeBfeStateChanges() > > > > > > > > +{ > > > > > > > > + NTSTATUS status = STATUS_SUCCESS; > > > > > > > > + > > > > > > > > + if (gBfeSubscriptionHandle) { > > > > > > > > + status = > > + FwpmBfeStateUnsubscribeChanges(gBfeSubscriptionHandle); > > > > > > > > + if (!NT_SUCCESS(status)) { > > > > > > > > + OVS_LOG_ERROR( > > > > > > > > + "Failed to open unsubscribe BFE state change > > + callback, status: %x.", > > > > > > > > + status); > > > > > > > > + } > > > > > > > > + gBfeSubscriptionHandle = NULL; > > > > > > > > + } > > > > > > > > +} > > > > > > > > + > > > > > > > > +VOID OvsRegisterSystemProvider(PVOID deviceObject) { > > > > > > > > + NTSTATUS status = STATUS_SUCCESS; > > > > > > > > + HANDLE handle = NULL; > > > > > > > > + > > > > > > > > + status = OvsSubscribeBfeStateChanges(deviceObject); > > > > > > > > + if (NT_SUCCESS(status)) { > > > > > > > > + if (FWPM_SERVICE_RUNNING == FwpmBfeStateGet()) { > > > > > > > > + OvsTunnelEngineOpen(&handle); > > > > > > > > + if (handle) { > > > > > > > > + OvsTunnelAddSystemProvider(handle); > > > > > > > > + } > > > > > > > > + OvsTunnelEngineClose(&handle); > > > > > > > > + > > > > > > > > + OvsUnsubscribeBfeStateChanges(); > > > > > > > > + } > > > > > > > > + } > > > > > > > > +} > > > > > > > > + > > > > > > > > +VOID OvsUnregisterSystemProvider() > > > > > > > > +{ > > > > > > > > + HANDLE handle = NULL; > > > > > > > > + > > > > > > > > + OvsTunnelEngineOpen(&handle); > > > > > > > > + if (handle) { > > > > > > > > + OvsTunnelRemoveSystemProvider(handle); > > > > > > > > + } > > > > > > > > + OvsTunnelEngineClose(&handle); > > > > > > > > + > > > > > > > > + OvsUnsubscribeBfeStateChanges(); > > > > > > > > +} > > > > > > > > diff --git a/datapath-windows/ovsext/TunnelIntf.h > > b/datapath-windows/ovsext/TunnelIntf.h > > > > > > > > index b2bba30..728a53f 100644 > > > > > > > > --- a/datapath-windows/ovsext/TunnelIntf.h > > > > > > > > +++ b/datapath-windows/ovsext/TunnelIntf.h > > > > > > > > @@ -22,12 +22,8 @@ NTSTATUS OvsTunnelFilterInitialize(PDRIVER_OBJECT > > driverObject); > > > > > > > > > > > > > > > > VOID OvsTunnelFilterUninitialize(PDRIVER_OBJECT driverObject); > > > > > > > > > > > > > > > > -NTSTATUS OvsTunnelEngineOpen(HANDLE *handle); > > > > > > > > +VOID OvsRegisterSystemProvider(PVOID deviceObject); > > > > > > > > > > > > > > > > -VOID OvsTunnelEngineClose(HANDLE *handle); > > > > > > > > - > > > > > > > > -VOID OvsTunnelAddSystemProvider(HANDLE handle); > > > > > > > > - > > > > > > > > -VOID OvsTunnelRemoveSystemProvider(HANDLE handle); > > > > > > > > +VOID OvsUnregisterSystemProvider(); > > > > > > > > > > > > > > > > #endif /* __TUNNEL_INTF_H_ */ > > > > > > > > -- > > > > > > > > 1.9.0.msysgit.0 > > > > > > > > _______________________________________________ > > > > > > > > dev mailing list > > > > > > > > [email protected] > > > > > > > > https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_ > > ma > > ilman_listinfo_dev&d=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtX > > t- > > uEs&r=CWsgHUxi6ExLXY798tmo3LJ4e3geGYp56lkcH-5cLCY&m=MVriWGIsZGTQ5f_h > > aL > > S0X5H8R3YdfikbqnWKRpKksKU&s=IRTZMDQzH2bWNFdd7KZkHDqBBXjSOUx5lc0Z5tcI > > wa > > g&e= > > > > > > > > _______________________________________________ > > > > dev mailing list > > > > [email protected] > > > > https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_ > > ma > > ilman_listinfo_dev&d=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtX > > t- > > uEs&r=CWsgHUxi6ExLXY798tmo3LJ4e3geGYp56lkcH-5cLCY&m=mT4lWtSM2tE_562p > > Ks > > xJG3mDy1aE2fJR2aZfmUkTut4&s=UKaI73R8LrWspWbBYRIQT7jjdvvNs7YcESLYPRPk > > pp > > Y&e= > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_ > > ma > > ilman_listinfo_dev&d=AwIBAg&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtX > > t- > > uEs&r=CWsgHUxi6ExLXY798tmo3LJ4e3geGYp56lkcH-5cLCY&m=brNy9GH6PKKi-BlK > > 0b > > fuKRE9lgSomvUut9g-i-FDL6U&s=kLpPIbd8Bf67Wqt4S7Bo8zeiCSW_G_04cxszaNYb > > LG > > o&e= _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
