Date: Mon, 03 Mar 2008 11:54:20 +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: <20080303115420.6fm3xuto6c8kcssk@webmail.leidinger.net> In-Reply-To: <20080217162938.GA82845@freebsd.org>
index | next in thread | previous in thread | raw e-mail
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). 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? 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. kevent_to_epoll: You really asked to review a patch which says "XXX: error handling"? 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"). 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. 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. 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. 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). > 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? Bye, Alexander. -- The lunatic, the lover, and the poet, Are of imagination all compact... -- William Shakespeare, "A Midsummer Night's Dream" http://www.Leidinger.net Alexander @ Leidinger.net: PGP ID = B0063FE7 http://www.FreeBSD.org netchild @ FreeBSD.org : PGP ID = 72077137home | help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080303115420.6fm3xuto6c8kcssk>