[haiku-gsoc] Re: Xsi semaphorses: patch #1
- From: "Axel Dörfler" <axeld@xxxxxxxxxxxxxxxx>
- To: haiku-gsoc@xxxxxxxxxxxxx
- Date: Fri, 18 Jul 2008 15:24:34 +0200 (MEST)
Hi Salvatore,
> Anyway, I've attached a patch that (hopefully) fix all previous issue, and
> also (hopefully) add the rest of missing features without any other mistakes.
> It's still not 100% complete though.
Thanks for the update!
> I'll look into a test application next.
That would be great. I wouldn't be surprised if there are tests ready to be
used on the net (maybe the POSIX test suite already contains some).
Anyway, first the style related comments:
> +++ headers/private/kernel/posix/xsi_semaphore.h (revision 0)
> @@ -0,0 +1,32 @@
> +#ifndef KERNEL_XSI_H
> +#define KERNEL_XSI_H
Inconsistent usage of tab - should be a space only.
> +union semun {
> + int val;
> + struct semid_ds *buf;
> + unsigned short *array;
> +};
I thought you wanted to use better names? It' s a private structure after all.
> +++ headers/posix/sys/sem.h (revision 0)
> +/* Semaphore operation flags */
> [...]
> +/* Command definition for semctl */
> +#define GETPID 3 /* Get process ID of last element
> manipulating */
> [...]
Tab instead of space.
> +struct seminfo {
> + int semmni; /* Number of semaphore identifies */
Tab between "int" and the names? At least it looks a bit inconsistent with the
structures above.
> + XsiSemaphore()
> + : fLastPidOperation(0),
I usually only put inherited class constructors in the line after the ":", but
the style guide doesn't explicitly ask for that.
> + XsiSemaphoreSet(int nsems, int flags)
> + : fLastSemctlTime((time_t)real_time_clock()),
But there is definitely not a tab afterwards :-)
Also, nsems is not a good name.
+_user_xsi_semget(key_t key, int numberOfSemaphores, int flags)
[...]
+ if (key != IPC_PRIVATE) {
+ isPrivate = false;
+ // Check if key already have a semaphore
+ // associated with it
"Check if key already _has_ ..." - 80 column limit; the comment easily fits on
one line.
> + if (semaphoreSet == NULL) {
> + TRACE_ERROR(("xsi_semget: failed to allocate a new xsi "
> + "semaphore set\n"));
Only a single tab indent instead of 3 for the second line of TRACE_ERROR.
> + static vint32 sTotalNumberOfXsiSemaphores = 0;
Instead of that name, something like sXsiSemaphoreCount would be a bit more
handy. Since it's a global variable, the "Total" part at least isn't really
necessary.
Now to the real comments:
> if (fSemaphores == NULL) {
> TRACE_ERROR(("XsiSemaphoreSet::XsiSemaphore(): failed
> to allocate "
> "XsiSemaphore object\n"));
> return;
> }
This should better be moved out of the constructor, and into an Init() method -
in _user_semget() you don't even check if the semaphore array could be
allocated; subsequent uses will just crash.
> if (create) {
> // Create a new sempahore for this key
semaphore set.
> MutexLocker _(sXsiSemaphoreSetLock);
> semaphoreSet = new(std::nothrow)
> XsiSemaphoreSet(numberOfSemaphores,
> flags);
Why do you lock the mutex here? What does it protect? AFAICT there is nothing
to protect before you actually add the set into the hash. If it's because of
SetID(), then you should move this out of the constructor as well.
> if (isPrivate)
> semaphoreSet->SetIpcKey((key_t)-1);
> else {
> semaphoreSet->SetIpcKey(key);
> ipcKey->SetSemaphoreSetID(semaphoreSet);
> sSemaphoreHashTable.Insert(semaphoreSet);
> }
So private semaphores only get leaked and cannot be accessed anymore
afterwards? :-)
> case IPC_RMID: {
This doesn't give an error for private sem sets - is that intended?
> status_t
> _user_xsi_semop(int semaphoreID, struct sembuf *sops, size_t nsops)
> {
> XsiSemaphoreSet *semaphoreSet = sSemaphoreHashTable.Lookup(semaphoreID);
Locking missing.
sops, and nsops are bad names - why not just 'ops" and numOps?
> /*
> * We won't do partial request. If we must wait on a
> * semaphore, we undo all the operations already done
> * and go to sleep.
> */
What is a partial request and why aren't we implementing it? That's what the
comment should explain instead; there is no mention of a partial request in the
spec.
> struct sembuf operations[nsops];
> if (user_memcpy(&operations[0], sops,
You cannot allocate something like this on the stack without bounds check; this
easily crashes the kernel with a big enough "nsops". Allocate it on the heap
instead, and do a bounds check for "nsops", too.
Maybe it would be better to only copy what you need to a single sembuf.
> // Either we have to wait or an error occured
> if (goToSleep || result != 0) {
> // Undo all previosly done operations
"previously" - why not check the "nothing to be done" case first, and then
break out of the loop instead of using the otherwise superluous "notDone"?
> result = semaphore->Wait((int32)operations[i].sem_op,
> waitOnZero);
>
> // We are back to life.
> // Find out why!
> semaphoreSet = sSemaphoreHashTable.Lookup(semaphoreID);
Locking missing again.
In general, I can't see (or maybe just don't understand) how this loop is
supposed to work. Do you really need to execute the operations over and over
again? Why wouldn't it lock the second time? After all, you are reverting all
the ops before waiting (and then re-executing).
Anyway, nice to see you progressing! In general, I would prefer if you would
use the in-class function declarations less, since they make the code a bit
clumsy if they are long, and separated (like for the SetID() case).
I think writing test apps makes a lot sense right now; you can probably find
most of the remaining problems better than by having the code reviewed.
One more comment though: what about the lifetime of a semaphore set? I would
guess the private ones should be bound to the team, and should be deleted with
it.
Bye,
Axel.
Other related posts:
