[PATCH v2] POSIX ACL support

[PATCH v2] POSIX ACL support

[Romain Francoise]

Hi,

Thanks to everyone who commented on the first version of this patch.
This is another iteration with the following changes:
- C-level functions renamed to `file-acl' and `set-file-acl' in hope that
  they can be used with non-POSIX ACL implementations; conversely the
  parts of the implementation that are POSIX ACL specific are now inside
  HAVE_POSIX_ACL
- the additional argument to `copy-file' has been dropped, the existing
  argument `preserve-selinux-context' has been turned into a more generic
  `preserve-extended-attributes' argument which does both SELinux and ACL
- new functions `file-extended-attributes' and `set-file-extended-attributes'
  have been added to make the implementation cleaner in files.el, handling
  both SELinux context and ACL entries (do they need to be documented in
  the Elisp manual?)

Tested again on GNU/Linux with libacl, and FreeBSD.

Comments welcome!

Thanks,

diff --git a/configure.ac b/configure.ac
index 085ca83..7d533fa 100644
--- a/configure.ac
+++ b/configure.ac
@@ -184,6 +184,7 @@ OPTION_DEFAULT_ON([dbus],[don't compile with D-Bus support])
 OPTION_DEFAULT_ON([gconf],[don't compile with GConf support])
 OPTION_DEFAULT_ON([gsettings],[don't compile with GSettings support])
 OPTION_DEFAULT_ON([selinux],[don't compile with SELinux support])
+OPTION_DEFAULT_ON([acl],[don't compile with ACL support])
 OPTION_DEFAULT_ON([gnutls],[don't use -lgnutls for SSL/TLS support])
 
 ## For the times when you want to build Emacs but don't have
@@ -2170,6 +2171,23 @@ fi
 AC_SUBST(LIBGNUTLS_LIBS)
 AC_SUBST(LIBGNUTLS_CFLAGS)
 
+dnl POSIX ACL support: provided by libacl on GNU/Linux, by libc on FreeBSD.
+HAVE_POSIX_ACL=no
+LIBACL_LIBS=
+if test "${with_acl}" = "yes"; then
+  AC_CHECK_LIB([acl], [acl_set_file], HAVE_POSIX_ACL=yes, HAVE_POSIX_ACL=no)
+  if test "$HAVE_POSIX_ACL" = yes; then
+    AC_DEFINE(HAVE_POSIX_ACL, 1, [Define to 1 if using POSIX ACL support.])
+    LIBACL_LIBS=-lacl
+  else
+    AC_CHECK_FUNC(acl_set_file, HAVE_POSIX_ACL=yes, HAVE_POSIX_ACL=no)
+    if test "$HAVE_POSIX_ACL" = yes; then
+      AC_DEFINE(HAVE_POSIX_ACL, 1, [Define to 1 if using POSIX ACL support.])
+    fi
+ fi
+fi
+AC_SUBST(LIBACL_LIBS)
+
 dnl Do not put whitespace before the #include statements below.
 dnl Older compilers (eg sunos4 cc) choke on it.
 HAVE_XAW3D=no
diff --git a/doc/lispref/files.texi b/doc/lispref/files.texi
index a5710c7..ce2ebee 100644
--- a/doc/lispref/files.texi
+++ b/doc/lispref/files.texi
@@ -1350,6 +1350,22 @@ not support SELinux, or if Emacs was not compiled with SELinux
 support, then the return value is @code{(nil nil nil nil)}.
 @end defun
 
+@cindex ACL entries
+  If Emacs has been compiled with ACL support, you can use the function
+@code{file-acl} to retrieve a file's ACL entries.  The format is
+platform-specific; on GNU/Linux and BSD, Emacs uses the POSIX ACL
+interface.  For the function @code{set-file-acl}, see @ref{Changing
+Files}.
+
+@defun file-acl filename
+This function returns the ACL entries of the file @var{filename}.
+The return value is a string containing the textual representation of
+the ACL entries.
+
+If the file does not exist or is inaccessible, of if Emacs was not
+compiled with ACL support, then the return value is @code{nil}.
+@end defun
+
 @node Locating Files
 @subsection How to Locate Files in Standard Places
 @cindex locate file in path
@@ -1539,9 +1555,10 @@ non-@code{nil}, we attempt to copy the user and group ownership of the
 file.  This works only on some operating systems, and only if you have
 the correct permissions to do so.
 
-If the optional argument @var{preserve-selinux} is non-@code{nil}, and
-Emacs has been compiled with SELinux support, this function attempts
-to copy the file's SELinux context (@pxref{File Attributes}).
+If the optional argument @var{preserve-extended-attributes} is
+non-@code{nil}, and Emacs has been built with the appropriate support,
+this function attempts to copy the file's extended attributes, such as
+its SELinux context and ACL entries (@pxref{File Attributes}).
 @end deffn
 
 @deffn Command make-symbolic-link filename newname  &optional ok-if-exists
@@ -1682,6 +1699,14 @@ nothing if SELinux is disabled, or if Emacs was compiled without
 SELinux support.
 @end defun
 
+@defun set-file-acl filename acl-string
+This function sets the ACL entries of the file @var{filename} to
+@var{acl-string}.  @xref{File Attributes}, for a brief description of
+ACLs.  The @var{acl-string} argument should be a string containing the
+textual representation of the desired ACL entries.  The function does
+nothing if Emacs was compiled without ACL support.
+@end defun
+
 @node File Names
 @section File Names
 @cindex file names
diff --git a/lisp/files.el b/lisp/files.el
index 1bb140c..0bcdc7d 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -3878,6 +3878,27 @@ Interactively, confirmation is required unless you supply a prefix argument."
   ;; the one at the old location.
   (vc-find-file-hook))
 \f
+(defun file-extended-attributes (filename)
+  "Return an alist of extended attributes of file FILENAME.
+
+Extended attributes are platform-specific metadata about the file,
+such as SELinux context, list of ACL entries, etc."
+  `((acl . ,(file-acl filename))
+    (selinux-context . ,(file-selinux-context filename))))
+
+(defun set-file-extended-attributes (filename attributes)
+  "Set extended attributes of file FILENAME to ATTRIBUTES.
+
+ATTRIBUTES must be an alist of file attributes as returned by
+`file-extended-attributes'."
+  (dolist (elt attributes)
+    (let ((attr (car elt))
+	  (val (cdr elt)))
+      (cond ((eq attr 'acl)
+	     (set-file-acl filename val))
+	    ((eq attr 'selinux-context)
+	     (set-file-selinux-context filename val))))))
+\f
 (defun backup-buffer ()
   "Make a backup of the disk file visited by the current buffer, if appropriate.
 This is normally done before saving the buffer the first time.
@@ -3887,13 +3908,14 @@ variable `make-backup-files'.  If it's done by renaming, then the file is
 no longer accessible under its old name.
 
 The value is non-nil after a backup was made by renaming.
-It has the form (MODES SELINUXCONTEXT BACKUPNAME).
+It has the form (MODES EXTENDED-ATTRIBUTES BACKUPNAME).
 MODES is the result of `file-modes' on the original
 file; this means that the caller, after saving the buffer, should change
 the modes of the new file to agree with the old modes.
-SELINUXCONTEXT is the result of `file-selinux-context' on the original
-file; this means that the caller, after saving the buffer, should change
-the SELinux context of the new file to agree with the old context.
+EXTENDED-ATTRIBUTES is the result of `file-extended-attributes'
+on the original file; this means that the caller, after saving
+the buffer, should change the extended attributes of the new file
+to agree with the old attributes.
 BACKUPNAME is the backup file name, which is the old file renamed."
   (if (and make-backup-files (not backup-inhibited)
 	   (not buffer-backed-up)
@@ -3922,7 +3944,8 @@ BACKUPNAME is the backup file name, which is the old file renamed."
 				(y-or-n-p (format "Delete excess backup versions of %s? "
 						  real-file-name)))))
 		      (modes (file-modes buffer-file-name))
-		      (context (file-selinux-context buffer-file-name)))
+		      (extended-attributes
+		       (file-extended-attributes buffer-file-name)))
 		  ;; Actually write the back up file.
 		  (condition-case ()
 		      (if (or file-precious-flag
@@ -3942,10 +3965,12 @@ BACKUPNAME is the backup file name, which is the old file renamed."
 						   (<= (nth 2 attr) backup-by-copying-when-privileged-mismatch)))
 					  (or (nth 9 attr)
 					      (not (file-ownership-preserved-p real-file-name)))))))
-			  (backup-buffer-copy real-file-name backupname modes context)
+			  (backup-buffer-copy real-file-name backupname
+					      modes extended-attributes)
 			;; rename-file should delete old backup.
 			(rename-file real-file-name backupname t)
-			(setq setmodes (list modes context backupname)))
+			(setq setmodes (list modes extended-attributes
+					     backupname)))
 		    (file-error
 		     ;; If trouble writing the backup, write it in
 		     ;; .emacs.d/%backup%.
@@ -3953,7 +3978,8 @@ BACKUPNAME is the backup file name, which is the old file renamed."
 		     (message "Cannot write backup file; backing up in %s"
 			      backupname)
 		     (sleep-for 1)
-		     (backup-buffer-copy real-file-name backupname modes context)))
+		     (backup-buffer-copy real-file-name backupname
+					 modes extended-attributes)))
 		  (setq buffer-backed-up t)
 		  ;; Now delete the old versions, if desired.
 		  (if delete-old-versions
@@ -3965,7 +3991,7 @@ BACKUPNAME is the backup file name, which is the old file renamed."
 		  setmodes)
 	    (file-error nil))))))
 
