Re: [RFC/PATCH] inotify -- a dnotify replacement

[Chris Wedgwood <cw f00f org>]

On Sun, May 09, 2004 at 09:35:41PM -0400, John McCutchan wrote:

> The two biggest complaints people have about dnotify are
>
> 1) dnotify delivers events using signals.

is that really a problem?

>
> 2) dnotify needs a file to be kept open on the device, causing
> problems during unmount.

i never thought of that...  what about

3) dnotify cannot easily watch changes for a directory hierarchy

> @@ -127,6 +128,7 @@
>  	return dn_mask;
>  }
>
> +
>  int notify_change(struct dentry * dentry, struct iattr * attr)
>  {
>  	struct inode *inode = dentry->d_inode;

plz don't add unnecessary whitespace

> diff -ru clean/linux-2.6.5/fs/inode.c linux-2.6.5/fs/inode.c
> +++ linux-2.6.5/fs/inode.c	2004-05-09 21:10:12.000000000 -0400
> @@ -151,6 +151,10 @@
>  			mapping->backing_dev_info = sb->s_bdev->bd_inode->i_mapping->backing_dev_info;
>  		memset(&inode->u, 0, sizeof(inode->u));
>  		inode->i_mapping = mapping;
> +
> +		INIT_LIST_HEAD(&inode->watchers);
> +		atomic_set (&inode->watcher_count, 0);
                          ^

whitespace after if/for/while but not functions:

 if (..)
 func(...)

> +#define INOTIFY_MINOR 99

there is a registry for these, i think you can use -1 (to get one
dynamically allocated) if you've not been assigned one

> #define MAX_WATCHER_COUNT 8 /* We only support 8 watchers */

seems like this could be a problem for gui stuff

> #define MAX_WATCH_COUNT 128 /* A watcher can only be watching 128 inodes */

likewise

> static int watcher_count = 0;

global variables don't need explicitly initialized to zero and in fact
this will make the kernel larger when using older gcc versions

> /* A list of these structures is attached to each inode that is being watched.
>  *  * each item in the list represents a unique watcher.
>  *   * it tell us what events we are looking at and who is watching the inode.
>  *    */

heh, odd comment style

> struct inotify_event {
>         struct list_head        list;
>         unsigned long           i_no;
>         unsigned long           i_dev;
>         unsigned long           mask;
> };

i'm not so sure using unsigned long is a good idea here,  it's size
varies depending on arch (also consider 32-bit code on 64-bit
platforms)

we might also have a 64-bit ino_t on i386 some day?  (what a revolting
idea but we have PAE, etc. so it's possible)

> 		if (watcher->private_data == dev)
> 		{
> 			return watcher;
> 		}

minor nit:

	if (...)
	   return ...;

> 	if (NULL != watcher) {

i really hate that, what is wrong with

  if (watcher)

> 	list_for_each_entry (watch, dev->watch, list)

  list_for_each_entry (...) {

> static char inotify_dev_has_events (struct inotify_device *dev)
> {
> 	return dev->events && !list_empty(dev->events);
> }

why does this need to be a separate function?

> static int __init inotify_init (void)
> {
> 	int ret;
>
> 	ret = misc_register (&inotify_device);
>
> 	if (ret) {
> 		goto out;
> 	}
>
> 	ret = 0;

if we got here, ret must be 0



  --cw