Kernelspace Comments

[Neil T. Dantam]

Hi Kim,

I'm really thrilled that you put this together! Here is my suggestion
for merging and release:

1. Agree on suitable license
2. Make whitespace consistent (sounds trivial, but necessary to make
diffs readable)
3. Test, refactor duplicated code, and review (I'll help and try to
recruit a couple others as well)
4. Release new version

Now the comments in detail:

Copyright/Licensing
===================

I see that you added a GPLv2+ license header to src/module/ach.h. For
the userspace library, I would much prefer this to remain permissively
licensed, i.e., BSD-style, since that gives the fewest barriers for
users. A GPL libach would preclude its use in other commercial
software. Because src/ach.c includes src/module/ach.h, the GPLv2+
license for src/module/ach.h would result in the entire library being
GPLv2+ -- modulo the argument raised by some that headers define only
interfaces and are not copyrightable (I'd rather not depend on that
argument).

Since the Linux kernel is GPLv2-only, BSD vs. GPL for kernelspace-only
code seems to make little difference there, though a single license for
Ach seems simpler.

Would you be able to release your code under the same 2-clause BSD
license as the rest of Ach?

I'll have a chat with my school's copyright office next week to make
sure we do this correctly.

Whitespace
==========

Let's be consistent. I've used 4-space indents (and no tabs)
throughout ach up to this point. Standard for the Linux kernel is
hard tabs at 8-char width, so that would be fine for the kernel module.

General Structure
=================

It seems that we have a lot of code duplication between ach.c and
module/ach.c. Totally fine for now to get things working. It would be
nice if we could refactor to eliminate duplicate code. Might mean
having one file with the shared functions that gets compiled into both
the userspace library and kernelspace module.

ACH_O_WAIT and absolute/relative timeouts
=========================================

I agree with Kim on keeping features as consistent as possible between
two modes. My suggestion is to get to clean, working code and then --
if necessary -- try to optimize.

Specific Code Comments
======================

ach.h
-----

> #define ACH_SHM_CHAN_NAME_PREFIX_PATH "/dev/shm/"

On my debian stable system, the tmpfs for POSIX shared memory is
actually mounted to /run/shm, though there is a symlink from /dev/shm
-> /run/shm. I don't think any part of the POSIX shared memory
standard specifies that the shared memory is accessible via the
filesystem, so Linux may well change this again in the future.
Probably best not to depend on this (this is also a potential lurking
bug in achtool.c).

> #define ACH_CHAR_CHAN_NAME_PREFIX_NAME "ach-"

Can we put these files in a subdirectory to keep /dev less cluttered?

ach.c
-----

> #define IS_KERNEL_DEVICE(chan) (NULL == chan->shm)

This would also be true for a zero-initialized ach_channel_t. Would
it be better -- i.e., giving an additional error check -- to have an
explicit flag?

> static enum ach_status
> channel_exists_as_shm_device(const char* name) {
> ...
> }

Let's do this using fd_for_channel_name() and then checking if fd > 0
or ENOENT==errno. That seems more standards-friendly since we don't
depend on the POSIX shared memory existing at a particular place in
the filesystem.

> static enum ach_status
> channel_exists_as_kernel_device(const char* name) {
> ...
> }

Ok, using stat here makes sense. We could be paranoid and do an
S_ISCHAR(buf.st_mode).

ach_xput()
----------

I meant this as an additional external interface to allow direct
serialization to the channel buffer. Probably no way to do this with
kernel-space channels. I think we can leave this feature intact and
permit it only for userspace channels.

Cheers,
-ntd

[Kim Bøndergaard]

Hi Neil

A short update. I've made an 'upstream2' branch with same functional content like branch 'upstream'. Only difference is that upstream2 has been beautified meaning that existing ach-files are now again using space-indent of 4 all over and files related to the kernel module follows the kernel coding style.

Furthermore I've updated the licensing also to be match the other ach-files.

We've decided not to make any more changes until further testing has proven the source doesn't have to be refactored a lot.

/Kim