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_openvswitch_ovs-2Dissues_issues_65&d=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=CWsgHUxi6ExLXY798tmo3LJ4e3geGYp56lkcH-5cLCY&m=MVriWGIsZGTQ5f_haLS0X5H8R3YdfikbqnWKRpKksKU&s=1iWqFul95i3o_mD41jKjZNdLnK-df_QWRzSY1614RTw&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_mailman_listinfo_dev&d=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=CWsgHUxi6ExLXY798tmo3LJ4e3geGYp56lkcH-5cLCY&m=MVriWGIsZGTQ5f_haLS0X5H8R3YdfikbqnWKRpKksKU&s=IRTZMDQzH2bWNFdd7KZkHDqBBXjSOUx5lc0Z5tcIwag&e= > > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailman_listinfo_dev&d=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=CWsgHUxi6ExLXY798tmo3LJ4e3geGYp56lkcH-5cLCY&m=mT4lWtSM2tE_562pKsxJG3mDy1aE2fJR2aZfmUkTut4&s=UKaI73R8LrWspWbBYRIQT7jjdvvNs7YcESLYPRPkppY&e= > > > _______________________________________________ > dev mailing list > [email protected] > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
