epoll patch for review

Mon Mar 3 16:41:23 UTC 2008

Quoting Roman Divacky <rdivacky at freebsd.org> (from Mon, 3 Mar 2008  
16:06:07 +0100):

> On Mon, Mar 03, 2008 at 11:54:20AM +0100, Alexander Leidinger wrote:

>> 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.
>
> I am not sure if its semantically correct, it seems to work for my testing
> program. I'll remove the comment.

I'm not sure your testing program covers all cases. So just removing  
the comment to make me happy is not the right thing to do. :)

> I'll add the printf about not handle events
>
>
>> kevent_to_epoll:
>> You really asked to review a patch which says "XXX: error handling"?
>
> 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").
>
> I'll take a look at it once more, the problem is that this funstion
> is used for two purposes..

Try to come up with something sensible. It can produce errors and  
those should be handled.

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

It doesn't matter much IMHO, that kqueue calls this copyin. In your  
code the memcpy is what happening there and I prefer kev_copy (or  
kev_memcpy or similar) instead of kev_copyin.

>> 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.
>
> why not? I dont think this is widely or at all so EINVAL looks fine
> for me (now)

What I meant was: linux supports the MOD so we should support it too.  
You already thought about it (ADD+DEL), so think a little bit more and  
finish it (it doesn't look like it is a lot of code to write for  
this). And as a second thought: the action to do may depend upon the  
modification requested (when it is in fact a null op, we should maybe  
do just nothing)...

>> 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).
>
> the constants are simple time conversions. its quite obvious. the type

Add a comment then. It may be obvious for some, but students browsing  
over the code may not think it's obvious. When a litte comment can  
explain the high-level thing several lines of code do, adding the  
little comment is a good thing to do. It helps understanding foreign  
code.

> 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
> in reality.

Write a comment about it. Something like "epoll passes in XXX, but we  
pretend it is YYY, it is handled at/in/whatever ZZZ".

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

Oh... with fc4 or f7? What's the lib? Maybe we (bsam and/or me) can a  
find it and add it as a port.

> thnx for the review I'll fix things and post updated version in a few days

Many thanks for working at this!

Bye,
Alexander.

-- 
In every non-trivial program there is at least one bug.

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