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>