From owner-freebsd-emulation@FreeBSD.ORG Thu Mar 6 06:47:03 2008 Return-Path: <owner-freebsd-emulation@FreeBSD.ORG> 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 DD91F1065671; Thu, 6 Mar 2008 06:47:03 +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 8A15C8FC12; Thu, 6 Mar 2008 06:47:03 +0000 (UTC) (envelope-from alexander@leidinger.net) Received: from outgoing.leidinger.net (p54A55E46.dip.t-dialin.net [84.165.94.70]) by redbull.bpaserver.net (Postfix) with ESMTP id A6EFB2E279; Thu, 6 Mar 2008 07:46:57 +0100 (CET) Received: from webmail.leidinger.net (webmail.leidinger.net [192.168.1.102]) by outgoing.leidinger.net (Postfix) with ESMTP id C1AF296B33; Thu, 6 Mar 2008 07:46:52 +0100 (CET) Received: (from www@localhost) by webmail.leidinger.net (8.14.2/8.13.8/Submit) id m266kpjJ045827; Thu, 6 Mar 2008 07:46:51 +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; Thu, 06 Mar 2008 07:46:50 +0100 Message-ID: <20080306074650.dmiymxgogok4wok4@webmail.leidinger.net> X-Priority: 3 (Normal) Date: Thu, 06 Mar 2008 07:46:50 +0100 From: Alexander Leidinger <Alexander@Leidinger.net> To: Roman Divacky <rdivacky@freebsd.org> References: <20080304183529.kpaf3f76gcg4g0cs@webmail.leidinger.net> <20080305181741.GA15620@freebsd.org> In-Reply-To: <20080305181741.GA15620@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_35 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 review continued X-BeenThere: freebsd-emulation@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Development of Emulators of other operating systems <freebsd-emulation.freebsd.org> List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/freebsd-emulation>, <mailto:freebsd-emulation-request@freebsd.org?subject=unsubscribe> List-Archive: <http://lists.freebsd.org/pipermail/freebsd-emulation> List-Post: <mailto:freebsd-emulation@freebsd.org> List-Help: <mailto:freebsd-emulation-request@freebsd.org?subject=help> List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/freebsd-emulation>, <mailto:freebsd-emulation-request@freebsd.org?subject=subscribe> X-List-Received-Date: Thu, 06 Mar 2008 06:47:04 -0000 Quoting Roman Divacky <rdivacky@freebsd.org> (from Wed, 5 Mar 2008 =20 19:17:41 +0100): > On Tue, Mar 04, 2008 at 06:35:29PM +0100, Alexander Leidinger wrote: >> >@@ -57,7 +57,9 @@ >> > >> >=09if (args->size <=3D 0) >> >=09=09return (EINVAL); >> >-=09/* args->size is unused. Linux ignores it as well. */ >> >+=09/* +=09 * args->size is unused. Linux just tests it +=09 * and then >> >forgets it as well. */ >> >> My proposal ("Submitted by: netchild" in the log): >> /* >> * The size is documented to be only a hint. We don't need >> * it with kevent. The epoll_create man-page tells that the >> * size has to be not-negative, but the linux kernel test >> * for a value of at least 1, so for error compatibility we >> * do the same. >> */ >> >> The above comment should be before the if, not after. > > given that man pages and the source of the kernel are totally independent > I would not trust the man pages... and certainly would not cite > that as a reliable source This comment is supposed to show that we are aware of the man page and =20 about what the kernel does. Whoever reads this comment will get the =20 impression, that we did our homework and also gets told why we do the =20 check this way. When anything in linux (either the kernel or the man =20 page) changes, then anyone reading this comment will see, that we are =20 not up-to-date and know why the code is like it is and hopefully =20 notifies us. I agree that it is of no use for us at this moment, but =20 it improve the code quality a lot, as it helps maintaining this code =20 in the future. Just imagine how fast you would have understood some other parts of =20 the linuxulator, if there would have been similar comments. I'm sure =20 you would have fixed some bugs with more confidence, don't you? >> >static int >> >linux_kev_copyin(void *arg, struct kevent *kevp, int count) >> >@@ -193,7 +197,8 @@ >> >=09=09break; >> >=09case LINUX_EPOLL_CTL_MOD: >> >=09=09=09/* TODO: DELETE && ADD maybe? */ >> >-=09=09=09return (EINVAL); >> >+=09=09=09printf("linux_epoll_ctl: CTL_MOD not yet >> >>implemented.\n"); >> >+=09=09=09return (ENOSYS); >> >> ENOSYS is "syscall not implemented". But we implement the syscall. >> When glibc tests this syscall and gets back a ENOSYS, it may switch to >> a different way of monitoring files. I think the EINVAL was better, as >> it will result in an application error and notify us that we need to >> take care about this part of the syscall. > > #define ENOSYS 78 /* Function not implemented */ > > its used for this purposes all over the kernel so I think its ok I've seen only complete syscalls behaving like this. Can you please =20 point out subfeatures in syscalls not written by you which do this? >> >=09=09break; >> >=09case LINUX_EPOLL_CTL_DEL: >> >=09=09=09kev.flags =3D EV_DELETE | EV_DISABLE; >> >@@ -241,6 +246,9 @@ >> > >> >=09error =3D kern_kevent(td, args->epfd, 0, args->maxevents, &k_ops, &ts= ); >> > >> >-=09/* translation? */ >> >+=09/* +=09 * kern_keven might return ENOMEM which is not expected from >> >epoll_wait. >> >+=09 * Maybe we should translate that but I don't think it matters at >> >all. >> >+=09 */ >> >=09return (error); >> >} >> >> It is not even ENOMEM. After a quick look I think it should be like: >> - ESRCH, ENOENT should map to EINVAL >> - EACCESS should map to EFAULT or EINVAL (don't really know) >> - ENOMEM should map to... ? >> >> You also need to deliver EINVAL, if maxevents is <=3D 0 (according to >> the man page of epoll_wait). > > I fixed the maxevents bug but... > > the ESRCH/ENOENT/EACCESS apply only to epoll_wait (as that does the > registering). Yes. The comment which was added by you in the source talks about =20 epoll_wait, so was I. And I thought I checked that this part was =20 really part of epoll_wait. To make it specific: I haven't seen an =20 errno translation in epoll_wait, but epoll_wait needs to do this. > I'd let it as it is and fix it if problems from real usage arise. I will not try to get time to commit this without an errno translation. Bye, Alexander. --=20 We may not return the affection of those who like us, but we always respect their good judgment. http://www.Leidinger.net Alexander @ Leidinger.net: PGP ID =3D B0063FE7 http://www.FreeBSD.org netchild @ FreeBSD.org : PGP ID =3D 72077137