-(defun backup-buffer-copy (from-name to-name modes context)
+(defun backup-buffer-copy (from-name to-name modes extended-attributes)
   (let ((umask (default-file-modes)))
     (unwind-protect
 	(progn
@@ -3993,8 +4019,8 @@ BACKUPNAME is the backup file name, which is the old file renamed."
       (set-default-file-modes umask)))
   (and modes
        (set-file-modes to-name (logand modes #o1777)))
-  (and context
-       (set-file-selinux-context to-name context)))
+  (and extended-attributes
+       (set-file-extended-attributes to-name extended-attributes)))
 
 (defvar file-name-version-regexp
   "\\(?:~\\|\\.~[-[:alnum:]:#@^._]+\\(?:~[[:digit:]]+\\)?~\\)"
@@ -4570,7 +4596,8 @@ Before and after saving the buffer, this function runs
 		(condition-case ()
 		    (progn
 		      (set-file-modes buffer-file-name (car setmodes))
-		      (set-file-selinux-context buffer-file-name (nth 1 setmodes)))
+		      (set-file-extended-attributes buffer-file-name
+						    (nth 1 setmodes)))
 		  (error nil))))
 	  ;; If the auto-save file was recent before this command,
 	  ;; delete it now.
@@ -4583,7 +4610,8 @@ Before and after saving the buffer, this function runs
 ;; This does the "real job" of writing a buffer into its visited file
 ;; and making a backup file.  This is what is normally done
 ;; but inhibited if one of write-file-functions returns non-nil.
-;; It returns a value (MODES SELINUXCONTEXT BACKUPNAME), like backup-buffer.
+;; It returns a value (MODES EXTENDED-ATTRIBUTES BACKUPNAME), like
+;; backup-buffer.
 (defun basic-save-buffer-1 ()
   (prog1
       (if save-buffer-coding-system
@@ -4595,7 +4623,8 @@ Before and after saving the buffer, this function runs
       (setq buffer-file-coding-system-explicit
 	    (cons last-coding-system-used nil)))))
 
-;; This returns a value (MODES SELINUXCONTEXT BACKUPNAME), like backup-buffer.
+;; This returns a value (MODES EXTENDED-ATTRIBUTES BACKUPNAME), like
+;; backup-buffer.
 (defun basic-save-buffer-2 ()
   (let (tempsetmodes setmodes)
     (if (not (file-writable-p buffer-file-name))
@@ -4670,7 +4699,7 @@ Before and after saving the buffer, this function runs
 	    (setq setmodes (or setmodes
  			       (list (or (file-modes buffer-file-name)
 					 (logand ?\666 umask))
-				     (file-selinux-context buffer-file-name)
+				     (file-extended-attributes buffer-file-name)
 				     buffer-file-name)))
 	    ;; We succeeded in writing the temp file,
 	    ;; so rename it.
@@ -4682,10 +4711,10 @@ Before and after saving the buffer, this function runs
 	(cond ((and tempsetmodes (not setmodes))
 	       ;; Change the mode back, after writing.
 	       (setq setmodes (list (file-modes buffer-file-name)
-				    (file-selinux-context buffer-file-name)
+				    (file-extended-attributes buffer-file-name)
 				    buffer-file-name))
 	       (set-file-modes buffer-file-name (logior (car setmodes) 128))
-	       (set-file-selinux-context buffer-file-name (nth 1 setmodes)))))
+	       (set-file-extended-attributes buffer-file-name (nth 1 setmodes)))))
 	(let (success)
 	  (unwind-protect
 	      (progn
diff --git a/src/Makefile.in b/src/Makefile.in
index d034ad0..060d62a 100644
--- a/src/Makefile.in
+++ b/src/Makefile.in
@@ -283,6 +283,8 @@ LIBSELINUX_LIBS = @LIBSELINUX_LIBS@
 LIBGNUTLS_LIBS = @LIBGNUTLS_LIBS@
 LIBGNUTLS_CFLAGS = @LIBGNUTLS_CFLAGS@
 
+LIBACL_LIBS = @LIBACL_LIBS@
+
 LIB_PTHREAD_SIGMASK = @LIB_PTHREAD_SIGMASK@
 
 INTERVALS_H = dispextern.h intervals.h composite.h
@@ -398,7 +400,7 @@ LIBES = $(LIBS) $(W32_LIBS) $(LIBX_BASE) $(LIBIMAGE) \
    $(LIBXML2_LIBS) $(LIBGPM) $(LIBRESOLV) $(LIBS_SYSTEM) \
    $(LIBS_TERMCAP) $(GETLOADAVG_LIBS) $(SETTINGS_LIBS) $(LIBSELINUX_LIBS) \
    $(FREETYPE_LIBS) $(FONTCONFIG_LIBS) $(LIBOTF_LIBS) $(M17N_FLT_LIBS) \
-   $(LIBGNUTLS_LIBS) $(LIB_PTHREAD) $(LIB_PTHREAD_SIGMASK) \
+   $(LIBACL_LIBS) $(LIBGNUTLS_LIBS) $(LIB_PTHREAD) $(LIB_PTHREAD_SIGMASK) \
    $(LIB_GCC) $(LIB_MATH) $(LIB_STANDARD) $(LIB_GCC)
 
 all: emacs$(EXEEXT) $(OTHER_FILES)
diff --git a/src/fileio.c b/src/fileio.c
index 48dbf20..9b5d96a 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -36,6 +36,10 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 #include <selinux/context.h>
 #endif
 
+#ifdef HAVE_POSIX_ACL
+#include <sys/acl.h>
+#endif
+
 #include <c-ctype.h>
 
 #include "lisp.h"
@@ -236,6 +240,8 @@ static Lisp_Object Qset_file_modes;
 static Lisp_Object Qset_file_times;
 static Lisp_Object Qfile_selinux_context;
 static Lisp_Object Qset_file_selinux_context;
+static Lisp_Object Qfile_acl;
+static Lisp_Object Qset_file_acl;
 static Lisp_Object Qfile_newer_than_file_p;
 Lisp_Object Qinsert_file_contents;
 Lisp_Object Qwrite_region;
@@ -1881,9 +1887,10 @@ A prefix arg makes KEEP-TIME non-nil.
 If PRESERVE-UID-GID is non-nil, we try to transfer the
 uid and gid of FILE to NEWNAME.
 
-If PRESERVE-SELINUX-CONTEXT is non-nil and SELinux is enabled
-on the system, we copy the SELinux context of FILE to NEWNAME.  */)
-  (Lisp_Object file, Lisp_Object newname, Lisp_Object ok_if_already_exists, Lisp_Object keep_time, Lisp_Object preserve_uid_gid, Lisp_Object preserve_selinux_context)
+If PRESERVE-EXTENDED-ATTRIBUTES is non-nil, we try to copy additional
+attributes of FILE to NEWNAME, such as its SELinux context and ACL
+entries (depending on how Emacs was built).  */)
+  (Lisp_Object file, Lisp_Object newname, Lisp_Object ok_if_already_exists, Lisp_Object keep_time, Lisp_Object preserve_uid_gid, Lisp_Object preserve_extended_attributes)
 {
   int ifd, ofd;
   int n;
@@ -1898,6 +1905,9 @@ on the system, we copy the SELinux context of FILE to NEWNAME.  */)
   security_context_t con;
   int conlength = 0;
 #endif
+#ifdef HAVE_POSIX_ACL
+  acl_t acl = NULL;
+#endif
 
   encoded_file = encoded_newname = Qnil;
   GCPRO4 (file, newname, encoded_file, encoded_newname);
@@ -1920,7 +1930,7 @@ on the system, we copy the SELinux context of FILE to NEWNAME.  */)
   if (!NILP (handler))
     RETURN_UNGCPRO (call7 (handler, Qcopy_file, file, newname,
 			   ok_if_already_exists, keep_time, preserve_uid_gid,
-			   preserve_selinux_context));
+			   preserve_extended_attributes));
 
   encoded_file = ENCODE_FILE (file);
   encoded_newname = ENCODE_FILE (newname);
