epoll patch for review

[Alexander Leidinger]

Quoting Roman Divacky <rdivacky at freebsd.org> (from Sun, 17 Feb 2008  
17:29:38 +0100):

> hi
>
> www.vlakno.cz/~rdivacky/linux_epoll.patch
>
> patch that implements epoll() in linuxulator. its fairly trivial
> so I'd love to get this reviewed/commited by someone.

I don't comment about the correctness of the use of kevent or the  
behavior of epoll, I only have time to do a high-level review ATM.

Short review: fix the XXX, some more docs

Long review:

In general:
I would prefer to lift the quality to a higher level. What about  
adding comments before each functions describing the behavior it  
should have (not a description of what the code does, I'm talking  
about things you would like to read in an API documentation, e.g. like  
in the function mixer_get_recroute in the file  
http://www.freebsd.org/cgi/cvsweb.cgi/src/sys/dev/sound/pcm/mixer.c?rev=1.55;content-type=text%2Fx-cvsweb-markup).

epoll_create:
You write that linux ignores the size as well. I think it would be  
better to come up with some size checks. If it is technically not  
possible to create better size checks, then convert the XXX into a  
regular comment. Are there some missing parentheses around the return  
value (I can't check style(9) ATM)? No std debug messages for this?

epoll_to_kevent:
Please check the part which you are not sure about. If you have  
multiple possibilities, please write a comment regarding the  
possibilities, and why you have chosen the way it is coded. There's no  
code which detects new stuff and does a kprintf. It would be good if  
it prints out a warning that there's something which is not handled by  
it.

kevent_to_epoll:
You really asked to review a patch which says "XXX: error handling"?
I think the break statements need some style(9). Shouldn't a function  
which can produce errors have a return type which allows to tell the  
caller about errors? What about a default case with a kprintf telling  
the user that there's something new which is not handled? This would  
make problems reports much better in case there are some changes in  
the future ("insurance for the future").

kev_copyout:
It uses kevent_to_epoll, and as such it should return an error and not  
do the copyout, if there was an error.

kev_copyin:
You use memcpy, and not copyin. This is confusing, as the name suggest  
you are doing a copyin. Something needs to be changed there.

epoll_ctl:
Again, the XXX: I don't know if you can add+del, but having the MOD  
part return EINVAL every time is not ok.

epoll_wait:
Hardcoded constants for the time and no docs of why you use those  
numbers. The comment about the wrong type-cast is also not  
encouraging. When I read it the first questions I have are: Why the  
wrong typecast? What's the right typecast and why can't you use it?  
Again, a XXX comment. This needs to be resolved (I assume that a  
translation would be the way to go, with a kprintf for things we don't  
know how to translate, so that in case of changes in linux, we/users  
can see it).

> it's basically a thin translation layer above kqueue. I tested
> this using http://www.vlakno.cz/~rdivacky/epoll.c

i is not initialized. Compilers may opt to initialize them to 0.

You don't test all possibilities. Have you checked if more recent LTP  
tests contain epoll tests?

Bye,
Alexander.

-- 
The lunatic, the lover, and the poet,
Are of imagination all compact...
		-- William Shakespeare, "A Midsummer Night's Dream"

http://www.Leidinger.net    Alexander @ Leidinger.net: PGP ID = B0063FE7
http://www.FreeBSD.org       netchild @ FreeBSD.org  : PGP ID = 72077137