From owner-freebsd-emulation@FreeBSD.ORG Mon Mar 3 10:54:31 2008 Return-Path: Delivered-To: emulation@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 3B732106566C; Mon, 3 Mar 2008 10:54:31 +0000 (UTC) (envelope-from alexander@leidinger.net) Received: from redbull.bpaserver.net (redbullneu.bpaserver.net [213.198.78.217]) by mx1.freebsd.org (Postfix) with ESMTP id CE39E8FC21; Mon, 3 Mar 2008 10:54:30 +0000 (UTC) (envelope-from alexander@leidinger.net) Received: from outgoing.leidinger.net (p54A55014.dip.t-dialin.net [84.165.80.20]) by redbull.bpaserver.net (Postfix) with ESMTP id 349FE2E1FE; Mon, 3 Mar 2008 11:54:27 +0100 (CET) Received: from webmail.leidinger.net (webmail.leidinger.net [192.168.1.102]) by outgoing.leidinger.net (Postfix) with ESMTP id 9C3C57DCDC; Mon, 3 Mar 2008 11:54:21 +0100 (CET) Received: (from www@localhost) by webmail.leidinger.net (8.14.2/8.13.8/Submit) id m23AsLAM054658; Mon, 3 Mar 2008 11:54:21 +0100 (CET) (envelope-from Alexander@Leidinger.net) Received: from pslux.cec.eu.int (pslux.cec.eu.int [158.169.9.14]) by webmail.leidinger.net (Horde MIME library) with HTTP; Mon, 03 Mar 2008 11:54:20 +0100 Message-ID: <20080303115420.6fm3xuto6c8kcssk@webmail.leidinger.net> X-Priority: 3 (Normal) Date: Mon, 03 Mar 2008 11:54:20 +0100 From: Alexander Leidinger To: Roman Divacky References: <20080217162938.GA82845@freebsd.org> In-Reply-To: <20080217162938.GA82845@freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8; DelSp="Yes"; format="flowed" Content-Disposition: inline Content-Transfer-Encoding: quoted-printable User-Agent: Internet Messaging Program (IMP) H3 (4.1.5) / FreeBSD-8.0 X-BPAnet-MailScanner-Information: Please contact the ISP for more information X-BPAnet-MailScanner: Found to be clean X-BPAnet-MailScanner-SpamCheck: not spam, SpamAssassin (not cached, score=-12.904, required 6, BAYES_00 -15.00, J_CHICKENPOX_33 0.60, MIME_QP_LONG_LINE 1.40, RDNS_DYNAMIC 0.10) X-BPAnet-MailScanner-From: alexander@leidinger.net X-Spam-Status: No Cc: emulation@freebsd.org Subject: Re: epoll patch for review X-BeenThere: freebsd-emulation@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Development of Emulators of other operating systems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 03 Mar 2008 10:54:31 -0000 Quoting Roman Divacky (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