epoll review continued

[Alexander Leidinger]

Quoting Roman Divacky <rdivacky at freebsd.org> (from Wed, 5 Mar 2008  
19:17:41 +0100):

> 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

This comment is supposed to show that we are aware of the man page and  
about what the kernel does. Whoever reads this comment will get the  
impression, that we did our homework and also gets told why we do the  
check this way. When anything in linux (either the kernel or the man  
page) changes, then anyone reading this comment will see, that we are  
not up-to-date and know why the code is like it is and hopefully  
notifies us. I agree that it is of no use for us at this moment, but  
it improve the code quality a lot, as it helps maintaining this code  
in the future.

Just imagine how fast you would have understood some other parts of  
the linuxulator, if there would have been similar comments. I'm sure  
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 @@
>> >		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

I've seen only complete syscalls behaving like this. Can you please  
point out subfeatures in syscalls not written by you which do this?

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

Yes. The comment which was added by you in the source talks about  
epoll_wait, so was I. And I thought I checked that this part was  
really part of epoll_wait. To make it specific: I haven't seen an  
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.

-- 
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 = B0063FE7
http://www.FreeBSD.org       netchild @ FreeBSD.org  : PGP ID = 72077137