[haiku-bugs] Re: [Haiku] #2657: Implement XSI shared memory <sys/shm.h>
- From: "bonefish" <trac@xxxxxxxxxxxx>
- Date: Sun, 02 Jan 2011 02:29:13 -0000
#2657: Implement XSI shared memory <sys/shm.h>
---------------------------+------------------------------
Reporter: kaliber | Owner: emitrax
Type: enhancement | Status: new
Priority: normal | Milestone: R1
Component: System/POSIX | Version: R1/Development
Resolution: | Keywords:
Blocked By: | Has a Patch: 1
Platform: All | Blocking:
---------------------------+------------------------------
Comment (by bonefish):
Thanks for the patch. I haven't looked too closely yet, but it seems a
good start. There's a whole bunch of coding style issues, though. Just
pointing out a few:
- Two blank lines between groups of unrelated stuff -- e.g. before and
after `#include`, `#define`, variable declaration, etc. blocks and between
functions. Never within functions or class definitions, though (max. one
blank line there). No blank line between copyright header and header
guard.
- No blank line at the beginning or end of blocks
(`SharedMemoryEntry::HasPermission()`).
- Don't indent/align with anything but tabs. Only exception is in class
definitions when e.g. the type needs more space than there is in its
column. Then a single space can be used to separate it from the next
column.
- Don't indent names in local variable definitions.
- No spaces after C cast operator (wrong: `(int) fArea`).
- No space between function name and opening parenthesis (also
`new(std::nothrow)`), but single space between `if`/`switch`/... and
opening parenthesis.
- Always name parameters in function/method prototypes.
- Don't use uncommon abbreviations in identifiers. E.g. `addr` should be
`address`, `fDs` should be `fData` or `fDefinition` or whatever that might
stand for.
- Use a consistent pointer style, preferably `Foo* foo`.
- No unnecessary parentheses (wrong: `fDs.shm_perm.mode = (flags &
0x01ff);`).
- In function definitions define variables where they are first used, not
at the beginning of the function. Whenever possible combine definition and
initialization (`status_t status = ...`).
- `if (fArea < B_OK)`: Compare with `< 0` instead, when values `>= 0` are
valid. Use the stricter comparison `!= B_OK`, when the value is supposed
to be an error code (i.e. `B_OK` or another error code, normally of type
`status_t`).
- Explicitly force bit check expressions to boolean (`(flags & IPC_CREAT)
!= 0`).
- When wrapping an expression, the operator always goes to beginning of
the next line.
- Broken indentation in `SharedMemoryEntry::HasReadPermission()` and
`_user_xsi_shmget()`.
I haven't really looked at the implementation, but I noticed:
- `_user_xsi_shmat()`: `raddr` needs to be copied out via
`user_memcpy()`. Besides, it needs a better name. Also, it is recommended
to prefix output parameters with `_`, e.g. `_mappedAddress` in this case.
- Using fully locked kernel areas is not a good idea. The locking isn't
necessary at all. But the bigger issue is that kernel address space is a
rather limited resource. I'd only create a `VMCache` and map it into the
user address spaces as needed, but not into the kernel address space at
all.
--
Ticket URL: <http://dev.haiku-os.org/ticket/2657#comment:16>
Haiku <http://dev.haiku-os.org>
Haiku - the operating system.
Other related posts:
