[Bug 1656] GSoC AIO patch
bugzilla-daemon at rtems.org
bugzilla-daemon at rtems.org
Mon Aug 9 05:12:24 UTC 2010
--- Comment #3 from Ralf Corsepius <ralf.corsepius at rtems.org> 2010-08-09 00:12:23 CDT ---
(In reply to comment #2)
> Comments in no order..
> + Naming violations.. get_elem is a good example. Do not abbreviate and
> it is not static (don't know whether it can be or not)
It can be made static.
> and doesn't include
> aio on the front. All methods need an indication of their subsystem.
Not quite. All symbols need to be name-space safe.
> + No test code
Very valid. Having this is a must.
> + No confdefs.h changes. There may be resources needed when you want
> aio at all (e.g. CONFIGURE_AIO_ENABLE). There may have to be some per
> file you want to do AIO on (e.g. CONFIGURE_MAXIMUM_AIO_FILES).
For now, I deliberately abstain from commenting on this.
To me the key questions would be: "The price of aio" and initialization-API
In other words:
* Is the impact having aio small enough to be considered "negligible" when
taking into account the impact RTEMS POSIX already has or isn't?
If it is, I doen't see a reason to add something to confdefs.h's.
* Is aio initialization transparent or not to RTEMS applications.
> + aio_testinit is either in the wrong place or horribly misnamed.
> + rtems_chn is a bad name.. doesn't reflect purpose.
> Internal routines
> and structures for AIO probably could be named _AIO_ or AIO_ depending
> on whether they are methods or structures/constants.
I disagree. All rtems internal function should use an _rtems_* prefix.
> + there are some alignment issues with parameters and structure elements.
Which? So far I haven't noticed any.
Conversely, the code seems pretty clean to me.
> + Can't compile with warnings.
Which? It compiles without any problems for me.
> I caught all this on a visual review of the diff. Did not try to compile.
Triggers warnings without compilation? Joel, ...?
Further remarks from me:
* Missing: All files need copyright/License disclaimers to be added.
Alin, my recommendation, add something similar to this to each of the source
files you are the author of
* Copyright 2010, Alin Rus <more ...>
* The license and distribution terms for this file may be
* found in the file LICENSE in this distribution or at
* posix/src/aio_notify.c was missing from the patch.
I grabbed it from Alin's svn.
* aio_misc.h is being installed to $(includedir).
As this file is not covered by POSIX, it should be installed elsewhere, e.g.
rtems/posix or similar.
I have a merger of this patch into my local source tree, which I could commit
at any time. Preventing me from doing so is the copyright/license issue.
Configure bugmail: https://www.rtems.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching all bug changes.
More information about the bugs