Date: Mon, 03 Mar 2008 17:41:01 +0100 From: Alexander Leidinger <Alexander@Leidinger.net> To: Roman Divacky <rdivacky@freebsd.org> Cc: emulation@freebsd.org Subject: Re: epoll patch for review Message-ID: <20080303174101.e6xfqplzuo404wkw@webmail.leidinger.net> In-Reply-To: <20080303150607.GB47887@freebsd.org> References: <20080217162938.GA82845@freebsd.org> <20080303115420.6fm3xuto6c8kcssk@webmail.leidinger.net> <20080303150607.GB47887@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Quoting Roman Divacky <rdivacky@freebsd.org> (from Mon, 3 Mar 2008 =20
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 =20
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 =20
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 =20
code the memcpy is what happening there and I prefer kev_copy (or =20
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. =20
You already thought about it (ADD+DEL), so think a little bit more and =20
finish it (it doesn't look like it is a lot of code to write for =20
this). And as a second thought: the action to do may depend upon the =20
modification requested (when it is in fact a null op, we should maybe =20
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 =20
over the code may not think it's obvious. When a litte comment can =20
explain the high-level thing several lines of code do, adding the =20
little comment is a good thing to do. It helps understanding foreign =20
code.
> thing cannot be solved because epoll() passes in something entirely differ=
ent
> 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 =20
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 =20
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.
--=20
In every non-trivial program there is at least one bug.
http://www.Leidinger.net Alexander @ Leidinger.net: PGP ID =3D B0063FE7
http://www.FreeBSD.org netchild @ FreeBSD.org : PGP ID =3D 72077137
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080303174101.e6xfqplzuo404wkw>