Re: epoll patch for review
On Mon, Mar 03, 2008 at 11:54:20AM +0100, Alexander Leidinger wrote:[color=blue]
> Quoting Roman Divacky <firstname.lastname@example.org> (from Sun, 17 Feb 2008
> 17:29:38 +0100):
> >patch that implements epoll() in linuxulator. its fairly trivial
> >so I'd love to get this reviewed/commited by someone.[/color]
> 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
makes sense... I'll try to come up with something
> 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?[/color]
there is nothing to check, the size argument is ignored and "kqueue"
syscall ignores its parameter too...
I'll change the comment and improve the style
> 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
I am not sure if its semantically correct, it seems to work for my testing
program. I'll remove the comment.
I'll add the printf about not handle events
> You really asked to review a patch which says "XXX: error handling"?[/color]
yes.. that should be handled. when I look at it now it seems that this
can happen during event registration. I probably had some idea about
it but forgot :)
> 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").[/color]
I'll take a look at it once more, the problem is that this funstion
is used for two purposes..
> It uses kevent_to_epoll, and as such it should return an error and not
> do the copyout, if there was an error.[/color]
I am not absolutely sure what should happen when there is a problem
during registration (which is the only place the EV_ERROR is triggered).
> You use memcpy, and not copyin. This is confusing, as the name suggest
> you are doing a copyin. Something needs to be changed there.[/color]
the function is named "...copyin" because this is kqueue nomenclature
but my function wants to copy from kernel space to kernel space. so I
guess its ok.
> Again, the XXX: I don't know if you can add+del, but having the MOD
> part return EINVAL every time is not ok.[/color]
why not? I dont think this is widely or at all so EINVAL looks fine
for me (now)
> 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).[/color]
the constants are simple time conversions. its quite obvious. the type
thing cannot be solved because epoll() passes in something entirely different
to kqueue but our copyin() function knows about it so its not a problem
> >it's basically a thin translation layer above kqueue. I tested
> >this using [url]http://www.vlakno.cz/~rdivacky/epoll.c[/url][/color]
> 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?[/color]
the LTP test requires some library which I was not able to get. I dont
remember the details but I was unable to run the tests.
thnx for the review I'll fix things and post updated version in a few days
[email]email@example.com[/email] mailing list
To unsubscribe, send any mail to "firstname.lastname@example.org"