Re: [PATCH] inotify to minimize stat() calls

>
> Robert Zeh <robert.allan.zeh@gmail.com> writes:
>
> > Here is a patch that creates a daemon that tracks file
> > state with inotify, writes it out to a file upon request,
> > and changes most of the calls to stat to use said cache.
> >
> > It has bugs, but I figured it would be smarter to see
> > if the approach was acceptable at all before spending the
> > time to root the bugs out.
>
> Thanks for tackling this; it's probably about time we got a inotify
> support :-(

> > I've implemented the communication with a file, and not a socket,
> > because I think implementing a socket is going to create
> > security issues on multiuser systems.  For example, would a
> > socket allow stat information to cross user boundaries?
>
> This ties in with an issue discussed in an earlier thread:
>
>   http://thread.gmane.org/gmane.comp.version-control.git/217817/focus=218307
>
> The conclusion there was that the default limits are set such that it is
> not feasible to run one daemon per repository (that would quickly hit
> the limits when e.g. iterating all repos in a typical android tree using
> repo).
>
> So whatever you use for communication needs to work as a global daemon.
>
> I'd just trust the SSH folks to know about security; on my system
> ssh-agent creates
>
>   /tmp/ssh-RANDOMSTRING/agent.PID
>
> where the directory has mode 0700, and the file is a unit socket with
> mode 0600.  That should make doubly sure that no other user can open the
> socket.
>
> >  filechange-cache.c   | 203
> > +++++++++++++++++++++++++++++++++++++++++++++++++++
>
> Is your MUA wrapping the patch?

> > +static void watch_directory(int inotify_fd)
> > +{
> > +     char buf[PATH_MAX];
> > +
> > +     if (!getcwd(buf, sizeof(buf)))
> > +             die_errno("Unable to get current directory");
> > +
> > +     int i = 0;
> > +     struct dir_struct dir;
> > +     const char *pathspec[1] = { buf, NULL };
> > +
> > +     memset(&dir, 0, sizeof(dir));
> > +     setup_standard_excludes(&dir);
> > +
> > +     fill_directory(&dir, pathspec);
> > +     for(i = 0; i < dir.nr; i++) {
> > +             struct dir_entry *ent = dir.entries[i];
> > +             watch_file(inotify_fd, ent->name);
> > +             free(ent);
> > +     }
>
> I don't get this bit.  The lstat() are run over all files listed in the
> index.  So shouldn't your daemon watch exactly those (or rather, all
> dirnames of such files)?

> > +     while (1) {
> > +             int i = 0;
> > +             length = read(inotify_fd, buffer, sizeof(buffer));
> > +             for(i = 0; i < length; ) {
> > +                     struct inotify_event *event =
> > +                             (struct inotify_event*)(buffer+i);
> > +                     /* printf("event: %d %x %d %s\n", event->wd, event->mask,
> > +                        event->len, event->name); */
> > +                     if (request_watch_descriptor == event->wd) {
> > +                             write_stat_cache();
> > +                     } else if (root_directory_watch_descriptor
> > +                                == event->wd) {
> > +                             printf("root directory died!\n");
> > +                             exit(0);
> > +                     } else if (event->mask & IN_Q_OVERFLOW) {
> > +                             restart();
>
> Good.
>
> > +                     } else if (event->mask & IN_MODIFY) {
> > +                             if (event->len)
> > +                                     update_stat_cache(event->name);
> > +                     }
>
> So whenever a file changes, you stat() it.  That's good for simplicity
> now, but I suspect it will provide some optimization opportunities
> later.

> The last one is just based on my personal experience with messing with
> inotify; renaming directories is the "hard" case for that API.  We may
> already cover this in the test suite, or we may not; but it must be
> tested.
>
> Other than that last point, focus your tests not on small tests but on
> the test suite.  It would seem rather unlikely to me that you could
> manage to pass the entire test suite with this daemon active but broken.