Bug 506520 - SAF Test lck: saLckResourceUnlock/5.c
SAF Test lck: saLckResourceUnlock/5.c
Status: CLOSED UPSTREAM
Product: Fedora
Classification: Fedora
Component: openais (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Ryan O'Hara
Fedora Extras Quality Assurance
: Reopened
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-06-17 11:56 EDT by Jan Friesse
Modified: 2009-06-26 05:44 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-06-23 18:25:54 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
5.c (4.90 KB, text/x-csrc)
2009-06-17 11:56 EDT, Jan Friesse
no flags Details
5.c (4.31 KB, text/x-csrc)
2009-06-18 12:05 EDT, Jan Friesse
no flags Details
3-1.c (5.62 KB, text/x-csrc)
2009-06-18 12:08 EDT, Jan Friesse
no flags Details
3-1-fork.c (4.23 KB, text/x-csrc)
2009-06-18 12:08 EDT, Jan Friesse
no flags Details
9.c (4.89 KB, text/x-csrc)
2009-06-18 12:15 EDT, Jan Friesse
no flags Details
Modified 5.c test case. (6.18 KB, text/x-csrc)
2009-06-21 23:40 EDT, Ryan O'Hara
no flags Details
Fix lck_unlock() to not send lock grant callback. (409 bytes, application/octet-stream)
2009-06-21 23:52 EDT, Ryan O'Hara
no flags Details

  None (edit)
Description Jan Friesse 2009-06-17 11:56:19 EDT
Created attachment 348282 [details]
5.c

Description of problem:
Test in SUBJ doesn't work

Version-Release number of selected component (if applicable):
Trunk

How reproducible:
Run it

Steps to Reproduce:
1.
2.
3.
  
Actual results:
select error!

Expected results:
Test runs ok

Additional info:
It looks like saLckResourceUnlock doesn't cancel pending request lock
Comment 1 Jan Friesse 2009-06-18 12:05:53 EDT
Created attachment 348512 [details]
5.c

saLckResourceClose Seems like a have similar problem.
Comment 2 Jan Friesse 2009-06-18 12:08:08 EDT
Created attachment 348514 [details]
3-1.c

Similar like two above, but with child.
Comment 3 Jan Friesse 2009-06-18 12:08:38 EDT
Created attachment 348515 [details]
3-1-fork.c

Child for 3-1.c
Comment 4 Jan Friesse 2009-06-18 12:15:45 EDT
Created attachment 348518 [details]
9.c

Looks main problem is similar like previous 3.
Comment 5 Ryan O'Hara 2009-06-19 18:46:33 EDT
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.
Comment 6 Ryan O'Hara 2009-06-20 00:34:15 EDT
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.
Comment 7 Steven Dake 2009-06-20 00:52:06 EDT
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.
Comment 8 Ryan O'Hara 2009-06-21 23:37:48 EDT
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.
Comment 9 Ryan O'Hara 2009-06-21 23:40:29 EDT
Created attachment 348846 [details]
Modified 5.c test case.

Here is a new saLckResourceUnlock/5.c test case.
Comment 10 Ryan O'Hara 2009-06-21 23:49:03 EDT
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.
Comment 11 Ryan O'Hara 2009-06-21 23:52:53 EDT
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.
Comment 12 Ryan O'Hara 2009-06-23 11:24:29 EDT
Closing this as fixed upstream.
Comment 13 Ryan O'Hara 2009-06-23 17:23:57 EDT
(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.
Comment 14 Ryan O'Hara 2009-06-23 17:27:44 EDT
(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?
Comment 15 Ryan O'Hara 2009-06-23 17:34:00 EDT
(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.
Comment 16 Jan Friesse 2009-06-24 10:23:32 EDT
(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.
Comment 17 Jan Friesse 2009-06-24 10:26:07 EDT
(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!).
Comment 18 Jan Friesse 2009-06-24 10:27:30 EDT
(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.
Comment 19 Jan Friesse 2009-06-26 05:44:33 EDT
saLckResourceClose/5.c, saLckResourceClose/3-1.c, saLckResourceLockAsync/9.c splited to #508231, #508232 and #508233

Note You need to log in before you can comment on or make changes to this bug.