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>
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> 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>
> >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>
> >> ---
> >> 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
> >>
> >>
> 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
> >
> 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
