Hi Yin, I think you are confused about what this patch does. Or I may not be understanding this correctly.
As it stands, Event.c in OVS master has 2 bugs that I identified while looking at it. 1. OvsSubscribeEventIoctl - Subscribe function doesn’t set the right status parameter when queue is subscribed 2. OvsWaitEventIoctl - Releasing an acquired lock instead of simply returning back. The 2nd bug in question is on line 359 in Event.c on master - https://github.com/openvswitch/ovs/blob/15850211ce88d540e57a6f2fc80963465b9a5475/datapath-windows/ovsext/Event.c#L359 And that’s exactly what this patch addresses. The only thing special about OvsWaitEventIoctl is that it has a bug that needed to be fixed before I modified it further with my original changes. I kept the bug fixes separate from the intended changes mainly because we can use this patch to address issues on ToT without being blocked on my other changes. Thanks, Sairam From: Yin Lin <yinli...@gmail.com<mailto:yinli...@gmail.com>> Date: Thursday, July 21, 2016 at 1:12 PM To: Sairam Venugopal <vsai...@vmware.com<mailto:vsai...@vmware.com>> Cc: "dev@openvswitch.org<mailto:dev@openvswitch.org>" <dev@openvswitch.org<mailto:dev@openvswitch.org>> Subject: Re: [ovs-dev] [PATCH 4/9] datapath-windows: Fix bugs in Event.c around subscribe and lock Can you refer me to the patch you fix the lock in line 359? I don't see why function OvsWaitEventIoctl is so special that we need to fix it in another patch. It is the same thing as other places you fixed so it's better to address them together.. On Mon, Jul 18, 2016 at 11:27 AM, Sairam Venugopal <vsai...@vmware.com<mailto:vsai...@vmware.com>> wrote: Hi Yin, Thanks for reviewing this. (1) has been addressed in a different patch. (2) We don¹t need the lock for OvsGetOpenInstance(). Thanks, Sairam On 7/18/16, 11:07 AM, "Yin Lin" <yinli...@gmail.com<mailto:yinli...@gmail.com>> wrote: >Thanks Sai for fixing the bugs. A few questions: > >1. There is also an unrelease lock in line 359, function >OvsWaitEventIoctl. >2. Do we need the lock before OvsGetOpenInstance? If so, please release >the >lock inside "if (instance == NULL)"; if not, you should do the same thing >on line 359. > > > > >On Wed, Jul 13, 2016 at 4:38 PM, Sairam Venugopal ><vsai...@vmware.com<mailto:vsai...@vmware.com>> >wrote: > >> When userspace tries to resubscribe to an existing queue, return >> STATUS_INVALID_PARAMETER since it's not supported. The current bug >> overwrites status to STATUS_SUCCESS. >> >> The second bug fix is around releasing the EventQueue lock if an open >> instance couldn't be found. The current version returns back without >> releasing the lock. Moving the OvsAcquireEventQueueLock() after the >> instance is verified. >> >> Signed-off-by: Sairam Venugopal >> <vsai...@vmware.com<mailto:vsai...@vmware.com>> >> --- >> datapath-windows/ovsext/Event.c | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/datapath-windows/ovsext/Event.c >> b/datapath-windows/ovsext/Event.c >> index 8c7c3ec..8ff0322 100644 >> --- a/datapath-windows/ovsext/Event.c >> +++ b/datapath-windows/ovsext/Event.c >> @@ -217,9 +217,8 @@ OvsSubscribeEventIoctl(PFILE_OBJECT fileObject, >> if (queue->mask != request->mask) { >> status = STATUS_INVALID_PARAMETER; >> OVS_LOG_WARN("Can not chnage mask when the queue is >> subscribed"); >> + goto done_event_subscribe; >> } >> - status = STATUS_SUCCESS; >> - goto done_event_subscribe; >> } else if (!request->subscribe && queue == NULL) { >> status = STATUS_SUCCESS; >> goto done_event_subscribe; >> @@ -356,8 +355,6 @@ OvsWaitEventIoctl(PIRP irp, >> } >> poll = (POVS_EVENT_POLL)inputBuffer; >> >> - OvsAcquireEventQueueLock(); >> - >> instance = OvsGetOpenInstance(fileObject, poll->dpNo); >> if (instance == NULL) { >> OVS_LOG_TRACE("Exit: Can not find open instance, dpNo: %d", >> @@ -365,6 +362,7 @@ OvsWaitEventIoctl(PIRP irp, >> return STATUS_INVALID_PARAMETER; >> } >> >> + OvsAcquireEventQueueLock(); >> queue = (POVS_EVENT_QUEUE)instance->eventQueue; >> if (queue == NULL) { >> OVS_LOG_TRACE("Exit: Event queue does not exist"); >> -- >> 2.9.0.windows.1 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org<mailto:dev@openvswitch.org> >> >>https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailm >>an_listinfo_dev&d=CwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r= >>Dcruz40PROJ40ROzSpxyQSLw6fcrOWpJgEcEmNR3JEQ&m=_qUqkq3AbylbGTt0FRPDctPJYJN >>u3r80SqmGYDgiNA4&s=HbFssXsaXxSrxCuEo36BQmPdk4BlTDTMpnC_OQkp_Ok&e= >> >_______________________________________________ >dev mailing list >dev@openvswitch.org<mailto:dev@openvswitch.org> >https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailma >n_listinfo_dev&d=CwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=Dc >ruz40PROJ40ROzSpxyQSLw6fcrOWpJgEcEmNR3JEQ&m=_qUqkq3AbylbGTt0FRPDctPJYJNu3r >80SqmGYDgiNA4&s=HbFssXsaXxSrxCuEo36BQmPdk4BlTDTMpnC_OQkp_Ok&e= _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