@@ -1974,14 +1984,23 @@ on the system, we copy the SELinux context of FILE to NEWNAME.  */)
      copyable by us. */
   input_file_statable_p = (fstat (ifd, &st) >= 0);
 
-#if HAVE_LIBSELINUX
-  if (!NILP (preserve_selinux_context) && is_selinux_enabled ())
+  if (!NILP (preserve_extended_attributes))
     {
-      conlength = fgetfilecon (ifd, &con);
-      if (conlength == -1)
-	report_file_error ("Doing fgetfilecon", Fcons (file, Qnil));
-    }
+#if HAVE_LIBSELINUX
+      if (is_selinux_enabled ())
+	{
+	  conlength = fgetfilecon (ifd, &con);
+	  if (conlength == -1)
+	    report_file_error ("Doing fgetfilecon", Fcons (file, Qnil));
+	}
+#endif
+
+#ifdef HAVE_POSIX_ACL
+      acl = acl_get_fd (ifd);
+      if (acl == NULL && errno != ENOTSUP)
+	report_file_error ("Getting ACL", Fcons (file, Qnil));
 #endif
+    }
 
   if (out_st.st_mode != 0
       && st.st_dev == out_st.st_dev && st.st_ino == out_st.st_ino)
@@ -2071,6 +2090,17 @@ on the system, we copy the SELinux context of FILE to NEWNAME.  */)
     }
 #endif
 
