Bug 506520

Created attachment 348512 [details]
5.c

saLckResourceClose Seems like a have similar problem.

Created attachment 348514 [details]
3-1.c

Similar like two above, but with child.

Created attachment 348515 [details]
3-1-fork.c

Child for 3-1.c

Created attachment 348518 [details]
9.c

Looks main problem is similar like previous 3.

I appears that the problem here is that the callback associated with the saLckResourceLockAsync is made despite the fact that the lock request was cancelled. If that is the case, the fix would be to verify that the lockId is still valid before we do the callback.

Actually, we already deal with the case that I described in comment #5. Furthermore, I believe that this specific test case is invalid and contradicts the lock service specification.

From the SAForum Lock Service Specifiction B.03.01 for SaLckLockGrantCallbackT:

error -
...
  SA_AIS_ERR_NOT_EXIST - The lock id that was returned in the
  corresponding invocation of the saLckResourceLockAsync() function has
  become invalid prior to the invocation of this callback, due to the
  lock request having been canceled in the period between
  saLckResourceLockAsync() and the execution of this callback.

This is exactly what our code does. In saLckDispatch, we verify that the lock id is valid. If not, we proceed with the callback and set error to SA_AIS_ERR_NOT_EXIST.

This test case expects that we will *not* get a callback if we unlock (cancel) the lock. This is not true. If the lock was granted and cancelled before the callback occurs, the callback will be made with SA_AIS_ERR_NOT_EXIST.

This is a test case for unlock not grant callback.  Reading the spec shows that the test intends to test the following:

An invocation of this function releases synchronously the lock identified by lockId. If the lockId identifies a pending lock request, then the pending lock request will be canceled.

The proper way to test this is create a thread, and 
lock (ex)
create thread
lock (pr) in thread
unlock (pr) in thread
unlock (ex)
pthread_join the thread that we unlocked
verify no grant callback occurs for the pr lock id.

I am sure the lock service works properly in this test case, but the test case is invalid.

Steve is correct, the test case is invalid. The description states that this code is meant to test the following:

SA_AIS_ERR_BAD_HANDLE - The handle lockResourceHandle, which was specified in the saLckResourceOpen() or saLckResourceOpenAsync() call, leading to the handle lockResourceHandle, which was specified in the corresponding saLckResourceLock() or saLckResourceLockAsync() call to obtain the lockId has become invalid due to one or both of the reasons below:
 - It is corrupted, or the corresponding lock resource has already been closed.
 - The handle lckHandle that was passed to the functions saLckResourceOpen() or
   saLckResourceOpenAsync() has already been finalized.

I'm attaching a new test case that tests this requirement. Here is how it works:

1. Open a resource.
2. Request EX lock on resource (async), which will be granted.
3. Call dispatch to get lock grant callback.
4. Request PR lock on resource (async), which will be queued.
5. Call dispatch to get lock waiter callback.
6. Unlock (cancel) PR lock.
7. Unlock EX lock.
8. Verify no lock grant callback occurs for PR lock.

This seems to work. Note that there are some extra printf statements in the callbacks (for debugging) and this code doesn't check for errors when calling saLckDispatch.

Created attachment 348846 [details]
Modified 5.c test case.

Here is a new saLckResourceUnlock/5.c test case.

I think there is another problem in the code. In the existing code, when a pending lock is unlocked (cancelled) we send a callback/response for the lock. We only need to do this for locks that were requested with saLckResourceLock, not async. Basically the idea is that if we cancel a pending lock that was requested via saLckResourceLock (sync), we send a response to the library. If that lock was requested with saLckResourceLockAsync, we can simply drop the lock request and we do *not* need to send a callback.

I'm attaching a patch that removes the callback for this situation.

Created attachment 348847 [details]
Fix lck_unlock() to not send lock grant callback.

This patch removes an unnecessary lock grant callback from lck_unlock() when unlocking pending lock requested that were request the async lock request.

Closing this as fixed upstream.

(In reply to comment #4)
> Created an attachment (id=348518) [details]
> 9.c
> 
> Looks main problem is similar like previous 3.  

This test is bogus. Also worth noting here that the "9.c" test case referred to here is for saLckResourceLockAsync.

I am not sure what this test is supposed to accompish. The test case itself states that it is testing the ability to unlock a lock. That is fine, but I don't see why the code does a select after the unlock. That is where is is failing. This test shouldn't have anything to do with callbacks. Furthermore, it is completely normal for a lock grant callback to occur in this test.

(In reply to comment #2)
> Created an attachment (id=348514) [details]
> 3-1.c
> 
> Similar like two above, but with child.  

What exactly is wrong with this test case and how does it relate to saLckResourceUnlock/5.c?

(In reply to comment #1)
> Created an attachment (id=348512) [details]
> 5.c
> 
> saLckResourceClose Seems like a have similar problem.  

This test case is for the saLckResourceClose API call. It has nothing to do with saLckResourceUnlock.

(In reply to comment #13)
> (In reply to comment #4)
> > Created an attachment (id=348518) [details] [details]
> > 9.c
> > 
> > Looks main problem is similar like previous 3.  
> 
> This test is bogus. Also worth noting here that the "9.c" test case referred to
> here is for saLckResourceLockAsync.
> 
> I am not sure what this test is supposed to accompish. The test case itself
> states that it is testing the ability to unlock a lock. That is fine, but I
> don't see why the code does a select after the unlock. That is where is is
> failing. This test shouldn't have anything to do with callbacks. Furthermore,
> it is completely normal for a lock grant callback to occur in this test.  

In case you are sure about correctness of your implementation, I will delete that select.

(In reply to comment #14)
> (In reply to comment #2)
> > Created an attachment (id=348514) [details] [details]
> > 3-1.c
> > 
> > Similar like two above, but with child.  
> 
> What exactly is wrong with this test case and how does it relate to
> saLckResourceUnlock/5.c?  

Returns same error (select error!).

(In reply to comment #15)
> (In reply to comment #1)
> > Created an attachment (id=348512) [details] [details]
> > 5.c
> > 
> > saLckResourceClose Seems like a have similar problem.  
> 
> This test case is for the saLckResourceClose API call. It has nothing to do
> with saLckResourceUnlock.  

Yes, it's nothing to do with saLckResourceUnlock, but returns same error (select error!). If you want separate bugzilla, I can create to you.

saLckResourceClose/5.c, saLckResourceClose/3-1.c, saLckResourceLockAsync/9.c splited to #508231, #508232 and #508233