epoll review continued

[Roman Divacky]

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