+#ifdef HAVE_POSIX_ACL
+  if (acl != NULL)
+    {
+      bool fail = acl_set_fd (ofd, acl) != 0;
+      if (fail && errno != ENOTSUP)
+	report_file_error ("Setting ACL", Fcons (newname, Qnil));
+
+      acl_free (acl);
+    }
+#endif
+
   if (input_file_statable_p)
     {
       if (!NILP (keep_time))
@@ -2960,6 +2990,104 @@ compiled with SELinux support.  */)
   return Qnil;
 }
 \f
+DEFUN ("file-acl", Ffile_acl, Sfile_acl, 1, 1, 0,
+       doc: /* Return ACL entries of file named FILENAME, as a string.
+Return nil if file does not exist or is not accessible, or if Emacs was
+not compiled with ACL support.  */)
+  (Lisp_Object filename)
+{
+  Lisp_Object absname;
+  Lisp_Object handler;
+#ifdef HAVE_POSIX_ACL
+  acl_t acl;
+  Lisp_Object acl_string;
+  char *str;
+#endif
+
+  absname = expand_and_dir_to_file (filename,
+				    BVAR (current_buffer, directory));
+
+  /* If the file name has special constructs in it,
+     call the corresponding file handler.  */
+  handler = Ffind_file_name_handler (absname, Qfile_acl);
+  if (!NILP (handler))
+    return call2 (handler, Qfile_acl, absname);
+
+#ifdef HAVE_POSIX_ACL
+  absname = ENCODE_FILE (absname);
+
+  acl = acl_get_file (SSDATA (absname), ACL_TYPE_ACCESS);
+  if (acl == NULL)
+    return Qnil;
+
+  str = acl_to_text (acl, NULL);
+  if (str == NULL)
+    {
+      acl_free (acl);
+      return Qnil;
+    }
+
+  acl_string = build_string (str);
+  acl_free (str);
+  acl_free (acl);
+
+  return acl_string;
+#endif
+
+  return Qnil;
+}
+
+DEFUN ("set-file-acl", Fset_file_acl, Sset_file_acl,
+       2, 2, 0,
+       doc: /* Set ACL of file named FILENAME to ACL-STRING.
+ACL-STRING should contain the textual representation of the ACL
+entries in a format suitable for the platform.
+
+This function does nothing if Emacs was not compiled with ACL
+support.  */)
+  (Lisp_Object filename, Lisp_Object acl_string)
+{
+  Lisp_Object absname;
+  Lisp_Object handler;
+#ifdef HAVE_POSIX_ACL
+  Lisp_Object encoded_absname;
+  acl_t acl;
+  bool fail;
+#endif
+
+  absname = Fexpand_file_name (filename, BVAR (current_buffer, directory));
+
+  /* If the file name has special constructs in it,
+     call the corresponding file handler.  */
+  handler = Ffind_file_name_handler (absname, Qset_file_acl);
+  if (!NILP (handler))
+    return call3 (handler, Qset_file_acl, absname, acl_string);
+
+#ifdef HAVE_POSIX_ACL
+  if (STRINGP (acl_string))
+    {
+      acl = acl_from_text (SSDATA (acl_string));
+      if (acl == NULL)
+	{
+	  report_file_error ("Converting ACL", Fcons (absname, Qnil));
+	  return Qnil;
+	}
+
+      encoded_absname = ENCODE_FILE (absname);
+
+      fail = (acl_set_file (SSDATA (encoded_absname), ACL_TYPE_ACCESS,
+			    acl)
+	      != 0);
+      if (fail && errno != ENOTSUP)
+	report_file_error ("Setting ACL", Fcons (absname, Qnil));
+
+      acl_free (acl);
+    }
+#endif
+
+  return Qnil;
+}
+\f
 DEFUN ("file-modes", Ffile_modes, Sfile_modes, 1, 1, 0,
        doc: /* Return mode bits of file named FILENAME, as an integer.
 Return nil, if file does not exist or is not accessible.  */)
