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> References: <20080217162938.GA82845@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Quoting Roman Divacky <rdivacky@freebsd.org> (from Sun, 17 Feb 2008 =20 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 =20 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 =20 adding comments before each functions describing the behavior it =20 should have (not a description of what the code does, I'm talking =20 about things you would like to read in an API documentation, e.g. like =20 in the function mixer_get_recroute in the file =20 http://www.freebsd.org/cgi/cvsweb.cgi/src/sys/dev/sound/pcm/mixer.c?rev=3D1.= 55;content-type=3Dtext%2Fx-cvsweb-markup). epoll_create: You write that linux ignores the size as well. I think it would be =20 better to come up with some size checks. If it is technically not =20 possible to create better size checks, then convert the XXX into a =20 regular comment. Are there some missing parentheses around the return =20 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 =20 multiple possibilities, please write a comment regarding the =20 possibilities, and why you have chosen the way it is coded. There's no =20 code which detects new stuff and does a kprintf. It would be good if =20 it prints out a warning that there's something which is not handled by =20 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 =20 which can produce errors have a return type which allows to tell the =20 caller about errors? What about a default case with a kprintf telling =20 the user that there's something new which is not handled? This would =20 make problems reports much better in case there are some changes in =20 the future ("insurance for the future"). kev_copyout: It uses kevent_to_epoll, and as such it should return an error and not =20 do the copyout, if there was an error. kev_copyin: You use memcpy, and not copyin. This is confusing, as the name suggest =20 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 =20 part return EINVAL every time is not ok. epoll_wait: Hardcoded constants for the time and no docs of why you use those =20 numbers. The comment about the wrong type-cast is also not =20 encouraging. When I read it the first questions I have are: Why the =20 wrong typecast? What's the right typecast and why can't you use it? =20 Again, a XXX comment. This needs to be resolved (I assume that a =20 translation would be the way to go, with a kprintf for things we don't =20 know how to translate, so that in case of changes in linux, we/users =20 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 =20 tests contain epoll tests? Bye, Alexander. --=20 The lunatic, the lover, and the poet, Are of imagination all compact... =09=09-- William Shakespeare, "A Midsummer Night's Dream" 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?20080303115420.6fm3xuto6c8kcssk>