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);
I hope Robert doesn't mind if I drop a piece from our old offlist mail
conversation into this thread, for some background.
-------- Original Message --------
Subject: Re: Use of uint32_t in inotify APIs
Date: Tue, 20 Jun 2006 09:07:17 -0400
From: Robert Love <[email protected]>
On 6/20/06, Michael Kerrisk wrote:
Hi,
> The use of uint32_t seems a little strange in at least some of the
> above. In particular, the watch descriptor returned by
> inotify_add_watch() is an "int", as is the "wd" field of the
> "inotify_event" structure, but in the prototype for inotify_rm_watch(),
> "wd" is uint32_t. Is there a reason for this, or is it just cruft?
The unsigned parameter in inotify_rm_watch() is wrong. I cannot think
we that should be the case. But the problem lies not with glibc --
the function definition in the kernel is wrong, too. It should be
fixed; everywhere else a watch descriptor is an __s32.
> Following on from this, is it really necessary that "mask" is uint32_t
> in the prototype for inotify_rm_watch()? In most other glibc
> interfaces, bit masks are simply "int".
I think masks should be unsigned. This definitely makes sense.
In the kernel, most of the inotify data types are __u32.
[...]
Also the 'wd' in the event structure should be 'int32_t' -- the kernel
uses the type '__s32' -- but perhaps glibc developers prefer to use a
straight 'int' since the types are identical.
In that case, inotify_rm_watch()'s 'wd' should be changed to '__s32'
in the kernel and 'int' in glibc. Otherwise, '__s32' in the kernel
and 'int32_t' in glibc. Since this change will affect ABI, it needs
to be made cautiously, but I don't see any issues.
Robert Love
Ulrich,
Below are the changes that I believe are required in glibc.
Thanks,
Michael
diff -ruN glibc-2.9/sysdeps/unix/sysv/linux/alpha/sys/inotify.h glibc-2.9-mod/sysdeps/unix/sysv/linux/alpha/sys/inotify.h
--- glibc-2.9/sysdeps/unix/sysv/linux/alpha/sys/inotify.h 2008-07-24 23:43:59.000000000 -0500
+++ glibc-2.9-mod/sysdeps/unix/sysv/linux/alpha/sys/inotify.h 2008-11-18 15:30:53.000000000 -0500
@@ -35,7 +35,7 @@
/* Structure describing an inotify event. */
struct inotify_event
{
- int wd; /* Watch descriptor. */
+ int32_t wd; /* Watch descriptor. */
uint32_t mask; /* Watch mask. */
uint32_t cookie; /* Cookie to synchronize two events. */
uint32_t len; /* Length (including NULs) of name. */
@@ -98,7 +98,7 @@
__THROW;
/* Remove the watch specified by WD from the inotify instance FD. */
-extern int inotify_rm_watch (int __fd, uint32_t __wd) __THROW;
+extern int inotify_rm_watch (int __fd, int __wd) __THROW;
__END_DECLS
diff -ruN glibc-2.9/sysdeps/unix/sysv/linux/sparc/sys/inotify.h glibc-2.9-mod/sysdeps/unix/sysv/linux/sparc/sys/inotify.h
--- glibc-2.9/sysdeps/unix/sysv/linux/sparc/sys/inotify.h 2008-07-24 23:48:03.000000000 -0500
+++ glibc-2.9-mod/sysdeps/unix/sysv/linux/sparc/sys/inotify.h 2008-11-18 15:31:08.000000000 -0500
@@ -35,7 +35,7 @@
/* Structure describing an inotify event. */
struct inotify_event
{
- int wd; /* Watch descriptor. */
+ int32_t wd; /* Watch descriptor. */
uint32_t mask; /* Watch mask. */
uint32_t cookie; /* Cookie to synchronize two events. */
uint32_t len; /* Length (including NULs) of name. */
@@ -98,7 +98,7 @@
__THROW;
/* Remove the watch specified by WD from the inotify instance FD. */
-extern int inotify_rm_watch (int __fd, uint32_t __wd) __THROW;
+extern int inotify_rm_watch (int __fd, int __wd) __THROW;
__END_DECLS
diff -ruN glibc-2.9/sysdeps/unix/sysv/linux/sys/inotify.h glibc-2.9-mod/sysdeps/unix/sysv/linux/sys/inotify.h
--- glibc-2.9/sysdeps/unix/sysv/linux/sys/inotify.h 2008-07-24 23:50:41.000000000 -0500
+++ glibc-2.9-mod/sysdeps/unix/sysv/linux/sys/inotify.h 2008-11-18 15:31:23.000000000 -0500
@@ -35,7 +35,7 @@
/* Structure describing an inotify event. */
struct inotify_event
{
- int wd; /* Watch descriptor. */
+ int32_t wd; /* Watch descriptor. */
uint32_t mask; /* Watch mask. */
uint32_t cookie; /* Cookie to synchronize two events. */
uint32_t len; /* Length (including NULs) of name. */
@@ -98,7 +98,7 @@
__THROW;
/* Remove the watch specified by WD from the inotify instance FD. */
-extern int inotify_rm_watch (int __fd, uint32_t __wd) __THROW;
+extern int inotify_rm_watch (int __fd, int __wd) __THROW;
__END_DECLS
On Tue, 18 Nov 2008 15:18:21 -0500 Michael Kerrisk <[email protected]> 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);
hrm. I see no sane reason why a watch descriptor should take on
negative values, so in some ways the u32 was logical. (Ditto file
descriptors, but I don't think I want to change that ;))
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 ;)
(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
<[email protected]> wrote:
> On Tue, 18 Nov 2008 15:18:21 -0500 Michael Kerrisk <[email protected]> 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
On Wed, Nov 19, 2008 at 2:10 PM, Michael Kerrisk
<[email protected]> wrote:
> (Would be nice to see an Aacked-by from Robert or John on this patch.)
I hesitate because I haven't looked at glibc in ages to see what it
and other userland consumers are doing with inotify.
But my original analysis still seems correct, and whatever we do we
currently have inconsistencies, so this looks good to me.
Acked-by: Robert Love <[email protected]>
Robert
On Wed, Nov 19, 2008 at 11:10 AM, Michael Kerrisk
<[email protected]> wrote:
> (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
> <[email protected]> wrote:
>> On Tue, 18 Nov 2008 15:18:21 -0500 Michael Kerrisk <[email protected]> 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 ;)
>
Seems sane enough.
Acked-by: John McCutchan <[email protected]>
--
John McCutchan <[email protected]>