[haiku-gsoc] Re: Xsi semaphorses: patch #1
- From: Ingo Weinhold <ingo_weinhold@xxxxxx>
- To: haiku-gsoc@xxxxxxxxxxxxx
- Date: Sun, 13 Jul 2008 23:36:58 +0200
On 2008-07-13 at 15:43:59 [+0200], Salvatore Benedetto <emitrax@xxxxxxxxx>
wrote:
>
> the attached is a first implementation of the xsi semaphore.
> Althought not complete, it has everything it needs to get bonnie++
> up and running (which by they way I already ran and it already corrupted my
> haiku partition on vmware).
Well, as the implementation looks ATM, XSI semaphore won't work at all.
Please do at least write some tests (e.g. cf.
src/tests/system/libroot/posix/realtime_sem_test1.cpp for how this could
look like). There might also be tests in the POSIX test suite.
And please complete the XSI semaphores implementation before actually
looking into bonnie++. E.g. APR favors XSI semaphores over realtime
semaphores, because the former have the SEM_UNDO feature.
Index: src/system/kernel/posix/xsi_semaphore.cpp
===================================================================
--- src/system/kernel/posix/xsi_semaphore.cpp (revision 0)
+++ src/system/kernel/posix/xsi_semaphore.cpp (revision 0)
@@ -0,0 +1,735 @@
+/*
+ * Copyright 2008, Haiku Inc. All rights reserved.
+ * Distributed under the terms of the MIT License.
+ *
+ * Authors:
+ * Salvatore Benedetto <salvatore.benedetto@xxxxxxxxx>
+ */
+
+#include <posix/xsi_semaphore.h>
+#include <posix/realtime_sem_defs.h>
+
+#include <new>
+
+#include <OS.h>
+
+#include <kernel.h>
+#include <syscall_restart.h>
+
+#include <sys/ipc.h>
+#include <sys/types.h>
Coding style: Public POSIX headers should be included before public
BeOS/Haiku headers.
+
+#include <util/DoublyLinkedList.h>
+#include <util/OpenHashTable.h>
+#include <util/Vector.h>
+//#define TRACE_XSI_SEM
+#ifdef TRACE_XSI_SEM
+# define TRACE(x) dprintf x
+# define TRACE_ERROR(x) dprintf x
+#else
+# define TRACE(x) /* nothing */
+# define TRACE_ERROR(x) dprintf x
+#endif
Coding style: Leave at least one blank line between includes and the
debugging stuff.
+
+//#define KTRACE_XSI_SEM
+#ifdef KTRACE_XSI_SEM
+# define KTRACE(x...) ktrace_printf(x)
+#else
+# define KTRACE(x...)
+#endif
+
+// Queue for holding blocked threads
+struct queued_thread : DoublyLinkedListLinkImpl<queued_thread> {
+ queued_thread(struct thread *thread, int32 count)
+ :
+ thread(thread),
+ count(count),
+ queued(false)
+ {
+ }
+
+ struct thread *thread;
+ int32 count;
+ bool queued;
+ bool destroyed;
+};
"destroyed" is not needed.
+
+typedef DoublyLinkedList<queued_thread> ThreadQueue;
+
+
+// Xsi semaphore definition
+class XsiSemaphore {
+public:
+ XsiSemaphore()
+ : fLastPidOperation(0),
+ fProcessesWaitingToIncrease(0),
+ fProcessesWaitingToBeZero(0),
+ fValue(0)
+ {
+ }
+
+ ~XsiSemaphore()
+ {
+ // For some reason the semaphore is getting destroyed.
+ // Wake up any remaing awaiting threads
+ GRAB_THREAD_LOCK();
You need to disable interrupts before trying to acquire a spinlock. That's
not done anywhere in the code.
+ while (queued_thread *entry =
fWaitingToIncreaseQueue.RemoveHead()) {
+ entry->queued = false;
+ entry->destroyed = true;
+ thread_unblock_locked(entry->thread, EIDRM);
+ }
+ while (queued_thread *entry =
fWaitingToBeZeroQueue.RemoveHead()) {
+ entry->queued = false;
+ entry->destroyed = true;
+ thread_unblock_locked(entry->thread, EIDRM);
+ }
+ RELEASE_THREAD_LOCK();
+ }
+
+ // We return true in case the operation causes the
+ // caller to wait, so it can undo all the operations
+ // previously done, by calling the same function
+ // with force set to true, which causes the operation
+ // to take place despite the result of the algebric sum
+ bool Add(short value, bool force)
+ {
+ if (!force) {
+ if ((int)(fValue + value) < 0) {
+ TRACE(("XsiSemaphore::Add: going to sleep\n"));
+ return true;
+ } else {
+ fValue += value;
+ if ((fValue == 0) && (fProcessesWaitingToBeZero
> 0))
+ WakeUpThread(true);
+ else if ((fValue > 0) &&
(fProcessesWaitingToIncrease > 0))
+ WakeUpThread(false);
+ return false;
+ }
+ }
+ // We are being forced to revert our changes
+ fValue -= value;
+ return false;
+ }
This method should be split up in two. ATM, depending on "force" two
completely different things (that share no common code at all) happen.
+
+ pid_t GetPid() const
+ {
+ return fLastPidOperation;
+ }
Coding style: Getters generally don't have a "Get" prefix, only when the
method name would otherwise clash with a type name.
+ ushort GetProcessesWaitingToIncrease() const
+ {
+ return fProcessesWaitingToIncrease;
+ }
+
+ ushort GetProcessesWaitingToBeZero() const
+ {
+ return fProcessesWaitingToBeZero;
+ }
+
+ ushort GetValue() const
+ {
+ return fValue;
+ }
+
+ void SetPid(pid_t pid)
+ {
+ fLastPidOperation = pid;
+ }
+
+ void SetValue(ushort value)
+ {
+ fValue = value;
+ }
+
+ status_t Wait(int32 count, bool waitForZero)
+ {
+ // enqueue the thread in the appropriate
+ // queue and get ready to wait
+ struct thread *thread = thread_get_current_thread();
+ queued_thread queueEntry(thread, count);
+ if (waitForZero) {
+ fWaitingToBeZeroQueue.Add(&queueEntry);
+ fProcessesWaitingToBeZero++;
+ } else {
+ fWaitingToIncreaseQueue.Add(&queueEntry);
+ fProcessesWaitingToIncrease++;
+ }
+ queueEntry.queued = true;
+
+ thread_prepare_to_block(thread, 0, THREAD_BLOCK_TYPE_OTHER,
+ (void*)"xsi semaphore");
+ GRAB_THREAD_LOCK();
+ status_t result = thread_block_locked(thread);
Waiting must be interruptable! Pass B_CAN_INTERRUPT to
thread_prepare_to_block().
+ RELEASE_THREAD_LOCK();
+
+ return result;
+ }
+
+ void WakeUpThread(bool waitingForZero)
+ {
+ queued_thread *entry;
+ bool unblock = false;
+
+ GRAB_THREAD_LOCK();
+ if (waitingForZero) {
+ unblock = true;
+ entry = fWaitingToBeZeroQueue.RemoveHead();
+ } else {
+ entry = fWaitingToIncreaseQueue.Head();
+ if ((entry->count + fValue) > 0) {
+ unblock = true;
+ entry = fWaitingToIncreaseQueue.RemoveHead();
+ }
+ }
+ if (unblock) {
+ entry->queued = false;
+ entry->destroyed = true;
+ thread_unblock_locked(entry->thread, 0);
+ }
+ RELEASE_THREAD_LOCK();
+ }
This method only unblocks one thread. In the "wait for zero" case other
threads will be stuck.
+private:
+ pid_t fLastPidOperation;
// sempid
+ ushort fProcessesWaitingToIncrease; // semncnt
+ ushort fProcessesWaitingToBeZero; //
semzcnt
Actually "Processes" should be "Threads", even if the standard speaks of
processes.
+ short fValue;
// semval
+
+ ThreadQueue fWaitingToIncreaseQueue;
+ ThreadQueue fWaitingToBeZeroQueue;
+};
+
+
+
+// Xsi semaphore set definition (semid_ds)
+class XsiSemaphoreSet {
+public:
+ XsiSemaphoreSet(int nsems, int flags)
+ : fLastSemctlTime((time_t)real_time_clock()),
+ fLastSemopTime(0),
+ fNumberOfSemaphores(nsems),
+ fSemaphores(0)
+ {
+ SetID();
+ SetIpcKey((key_t)-1);
+ SetPermissions(flags);
+
+ for (int i = 0; i < nsems; i++) {
+ XsiSemaphore *current = new(std::nothrow)
XsiSemaphore();
+ if (current == NULL) {
+ TRACE_ERROR(("XsiSemaphoreSet::XsiSemaphore():
failed to allocate "
+ "XsiSemaphore object\n"));
+ return;
+ }
+ fSemaphores.PushBack(current);
+ }
+ }
+
+ ~XsiSemaphoreSet()
+ {
+ for (Vector<XsiSemaphore *>::Iterator i = fSemaphores.Begin();
+ i != fSemaphores.End(); i++) {
+ delete (*i);
+ }
+ }
+
+ int GetID() const
+ {
+ return fID;
+ }
+
+ key_t GetIpcKey() const
+ {
+ return fPermissions.key;
+ }
+
+ ushort GetNumberOfSemaphores() const
+ {
+ return fNumberOfSemaphores;
+ }
+
+ XsiSemaphore* GetSemaphore(int nth) const
+ {
+ return fSemaphores[nth];
+ }
+
+ // Implemented after sSemaphoreHashTable is declared
+ void SetID();
+
+ void SetIpcKey(key_t key)
+ {
+ fPermissions.key = key;
+ }
+
+ void SetLastSemctlTime()
+ {
+ fLastSemctlTime = real_time_clock();
+ }
+
+ void SetLastSemopTime()
+ {
+ fLastSemopTime = real_time_clock();
+ }
+
+ void SetPermissions(int flags)
+ {
+ fPermissions.uid = fPermissions.cuid = geteuid();
+ fPermissions.gid = fPermissions.cgid = getegid();
+ fPermissions.mode = (flags & 0x01ff);
+ }
+
+ bool HasPermission() const
+ {
+ if ((fPermissions.mode & S_IWOTH) != 0)
+ return true;
+
+ uid_t uid = geteuid();
+ if (uid == 0 || (uid == fPermissions.uid && (fPermissions.mode
& S_IWUSR)
!= 0))
+ return true;
+
+ gid_t gid = getegid();
+ if (gid == fPermissions.gid && (fPermissions.mode & S_IWGRP) !=
0)
+ return true;
+
+ return false;
+ }
+
+ bool HasReadPermission() const
+ {
+ // TODO: fix this
+ return HasPermission();
+ }
+
+ HashTableLink<XsiSemaphoreSet>* Link()
+ {
+ return &fLink;
+ }
+
+private:
+ int fID;
// semaphore set id
+ time_t fLastSemctlTime;
// sem_ctime
+ time_t fLastSemopTime;
// sem_otime
+ ushort fNumberOfSemaphores;
// sem_nsems
+ struct ipc_perm fPermissions;
// sem_perm
+ Vector<XsiSemaphore*> fSemaphores;
Why not just use a XsiSemaphore[]? Unless I miss something the number of
semaphores never changes after having been created.
+
+ ::HashTableLink<XsiSemaphoreSet> fLink;
+};
+
+// Xsi semaphore set hash table
+struct SemaphoreHashTableDefinition {
+ typedef int KeyType;
+ typedef XsiSemaphoreSet ValueType;
+
+ size_t HashKey (const int key) const
+ {
+ return (size_t)key;
+ }
+ size_t Hash(XsiSemaphoreSet* variable) const
+ {
+ return (size_t)variable->GetID();
+ }
+ bool Compare(const int key, XsiSemaphoreSet* variable) const
+ {
+ return (int)key == (int)variable->GetID();
+ }
+ HashTableLink<XsiSemaphoreSet>* GetLink(XsiSemaphoreSet* variable) const
+ {
+ return variable->Link();
+ }
+};
+
+static OpenHashTable<SemaphoreHashTableDefinition> sSemaphoreHashTable;
+static int sNextAvailableID = 0;
+static spinlock xsi_semaphore_set_spinlock = B_SPINLOCK_INITIALIZER;
+#define GRAB_SEM_LOCK()
acquire_spinlock(&xsi_semaphore_set_spinlock)
+#define RELEASE_SEM_LOCK()
release_spinlock(&xsi_sempahore_set_spinlock)
Any particular reason for using a spinlock instead of a mutex? Besides the
lock is not even used. In fact the complete implementation is totally
lacking locking.
+// Arbitrary limit
+#define MAX_XSI_SEMAPHORE 512
+static int sTotalNumberOfXsiSemaphores = 0;
+
+void
+XsiSemaphoreSet::SetID()
+{
+ // The spinlock is held upon creation of the object
+ while (true) {
+ if (sSemaphoreHashTable.Lookup(sNextAvailableID) == NULL)
+ break;
+ sNextAvailableID++;
+ }
+ fID = sNextAvailableID++;
+}
+
+
+// IPC class
+class Ipc {
+public:
+ Ipc(key_t key)
+ : fKey(key),
+ fSemaphoreSetId(-1)
+ {
+ }
+
+ key_t GetKey() const
+ {
+ return fKey;
+ }
+
+ int GetSemaphoreSetID() const
+ {
+ return fSemaphoreSetId;
+ }
+
+ void SetSemaphoreSetID(XsiSemaphoreSet *semaphoreSet)
+ {
+ fSemaphoreSetId = semaphoreSet->GetID();
+ }
+
+ bool HasSemaphoreSet()
+ {
+ if (fSemaphoreSetId != -1)
+ return true;
+ return false;
+ }
+
+ HashTableLink<Ipc>* Link()
+ {
+ return &fLink;
+ }
+
+private:
+ key_t fKey;
+ int fSemaphoreSetId;
+ HashTableLink<Ipc> fLink;
+};
+
+
+struct IpcHashTableDefinition {
+ typedef key_t KeyType;
+ typedef Ipc ValueType;
+
+ size_t HashKey (const key_t key) const
+ {
+ // TODO: Define a better hash function
+ // key_t is a 32 bit integer value.
+ return (size_t)(key & 0x00000fff) % 32;
How about the obvious: "return (size_t)key"?
+ }
+ size_t Hash(Ipc* variable) const
+ {
+ return HashKey(variable->GetKey());
+ }
+ bool Compare(const key_t key, Ipc* variable) const
+ {
+ return (key_t)key == (key_t)variable->GetKey();
+ }
+ HashTableLink<Ipc>* GetLink(Ipc* variable) const
+ {
+ return variable->Link();
+ }
+};
+
+static OpenHashTable<IpcHashTableDefinition> sIpcHashTable;
+static spinlock ipc_spinlock = B_SPINLOCK_INITIALIZER;
+#define GRAB_IPC_TABLE_LOCK() acquire_spinlock(&ipc_spinlock);
+#define RELEASE_IPC_TABLE_LOCK() release_spinlock(&ipc_spinlock);
Same here: Why a spinlock?
+
+
+status_t
+haiku_xsi_ipc_init(struct kernel_args *args)
+{
+ // Initialize hash tables
+ status_t error = sIpcHashTable.Init();
+ if (error != B_OK)
+ return error;
+ return sSemaphoreHashTable.Init();
+}
This function is never called. I suppose you want to do that in main.cpp.
+
+
+int
+_user_xsi_semget(key_t key, int numberOfSemaphores, int flags)
+{
+ XsiSemaphoreSet *semaphoreSet = NULL;
+ Ipc *IpcKey = NULL;
Coding style: ipcKey.
+ // Default assumptions
+ bool isPrivate = true;
+ bool create = true;
+
+ if (key != IPC_PRIVATE) {
+ isPrivate = false;
+ // Check if key already have a semaphore
+ // associated with it
+ IpcKey = sIpcHashTable.Lookup(key);
+ if (IpcKey == NULL) {
+ // The ipc key have probably just been created
+ // by the caller, add it to the system
+ IpcKey = new(std::nothrow) Ipc(key);
+ if (IpcKey == NULL) {
+ TRACE_ERROR(("xsi_semget: failed to create new
Ipc object "
+ "for key %d\n", (int)key));
+ return ENOMEM;
+ }
+ sIpcHashTable.Insert(IpcKey);
+ } else if (IpcKey->HasSemaphoreSet()) {
+ // The IPC key exist and it already has a semaphore
+ if ((flags & IPC_CREAT) && (flags & IPC_EXCL)) {
+ TRACE_ERROR(("xsi_semget: key %d already
exist\n", (int)key));
+ return EEXIST;
+ }
+ int semaphoreSetID = IpcKey->GetSemaphoreSetID();
+ semaphoreSet =
sSemaphoreHashTable.Lookup(semaphoreSetID);
+ if (!semaphoreSet->HasPermission()) {
+ TRACE_ERROR(("xsi_semget: calling process has
not permission "
+ "on semaphore %d, key %d\n",
semaphoreSet->GetID(),
+ (int)semaphoreSet->GetIpcKey()));
+ return EACCES;
+ }
+ if ((semaphoreSet->GetNumberOfSemaphores() <
numberOfSemaphores)
+ && (numberOfSemaphores != 0)) {
+ TRACE_ERROR(("xsi_semget: nsems greater than
the one "
+ "associated with semaphore %d, key
%d\n",
+ semaphoreSet->GetID(),
(int)semaphoreSet->GetIpcKey()));
+ return EINVAL;
+ }
+ create = false;
+ } else {
I don't see this case happening, if one employs proper locking. I.e.
creation of the semaphore and entering it into the table would be atomic.
+ // The IPC key exist but it has not semaphore
associated with it
+ if (!(flags & IPC_CREAT)) {
+ TRACE_ERROR(("xsi_semget: key %d has not
semaphore associated "
+ "with it and caller did not ask for
creation\n",(int)key));
+ return ENOENT;
+ }
+ }
+
+ if (create) {
+ // Create a new sempahore for this key
+ if (numberOfSemaphores < 0
+ || numberOfSemaphores >
MAX_POSIX_SEMS_PER_TEAM) {
+ TRACE_ERROR(("xsi_semget: nsems out of
range\n"));
+ return EINVAL;
+ }
MAX_POSIX_SEMS_PER_TEAM is really not related.
+ if (sTotalNumberOfXsiSemaphores < MAX_XSI_SEMAPHORE) {
+ TRACE_ERROR(("xsi_semget: reached limit of
maximum number of "
+ "semaphores
allowed\n"));
+ return ENOSPC;
+ }
+ sTotalNumberOfXsiSemaphores++;
Should be "if (sTotalNumberOfXsiSemaphores >= MAX_XSI_SEMAPHORE) {".
+ semaphoreSet = new(std::nothrow)
XsiSemaphoreSet(numberOfSemaphores,flags);
Coding style: Missing space after ",". 80 columns limit.
+ if (semaphoreSet == NULL) {
+ TRACE_ERROR(("xsi_semget: failed to allocate a
new xsi "
+ "semaphore set\n"));
+ sTotalNumberOfXsiSemaphores--;
+ return ENOMEM;
+ }
+ if (isPrivate)
+ semaphoreSet->SetIpcKey((key_t)-1);
+ else {
+ semaphoreSet->SetIpcKey(key);
+ IpcKey->SetSemaphoreSetID(semaphoreSet);
+ }
Entering the semaphore into sSemaphoreHashTable is missing. I suppose you
meant it to happen in XsiSemaphoreSet::SetID().
+ }
+ }
+
+ return semaphoreSet->GetID();
The whole "if (create) {...}" block needs to be pulled out of the "if (key
!= IPC_PRIVATE) {...}". Otherwise we can't create private semaphores (and
actually segfault at this point).
+}
+
+
+int
+_user_xsi_semctl(int semaphoreID, int semaphoreNumber, int command, union
semun* args)
+{
+ XsiSemaphoreSet *semaphoreSet = sSemaphoreHashTable.Lookup(semaphoreID);
+ if (semaphoreSet == NULL) {
+ TRACE_ERROR(("xsi_semctl: semaphore set id %d not valid\n",
semaphoreID));
+ return EINVAL;
+ }
+ if ((semaphoreNumber < 0)
+ || (semaphoreNumber >= semaphoreSet->GetNumberOfSemaphores())) {
+ TRACE_ERROR(("xsi_semctl: semaphore number %d not valid for "
+ "semaphore %d\n", semaphoreNumber, semaphoreID));
+ return EINVAL;
+ }
+
+ if ((args != 0) && !(IS_USER_ADDRESS(args))) {
Coding style: Superfluous parentheses (in both expressions). Encountered
several times throughout the sources.
+ TRACE_ERROR(("xsi_semctl: semun address is not valid\n"));
+ return B_BAD_ADDRESS;
+ }
+
+ XsiSemaphore *semaphore = semaphoreSet->GetSemaphore(semaphoreNumber);
+ switch (command) {
+ case GETVAL:
+ if (!(semaphoreSet->HasReadPermission())) {
+ TRACE_ERROR(("xsi_semctl: calling process has
not permission "
+ "on semaphore %d, key %d\n",
semaphoreSet->GetID(),
+ (int)semaphoreSet->GetIpcKey()));
+ return EACCES;
+ }
+ return semaphore->GetValue();
+
+ case SETVAL:
+ if (!(semaphoreSet->HasPermission())) {
+ TRACE_ERROR(("xsi_semctl: calling process has
not permission "
+ "on semaphore %d, key %d\n",
semaphoreSet->GetID(),
+ (int)semaphoreSet->GetIpcKey()));
+ return EACCES;
+ }
+ int value;
+ user_memcpy(&value, &args->val, sizeof(int));
The return value of user_memcpy() must be checked!
+ if (value > USHRT_MAX) {
+ TRACE_ERROR(("xsi_semctl: value %d out of
range\n", value));
+ return ERANGE;
+ }
+ semaphore->SetValue(value);
I suppose this should potentially unblock threads.
+ return 0;
+
+ case GETPID:
+ if (!(semaphoreSet->HasReadPermission())) {
+ TRACE_ERROR(("xsi_semctl: calling process has not
permission "
+ "on semaphore %d, key %d\n",
semaphoreSet->GetID(),
+ (int)semaphoreSet->GetIpcKey()));
+ return EACCES;
+ }
Indentation.
+ return semaphore->GetPid();
+
+ case GETNCNT:
+ if (!(semaphoreSet->HasReadPermission())) {
+ TRACE_ERROR(("xsi_semctl: calling process has
not permission "
+ "on semaphore %d, key %d\n",
semaphoreSet->GetID(),
+ (int)semaphoreSet->GetIpcKey()));
+ return EACCES;
+ }
+ return semaphore->GetProcessesWaitingToIncrease();
+
+ case GETZCNT:
+ if (!(semaphoreSet->HasReadPermission())) {
+ TRACE_ERROR(("xsi_semctl: calling process has
not permission "
+ "on semaphore %d, key %d\n",
semaphoreSet->GetID(),
+ (int)semaphoreSet->GetIpcKey()));
+ return EACCES;
+ }
+ return semaphore->GetProcessesWaitingToBeZero();
+
+ case GETALL:
+ case SETALL:
+ // TODO
+ return -1;
+
+ case IPC_STAT:
+ case IPC_SET:
+ // TODO
If something is not implemented, it should fail, not do something totally
unexpected (like destroying the semaphore set when a stat was requested).
+ case IPC_RMID:
+ if (!(semaphoreSet->HasPermission())) {
+ TRACE_ERROR(("xsi_semctl: calling
process has not permission "
+ "on semaphore %d, key %d\n",
semaphoreSet->GetID(),
+
(int)semaphoreSet->GetIpcKey()));
+ return EACCES;
+ }
+ sSemaphoreHashTable.Remove(semaphoreSet);
+ delete semaphoreSet;
+
+
Should also remove the IPC key from sIpcHashTable.
Coding style: Superfluous spacing.
+ default:
+ TRACE_ERROR(("xsi_semctl: command %d not valid\n",
command));
+ return EINVAL;
+ }
+}
+
+
+status_t
+_user_xsi_semop(int semaphoreID, struct sembuf *sops, size_t nsops)
+{
+ XsiSemaphoreSet *semaphoreSet = sSemaphoreHashTable.Lookup(semaphoreID);
+ if (semaphoreSet == NULL) {
+ TRACE_ERROR(("xsi_semop: semaphore set id %d not valid\n",
semaphoreID));
+ return EINVAL;
+ }
+
+ if (!IS_USER_ADDRESS(sops)) {
+ TRACE_ERROR(("xsi_semop: sembuf address is not valid\n"));
+ return B_BAD_ADDRESS;
+ }
+
+ struct sembuf operations[nsops];
+ user_memcpy(&operations[0], sops, (sizeof(struct sembuf) * nsops));
Check return value.
+ /*
+ * We won't do partial request. If we must wait on a
+ * semaphore, we undo all the operations already done
+ * and go to sleep.
+ */
+ bool notDone = true;
+ bool goToSleep = false;
+ status_t result = 0;
+ while (notDone) {
+ XsiSemaphore *semaphore = NULL;
+ short numberOfSemaphores =
semaphoreSet->GetNumberOfSemaphores();
+
+ int i = 0;
+ for (; i < nsops; i++) {
+ short semaphoreNumber = operations[i].sem_num;
+ if (semaphoreNumber > numberOfSemaphores) {
Should be >=.
+ TRACE_ERROR(("xsi_semop: %d invalid semaphore
number\n", i));
+ result = EINVAL;
+ break;
+ }
+ semaphore = semaphoreSet->GetSemaphore(semaphoreNumber);
+ unsigned short value = semaphore->GetValue();
+ short operation = operations[i].sem_op;
+ if (operation < 0) {
+ if (semaphore->Add(operation, false)) {
+ goToSleep = true;
+ break;
+ }
+ } else if (operation == 0) {
+ if (value == 0)
+ continue;
+ else if (operations[i].sem_flg & IPC_NOWAIT) {
+ result = EAGAIN;
+ break;
+ } else {
+ goToSleep = true;
+ break;
+ }
+ } else {
+ // Operation must be greater than zero,
+ // just add the value and continue
+ semaphore->Add(operation, false);
+ }
+ }
+
+ // Either we have to wait or an error occured
+ if (goToSleep || result != 0) {
+ // Undo all previosly done operations
+ for (int j = 0; j < i; j++) {
+ short semaphoreNumber = operations[j].sem_num;
+ semaphore =
semaphoreSet->GetSemaphore(semaphoreNumber);
+ short operation = operations[j].sem_op;
+ if (operation != 0)
+ semaphore->Add(operation, true);
+ }
+ if (result != 0)
+ return result;
+
+ bool waitOnZero = true;
+ if (operations[i].sem_op != 0)
+ waitOnZero = false;
+
+ result = semaphore->Wait((int32)operations[i].sem_op,
waitOnZero);
+
+ // We are back to life.
+ // Find out why!
+ semaphoreSet = sSemaphoreHashTable.Lookup(semaphoreID);
+ if (semaphoreSet == NULL || result == EIDRM) {
+ TRACE_ERROR(("xsi_semop: semaphore set id %d
got destroyed\n",
+ semaphoreID));
+ result = EIDRM;
+ notDone = false;
+ }
+ } else
+ // everything worked like a charm
+ notDone = false;
+ // TODO: Handle SEM_UNDO requests
+ }
+ return result;
+}
I'm not sure, if this implementation works as it is supposed to work. It
only waits for one semaphore at a time. Thus at least the semncnt and
semzcnt won't be correct when it actually would wait for more than one
semaphore.
Index: src/system/libroot/posix/sys/xsi_sem.c
===================================================================
--- src/system/libroot/posix/sys/xsi_sem.c (revision 0)
+++ src/system/libroot/posix/sys/xsi_sem.c (revision 0)
@@ -0,0 +1,81 @@
+/*
+ * Copyright 2008, Haiku Inc. All rights reserved.
+ * Distributed under the terms of the MIT License.
+ *
+ * Authors:
+ * Salvatore Benedetto <salvatore.benedetto@xxxxxxxxx>
+ */
+
+#include <sys/sem.h>
+
+#include <errno.h>
+#include <fcntl.h>
+#include <stdarg.h>
+#include <stdlib.h>
+
+#include <OS.h>
+
+#include <posix/realtime_sem_defs.h>
+#include <syscalls.h>
+
+#define RETURN_AND_SET_ERRNO(status) \
+{ \
+ if (status < 0) { \
+ errno = status; \
+ return -1; \
+ } \
+ return status; \
+}
Please use the the macro defined in headers/private/shared/syscall_utils.h.
Particularly note that your version instantiates the argument three times
and evaluates it least twice, which makes the code below incorrect.
[...]
Index: headers/posix/sys/types.h
===================================================================
--- headers/posix/sys/types.h (revision 26404)
+++ headers/posix/sys/types.h (working copy)
@@ -1,5 +1,6 @@
/*
- * Distributed under the terms of the OpenBeOS license
+ * Copyright 2008, Haiku Inc. All Rights Reserved.
+ * Distributed under the terms of the MIT License.
*/
#ifndef _SYS_TYPES_H
#define _SYS_TYPES_H
This is a totally unrelated change.
Index: headers/posix/sys/sem.h
===================================================================
--- headers/posix/sys/sem.h (revision 0)
+++ headers/posix/sys/sem.h (revision 0)
@@ -0,0 +1,63 @@
+/*
+ * Copyright 2008, Haiku Inc. All Rights Reserved.
+ * Distributed under the terms of the MIT License.
+ */
+#ifndef _SYS_SEM_H
+#define _SYS_SEM_H
+
+#include <sys/cdefs.h>
+#include <sys/ipc.h>
+#include <sys/types.h>
+
+
+// Semaphore operation flags
+#define SEM_UNDO 10
Use C-style comments in public POSIX headers.
[...]
Index: headers/posix/sys/resource.h
===================================================================
--- headers/posix/sys/resource.h (revision 26404)
+++ headers/posix/sys/resource.h (working copy)
[...]
Unrelated.
Index: headers/private/kernel/posix/xsi_semaphore.h
===================================================================
--- headers/private/kernel/posix/xsi_semaphore.h (revision 0)
+++ headers/private/kernel/posix/xsi_semaphore.h (revision 0)
@@ -0,0 +1,35 @@
+/*
+ * Copyright 2008, Haiku Inc. All rights reserved.
+ * Distributed under the terms of the MIT License.
+ *
+ * Authors:
+ * Salvatore Benedetto <salvatore.benedetto@xxxxxxxxx>
+ */
+#ifndef KERNEL_XSI_H
+#define KERNEL_XSI_H
+
+#include <sys/sem.h>
+#include <sys/cdefs.h>
+
+#include <OS.h>
+
+struct kernel_args;
+
+union semun {
+ int val;
+ struct semid_ds *buf;
+ unsigned short *array;
+};
+
+__BEGIN_DECLS
+
+extern status_t haiku_xsi_ipc_init(struct kernel_args *args);
+
+/* user calls */
+int _user_xsi_semget(key_t key, int numberOfSemaphores, int flags);
+int _user_xsi_semctl(int semaphoreID, int semaphoreNumber, int command,
union semun* args);
+status_t _user_xsi_semop(int semaphoreID, struct sembuf *sops, size_t
nsops);
80 columns limit.
Index: headers/private/kernel/util/OpenHashTable.h
===================================================================
--- headers/private/kernel/util/OpenHashTable.h (revision 26404)
+++ headers/private/kernel/util/OpenHashTable.h (working copy)
@@ -30,7 +30,7 @@
size_t HashKey(int key) const { return key >> 1; }
size_t Hash(Foo *value) const { return HashKey(value->bar); }
bool Compare(int key, Foo *value) const { return value->bar ==
key; }
- HashTableLink<Foo> *GetLink(Foo *value) const { return value; }
+ HashTableLink<Foo> *GetLink(Foo *value) const { return
&value->otherLink;
}
};
*/
Unrelated, and not even something that needs changing.
Index: headers/private/system/syscalls.h
===================================================================
--- headers/private/system/syscalls.h (revision 26404)
+++ headers/private/system/syscalls.h (working copy)
@@ -27,6 +27,7 @@
struct net_stat;
struct pollfd;
struct rlimit;
+struct sembuf;
struct sigaction;
struct stat;
struct _sem_t;
@@ -38,6 +39,7 @@
struct user_disk_device_job_info;
struct user_disk_system_info;
+union semun;
// This marks the beginning of the syscalls prototypes for gensyscallinfos.
// NOTE:
// * Nothing but those prototypes may live here.
Spacing: the "union semun;" declaration really has nothing to do with the
following comment lines. Besides, I would simply also put it into the list
above.
@@ -89,6 +91,11 @@
extern status_t _kern_realtime_sem_post(sem_id semID);
extern status_t _kern_realtime_sem_wait(sem_id semID, bigtime_t
timeout);
+/* POSIX XSI sem syscalls */
+extern int _kern_xsi_semget(key_t key, int
numberOfSemaphores, int flags);
+extern int _kern_xsi_semctl(int semaphoreID, int
semaphoreNumber, int
command, union semun* args);
+extern status_t _kern_xsi_semop(int semaphoreID, struct sembuf
*sops,
size_t nsops);
+
80 columns limit.
CU, Ingo
Other related posts:
