[haiku-gsoc] Re: Xsi semaphorses: patch #1
- From: "Axel Dörfler" <axeld@xxxxxxxxxxxxxxxx>
- To: haiku-gsoc@xxxxxxxxxxxxx
- Date: Sun, 13 Jul 2008 23:48:30 +0200 CEST
Hi Salvatore,
"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).
Great! I knew it was the perfect test app :-))
I don't have much time now, so you only get a few style issues from me,
but I hope I can give you some better review tomorrow evening:
> +#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>
> +
> +#include <util/DoublyLinkedList.h>
> [...]
Include order is:
* single private header matching the file (ie. "MyClass.h" for
"MyClass.cpp")
* then from most public (POSIX/STL, ...) to most private (ie: Haiku
headers, private kernel headers, private headers)
> + queued_thread(struct thread *thread, int32 count)
> [...]
> + struct thread *thread;
> [...]
> + size_t Hash(XsiSemaphoreSet* variable) const
Please choose one "* "/" *" style and stick to it consistently.
> +static OpenHashTable<SemaphoreHashTableDefinition>
> sSemaphoreHashTable;
> +static int sNextAvailableID = 0;
First declarations, then definitions, ie. actual variables come last
and together.
> +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)
While that copies existing code, it is a style guide violation, and
should be fixed everywhere. First of all, the use of macros for
something like this should be removed, and global static variables are
called as the ones above (ie. sVariableName).
> +#define MAX_XSI_SEMAPHORE 512
> +static int sTotalNumberOfXsiSemaphores = 0;
> +
> +void
No tab after #define, const variables are allowed to be used, too, and
there are two blanks between declarations and functions, too.
> + ushort fNumberOfSemaphores;
> // sem_nsems
Prefer uint16 over ushort when it's what you mean; at least
theoretically types can change their meaning with a compiler/
architecture switch (like "long" in 64bit platforms).
Also, less long names certainly wouldn't hurt; names should be as short
and precise as possible, and their context dictates (ie. no reason to
call an index variable threadTableIndex if there is only a single index
variable involved). "fProcessesWaitingToIncrease" is also a candidate
for a better name.
> +class Ipc {
> [...]
> + key_t GetKey() const
> [...]
> + int GetSemaphoreSetID() const
(other places, too)
Get*() is only used for the form:
status_t Get*(type* myVar)
(or if the *() form is already taken by a class name, and someone was
lazy, like in DoublyLinkedList::GetIterator())
You should only use Key() and SemaphoreSetID() instead here.
> + if ((semaphoreNumber < 0) [...]
[...]
> + if ((args != 0) && !(IS_USER_ADDRESS(args))) {
[...]
> + if (!(semaphoreSet->HasPermission())) {
Please don't use more parenthesis than needed.
Operator precedence is much more clear in most cases.
> + Ipc *IpcKey = NULL;
Variable names always start with a lower case character.
> + case SETVAL:
> [...]
> + user_memcpy(&value, &args->val, sizeof(int));
user_memcpy() can fail, and you should test for that and return a
proper error then (there are more places like this).
> + 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;
> + }
> + return semaphore->GetPid();
Wrong indentation, too many parenthesis.
> +key_t
> +ftok(const char *path, int id)
> +{
> [...]
> + return (key_t) (id << 24 | (st.st_dev & 0xff) << 16 | (st.st_ino &
> 0xffff));
> +}
No space between (key_t) and (id...);
> +union semun {
> + int val;
> + struct semid_ds *buf;
> + unsigned short *array;
> +};
> +
> +int
> +semget(key_t key, int num_sems, int sem_flags)
Again order and two spaces.
Also numSems and semFlags instead of those names (other examples follow
here as well).
I would also prefer to use real names like "value" and "buffer" instead
of those above.
> +++ 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.
> */
I'm pretty sure sys/types.h is a lot older than 2008 ("svn log" will
tell you more).
Same for sys/resource.h - also, it's preferred to separate diffs for
patches like these and functional changes like implementing XSI
semaphores.
Maybe union semun should be moved into a shared private header instead
of being declared at several places.
Also, there is an 80 column line limit you don't always honour.
Finally the RETURN_AND_SET_ERRNO() problem that Stephan pointed out.
Regarding the actual contents of your patch, I will get back to you
tomorrow.
Thanks, looks very nice so far!
Bye,
Axel.
Other related posts:
