[Bug 1656] GSoC AIO patch

[bugzilla-daemon at rtems.org]

https://www.rtems.org/bugzilla/show_bug.cgi?id=1656

--- 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
compatibility.

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.
Agreed.

> + rtems_chn is a bad name.. doesn't reflect purpose.
Agreed.

> 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
 * http://www.rtems.com/license/LICENSE.
 *
 * $Id$
 */


* 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.


Final remark: 
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.