Messages in this thread | | | Date | Sun, 9 May 2004 19:11:42 -0700 | From | Chris Wedgwood <> | Subject | Re: [RFC/PATCH] inotify -- a dnotify replacement |
| |
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |