Date: Mon, 3 Mar 2008 16:06:07 +0100 From: Roman Divacky <rdivacky@freebsd.org> To: Alexander Leidinger <Alexander@Leidinger.net> Cc: emulation@freebsd.org Subject: Re: epoll patch for review Message-ID: <20080303150607.GB47887@freebsd.org> In-Reply-To: <20080303115420.6fm3xuto6c8kcssk@webmail.leidinger.net> References: <20080217162938.GA82845@freebsd.org> <20080303115420.6fm3xuto6c8kcssk@webmail.leidinger.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Mar 03, 2008 at 11:54:20AM +0100, Alexander Leidinger wrote: > Quoting Roman Divacky <rdivacky@freebsd.org> (from Sun, 17 Feb 2008 > 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 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 > http://www.freebsd.org/cgi/cvsweb.cgi/src/sys/dev/sound/pcm/mixer.c?rev=1.55;content-type=text%2Fx-cvsweb-markup). makes sense... I'll try to come up with something > > epoll_create: > 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? 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 > 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'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.. > > 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. > 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) > 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 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. > >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. thnx for the review I'll fix things and post updated version in a few days roman
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080303150607.GB47887>