@@ -5838,6 +5966,8 @@ This includes interactive calls to `delete-file' and
   defsubr (&Sset_file_modes);
   defsubr (&Sset_file_times);
   defsubr (&Sfile_selinux_context);
+  defsubr (&Sfile_acl);
+  defsubr (&Sset_file_acl);
   defsubr (&Sset_file_selinux_context);
   defsubr (&Sset_default_file_modes);
   defsubr (&Sdefault_file_modes);

Re: [PATCH v2] POSIX ACL support

[Eli Zaretskii]

> From: Romain Francoise <romain@orebokech.com>
> Date: Sun, 02 Dec 2012 21:04:19 +0100
> 
> This is another iteration with the following changes:
> - C-level functions renamed to `file-acl' and `set-file-acl' in hope that
>   they can be used with non-POSIX ACL implementations; conversely the
>   parts of the implementation that are POSIX ACL specific are now inside
>   HAVE_POSIX_ACL
> - the additional argument to `copy-file' has been dropped, the existing
>   argument `preserve-selinux-context' has been turned into a more generic
>   `preserve-extended-attributes' argument which does both SELinux and ACL
> - new functions `file-extended-attributes' and `set-file-extended-attributes'
>   have been added to make the implementation cleaner in files.el, handling
>   both SELinux context and ACL entries (do they need to be documented in
>   the Elisp manual?)
> 
> Tested again on GNU/Linux with libacl, and FreeBSD.
> 
> Comments welcome!

