[haiku-gsoc] Re: XSI semaphore implamentation patch #2

Log:
- Fixed warnings
- Fixed deadlock in xsi_sem_undo - RecordUndo
- Fixed issue in xsi_sem_undo: if the semaphore set does not exist
anymore, ignore the request
but do remove the process from the sUndoList, which wasn't previously done.

Regards,
-- 
Salvatore Benedetto (a.k.a. emitrax)
Student of Computer Engineer
University of Pisa
www.haiku-os.it

Index: src/system/kernel/posix/xsi_semaphore.cpp
===================================================================
--- src/system/kernel/posix/xsi_semaphore.cpp   (revision 26677)
+++ src/system/kernel/posix/xsi_semaphore.cpp   (working copy)
@@ -133,7 +133,7 @@
        int RecordUndo(int semaphoreSetID, short semaphoreNumber, short value);
 
        // Implemented after sUndoListLock is declared
-       int RemoveUndo(int semaphoreSetID, short semaphoreNumber, short value);
+       void RemoveUndo(int semaphoreSetID, short semaphoreNumber, short value);
 
        void Revert(short value)
        {
@@ -526,7 +526,8 @@
                        // Update its undo value
                        TRACE(("XsiSemaphore::RecordUndo: found record. Team = 
%d, "
                                "semaphoreSetID = %d, semaphoreNumber = %d, 
value = %d\n",
-                               team->id, semaphoreSetID, semaphoreNumber, 
current->undo_value));
+                               (int)team->id, semaphoreSetID, semaphoreNumber,
+                               current->undo_value));
                        int newValue = current->undo_value + value;
                        if (newValue > USHRT_MAX || newValue < -USHRT_MAX) {
                                TRACE_ERROR(("XsiSemaphore::RecordUndo: 
newValue %d out of range\n",
@@ -559,13 +560,14 @@
                sUndoList.Add(request);
                TRACE(("XsiSemaphore::RecordUndo: new record added. Team = %d, "
                        "semaphoreSetID = %d, semaphoreNumber = %d, value = 
%d\n",
-                       team->id, semaphoreSetID, semaphoreNumber, 
request->undo_value));
+                       (int)team->id, semaphoreSetID, semaphoreNumber,
+                       request->undo_value));
        }
        return B_OK;
 }
 
 
-int
+void
 XsiSemaphore::RemoveUndo(int semaphoreSetID, short semaphoreNumber, short 
value)
 {
        // This can be called only when RecordUndo fails.
@@ -643,6 +645,11 @@
        if (numberOfUndos <= 0)
                return;
 
+       // Since we only have one lock for both the semaphore set
+       // hash table and the semaphore set itself (not worth to add
+       // a lock per set), we must hold this mutex before the sem_undo
+       // one in order to avoid deadlock with RecordUndo
+       MutexLocker lock(sXsiSemaphoreSetLock);
        MutexLocker _(sUndoListLock);
        DoublyLinkedList<sem_undo>::Iterator iterator = sUndoList.GetIterator();
        // Look for all sem_undo request from this team
@@ -651,21 +658,21 @@
                if (current->team->id == teamID) {
                        // Check whether the semaphore set still exist
                        int semaphoreSetID = current->semaphore_set_id;
-                       MutexLocker _(sXsiSemaphoreSetLock);
                        XsiSemaphoreSet *semaphoreSet
                                = sSemaphoreHashTable.Lookup(semaphoreSetID);
-                       if (semaphoreSet == NULL) {
+                       if (semaphoreSet != NULL) {
+                               // Revert the changes done by this process
+                               XsiSemaphore *semaphore
+                                       = 
semaphoreSet->Semaphore(current->semaphore_number);
+                               TRACE(("xsi_do_undo: TeamID = %d, 
SemaphoreSetID = %d, "
+                                       "SemaphoreNumber = %d, undo value = 
%d\n", (int)teamID,
+                                       semaphoreSetID, 
current->semaphore_number,
+                                       current->undo_value));
+                               semaphore->Revert(current->undo_value); 
+                       } else
                                TRACE(("xsi_do_undo: semaphore set %d does not 
exist "
                                        "anymore. Ignore record.\n", 
semaphoreSetID));
-                               continue;
-                       }
-                       // Revert the changes done by this process
-                       XsiSemaphore *semaphore
-                               = 
semaphoreSet->Semaphore(current->semaphore_number);
-                       TRACE(("xsi_do_undo: TeamID = %d, SemaphoreSetID = %d, "
-                               "SemaphoreNumber = %d, undo value = %d\n", 
teamID,
-                               semaphoreSetID, current->semaphore_number, 
current->undo_value));
-                       semaphore->Revert(current->undo_value);
+
                        // Remove and free the sem_undo structure from sUndoList
                        iterator.Remove();
                        free(current);
@@ -976,7 +983,7 @@
 status_t
 _user_xsi_semop(int semaphoreID, struct sembuf *ops, size_t numOps)
 {
-       TRACE(("xsi_semop: semaphoreID = %d, ops = %p, numOps = %d\n",
+       TRACE(("xsi_semop: semaphoreID = %d, ops = %p, numOps = %ld\n",
                semaphoreID, ops, numOps));
        MutexLocker lock(sXsiSemaphoreSetLock);
        XsiSemaphoreSet *semaphoreSet = sSemaphoreHashTable.Lookup(semaphoreID);
@@ -1021,14 +1028,13 @@
        while (notDone) {
                XsiSemaphore *semaphore = NULL;
                short numberOfSemaphores = semaphoreSet->NumberOfSemaphores();
-               // TODO: Perhaps lock the set itself?
                bool goToSleep = false;
 
                uint32 i = 0;
                for (; i < numOps; i++) {
                        short semaphoreNumber = operations[i].sem_num;
                        if (semaphoreNumber >= numberOfSemaphores) {
-                               TRACE_ERROR(("xsi_semop: %d invalid semaphore 
number\n", i));
+                               TRACE_ERROR(("xsi_semop: %ld invalid semaphore 
number\n", i));
                                result = EINVAL;
                                break;
                        }
@@ -1100,12 +1106,10 @@
                        // We acquired the semaphore, now records the sem_undo
                        // requests
                        XsiSemaphore *semaphore = NULL;
-                       short numberOfSemaphores = 
semaphoreSet->NumberOfSemaphores();
                        uint32 i = 0;
                        for (; i < numOps; i++) {
                                short semaphoreNumber = operations[i].sem_num;
                                semaphore = 
semaphoreSet->Semaphore(semaphoreNumber);
-                               unsigned short value = semaphore->Value();
                                short operation = operations[i].sem_op;
                                if (operations[i].sem_flg & SEM_UNDO)
                                        if 
(semaphore->RecordUndo(semaphoreSet->ID(),
@@ -1129,7 +1133,8 @@
                                                result = ENOSPC;
                                }
                        }
-                       // TODO: Also set last PID for this semaphore set
+                       // We did it. Set the pid.
+                       semaphore->SetPid(getpid());
                }
        }
        return result;

Other related posts: