From owner-freebsd-emulation@FreeBSD.ORG Mon Mar 3 16:41:23 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 9BC3F106566B; Mon, 3 Mar 2008 16:41:23 +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 27D778FC15; Mon, 3 Mar 2008 16:41:23 +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 F03852E26F; Mon, 3 Mar 2008 17:41:09 +0100 (CET) Received: from webmail.leidinger.net (webmail.leidinger.net [192.168.1.102]) by outgoing.leidinger.net (Postfix) with ESMTP id 8FF0697835; Mon, 3 Mar 2008 17:41:02 +0100 (CET) Received: (from www@localhost) by webmail.leidinger.net (8.14.2/8.13.8/Submit) id m23Gf1Ls012306; Mon, 3 Mar 2008 17:41:01 +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 17:41:01 +0100 Message-ID: <20080303174101.e6xfqplzuo404wkw@webmail.leidinger.net> X-Priority: 3 (Normal) Date: Mon, 03 Mar 2008 17:41:01 +0100 From: Alexander Leidinger To: Roman Divacky References: <20080217162938.GA82845@freebsd.org> <20080303115420.6fm3xuto6c8kcssk@webmail.leidinger.net> <20080303150607.GB47887@freebsd.org> In-Reply-To: <20080303150607.GB47887@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=-13.25, required 6, BAYES_00 -15.00, J_CHICKENPOX_33 0.60, MIME_QP_LONG_LINE 1.40, RDNS_DYNAMIC 0.10, SMILEY -0.50, TW_CP 0.08, TW_HN 0.08) 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 16:41:23 -0000 Quoting Roman Divacky (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