This is a discussion on Re: epoll patch for review - FreeBSD ; 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 ...
Quoting Roman Divacky
(from Sun, 17 Feb 2008 =20
> 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
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
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?
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
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").
It uses kevent_to_epoll, and as such it should return an error and not =20
do the copyout, if there was an error.
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.
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.
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?
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
firstname.lastname@example.org mailing list
To unsubscribe, send any mail to "email@example.com"