GNU bug report logs - #14295
Support copy-file ACLs for Solaris etc.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 14295 in the body.
You can then email your comments to 14295 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: bug-gnu-emacs <at> gnu.org
Cc: Eli Zaretskii <eliz <at> gnu.org>
Subject: Support copy-file ACLs for Solaris etc.
Date: Sat, 27 Apr 2013 20:33:19 -0700

[Message part 1 (text/plain, inline)]

Tags: patch

Recently copy-file was augmented to add support for
copying ACLs on abandoned-POSIX-style hosts.  Here's
an additional patch to add support for copying ACLs on
Solaris, HP-UX, etc.  Basically, the idea is to use
Gnulib's support for ACLs.  I've tested this on
GNU/Linux and Solaris, but not on Microsoft platforms,
and am CC'ing this to Eli as a heads-up for that.

This changes the 'configure' option from --without-acl to
--disable-acl if one wishes to disable ACL support when
configuring Emacs; this is the option spelling that other GNU
packages use.

[acl.txt (text/plain, attachment)]

Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: bug-gnu-emacs <at> gnu.org
Subject: Re: Support copy-file ACLs for Solaris etc.
Date: Sun, 28 Apr 2013 20:16:29 +0300

> Date: Sat, 27 Apr 2013 20:33:19 -0700
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> CC: Eli Zaretskii <eliz <at> gnu.org>
> 
> Recently copy-file was augmented to add support for
> copying ACLs on abandoned-POSIX-style hosts.  Here's
> an additional patch to add support for copying ACLs on
> Solaris, HP-UX, etc.  Basically, the idea is to use
> Gnulib's support for ACLs.  I've tested this on
> GNU/Linux and Solaris, but not on Microsoft platforms,
> and am CC'ing this to Eli as a heads-up for that.

Thanks; see a few comments below.

> This changes the 'configure' option from --without-acl to
> --disable-acl if one wishes to disable ACL support when
> configuring Emacs; this is the option spelling that other GNU
> packages use.

How hard would it to support both?

> +bool
> +acl_errno_valid (int errnum)
> +{
> +  /* Recognize some common errors such as from an NFS mount that does
> +     not support ACLs, even when local drives do.  */
> +  switch (errnum)
> +    {
> +    case EBUSY: return false;
> +    case EINVAL: return false;
> +#if defined __APPLE__ && defined __MACH__
> +    case ENOENT: return false;
> +#endif
> +    case ENOSYS: return false;
> +#if defined ENOTSUP && ENOTSUP != EOPNOTSUPP
> +    case ENOTSUP: return false;
> +#endif
> +    case EOPNOTSUPP: return false;
> +    default: return true;
> +    }
> +}

This uses EOPNOTSUPP without #ifdef guards; is that universally
available?

>  #ifdef WINDOWSNT
> -  if (!NILP (preserve_extended_attributes))
> -    {
> -#ifdef HAVE_POSIX_ACL
> -      acl = acl_get_file (SDATA (encoded_file), ACL_TYPE_ACCESS);
> -      if (acl == NULL && !ACL_NOT_WELL_SUPPORTED (errno))
> -	report_file_error ("Getting ACL", Fcons (file, Qnil));
> -#endif
> -    }
>    if (!CopyFile (SDATA (encoded_file),
>  		 SDATA (encoded_newname),
>  		 FALSE))
> @@ -2069,17 +2040,6 @@
>        /* Restore original attributes.  */
>        SetFileAttributes (filename, attributes);
>      }
> -#ifdef HAVE_POSIX_ACL
> -  if (acl != NULL)
> -    {
> -      bool fail =
> -	acl_set_file (SDATA (encoded_newname), ACL_TYPE_ACCESS, acl) != 0;
> -      if (fail && !ACL_NOT_WELL_SUPPORTED (errno))
> -	report_file_error ("Setting ACL", Fcons (newname, Qnil));
> -
> -      acl_free (acl);
> -    }
> -#endif
>  #else /* not WINDOWSNT */
>    immediate_quit = 1;
>    ifd = emacs_open (SSDATA (encoded_file), O_RDONLY, 0);
> @@ -2103,12 +2063,6 @@
>  	    report_file_error ("Doing fgetfilecon", Fcons (file, Qnil));
>  	}
>  #endif
> -
> -#ifdef HAVE_POSIX_ACL
> -      acl = acl_get_fd (ifd);
> -      if (acl == NULL && !ACL_NOT_WELL_SUPPORTED (errno))
> -	report_file_error ("Getting ACL", Fcons (file, Qnil));
> -#endif
>      }
>  
>    if (out_st.st_mode != 0
> @@ -2156,7 +2110,7 @@
>    immediate_quit = 0;
>  
>  #ifndef MSDOS
> -  /* Preserve the original file modes, and if requested, also its
> +  /* Preserve the original file permissions, and if requested, also its
>       owner and group.  */
>    {
>      mode_t mode_mask = 07777;
> @@ -2173,8 +2127,16 @@
>  	      mode_mask |= 02000;
>  	  }
>        }
> -    if (fchmod (ofd, st.st_mode & mode_mask) != 0)
> -      report_file_error ("Doing chmod", Fcons (newname, Qnil));
> +
> +    switch (!NILP (preserve_extended_attributes)
> +	    ? qcopy_acl (SSDATA (encoded_file), ifd,
> +			 SSDATA (encoded_newname), ofd,
> +			 st.st_mode & mode_mask)
> +	    : fchmod (ofd, st.st_mode & mode_mask))
> +      {
> +      case -2: report_file_error ("Copying permissions from", list1 (file));
> +      case -1: report_file_error ("Copying permissions to", list1 (newname));
> +      }
>    }
>  #endif	/* not MSDOS */

