Date: Thu, 06 Mar 2008 07:46:50 +0100 From: Alexander Leidinger <Alexander@Leidinger.net> To: Roman Divacky <rdivacky@freebsd.org> Cc: emulation@freebsd.org Subject: Re: epoll review continued Message-ID: <20080306074650.dmiymxgogok4wok4@webmail.leidinger.net> In-Reply-To: <20080305181741.GA15620@freebsd.org> References: <20080304183529.kpaf3f76gcg4g0cs@webmail.leidinger.net> <20080305181741.GA15620@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080306074650.dmiymxgogok4wok4>