Thanks.

> +@cindex ACL entries

Suggest an additional entry "@cindex access control list" here.

> +  If Emacs has been compiled with ACL support, you can use the function
> +@code{file-acl} to retrieve a file's ACL entries.  The format is
> +platform-specific; on GNU/Linux and BSD, Emacs uses the POSIX ACL
> +interface.  For the function @code{set-file-acl}, see @ref{Changing
> +Files}.

We don't mention "ACL" or explain it anywhere else in the manual.  So
this acronym should be explained here, and its first use should use
@dfn, as we do with any new terminology.

> +DEFUN ("file-acl", Ffile_acl, Sfile_acl, 1, 1, 0,
> +       doc: /* Return ACL entries of file named FILENAME, as a string.
> +Return nil if file does not exist or is not accessible, or if Emacs was
> +not compiled with ACL support.  */)

The last sentence of the doc string is inaccurate, because remote
files are supported even if Emacs was not built with ACL support.

> +DEFUN ("set-file-acl", Fset_file_acl, Sset_file_acl,
> +       2, 2, 0,
> +       doc: /* Set ACL of file named FILENAME to ACL-STRING.
> +ACL-STRING should contain the textual representation of the ACL
> +entries in a format suitable for the platform.
> +
> +This function does nothing if Emacs was not compiled with ACL
> +support.  */)