This hunk looks wrong, or maybe I'm missing something: it looks like
you removed the ACL support code from the WINDOWSNT blocks, but added
the replacement qcopy_acl in a block that is not compiled on Windows
at all (and uses fchmod and st which are not initialized on Windows).

If I'm right, perhaps it is best to leave the WINDOWSNT parts alone:
since acl_get_file and acl_set_file are still being left in Emacs
sources elsewhere, there's no need to remove their calls here, and
leaving them alone will avoid the need to add qcopy_acl to w32.c.

> -#ifdef HAVE_POSIX_ACL
> +#ifdef HAVE_ACL_SET_FILE
>    absname = ENCODE_FILE (absname);
>  
>    acl = acl_get_file (SSDATA (absname), ACL_TYPE_ACCESS);

It sounds wrong to me to condition a call to acl_get_file with a macro
called HAVE_ACL_SET_FILE.

> @@ -3193,7 +3144,7 @@
>        fail = (acl_set_file (SSDATA (encoded_absname), ACL_TYPE_ACCESS,
>  			    acl)
>  	      != 0);
> -      if (fail && !ACL_NOT_WELL_SUPPORTED (errno))
> +      if (fail && acl_errno_valid (errno))

For this to work, acl-errno-valid.c will have to be compiled on
MS-Windows.  And for that, we will need to solve the EOPNOTSUPP issue
mentioned above, because Windows doesn't define it.

Message #11 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: bug-gnu-emacs <at> gnu.org
Subject: Re: Support copy-file ACLs for Solaris etc.
Date: Sun, 28 Apr 2013 23:15:06 -0700

[Message part 1 (text/plain, inline)]

> How hard would it to support both?

I don't know, and I'd rather not support --without-acl.  This should
have been an --enable/--disable option in the first place, as it's
enabling or disabling a feature, not building with or without a
package.  There's little point to Emacs departing from existing
GNU practice here.

> This uses EOPNOTSUPP without #ifdef guards; is that universally
> available?

Mostly, except for Windows, but to play it safe we can incorporate the
Gnulib errno module, which defines EOPNOTSUPP if it's not already
defined.

> If I'm right, perhaps it is best to leave the WINDOWSNT parts alone:

Sure, we can do that.

> It sounds wrong to me to condition a call to acl_get_file with a macro
> called HAVE_ACL_SET_FILE.

In practice both are available if either is; in particular, there's no
point calling acl_get_file if you can't invoke acl_set_file later.

> For this to work, acl-errno-valid.c will have to be compiled on
> MS-Windows.  And for that, we will need to solve the EOPNOTSUPP issue
> mentioned above, because Windows doesn't define it.

OK.  Revised patch attached, which tries to address the above.
It relies on Gnulib for EOPNOTSUPP, except for WINDOWSNT where
'configure' isn't working yet, and there it adds a line to
nt/config.nt to deal with this until we get 'configure' working.

[acl.txt (text/plain, attachment)]

Message #14 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: bug-gnu-emacs <at> gnu.org
Subject: Re: Support copy-file ACLs for Solaris etc.
Date: Mon, 29 Apr 2013 20:07:43 +0300

> Date: Sun, 28 Apr 2013 23:15:06 -0700
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> CC: bug-gnu-emacs <at> gnu.org
> 
> > How hard would it to support both?
> 
> I don't know, and I'd rather not support --without-acl.  This should
> have been an --enable/--disable option in the first place, as it's
> enabling or disabling a feature, not building with or without a
> package.  There's little point to Emacs departing from existing
> GNU practice here.

I just wonder how many people are used to that option.  Maybe not too
many, since this is a new feature.

> > This uses EOPNOTSUPP without #ifdef guards; is that universally
> > available?
> 
> Mostly, except for Windows, but to play it safe we can incorporate the
> Gnulib errno module, which defines EOPNOTSUPP if it's not already
> defined.

That almost works, but there are 2 issues:

 . I'd prefer to have the definition of EOPNOTSUPP in nt/inc/ms-w32.h,
   to make synchronization of config.nt with src/config.in easier.

 . More importantly, ENOTSUP is defined on Windows with the same value
   as ENOSYS (see ms-w32.h), so acl-errno-valid.c will fail to compile
   due to 2 identical case values in a switch.

> > If I'm right, perhaps it is best to leave the WINDOWSNT parts alone:
> 
> Sure, we can do that.

Thanks.

> OK.  Revised patch attached, which tries to address the above.

It looks fine, other than the 2 minor issues above, thanks.

> It relies on Gnulib for EOPNOTSUPP, except for WINDOWSNT where
> 'configure' isn't working yet, and there it adds a line to
> nt/config.nt to deal with this until we get 'configure' working.

'configure' is already working, I just need time to finish up the
branch and merge it.  But even when that is done (hopefully in a few
days), we will not immediately abandon the current configury.  It will
take some time.

Message #19 received at 14295-done <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 14295-done <at> debbugs.gnu.org
Subject: Re: Support copy-file ACLs for Solaris etc.
Date: Tue, 07 May 2013 14:36:54 -0700

On 04/29/13 10:07, Eli Zaretskii wrote:

> That almost works, but there are 2 issues:
> 
>  . I'd prefer to have the definition of EOPNOTSUPP in nt/inc/ms-w32.h,
>    to make synchronization of config.nt with src/config.in easier.
> 
>  . More importantly, ENOTSUP is defined on Windows with the same value
>    as ENOSYS (see ms-w32.h), so acl-errno-valid.c will fail to compile
>    due to 2 identical case values in a switch.

Thanks, I fixed those, installed it as trunk bzr 112507,
and am marking this bug as done.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.