Quoting Roman Divacky (from Sun, 17 Feb 2008 =20
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 =20
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 =20
adding comments before each functions describing the behavior it =20
should have (not a description of what the code does, I'm talking =20
about things you would like to read in an API documentation, e.g. like =20
in the function mixer_get_recroute in the file =20
http://www.freebsd.org/cgi/cvsweb.cg...er.c?rev=3D1.=
55;content-type=3Dtext%2Fx-cvsweb-markup).

epoll_create:
You write that linux ignores the size as well. I think it would be =20
better to come up with some size checks. If it is technically not =20
possible to create better size checks, then convert the XXX into a =20
regular comment. Are there some missing parentheses around the return =20
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 =20
multiple possibilities, please write a comment regarding the =20
possibilities, and why you have chosen the way it is coded. There's no =20
code which detects new stuff and does a kprintf. It would be good if =20
it prints out a warning that there's something which is not handled by =20
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 =20
which can produce errors have a return type which allows to tell the =20
caller about errors? What about a default case with a kprintf telling =20
the user that there's something new which is not handled? This would =20
make problems reports much better in case there are some changes in =20
the future ("insurance for the future").

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

kev_copyin:
You use memcpy, and not copyin. This is confusing, as the name suggest =20
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 =20
part return EINVAL every time is not ok.

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

Bye,
Alexander.

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

http://www.Leidinger.net Alexander @ Leidinger.net: PGP ID =3D B0063FE7
http://www.FreeBSD.org netchild @ FreeBSD.org : PGP ID =3D 72077137
_______________________________________________
freebsd-emulation@freebsd.org mailing list
http://lists.freebsd.org/mailman/lis...ebsd-emulation
To unsubscribe, send any mail to "freebsd-emulation-unsubscribe@freebsd.org"