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
> > >> [email protected]
> > >> https://lists.linux-foundation.org/mailman/listinfo/openais
> >
> > _______________________________________________
> > Openais mailing list
> > [email protected]
> > https://lists.linux-foundation.org/mailman/listinfo/openais
_______________________________________________
Openais mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/openais
