Bug 506529 - SAF Test lck: SaLckResourceUnlockCallbackT/7
Summary: SAF Test lck: SaLckResourceUnlockCallbackT/7
Keywords:
Status: CLOSED UPSTREAM
Alias: None
Product: Fedora
Classification: Fedora
Component: openais
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Ryan O'Hara
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-06-17 16:21 UTC by Jan Friesse
Modified: 2009-06-27 14:26 UTC (History)
3 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2009-06-27 14:26:26 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
7.c (6.07 KB, text/x-csrc)
2009-06-17 16:21 UTC, Jan Friesse
no flags Details
Another test with similar problem (5.62 KB, text/x-csrc)
2009-06-18 09:20 UTC, Jan Friesse
no flags Details
Change the way we do unlock operations. (3.82 KB, application/octet-stream)
2009-06-27 02:54 UTC, Ryan O'Hara
no flags Details

Description Jan Friesse 2009-06-17 16:21:06 UTC
Created attachment 348290 [details]
7.c

Description of problem:
SSIA

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


How reproducible:
Run test

Steps to Reproduce:
1.
2.
3.
  
Actual results:
  Does not conform the expected behaviors!
  saLckResourceUnlock, Return value: SA_AIS_ERR_NOT_EXIST, should be SA_AIS_OK
callback return should be SA_AIS_ERR_NOT_EXIST


Expected results:
Test will run ok

Additional info:
Code is not much clear, but it looks like:
saLckResourceUnlockAsync is called
select wait for event
saLckResourceUnlock is called - should unlock
saLckDispatch - should run real saLckResourceUnlockAsync

Comment 1 Jan Friesse 2009-06-18 09:20:43 UTC
Created attachment 348399 [details]
Another test with similar problem

Form my point of view, included file has some problem as original.

Comment 2 Ryan O'Hara 2009-06-26 03:53:49 UTC
In regards to the bug with SaLckResourceUnlockCallbackT/7.c ...

This is a tricky problem. The issue is that we remove the lock in the exec/service level immediately in both saLckResourceUnlock and saLckResourceUnlockAsync. Any subsequent calls to Unlock/UnlockAsync result is SA_AIS_ERR_NOT_EXIST being passed back to the library from the exec.
Also note that we remove the lockId form the database in the library as soon as either call returns from the exec without error. This might have to change.

This test seems to suggest that we should not actually remove the lock until the callback is processed (in saLckDispatch). Honestly, that seems a bit strange to me, it is what we would have to do in order to pass this test.

It is also important to note that the synchronous call, saLckResourceUnlock, takes a timeout value. Because of the way openais is implemented, we cna safely ignore this. The saLckResourceUnlock call never blocks, so we have no need for this timeout. Because of this details, saLckResourceUnlock and saLckResourceUnlockAsync behave exactly the same -- except that the async call generates a callback. At least that is how the code works now.

So, we could make these two calls behave differently by deferring removal of the lock (for the async call) until the callback is processed. This seems pointless, but as I said it will allow us to pass this test.

As far that the code for this test case is concerned, I believe that they are testing the following:

1) Create a lock.
2) Unlock the lock (async).
3) Unlock the lock (sync).
4) Process the callback from #2.

For this test to work, the async unlock (step #2) will not actually remove the lock. Then (step #3) the synchronous unlock occurs and does actually remove the lock. Finally (step #4), the callback is processed but the lock is gone, thus we get SA_AIS_ERR_NOT_EXIST.

Comment 3 Jan Friesse 2009-06-26 09:52:35 UTC
(In reply to comment #1)
> Created an attachment (id=348399) [details]
> Another test with similar problem
> 
> Form my point of view, included file has some problem as original.  

Filled as separate BZ 508236

Comment 4 Ryan O'Hara 2009-06-27 02:53:13 UTC
I'm attaching the patch I sent upstream that fixes this test case. Here is an explanation of said patch:

The old code would remove a lock as soon as either Unlock or
UnlockAsync received a result from the exec without error. The two
calls were exactly the same except that the async call generated a
callback.

This patch changes saLckResourceUnlockAsync such that the lock is not
removed from the database in the library until the callback it made.

This patch also changes both saLckResourceUnlock and
saLckResourceUnlockAsync such that if a lock is not found in the
executive layer, we do not return an error. it is valid for the
lock_id to exist in the library database but not the executive, as
this is a symptom of having an async unlock do the unlock but not
remove the lock from the database in the library because its callback
has not yet been processed.

Finally, saLckDispatch changes the way it deals with resource unlock
callbacks. If the lock_id specified in the callback exists, we remove
it. It it does not exist, it means that the lock was removed while the
callback was waiting to be processed. In this case we return
SA_AIS_ERR_NOT_EXIST, per the spec.

Comment 5 Ryan O'Hara 2009-06-27 02:54:58 UTC
Created attachment 349619 [details]
Change the way we do unlock operations.

Here is the patch I sent upstream. It fixes the problems with SaLckResourceUnlockCallbackT/7.c test case.

Comment 6 Ryan O'Hara 2009-06-27 14:26:26 UTC
Fixed upstream. OpenAIS trunk, revision 2005.


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