Same here.

Re: [PATCH v2] POSIX ACL support

[Glenn Morris]

Romain Francoise wrote:

> +interface.  For the function @code{set-file-acl}, see @ref{Changing
> +Files}.

I'd use:

 For the function @code{set-file-acl}, @pxref{Changing Files}.

> +The return value is a string containing the textual representation of
> +the ACL entries.

Maybe give an example?

Apart from those trivial comments, just needs a NEWS entry.
Otherwise looks good to me...

Re: [PATCH v2] POSIX ACL support

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Date: Tue, 04 Dec 2012 21:28:04 -0500
> Cc: emacs-devel@gnu.org
> 
> Romain Francoise wrote:
> 
> > +interface.  For the function @code{set-file-acl}, see @ref{Changing
> > +Files}.
> 
> I'd use:
> 
>  For the function @code{set-file-acl}, @pxref{Changing Files}.

While @pxref will work, it is better not to use it except in
parentheses.  I suggest "see @ref" instead.

Re: [PATCH v2] POSIX ACL support

[Glenn Morris]

Eli Zaretskii wrote:

> While @pxref will work, it is better not to use it except in
> parentheses. 

FWIW, I disagree (and the Texinfo manual doesn't seem to agree with you
either AFAICS).

Re: [PATCH v2] POSIX ACL support

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Cc: romain@orebokech.com,  emacs-devel@gnu.org
> Date: Tue, 04 Dec 2012 23:09:35 -0500
> 
> Eli Zaretskii wrote:
> 
> > While @pxref will work, it is better not to use it except in
> > parentheses. 
> 
> FWIW, I disagree

With which part?  I didn't say @pxref will not work.  If you think it
is better than "see @ref", please tell why.

> (and the Texinfo manual doesn't seem to agree with you
> either AFAICS).

That depends on the version of Texinfo.  The one for version 4.8
(which we support) still says

  8.7 `@pxref'
  ============

  The parenthetical reference command, `@pxref', is nearly the same as
  `@xref', but you use it _only_ inside parentheses and you do _not_ type
  a comma or period (or anything else) after the command's closing brace.

I don't understand why later versions now say

    In general, it is best to use `@ref' only when you need some word
  other than "see" to precede the reference.  When "see" (or "See") is
  ok, `@xref' and `@pxref' are preferable.

But they still say

  8.7 `@pxref'
  ============

  The parenthetical reference command, `@pxref', is nearly the same as
  `@xref', but it is best used at the end of a sentence or before a
  closing parenthesis.

IOW, the "parenthetical" part is kept, so saying it is preferable
outside of parens makes little sense to me.  (It makes even less sense
to work against my muscle memory, but that's another story.)

The problem is that people who use Texinfo only very rarely tend to
become confused about the precise use cases covered by each of the
*ref directives, and since 'p' stands for "parenthetical", telling
them to use it only in parens helps them remember.  And since both
forms work in this particular case, I think @pxref is better.

Re: [PATCH v2] POSIX ACL support

[Glenn Morris]

Eli Zaretskii wrote:

[from the Texinfo manual]

>     In general, it is best to use `@ref' only when you need some word
>   other than "see" to precede the reference.  When "see" (or "See") is
>   ok, `@xref' and `@pxref' are preferable.
[...]
>   The parenthetical reference command, `@pxref', is nearly the same as
>   `@xref', but it is best used at the end of a sentence or before a
>   closing parenthesis.

It's those two pieces of the manual (I usually look at the online
version), plus the fact that

https://www.gnu.org/software/texinfo/manual/texinfo/html_node/pxref.html

has a specific example of this very form:

   For more information, @pxref{More}.


But I don't think it's very important.

Re: [PATCH v2] POSIX ACL support

[Romain Francoise]

