From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.devel Subject: Re: [PATCH updated] Support for filesystem watching (inotify) Date: Thu, 07 Jul 2011 15:43:43 -0400 Message-ID: References: <201106040034.15598.ruediger@c-plusplus.de> <201106061825.25078.ruediger@c-plusplus.de> <201106240250.01247.ruediger@c-plusplus.de> NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: quoted-printable X-Trace: dough.gmane.org 1310069348 13024 80.91.229.12 (7 Jul 2011 20:09:08 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Thu, 7 Jul 2011 20:09:08 +0000 (UTC) Cc: Eli Zaretskii , Thien-Thi Nguyen , emacs-devel@gnu.org To: =?iso-8859-1?Q?R=FCdiger?= Sonderfeld Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Thu Jul 07 22:09:03 2011 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([140.186.70.17]) by lo.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1Qeusc-00078i-6K for ged-emacs-devel@m.gmane.org; Thu, 07 Jul 2011 22:08:58 +0200 Original-Received: from localhost ([::1]:58391 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qeusb-0007pu-7Q for ged-emacs-devel@m.gmane.org; Thu, 07 Jul 2011 16:08:57 -0400 Original-Received: from eggs.gnu.org ([140.186.70.92]:58233) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QeuUI-0001HR-PI for emacs-devel@gnu.org; Thu, 07 Jul 2011 15:43:52 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QeuUG-0002IN-2V for emacs-devel@gnu.org; Thu, 07 Jul 2011 15:43:50 -0400 Original-Received: from pruche.dit.umontreal.ca ([132.204.246.22]:32778) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QeuUF-0002IH-NR; Thu, 07 Jul 2011 15:43:47 -0400 Original-Received: from faina.iro.umontreal.ca (lechon.iro.umontreal.ca [132.204.27.242]) by pruche.dit.umontreal.ca (8.14.1/8.14.1) with ESMTP id p67Jhiqb016201; Thu, 7 Jul 2011 15:43:44 -0400 Original-Received: by faina.iro.umontreal.ca (Postfix, from userid 20848) id 5DCF1B4177; Thu, 7 Jul 2011 15:43:44 -0400 (EDT) In-Reply-To: <201106240250.01247.ruediger@c-plusplus.de> (=?iso-8859-1?Q?=22R=FCdiger?= Sonderfeld"'s message of "Fri, 24 Jun 2011 02:50:00 +0200") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.0.50 (gnu/linux) X-NAI-Spam-Flag: NO X-NAI-Spam-Threshold: 5 X-NAI-Spam-Score: 0 X-NAI-Spam-Rules: 1 Rules triggered RV3911=0 X-NAI-Spam-Version: 2.2.0.9286 : core <3911> : streams <658544> : uri <909043> X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 132.204.246.22 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:141741 Archived-At: > 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=FCdiger Sonderfeld > +;; 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 . > + > +;;; 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) (>=3D (length ev) 3)) > + do (signal 'filewatch-error (cons "Not a valid filewatch event" eve= nt)) > + 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 =3D 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 =3D Fcons (e, ret); > + events =3D Fcdr (events); > + e =3D Fcar (events); > + } > + aspects =3D Fcdr (aspects); > + i =3D 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 =3D Qnil; > + if (ev->mask & (IN_MODIFY|IN_CREATE) ) > + events =3D Fcons (Fcons (Qmodify, name), events); > + if (ev->mask & IN_MOVE_SELF) > + events =3D Fcons (Fcons (Qmove, name), events); > + if (ev->mask & IN_MOVED_FROM) > + events =3D 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 =3D 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 inoti= fy > + 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 =3D Fassoc (make_number (ev->wd), watch_lis= t); > + 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 =3D strlen (ev->name); > + name =3D make_unibyte_string (ev->name, min (len, ev->len)= ); > + name =3D DECODE_FILE (name); > + } > + else > + { > + name =3D CADR (watch_data); > + } > + > + events =3D 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 =3D CADDR (watch_data); > + while (!NILP (x)) > + { > + Lisp_Object cell =3D Fcar (x); > + Lisp_Object aspects =3D match_aspects (cell, events); > + if (!NILP (aspects)) > + { > + Lisp_Object id =3D 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 =3D watch_list; > + CHECK_STRING (file_name); > + > + while (!NILP (x)) > + { > + cell =3D Fcar (x); > + x =3D Fcdr (x); > + file_name_data =3D 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 even= t. The > +first element is a flag symbol. If a directory is watched the second el= ement is > +the name of the file that changed. If a file is moved from or to the di= rectory > +the second element is either 'from or 'to and the third element is the f= ile > +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 direc= tory. 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 =3D aspect_to_inotifymask (aspect); > + data =3D get_watch_data_by_file_name (file_name); > + if (!NILP (data)) > + mask |=3D IN_MASK_ADD; Would it hurt to always add IN_MASK_ADD? > + decoded_file_name =3D ENCODE_FILE (file_name); > + watchdesc =3D 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 =3D=3D uninitialized) > + return Qnil; Shouldn't this signal an error instead? Stefan