Date: Wed, 5 Mar 2008 19:17:41 +0100 From: Roman Divacky <rdivacky@freebsd.org> To: Alexander Leidinger <Alexander@Leidinger.net> Cc: emulation@freebsd.org Subject: Re: epoll review continued Message-ID: <20080305181741.GA15620@freebsd.org> In-Reply-To: <20080304183529.kpaf3f76gcg4g0cs@webmail.leidinger.net> References: <20080304183529.kpaf3f76gcg4g0cs@webmail.leidinger.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Mar 04, 2008 at 06:35:29PM +0100, Alexander Leidinger wrote:
> >@@ -57,7 +57,9 @@
> >
> > if (args->size <= 0)
> > return (EINVAL);
> >- /* args->size is unused. Linux ignores it as well. */
> >+ /* + * args->size is unused. Linux just tests it + * 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
> >static int
> >linux_kev_copyin(void *arg, struct kevent *kevp, int count)
> >@@ -193,7 +197,8 @@
> > break;
> > case LINUX_EPOLL_CTL_MOD:
> > /* TODO: DELETE && ADD maybe? */
> >- return (EINVAL);
> >+ printf("linux_epoll_ctl: CTL_MOD not yet
> >>implemented.\n");
> >+ return (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
> > break;
> > case LINUX_EPOLL_CTL_DEL:
> > kev.flags = EV_DELETE | EV_DISABLE;
> >@@ -241,6 +246,9 @@
> >
> > error = kern_kevent(td, args->epfd, 0, args->maxevents, &k_ops, &ts);
> >
> >- /* translation? */
> >+ /* + * kern_keven might return ENOMEM which is not expected from
> >epoll_wait.
> >+ * Maybe we should translate that but I don't think it matters at
> >all.
> >+ */
> > return (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 <= 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).
I'd let it as it is and fix it if problems from real usage arise.
can someone test this using java/apache/something? I dont have time/energy
for that :(
roman
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080305181741.GA15620>