Hi Eli,

Eli Zaretskii <eliz@gnu.org> writes:

> Suggest an additional entry "@cindex access control list" here.

Ok.

> We don't mention "ACL" or explain it anywhere else in the manual.  So
> this acronym should be explained here, and its first use should use
> @dfn, as we do with any new terminology.

Makes sense. I'll try to come up with a description general enough to
apply to all potential platforms.

>> +DEFUN ("file-acl", Ffile_acl, Sfile_acl, 1, 1, 0,
>> +       doc: /* Return ACL entries of file named FILENAME, as a string.
>> +Return nil if file does not exist or is not accessible, or if Emacs was
>> +not compiled with ACL support.  */)

> The last sentence of the doc string is inaccurate, because remote
> files are supported even if Emacs was not built with ACL support.

Yes... if there's an implementation of file-acl for the remote handler.
This bit was inherited from the SELinux functions, which have a similar
docstring. I'm not sure how to phrase it better, though.

Thanks!

Re: [PATCH v2] POSIX ACL support

[Romain Francoise]

Hi Glenn,

Glenn Morris <rgm@gnu.org> writes:

>> +The return value is a string containing the textual representation of
>> +the ACL entries.

> Maybe give an example?

Ok.

> Apart from those trivial comments, just needs a NEWS entry.
> Otherwise looks good to me...

Thanks for the review!

I'll add the NEWS and ChangeLog entries and then I can install it myself
to the trunk, but I think I need to get the go-ahead from Stefan and/or
Chong?

Re: [PATCH v2] POSIX ACL support

[Stefan Monnier]

> I'll add the NEWS and ChangeLog entries and then I can install it myself
> to the trunk, but I think I need to get the go-ahead from Stefan and/or
> Chong?

Go ahead!


        Stefan

Re: [PATCH v2] POSIX ACL support

[Michael Albinus]

Romain Francoise <romain@orebokech.com> writes:

> Hi Eli,

Hi,

>>> +DEFUN ("file-acl", Ffile_acl, Sfile_acl, 1, 1, 0,
>>> +       doc: /* Return ACL entries of file named FILENAME, as a string.
>>> +Return nil if file does not exist or is not accessible, or if Emacs was
>>> +not compiled with ACL support.  */)
>
>> The last sentence of the doc string is inaccurate, because remote
>> files are supported even if Emacs was not built with ACL support.
>
> Yes... if there's an implementation of file-acl for the remote handler.
> This bit was inherited from the SELinux functions, which have a similar
> docstring. I'm not sure how to phrase it better, though.

| Return nil if file does not exist or is not accessible, or if Emacs is
| not able to determine the ACL entries.  The latter happens, if Emacs was
| not compiled with ACL support, or a remote file handler returns nil
| ACL entries.  */)

Similar wording could be applied to file-selinux-context and
set-file-selinux-context.

Once the patch is installed in the trunk (i.e., the interface is
stabilized), I will try to implement corresponding file name handlers.

> Thanks!

Best regards, Michael.

Re: [PATCH v2] POSIX ACL support

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Fri, 07 Dec 2012 09:19:09 +0100
> 
> Romain Francoise <romain@orebokech.com> writes:
> 
> > Hi Eli,
> 
> Hi,
> 
> >>> +DEFUN ("file-acl", Ffile_acl, Sfile_acl, 1, 1, 0,
> >>> +       doc: /* Return ACL entries of file named FILENAME, as a string.
> >>> +Return nil if file does not exist or is not accessible, or if Emacs was
> >>> +not compiled with ACL support.  */)
> >
> >> The last sentence of the doc string is inaccurate, because remote
> >> files are supported even if Emacs was not built with ACL support.
> >
> > Yes... if there's an implementation of file-acl for the remote handler.
> > This bit was inherited from the SELinux functions, which have a similar
> > docstring. I'm not sure how to phrase it better, though.
> 
> | Return nil if file does not exist or is not accessible, or if Emacs is
> | not able to determine the ACL entries.  The latter happens, if Emacs was
> | not compiled with ACL support, or a remote file handler returns nil
> | ACL entries.  */)

My take of the last sentence is below:

  The latter can happen for local files if Emacs was not compiled with
  ACL support, or for remote files if the file handler returns nil for
  the file's ACL entries.