Re: [RFC][PATCH] Inotify kernel API

> +/* > + * inotify_add_watch - add a watch to this inotify instance. > + */ > +s32 inotify_add_watch(struct inotify_device *dev, struct inode *inode, > +??????????????? ? ? ?u32 mask, void *callback_arg) > +{ > +???????struct inotify_watch *watch, *old; > +???????int ret; > + > +???????down(&inode->inotify_sem); What happens if the inode is deleted when attempting to hold down the semaphore?

Perhaps a comment reminding the caller of this function they must hold the proper inode locks around this function?.

> +??????? * Handle the case of re-adding a watch on an (inode,dev) pair that we > +??????? * are already watching. ?We just update the mask and callback_arg and > +??????? * return its wd. > +??????? */ > +???????old = inode_find_dev(inode, dev); > +???????if (unlikely(old)) { > +???????????????old->mask = mask; > +???????????????old->callback_arg = callback_arg; You can leak memory here, right? Maybe there needs to be a old->callback_free field that, when set, knows how to clean up old->callback_arg, before pointing it else where. Not sure we can assume that the callback_arg is always going to be statically allocated.

> +/* Kernel producer API */ > ? > ?/** > ? * inotify_inode_queue_event - queue an event to all watches on this inode Since the purpose of this patch is to add a kernel API to Inotify, I think it should be as clean as possible. With this in mind, I think that since we _might_ be calling inotify_callback_event that the calling function not be inotify_inode_queue_event; it's not all that intuitive. Instead, I think this function should be renamed to something like, inotify_inode_handle_event...