From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: [PATCH] Support for filesystem watching (inotify) Date: Sat, 04 Jun 2011 14:30:42 +0300 Message-ID: <831uz9esv1.fsf@gnu.org> References: <201106040034.15598.ruediger@c-plusplus.de> Reply-To: Eli Zaretskii NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE X-Trace: dough.gmane.org 1307187053 19750 80.91.229.12 (4 Jun 2011 11:30:53 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Sat, 4 Jun 2011 11:30:53 +0000 (UTC) Cc: emacs-devel@gnu.org To: =?utf-8?Q?R=C3=BCdiger?= Sonderfeld Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Sat Jun 04 13:30:49 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 1QSp45-0004VW-6D for ged-emacs-devel@m.gmane.org; Sat, 04 Jun 2011 13:30:49 +0200 Original-Received: from localhost ([::1]:51544 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QSp43-0003Gb-Or for ged-emacs-devel@m.gmane.org; Sat, 04 Jun 2011 07:30:47 -0400 Original-Received: from eggs.gnu.org ([140.186.70.92]:58948) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QSp3p-0003GQ-Rr for emacs-devel@gnu.org; Sat, 04 Jun 2011 07:30:35 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QSp3o-0004Ni-De for emacs-devel@gnu.org; Sat, 04 Jun 2011 07:30:33 -0400 Original-Received: from mtaout22.012.net.il ([80.179.55.172]:41269) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QSp3n-0004Ne-WD for emacs-devel@gnu.org; Sat, 04 Jun 2011 07:30:32 -0400 Original-Received: from conversion-daemon.a-mtaout22.012.net.il by a-mtaout22.012.net.il (HyperSendmail v2007.08) id <0LM900L00JSI9Q00@a-mtaout22.012.net.il> for emacs-devel@gnu.org; Sat, 04 Jun 2011 14:30:30 +0300 (IDT) Original-Received: from HOME-C4E4A596F7 ([84.229.223.140]) by a-mtaout22.012.net.il (HyperSendmail v2007.08) with ESMTPA id <0LM900LAEJYS7130@a-mtaout22.012.net.il>; Sat, 04 Jun 2011 14:30:30 +0300 (IDT) In-reply-to: <201106040034.15598.ruediger@c-plusplus.de> X-012-Sender: halo1@inter.net.il X-detected-operating-system: by eggs.gnu.org: Solaris 10 (beta) X-Received-From: 80.179.55.172 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:140171 Archived-At: > From: R=C3=BCdiger Sonderfeld > Date: Sat, 4 Jun 2011 00:34:15 +0200 >=20 > I wrote a patch to support watching for file system events. It curr= ently only works with inotify (Linux) but it could be extended to sup= port kqueue=20 > (*BSD, OS X) or Win32. But I wanted to get some feedback first. Wat= ching for filesystem events would be very useful for, e.g., dired or = magit's status=20 > view. Thanks. A few comments below. > +#include Why do you need this header? > +// Assoc list of files being watched. We don't use C++-style comments, because we don't require C9x compile= r to build Emacs. > +static Lisp_Object data; The name "data" is too generic. Use something more specific and descriptive, like inotify_watch_alist. > +static void=20 > +inotify_callback(int fd, void *_, int for_read) { What's that _ in the argument list? Also, the style of braces is not according to GNU coding standards. > + if(ioctl(fd, FIONREAD, &to_read) =3D=3D -1) > + report_file_error("ioctl(2) on inotify", Qnil); Error messages that are shown to the user should not be cryptic. Please use some text that would make sense to a user who is not an expert on inotify. I would think something like this would be appropriate: File watching feature (inotify) is not available > + char *const buffer =3D xmalloc(to_read); > + ssize_t const n =3D read(fd, buffer, to_read); Can't declare locals in the middle of code: this isn't C++. > + if(n < 0) > + report_file_error("read from inotify", Qnil); The text of this message also needs work, to make it understandable b= y mere mortals. > + size_t i =3D 0; C++/C9x again. > + while(i < (size_t)n) I think this cast, in addition to being ugly, is also unneeded, because you already verified above that n is positive. So `i' can be ssize_t as well, and the need for the cast is gone. > + call[1] =3D Fcar(Fcdr(callback)); /* path */ GNU coding standards frown on using "path" for anything except PATH-style lists of directories separated by colons. Use "file name" instead. > + /* TODO check how to handle UTF-8 in file names: > + make_string_from_bytes NCHARS vs. NBYTES */ Not make_string_from_bytes, but DECODE_FILE. > + size_t const len =3D strlen(ev->name); C9x again. > + /* if a directory is watched name contains the name > + of the file that was changed */ Comments should begin with a capital letter and end with a period and 2 blanks. > + if(ev->mask & IN_IGNORED) > + { > + /* Event was removed automatically: Drop it from dat= a list. */ > + message("File-watch: \"%s\" will be ignored", SSDATA= (call[1])); > + data =3D Fdelete(callback, data); > + } Do we really need this message? Consider outputting it only to the *Messages* buffer. > + if(ev->mask & IN_Q_OVERFLOW) > + message1("File watch: Inotify Queue Overflow!"); Same here. > + free(buffer); xfree, not free. > +Watching a directory is not recursive. CALLBACK receives the event= s as a list > +with each list element being a list containing information about a= n event. The > +first element is a flag symbol. If a directory is watched the seco= nd element is > +the name of the file that changed. If a file is moved from or to t= he directory > +the second element is either :from or :to and the third element is= the file > +name. Please leave two blanks after a period that ends a sentence. Also, I think it's best to show the forms of the non-primitive arguments to CALLBACK, rather than trying to describe them: a picture is worth a thousand words. Finally, the lines are too long. > A fourth element contains a numeric identifier (cookie) that ca= n be used > +to identify matching move operations if a file is moved inside the= directory. This part is extremely unclear. How about an example? > + add_read_fd(inotifyfd, &inotify_callback, &data); I don't see any code that does the corresponding delete_read_fd. > + uint32_t mask =3D 0; Why not `unsigned'? uint32_t is not part of C90, so it's less portable. > + int watchdesc =3D inotify_add_watch(inotifyfd, SSDATA(args[0]), = mask); Declaration in the middle of code. More importantly, you cannot pass to inotify_add_watch a string in it= s internal Emacs representation. You need to run it through ENCODE_FIL= E first. > + if(watchdesc =3D=3D -1) { > + report_file_error("Watching file", Qnil); Again, this error message text is not helpful. It should also mentio= n the file's name. > + (Lisp_Object path) > +{ > + if(inotifyfd =3D=3D uninitialized) > + return Qnil; > + CHECK_STRING(path); > + > + Lisp_Object x =3D data; > + Lisp_Object cell, path_data; `path' and `path_data' again go against the GNU coding standards wrt using "path". > + while(!NILP(x)) > + { > + cell =3D Fcar(x); > + x =3D Fcdr(x); > + path_data =3D Fcdr(cell); ??? Isn't `data' an alist? If so, why not use Fassq etc.? > + if(!NILP(path_data) && STRINGP(Fcar(path_data)) > + && Fstring_equal(Fcar(path_data), path) > + && NUMBERP(Fcar(cell))) > + { > + int const magicno =3D XINT(Fcar(cell)); > + data =3D Fdelete(cell, data); > + if(inotify_rm_watch(inotifyfd, magicno) =3D=3D -1) > + report_file_error("Unwatch path", Qnil); > + return Qt; > + } > + } I think this should call delete_read_fd when `data' becomes empty.