[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openais
Subject:    Re: [Openais] [PATCH][openais] LckDispatch can get into a tight	loop
From:       "Ryan O'Hara" <rohara () redhat ! com>
Date:       2010-01-14 21:22:13
Message-ID: 20100114212213.GA7326 () redhat ! com
[Download RAW message or body]

On Thu, Jan 14, 2010 at 09:35:51AM -0700, Steven Dake wrote:
> On Thu, 2010-01-14 at 13:31 +0000, Christine Caulfield wrote:
> > On 13/01/10 20:32, Ryan O'Hara wrote:
> > > On Wed, Jan 13, 2010 at 09:58:29AM +0000, Christine Caulfield wrote:
> > >> If a callback is set to NULL and a message is received for that
> > >> callback, libSaLck will get into a tight loop because the continue
> > >> doesn't call coroipcc_dispatch_put.
> > >>
> > >> This patch fixes
> > >>
> > >> Chrissie
> > >
> > >
> > > I don't think the first 'goto dispatch_continue' is necessary (in the
> > > case where coroipcc_dispatch_get returns TRY_AGAIN.
> > 
> > Actually I think that probably is needed. Otherwise there is a 
> > coroipcc_dispatch_get() without a corresponding put. Or perhaps it ought 
> > to jump to error_put:  ?
> > 
> The patch looks good minus the part Ryan mentioned and is a great catch.
> We need this for all the other library services as well.
> 
> Ryan is correct about the incorrect use of coroipcc_dispatch_get calling
> put on TRY_AGAIN.  coroipcc_dispatch_put should only be called if
> dispatch_get returns CS_OK.  In coroipcc.c, when TRY_AGAIN is returned
> by coroipcc_dispatch_get, the original state is kept on the ipc
> connection before returning.  (ie it turns into a NOOP only returning
> TRY_AGAIN).

Also, would it be better (or at least equivalent) to replace 'continue'
with 'break'? The goto statement is fine, but I believe 'break' will
accomplish the same thing. I'd rather not add any more goto statements
if we can avoid it.

Ryan



> > 
> > Chrissie
> > 
> > > Other than that, ACK.
> > >
> > > Ryan
> > >
> > >
> > >
> > >> Index: lib/lck.c
> > >> ===================================================================
> > >> --- lib/lck.c	(revision 2097)
> > >> +++ lib/lck.c	(working copy)
> > >> @@ -332,7 +332,7 @@
> > >>   			if (dispatchFlags == CPG_DISPATCH_ALL) {
> > >>   				break;
> > >>   			} else {
> > >> -				continue;
> > >> +				goto dispatch_continue;
> > >>   			}
> > >>   		}
> > >>   		if (error != CS_OK) {
> > >> @@ -345,7 +345,7 @@
> > >>   		switch (dispatch_data->id) {
> > >>   		case MESSAGE_RES_LCK_RESOURCEOPEN_CALLBACK:
> > >>   			if (callbacks.saLckResourceOpenCallback == NULL) {
> > >> -				continue;
> > >> +				goto dispatch_continue;
> > >>   			}
> > >>   			res_lib_lck_resourceopen_callback =
> > >>   				(struct res_lib_lck_resourceopen_callback *)dispatch_data;
> > >> @@ -374,7 +374,7 @@
> > >>
> > >>   		case MESSAGE_RES_LCK_LOCKGRANT_CALLBACK:
> > >>   			if (callbacks.saLckLockGrantCallback == NULL) {
> > >> -				continue;
> > >> +				goto dispatch_continue;
> > >>   			}
> > >>   			res_lib_lck_lockgrant_callback =
> > >>   				(struct res_lib_lck_lockgrant_callback *)dispatch_data;
> > >> @@ -421,7 +421,7 @@
> > >>
> > >>   		case MESSAGE_RES_LCK_LOCKWAITER_CALLBACK:
> > >>   			if (callbacks.saLckLockWaiterCallback == NULL) {
> > >> -				continue;
> > >> +				goto dispatch_continue;
> > >>   			}
> > >>   			res_lib_lck_lockwaiter_callback =
> > >>   				(struct res_lib_lck_lockwaiter_callback *)dispatch_data;
> > >> @@ -451,7 +451,7 @@
> > >>
> > >>   		case MESSAGE_RES_LCK_RESOURCEUNLOCK_CALLBACK:
> > >>   			if (callbacks.saLckResourceUnlockCallback == NULL) {
> > >> -				continue;
> > >> +				goto dispatch_continue;
> > >>   			}
> > >>   			res_lib_lck_resourceunlock_callback =
> > >>   				(struct res_lib_lck_resourceunlock_callback *)dispatch_data;
> > >> @@ -498,6 +498,7 @@
> > >>   		default:
> > >>   			break;
> > >>   		}
> > >> +	dispatch_continue:
> > >>   		coroipcc_dispatch_put (lckInstance->ipc_handle);
> > >>
> > >>   		switch (dispatchFlags) {
> > >
> > >> _______________________________________________
> > >> Openais mailing list
> > >> Openais@lists.linux-foundation.org
> > >> https://lists.linux-foundation.org/mailman/listinfo/openais
> > 
> > _______________________________________________
> > Openais mailing list
> > Openais@lists.linux-foundation.org
> > https://lists.linux-foundation.org/mailman/listinfo/openais
_______________________________________________
Openais mailing list
Openais@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/openais
[prev in list] [next in list] [prev in thread] [next in thread]