epoll review continued

[Alexander Leidinger]

> @@ -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.

> 	return (kqueue(td, &k_args));
> }
> @@ -140,7 +142,9 @@
>
> /*
>  * Copyin callback used by kevent. This copies already
> - * converted filters to the kevent internal memory.
> + * converted filters from kernel memory to the kevent + * internal  
> kernel memory. Hence the memcpy instead of
> + * copyin.
>  */

Thanks.

> 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.

> 		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).

Bye,
Alexander.

-- 
If you sow your wild oats, hope for a crop failure.

http://www.Leidinger.net    Alexander @ Leidinger.net: PGP ID = B0063FE7
http://www.FreeBSD.org       netchild @ FreeBSD.org  : PGP ID = 72077137