[RFC][PATCH] Inotify kernel API

[Amy Griffis]

On Tue, Sep 06, 2005 at 05:29:32PM -0500, Timothy R. Chavez wrote:
> > +/*
> > + * 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?

The caller must pin the inode, for instance via path_lookup.

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

Sure, I'll add a comment.

> > +??????? * 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.

Inotify doesn't care what callback_arg points to -- that's why it's
void.  It is up to the consumer to manage the memory.  The
callback_arg should never be the last pointer to a chunk of memory.

> > +/* 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...

Okay.  Chris also mentioned on #audit that the queueing of events for
userspace could be split out of the core Inotify code.