Re: [PATCH updated] Support for filesystem watching (inotify)

[Stefan Monnier]

> I updated the patch to return a "file-watcher".  file-watch can now be
> called several times for the same file with different flags and
> callbacks.  This however required large changes to the patch.  So it
> would be great if someone could take a look at it again.  I added an
> automated test.  But it doesn't test the actually file-watching
> yet. This is a bit tricky to get right in a unit test.  But I'll add
> it.  The code requires some heavy testing.

Here are some comments, only based on a cursory read.  I do not know
anything about the inotify API and have not tried your code.

> --- /dev/null
> +++ b/lisp/filewatch.el
> @@ -0,0 +1,54 @@
> +;;; filewatch.el --- Elisp support for watching filesystem events.
> +
> +;; Copyright (C) 2011 Free Software Foundation, Inc.
> +
> +;; Author: Rüdiger Sonderfeld <address@hidden>
> +;; Keywords: files
> +
> +;; This file is part of GNU Emacs.
> +
> +;; GNU Emacs is free software: you can redistribute it and/or modify
> +;; it under the terms of the GNU General Public License as published by
> +;; the Free Software Foundation, either version 3 of the License, or
> +;; (at your option) any later version.
> +
> +;; GNU Emacs is distributed in the hope that it will be useful,
> +;; but WITHOUT ANY WARRANTY; without even the implied warranty of
> +;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +;; GNU General Public License for more details.
> +
> +;; You should have received a copy of the GNU General Public License
> +;; along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.
> +
> +;;; Commentary:
> +
> +;; This file contains the elisp part of the filewatch API.
> +
> +;;; Code:
> +
> +(eval-when-compile
> +  (require 'cl))
> +
> +(defun filewatch-event-p (event)
> +  "Check if EVENT is a filewatch event."
> +  (and (listp event)
> +       (eq (car event) 'filewatch-event)))
> +
> +;;;###autoload
> +(defun filewatch-handle-event (event)
> +  "Handle file system monitoring event.
> +If EVENT is a filewatch event then the callback is called.  If EVENT is
> +not a filewatch event then a `filewatch-error' is signaled."
> +  (interactive "e")
> +
> +  (unless (filewatch-event-p event)
> +    (signal 'filewatch-error (cons "Not a valid filewatch event" event)))
> +
> +  (loop for ev in (cdr event)
> +     unless (and (listp ev) (>= (length ev) 3))
> +     do (signal 'filewatch-error (cons "Not a valid filewatch event" event))
> +     do (funcall (cadr ev) (car ev) (caddr ev))))
> +
> +(provide 'filewatch)

Unless we expect many more functions in this file, we could move those
functions to subr.el (tho after removing the CL dependency since
subr.el can't use CL for bootstrapping reasons).

> +/* Assoc list of files being watched.
> + Format:
> + (id . (filename ((subid callback flags) (subid callbacks flags) ...))) */
> +static Lisp_Object watch_list;
> +
> +#define CADDR(x) Fcar (Fcdr (Fcdr (x)))
> +#define CADR(x) Fcar (Fcdr (x))

Most C code should use XCAR/XCDR rather than Fcr/Fcdr.

> +  while (!NILP (i))
> +    {
> +      Lisp_Object e = Fcar (events);

E.g. here, better test CONSP rather than !NILP, which lets you then use
XCAR/XCDR.

> +      while (!NILP (e))
> +        {
> +          if (EQ (Fcar (e), i))
> +            ret = Fcons (e, ret);
> +          events = Fcdr (events);
> +          e = Fcar (events);
> +        }
> +      aspects = Fcdr (aspects);
> +      i = Fcar (aspects);
> +    }

I'd have used an outside loop on events so that the inner loop on
aspects can use Fmemq.

> +  return ret;
> +}
> +
> +static Lisp_Object
> +inotifyevent_to_aspects (Lisp_Object name, struct inotify_event *ev)
> +{

It doesn't just return aspects but events (using a Lips_Object
representation), so the name is somewhat misleading.

> +  Lisp_Object events = Qnil;
> +  if (ev->mask & (IN_MODIFY|IN_CREATE) )
> +    events = Fcons (Fcons (Qmodify, name), events);
> +  if (ev->mask & IN_MOVE_SELF)
> +    events = Fcons (Fcons (Qmove, name), events);
> +  if (ev->mask & IN_MOVED_FROM)
> +    events = Fcons (Fcons (Qmove,
> +                           Fcons (Qfrom,
> +                                  Fcons (name,

The structure of those events is a bit odd.

It seems that each event is either of form (SYMBOL . FILENAME) where
FILENAME is the watched object, or of the form (SYMBOL from NAME . COOKIE)
where NAME is the file added/removed from the watched object (a
directory, presumably), but this object's FILENAME is not present.
Any particular reason for this inconsistency?

> +                                         make_number (ev->cookie)))),
> +                    events);
> +  if (ev->mask & IN_MOVED_TO)
> +    events = Fcons (Fcons (Qmove,
> +                           Fcons (Qto,
> +                                  Fcons (name,
> +                                         make_number (ev->cookie)))),

IIUC, these cookies are part of the patch to which Paul objected.
And IIUC their content has no importance, they're only checked for
equality, right?

So they don't have to be represented as numbers, but could be
represented by some other special object, as long as (eq cookie1
cookie2) returns the proper boolean value when comparing those
special objects.

I guess that several `ev' objects can have the same `ev->cookie'
value, right?

> +/* This callback is called when the FD is available FOR_READ.  The inotify
> +   events are read from FD and converted into input_events.  */
> +static void
> +inotify_callback (int fd, void *_, int for_read)
> +{

AFAICT, the void* argument will be a pointer to watch_list.
I think that either you should use it here, or you should pass NULL to
add_read_fd (rather than passing &watch_list).

> +      Lisp_Object watch_data = Fassoc (make_number (ev->wd), watch_list);
> +      if (!NILP (watch_data))
> +        {
> +          Lisp_Object name, events;
> +
> +          /* If a directory is watched name contains the name
> +             of the file that was changed.  */
> +          if (ev->len > 0)
> +            {
> +              size_t const len = strlen (ev->name);
> +              name = make_unibyte_string (ev->name, min (len, ev->len));
> +              name = DECODE_FILE (name);
> +            }
> +          else
> +            {
> +              name = CADR (watch_data);
> +            }
> +
> +          events = inotifyevent_to_aspects (name, ev);
> +
> +          if (!NILP (events))

Wouldn't it be a bug for events to be nil here (that would mean you
receive inotify events for files you do not know you're watching, or
something like that)?

> +            {
> +              Lisp_Object x = CADDR (watch_data);
> +              while (!NILP (x))
> +                {
> +                  Lisp_Object cell = Fcar (x);
> +                  Lisp_Object aspects = match_aspects (cell, events);
> +                  if (!NILP (aspects))
> +                    {
> +                      Lisp_Object id = list3 (Qfile_watch, make_number 
> (ev->wd),
> +                                              Fcar (cell));

I don't think you need ev->wd in your file-watch objects.  You could use
Fcons (Qfile_watch, cell) instead, IIUC.  It would be good, because it
would let you use a SAVE_VALUE object for ev->wd (since it wouldn't
escape to Lisp any more), which would solve the problem of losing a few
bits of data in make_number.

> +
> +static uint32_t
> +symbol_to_inotifymask (Lisp_Object symb, int in_list)
> +{
> +  if (EQ (symb, Qmodify))
> +    return IN_MODIFY | IN_CREATE;
> +  else if (EQ (symb, Qmove))
> +    return IN_MOVE_SELF | IN_MOVE;
> +  else if (EQ (symb, Qattrib))
> +    return IN_ATTRIB;
> +  else if (EQ (symb, Qdelete))
> +    return IN_DELETE_SELF | IN_DELETE;
> +  else if (!in_list && EQ (symb, Qall_modify))
> +    return IN_MODIFY | IN_CREATE | IN_MOVE_SELF | IN_MOVE | IN_ATTRIB
> +      | IN_DELETE_SELF | IN_DELETE;
> +  else if (EQ (symb, Qaccess))
> +    return IN_ACCESS;
> +  else if (EQ (symb, Qclose_write))
> +    return IN_CLOSE_WRITE;
> +  else if (EQ (symb, Qclose_nowrite))
> +    return IN_CLOSE_NOWRITE;
> +  else if (EQ (symb, Qopen))
> +    return IN_OPEN;
> +  else if (!in_list && (EQ (symb, Qt) || EQ (symb, Qall)))
> +    return IN_MODIFY | IN_CREATE | IN_MOVE_SELF | IN_MOVE | IN_ATTRIB
> +      | IN_DELETE_SELF | IN_DELETE | IN_ACCESS | IN_CLOSE_WRITE
> +      | IN_CLOSE_NOWRITE | IN_OPEN;
> +  else
> +    Fsignal (Qunknown_aspect, Fcons (symb, Qnil));

Just make it an `error'.

> +static Lisp_Object
> +get_watch_data_by_file_name (Lisp_Object file_name)
> +{
> +  Lisp_Object cell, file_name_data;
> +  Lisp_Object x = watch_list;
> +  CHECK_STRING (file_name);
> +
> +  while (!NILP (x))
> +    {
> +      cell = Fcar (x);
> +      x = Fcdr (x);
> +      file_name_data = Fcdr (cell);
> +      if (!NILP (file_name_data) && STRINGP (Fcar (file_name_data))
> +          && !NILP (Fstring_equal (Fcar (file_name_data), file_name))
> +          && NUMBERP (Fcar (cell)))

Why do you need NUMBERP?
Shouldn't it be an "eassert (NUMBERP (...))" instead?

> +Watching a directory is not recursive.  CALLBACK receives the events as a 
> list
> +with each list element being a list containing information about an event.  
> The
> +first element is a flag symbol.  If a directory is watched the second 
> element is
> +the name of the file that changed.  If a file is moved from or to the 
> directory
> +the second element is either 'from or 'to and the third element is the file
> +name.  A fourth element contains a numeric identifier (cookie) that can be 
> used
> +to identify matching move operations if a file is moved inside the directory.

We typically try to avoid such "third element" style and use forms like:
"the argument takes the form (OP . FILENAME) or (OP DIRECTION NAME COOKIE)
where OP is blabl, etc...".  Also please say how many arguments are
passed to CALLBACK and what means each one of them.

> +  mask = aspect_to_inotifymask (aspect);
> +  data = get_watch_data_by_file_name (file_name);
> +  if (!NILP (data))
> +    mask |= IN_MASK_ADD;

Would it hurt to always add IN_MASK_ADD?

> +  decoded_file_name = ENCODE_FILE (file_name);
> +  watchdesc = inotify_add_watch (inotifyfd, SSDATA (decoded_file_name), 
> mask);

Are we sure that when data is nil, watchdesc is different from all other
watchdesc we currently have, and that when data is non-nil, watchdesc is
equal to the watchdesc stored in data?
If so, let's says so via asserts, and if not, we should check and decide
what to do.

IIUC inotify operates on inodes whereas our watch_list is indexed by
file names, so get_watch_data_by_file_name may think we're changing an
existing watcher but in reality it may operate on a new inode and hence
return a new watchdesc.  Inversely, two different filenames can refer to
the same inode, so I suspect that there are also cases where
get_watch_data_by_file_name thinks this is a new watcher but the
watchdesc returned will be one that already exists.

IOW, I think we should always say IN_MASK_ADD (just in case) and we
should not use get_watch_data_by_file_name before calling
inotify_add_watch, but instead use get_watch_data_by_watchdesc afterwards.

> +  return list3 (Qfile_watch, make_number (watchdesc), Fcar (info));

See comment earlier about eliding watchdesc and including `info' instead.

> +  if (inotifyfd == uninitialized)
> +    return Qnil;

Shouldn't this signal an error instead?


        Stefan