Re: [patch] Fix type errors in inotify interfaces

["Michael Kerrisk" <mtk.manpages@googlemail.com>]

(Would be nice to see an Aacked-by from Robert or John on this patch.)

On Wed, Nov 19, 2008 at 3:03 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Tue, 18 Nov 2008 15:18:21 -0500 Michael Kerrisk <mtk.manpages@googlemail.com> wrote:
>
>> Andrew,
>>
>> Vegard reminded me of an issue with the inotify interface
>> that I raised quite a while ago, offlist, with Robert; Robert
>> acknowledged that things should be fixed, but then neither of
>> us actually did anything :-{.
>>
>> The problems lie in the types used for some inotify interfaces, both
>> at the kernel level and at the glibc level.  This mail addresses the
>> kernel problem.  I will follow up with some suggestions for glibc
>> changes.
>>
>> For the sys_inotify_rm_watch() interface, the type of the 'wd' argument
>> is currently 'u32', it should be '__s32' .  That is Robert's suggestion,
>> and is consistent with the other declarations of watch descriptors in
>> the kernel source, in particular, the inotify_event structure in
>> include/linux/inotify.h:
>>
>> struct inotify_event {
>>         __s32           wd;             /* watch descriptor */
>>         __u32           mask;           /* watch mask */
>>         __u32           cookie;         /* cookie to synchronize two events */
>>         __u32           len;            /* length (including nulls) of name */
>>         char            name[0];        /* stub for possible name */
>> };
>>
>> The patch below makes the changes needed for inotify_rm_watch().
>>
>> Thanks
>>
>> Michael
>>
>>
>> diff --git a/fs/inotify_user.c b/fs/inotify_user.c
>> index d367e9b..a71f764 100644
>> --- a/fs/inotify_user.c
>> +++ b/fs/inotify_user.c
>> @@ -704,7 +704,7 @@ fput_and_out:
>>       return ret;
>>  }
>>
>> -asmlinkage long sys_inotify_rm_watch(int fd, u32 wd)
>> +asmlinkage long sys_inotify_rm_watch(int fd, __s32 wd)
>>  {
>>       struct file *filp;
>>       struct inotify_device *dev;
>> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
>> index d6ff145..36983a5 100644
>> --- a/include/linux/syscalls.h
>> +++ b/include/linux/syscalls.h
>> @@ -550,7 +550,7 @@ asmlinkage long sys_inotify_init(void);
>>  asmlinkage long sys_inotify_init1(int flags);
>>  asmlinkage long sys_inotify_add_watch(int fd, const char __user *path,
>>                                       u32 mask);
>> -asmlinkage long sys_inotify_rm_watch(int fd, u32 wd);
>> +asmlinkage long sys_inotify_rm_watch(int fd, __s32 wd);
>>
>>  asmlinkage long sys_spu_run(int fd, __u32 __user *unpc,
>>                                __u32 __user *ustatus);
>

Yes -- there is no sane reason for a negative watch descriptor to
inotify_rm_watch(); this change is mainly about consistency.  (The
glibc change is probably more important.)

> otoh, the system call via which one _obtains_ watch descriptors most
> certainly wants to return -ve nunmbers, to signify errors.
>
> All too hard.  I think I'll stop thinking about it and merge the patch ;)

Thanks.

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git
man-pages online: http://www.kernel.org/doc/man-pages/online_pages.html
Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html