bug#21699: 24.5; Bug in backup-buffer-copy and/or set-file-extended-attributes etc

bug#21699: 24.5; Bug in backup-buffer-copy and/or set-file-extended-attributes etc

[Eli Barzilay]

(There are potentially multiple bugs here, I'll mark them as [n].)

After switching to 24.5 I started to see warnings like:

    Cannot write backup file; backing up in ~/.emacs.d/%backup%~

when saving a file on a remote directory (running on Windows, remote
is on Linux, but this is unlikely to be relevant).

I tracked this to `backup-buffer-copy', which has a seemingly bad use
of `with-demoted-errors'.  This looks like a minor update is needed
[1], since the macro source handles a no-format-string case as legacy.

The next suspect was `set-file-extended-attributes'.  This code comes
from commit ccad023bc3c70fc8368c00f7ed2f5ec947a4260d (2012-12-29), and
following the description there, I think that there's a bug in this
function: it has a single `dolist' which would always return nil,
which means that putting it in the `and' of `backup-buffer-copy' makes
the code always think that there was a failure.  Instead, there should
be some flag to be used in the loop to keep track of the `and' of all
of the results [2].  (Or maybe there's some andmap/every/... function
that does it more idiomatically.)

Next comes `set-file-selinux-context', which might have a problem.
It's documentation says:

    This function does nothing and returns nil if SELinux is disabled,
    or if Emacs was not compiled with SELinux support.

which I assume is my case since I'm running on Windows.  It is called
with (nil nil nil nil) which I'm guessing is some no-information-
since-we-don't-have-selinux thing -- but in that case,
`set-file-selinux-context' should not return nil when it's getting
that value.  So something is definitely broken here [3], either it
shouldn't return nil, or it shouldn't be called (avoid adding that
4xnil value to the extended-attributes, or avoid calling
`set-file-selinux-context' with that value), and possibly there should
be an explicit no-info value.

Finally, after the above problem makes it think that there was a
failure, it uses `set-file-modes' with the backup file, and that
throws an error for the backup file:

    (file-error "Doing chmod" "permission denied" "...path-to-backup...")

My setup is to have a single (local) directory for backups, so this
might be some result of applying the windows acl thing first, copying
the acl of the remote file to the one of the local backup.  Windows
shows that the file permissions for Everyone are: "Read & Execute" and
"Read", so that might be the problem.  I'm not sure how this whole
thing will look like, but it might be a problem to apply the acls this
way, so this problem might require some additional flag that allows
people to avoid copying the acls to the backups.

-- 
                    ((x=>x(x))(x=>x(x)))                   Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!

bug#21699: 24.5; Bug in backup-buffer-copy and/or set-file-extended-attributes etc

[Eli Zaretskii]

> From: Eli Barzilay <eli@barzilay.org>
> Date: Sun, 18 Oct 2015 00:34:42 -0400
> 
> (There are potentially multiple bugs here, I'll mark them as [n].)
> 
> After switching to 24.5 I started to see warnings like:
> 
>     Cannot write backup file; backing up in ~/.emacs.d/%backup%~
> 
> when saving a file on a remote directory (running on Windows, remote
> is on Linux, but this is unlikely to be relevant).
> 
> I tracked this to `backup-buffer-copy', which has a seemingly bad use
> of `with-demoted-errors'.

Do you mean to say that backup-buffer-copy fails in your case?  If so,
it means you have some customizations, or maybe the way your volume is
mounted causes backup-buffer-copy be called.  It isn't normally called
in "emacs -Q" and with local files, AFAICT.

Is that what happens in your case?

Do you see the problem in "emacs -Q"?

> The next suspect was `set-file-extended-attributes'.  This code comes
> from commit ccad023bc3c70fc8368c00f7ed2f5ec947a4260d (2012-12-29), and
> following the description there, I think that there's a bug in this
> function: it has a single `dolist' which would always return nil,
> which means that putting it in the `and' of `backup-buffer-copy' makes
> the code always think that there was a failure.  Instead, there should
> be some flag to be used in the loop to keep track of the `and' of all
> of the results [2].  (Or maybe there's some andmap/every/... function
> that does it more idiomatically.)
> 
> Next comes `set-file-selinux-context', which might have a problem.
> It's documentation says:
> 
>     This function does nothing and returns nil if SELinux is disabled,
>     or if Emacs was not compiled with SELinux support.
> 
> which I assume is my case since I'm running on Windows.  It is called
> with (nil nil nil nil) which I'm guessing is some no-information-
> since-we-don't-have-selinux thing -- but in that case,
> `set-file-selinux-context' should not return nil when it's getting
> that value.  So something is definitely broken here [3], either it
> shouldn't return nil, or it shouldn't be called (avoid adding that
> 4xnil value to the extended-attributes, or avoid calling
> `set-file-selinux-context' with that value), and possibly there should
> be an explicit no-info value.
> 
> Finally, after the above problem makes it think that there was a
> failure, it uses `set-file-modes' with the backup file, and that
> throws an error for the backup file:
> 
>     (file-error "Doing chmod" "permission denied" "...path-to-backup...")
> 
> My setup is to have a single (local) directory for backups, so this
> might be some result of applying the windows acl thing first, copying
> the acl of the remote file to the one of the local backup.  Windows
> shows that the file permissions for Everyone are: "Read & Execute" and
> "Read", so that might be the problem.  I'm not sure how this whole
> thing will look like, but it might be a problem to apply the acls this
> way, so this problem might require some additional flag that allows
> people to avoid copying the acls to the backups.

Thanks for looking into this.

I don't think there are so many problems in those functions.  I
suggest to step with Edebug into backup-buffer and its subroutines, or
maybe just into backup-buffer-copy, if you are sure it's the failing
function, and establish for sure which of them fails and why.  Then we
could take it from there.

My first guess would be some snafu in Windows permissions caused by
the method you used to mount the GNU/Linux volume.  But before I start
asking you for further information based on that hypothesis, I'd like
to be sure this is indeed the cause of your problems.

bug#21699: 24.5; Bug in backup-buffer-copy and/or set-file-extended-attributes etc

[Eli Barzilay]

On Sun, Oct 18, 2015 at 12:01 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>
> Do you mean to say that backup-buffer-copy fails in your case?  If so,
> it means you have some customizations, or maybe the way your volume is
> mounted causes backup-buffer-copy be called.  It isn't normally called
> in "emacs -Q" and with local files, AFAICT.
>
> Is that what happens in your case?
>
> Do you see the problem in "emacs -Q"?

Yes, I do have customizations.  Overall I'm not doing anything that
should be done -- though I'm guessing that not many people get to that
situation.  The main thing in my setup is that backups are done by
copying the file into a single directory for backups -- and in the
problem case the backup is on a local windows directory when the
original file is coming from a remote mount (on linux).  But the bugs
are easy to see:

1. `with-demoted-errors' is used in a bunch of places without a format
   string.  This is not a bug since the macro supports the case of a
   non-literal-string being the first expression, but's for legacy, so
   it's either better to add that format string, or the macro should
   support that without qualifying it as a legacy feature.

2. The `set-file-extended-attributes' function always returns nil, which
   is a proper bug:
   - In `backup-buffer-copy' its return value is used as if it indicates
     whether it succeeded -- that's currently broken because it always
     returns nil.
   - It's also used in `basic-save-buffer' -- but there its result is
     not used, and the code looks like it's expecting it to throw an
     error on failure.
   - It's also used in `basic-save-buffer-2', in a `with-demoted-errors'
   The commit message that I pointed to makes me think that it's
   expected to return nil on failure -- so it should be fixed to do
   that.  Another solution would be if it's expected to throw an error
   when it fails, and in this case the first use is broken and should
   not look at its result.

3. The third problem happens *if* the solution to #2 is to make it
   return a meaningful result.  In that case, the problem I'll run into
   is that on windows my extended modes include

       (selinux nil nil nil nil)

   which I'm guessing is because there's no selinux support, but then
   `set-file-selinux-context' should not fail when getting a value of
   (nil nil nil nil).

4. The last problem of chmod-ing failing after setting the windows acl
   is probably better to defer after resolving the above.

-- 
                    ((x=>x(x))(x=>x(x)))                   Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!

bug#21699: 24.5; Bug in backup-buffer-copy and/or set-file-extended-attributes etc

[Eli Zaretskii]

> Date: Sun, 18 Oct 2015 17:05:43 -0400
> From: Eli Barzilay <eli@barzilay.org>
> Cc: 21699@debbugs.gnu.org
> 
> On Sun, Oct 18, 2015 at 12:01 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> >
> > Do you mean to say that backup-buffer-copy fails in your case?  If so,
> > it means you have some customizations, or maybe the way your volume is
> > mounted causes backup-buffer-copy be called.  It isn't normally called
> > in "emacs -Q" and with local files, AFAICT.
> >
> > Is that what happens in your case?
> >
> > Do you see the problem in "emacs -Q"?
> 
> Yes, I do have customizations.  Overall I'm not doing anything that
> should be done -- though I'm guessing that not many people get to that
> situation.  The main thing in my setup is that backups are done by
> copying the file into a single directory for backups -- and in the
> problem case the backup is on a local windows directory when the
> original file is coming from a remote mount (on linux).

Please do tell if the problem happens in "emacs -Q".  We need to start
from same baseline which we understand.  It might be better to also
show the results in "emacs -Q" when backup-by-copying is non-nil, but
with local files on a Windows volume.

Btw, what kind of volume is your Windows disk where you have the
backup directory?  Is it NTFS, FAT32, something else?

Also, I need the results of calling file-attributes and file-acl on
the file for which backup fails and on the backup directory.

> 2. The `set-file-extended-attributes' function always returns nil, which
>    is a proper bug:
>    - In `backup-buffer-copy' its return value is used as if it indicates
>      whether it succeeded -- that's currently broken because it always
>      returns nil.

That's not a bug: set-file-extended-attributes is not supposed to
always return nil.  When it succeeds, it returns t.  It returns nil in
your case because it fails; the question is why.

>    - It's also used in `basic-save-buffer' -- but there its result is
>      not used, and the code looks like it's expecting it to throw an
>      error on failure.

That's not a bug, either: set-file-extended-attributes does signal an
error when it fails.  In backup-buffer-copy it is prevented from
signaling an error by with-demoted-errors.

>    - It's also used in `basic-save-buffer-2', in a `with-demoted-errors'
>    The commit message that I pointed to makes me think that it's
>    expected to return nil on failure -- so it should be fixed to do
>    that.  Another solution would be if it's expected to throw an error
>    when it fails, and in this case the first use is broken and should
>    not look at its result.

See above: these are not bugs, but intended behavior.  The question is
why the function fails in your case.  That is one reason why I asked
to see the results in "emacs -Q"

> 3. The third problem happens *if* the solution to #2 is to make it
>    return a meaningful result.  In that case, the problem I'll run into
>    is that on windows my extended modes include
> 
>        (selinux nil nil nil nil)
> 
>    which I'm guessing is because there's no selinux support, but then
>    `set-file-selinux-context' should not fail when getting a value of
>    (nil nil nil nil).

set-file-selinux-context is not supposed to be called on MS-Windows,
because SELinux APIs are not supported there.

> 4. The last problem of chmod-ing failing after setting the windows acl
>    is probably better to defer after resolving the above.

That's a big surprise: chmod is largely a no-op on Windows.  If you
invoke set-file-modes exactly like backup-buffer-copy does, but from
the "M-:" prompt, what error does it report?

Are you sure backup-buffer-copy even copies the file to the backup
directory?  IOW, does the call to copy-file inside backup-buffer-copy
work?  If it does not, that would explain why both
set-file-extended-attributes and set-file-modes fail afterwards.  So
perhaps manually running the code of backup-buffer-copy step by step
with a file on your Linux volume would show where the failure starts.

bug#21699: 24.5; Bug in backup-buffer-copy and/or set-file-extended-attributes etc [set-file-extended-attributes]

[Eli Barzilay]

On Mon, Oct 19, 2015 at 1:10 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>>
>> On Sun, Oct 18, 2015 at 12:01 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>> >
>> > Do you mean to say that backup-buffer-copy fails in your case?  If so,
>> > it means you have some customizations, or maybe the way your volume is
>> > mounted causes backup-buffer-copy be called.  It isn't normally called
>> > in "emacs -Q" and with local files, AFAICT.
>> >
>> > Is that what happens in your case?
>> >
>> > Do you see the problem in "emacs -Q"?
>>
>> Yes, I do have customizations.  Overall I'm not doing anything that
>> should be done -- though I'm guessing that not many people get to that
>> situation.  The main thing in my setup is that backups are done by
>> copying the file into a single directory for backups -- and in the
>> problem case the backup is on a local windows directory when the
>> original file is coming from a remote mount (on linux).
>
> Please do tell if the problem happens in "emacs -Q".  We need to start
> from same baseline which we understand.  It might be better to also
> show the results in "emacs -Q" when backup-by-copying is non-nil, but
> with local files on a Windows volume.

Getting from -Q to a point where the bug(s) are visible means recreating
the same backup configuration which will take a while.  I'll try to get
that kater, though I don't know how effective that will be.  The main
bug is very visible though, but perhaps it got lost in details.  So I'll
talk about just that now.  (I'll update the subject accordingly.)

>> 2. The `set-file-extended-attributes' function always returns nil,
>>    which is a proper bug:
>>    - In `backup-buffer-copy' its return value is used as if it
>>      indicates whether it succeeded -- that's currently broken
>>      because it always returns nil.
>
> That's not a bug: set-file-extended-attributes is not supposed to
> always return nil.

Yes, I'm saying that it *does* AFAICT always return nil, hence the bug.


> When it succeeds, it returns t.  It returns nil in your case because
> it fails; the question is why.

Its body is a single `dotimes' expression, and that always returns nil
-- do you see any way in which it will return anything else?

As a trivial test, I ran this:

    (set-file-extended-attributes ".emacs" nil)

and the result is the expected nil.

Fixing this bug, AFAICT, should look like

    (defun set-file-extended-attributes (filename attributes)
      "..."
      (let ((res t))
        (dolist (elt attributes)
          (let ((attr (car elt))
                (val (cdr elt)))
            (unless (cond ((eq attr 'acl)
                           (set-file-acl filename val))
                          ((eq attr 'selinux-context)
                           (set-file-selinux-context filename val)))
              (setq res nil))))
        res))

-- 
                    ((x=>x(x))(x=>x(x)))                   Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!

bug#21699: 24.5; Bug in backup-buffer-copy and/or set-file-extended-attributes etc [set-file-extended-attributes]

[Eli Zaretskii]

> Date: Mon, 19 Oct 2015 02:14:40 -0400
> From: Eli Barzilay <eli@barzilay.org>
> Cc: 21699@debbugs.gnu.org
> 
> > Please do tell if the problem happens in "emacs -Q".  We need to start
> > from same baseline which we understand.  It might be better to also
> > show the results in "emacs -Q" when backup-by-copying is non-nil, but
> > with local files on a Windows volume.
> 
> Getting from -Q to a point where the bug(s) are visible means recreating
> the same backup configuration which will take a while.  I'll try to get
> that kater, though I don't know how effective that will be.

Please do, I think it's important to start from a known behavior that
we understand.

> >> 2. The `set-file-extended-attributes' function always returns nil,
> >>    which is a proper bug:
> >>    - In `backup-buffer-copy' its return value is used as if it
> >>      indicates whether it succeeded -- that's currently broken
> >>      because it always returns nil.
> >
> > That's not a bug: set-file-extended-attributes is not supposed to
> > always return nil.
> 
> Yes, I'm saying that it *does* AFAICT always return nil, hence the bug.
> 
> 
> > When it succeeds, it returns t.  It returns nil in your case because
> > it fails; the question is why.
> 
> Its body is a single `dotimes' expression, and that always returns nil
> -- do you see any way in which it will return anything else?

Ah, yes, you are right.  I was instead looking at what set-file-acl
returns.

So after fixing set-file-extended-attributes as you suggest, does the
problem still happen for you?

bug#21699: 24.5; Bug in backup-buffer-copy and/or set-file-extended-attributes etc [set-file-extended-attributes]

[Eli Zaretskii]

> Date: Mon, 19 Oct 2015 09:38:08 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 21699@debbugs.gnu.org
> 
> So after fixing set-file-extended-attributes as you suggest, does the
> problem still happen for you?

Actually, your suggested variant also returns nil for me.  I need
something like this instead:

  (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'."
    (let (result)
      (dolist (elt attributes)
	(let ((attr (car elt))
	      (val (cdr elt)))
	  (cond ((eq attr 'acl)
		 (setq result (or result
				  (set-file-acl filename val))))
		((eq attr 'selinux-context)
		 (setq result (or result
				  (set-file-selinux-context filename val)))))))
      result))

With this change, evaluating

 (set-file-extended-attributes "~/.emacs" (file-extended-attributes "~/.emacs")

returns t here.

bug#21699: 24.5; Bug in backup-buffer-copy and/or set-file-extended-attributes etc [set-file-extended-attributes]

[Eli Zaretskii]

> Date: Mon, 19 Oct 2015 09:50:06 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 21699@debbugs.gnu.org
> 
> > Date: Mon, 19 Oct 2015 09:38:08 +0300
> > From: Eli Zaretskii <eliz@gnu.org>
> > Cc: 21699@debbugs.gnu.org
> > 
> > So after fixing set-file-extended-attributes as you suggest, does the
> > problem still happen for you?
> 
> Actually, your suggested variant also returns nil for me.  I need
> something like this instead:
> 
>   (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'."
>     (let (result)
>       (dolist (elt attributes)
> 	(let ((attr (car elt))
> 	      (val (cdr elt)))
> 	  (cond ((eq attr 'acl)
> 		 (setq result (or result
> 				  (set-file-acl filename val))))
> 		((eq attr 'selinux-context)
> 		 (setq result (or result
> 				  (set-file-selinux-context filename val)))))))
>       result))

I installed a slightly different variant of this (which always invokes
the corresponding low-level primitive for each type of extended
attributes, instead of skipping all those after the first success), to
keep the original semantics.

bug#21699: 24.5; Bug in backup-buffer-copy and/or set-file-extended-attributes etc [set-file-extended-attributes]

[Eli Barzilay]

[I'm working on the other things that you asked now...]


On Mon, Oct 19, 2015 at 3:09 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> Date: Mon, 19 Oct 2015 09:50:06 +0300
>> From: Eli Zaretskii <eliz@gnu.org>
>> Cc: 21699@debbugs.gnu.org
>>
>> > Date: Mon, 19 Oct 2015 09:38:08 +0300
>> > From: Eli Zaretskii <eliz@gnu.org>
>> > Cc: 21699@debbugs.gnu.org
>> >
>> > So after fixing set-file-extended-attributes as you suggest, does the
>> > problem still happen for you?
>>
>> Actually, your suggested variant also returns nil for me.

Well, it would return nil since the result of `file-extended-attributes'
would include (selinux-context nil nil nil nil) which sould make it try
to use `set-file-selinux-context' and that returns nil.  (That was my
original point #3, addressed below.)


>> I need something like this instead:
>> [...2nd version...]

This one would indeed return t for me, since it stops trying after the
first t result from `set-file-acl'.


> I installed a slightly different variant of this (which always invokes
> the corresponding low-level primitive for each type of extended
> attributes, instead of skipping all those after the first success), to
> keep the original semantics.

Well, you added this in the docstring:

    Value is t if the function succeeds in setting the attributes.

which is a little vague -- I think that it would be better to say

    Value is t if the function succeeds in setting at least one of the
    attributes.

*BUT* I doubt that this is a good idea, since on a system that supports
both acl and selinux-context you probably want a t result to indicate
that all of the extended settings worked.  So I think that my original
suggestion is better if `file-extended-attributes' doesn't produce
values that are not supported.

So ... moving on to that third problem, I fixed
`file-extended-attributes' to do that: it doesn't include an `acl' value
if `file-acl' returned nil, and doesn't include `selinux-context' if
`file-selinux-context' returned (nil nil nil nil).  I used this with my
version of `set-file-extended-attributes' (which returns t only when all
settings returned non-nil), and that resolved my backup problem.  The
code for the two functions, with updated docstrings, is below.

-------------------------------------------------------------------------------

(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.  Only supported
metadata is included."
  (let ((attrs '()) x)
    (unless (equal nil (setq x (file-acl filename)))
      (push `(acl . ,x) attrs))
    (unless (equal '(nil nil nil nil)
                   (setq x (file-selinux-context filename)))
      (push `(selinux-context . ,x) attrs))
    attrs))

(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'.  Value is t if the function succeeds
in setting all of the given attributes."
  (let ((result t))
    (dolist (elt attributes)
      (let ((attr (car elt))
            (val (cdr elt)))
        (unless (cond ((eq attr 'acl)
                       (set-file-acl filename val))
                      ((eq attr 'selinux-context)
                       (set-file-selinux-context filename val)))
          (setq result nil))))
    result))

-------------------------------------------------------------------------------

-- 
                    ((x=>x(x))(x=>x(x)))                   Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!

bug#21699: 24.5; Bug in backup-buffer-copy and/or set-file-extended-attributes etc

[Eli Barzilay]

[I'm not sure that this is relevant, but since I had most of it
written...]


On Mon, Oct 19, 2015 at 1:10 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> Date: Sun, 18 Oct 2015 17:05:43 -0400
>> From: Eli Barzilay <eli@barzilay.org>
>> Cc: 21699@debbugs.gnu.org
>>
>> On Sun, Oct 18, 2015 at 12:01 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>> >
>> > Do you mean to say that backup-buffer-copy fails in your case?  If so,
>> > it means you have some customizations, or maybe the way your volume is
>> > mounted causes backup-buffer-copy be called.  It isn't normally called
>> > in "emacs -Q" and with local files, AFAICT.
>> >
>> > Is that what happens in your case?
>> >
>> > Do you see the problem in "emacs -Q"?
>>
>> Yes, I do have customizations.  Overall I'm not doing anything that
>> should be done -- though I'm guessing that not many people get to that
>> situation.  The main thing in my setup is that backups are done by
>> copying the file into a single directory for backups -- and in the
>> problem case the backup is on a local windows directory when the
>> original file is coming from a remote mount (on linux).
>
> Please do tell if the problem happens in "emacs -Q".  We need to start
> from same baseline which we understand.  It might be better to also
> show the results in "emacs -Q" when backup-by-copying is non-nil, but
> with local files on a Windows volume.

OK, I verified that it happens with -Q.  The only customization I did
was:

    (setq backup-directory-alist '(("." . "c:/eli/.backups/")))

where that directory exists and is on the NTFS filesystem.  I then open
an "l:/tmp/x" file, edit it, save, and that produces that error.  That
drive is mounted from a linux VM via samba.  This happens regardless of
the value of `backup-by-copying'.


> Btw, what kind of volume is your Windows disk where you have the
> backup directory?  Is it NTFS, FAT32, something else?

NTFS.


> Also, I need the results of calling file-attributes and file-acl on
> the file for which backup fails and on the backup directory.

Here they are, running from "emacs -Q" too.  I also included the results
on the backup file itself (which *does* get created, as I said earlier):

    (file-attributes "l:/tmp/x")
      (nil 1 1000 513 (22052 36758 0 0) (22052 36761 0 0) (22052 36758
0 0) 21 "-rw-rw-rw-" t 0 (29879 . 49391))
    (file-acl "l:/tmp/x")
      "O:S-1-5-21-255753047-2700399607-2208692068-1000G:S-1-22-2-1000D:P(A;;0x12019f;;;S-1-5-21-255753047-2700399607-2208692068-1000)(A;;FR;;;S-1-22-2-1000)(A;;FR;;;WD)"

    (file-attributes "c:/eli/.backups")
      (t 1 1000 513 (22052 36737 0 0) (22052 36737 0 0) (21144 22619 0
0) 524288 "drwxrwxrwx" t (3584 0 . 63301) (58391 . 43832))
    (file-acl "c:/eli/.backups")
      "O:S-1-5-21-1288650996-1850016226-2761200207-1000G:S-1-5-21-1288650996-1850016226-2761200207-513D:P(A;;FA;;;S-1-5-21-1288650996-1850016226-2761200207-1000)(A;;0x120080;;;S-1-5-21-1288650996-1850016226-2761200207-513)(A;;0x120080;;;WD)(A;OICIIOID;GA;;;BA)(A;ID;FA;;;SY)(A;OICIIOID;GA;;;SY)(A;OICIID;0x1200a9;;;BU)(A;ID;0x1301bf;;;AU)(A;OICIIOID;SDGXGWGR;;;AU)"

    (file-attributes "c:/eli/.backups/!drive_l!tmp!x~")
      (nil 1 1000 513 (22052 36737 0 0) (22052 36735 0 0) (21757 58044
0 0) 17 "-rw-rw-rw-" t (384512 1 . 20580) (58391 . 43832))
    (file-acl "c:/eli/.backups/!drive_l!tmp!x~")
      "O:S-1-5-21-255753047-2700399607-2208692068-1000G:S-1-22-2-1000D:P(A;;0x12019f;;;S-1-5-21-255753047-2700399607-2208692068-1000)(A;;FR;;;S-1-22-2-1000)(A;;FR;;;WD)"


>> 2. The `set-file-extended-attributes' function always returns nil, which
>>    is a proper bug:
>>    - In `backup-buffer-copy' its return value is used as if it indicates
>>      whether it succeeded -- that's currently broken because it always
>>      returns nil.
>
> That's not a bug: set-file-extended-attributes is not supposed to
> always return nil.  When it succeeds, it returns t.  It returns nil in
> your case because it fails; the question is why. [...]

(Moved to the other email.)


>> 3. The third problem happens *if* the solution to #2 is to make it
>>    return a meaningful result.  In that case, the problem I'll run into
>>    is that on windows my extended modes include
>>
>>        (selinux nil nil nil nil)
>>
>>    which I'm guessing is because there's no selinux support, but then
>>    `set-file-selinux-context' should not fail when getting a value of
>>    (nil nil nil nil).
>
> set-file-selinux-context is not supposed to be called on MS-Windows,
> because SELinux APIs are not supported there.

That's resolved with my suggested fix for `file-extended-attributes',
making it drop the `selinux-context' when it's not supported.


>> 4. The last problem of chmod-ing failing after setting the windows
>>    acl is probably better to defer after resolving the above.
>
> That's a big surprise: chmod is largely a no-op on Windows.  If you
> invoke set-file-modes exactly like backup-buffer-copy does, but from
> the "M-:" prompt, what error does it report?

I suspect that some of those ACL entries mean that my user is not
allowed to change the file.  In any case, the error that I got when I
played with it is:

Debugger entered--Lisp error: (file-error "Doing chmod" "permission
denied" "c:/eli/.backups/!drive_l!lambda!tmp!x~")
  set-file-modes("c:/eli/.backups/!drive_l!lambda!tmp!x~" 438)
  eval((set-file-modes "c:/eli/.backups/!drive_l!lambda!tmp!x~" 438) nil)


> Are you sure backup-buffer-copy even copies the file to the backup
> directory?  IOW, does the call to copy-file inside backup-buffer-copy
> work?  If it does not, that would explain why both
> set-file-extended-attributes and set-file-modes fail afterwards.  So
> perhaps manually running the code of backup-buffer-copy step by step
> with a file on your Linux volume would show where the failure starts.

That should hopefully be clear now too, but yes -- the way it'd work is
* create the backup copy,
* set the acl (which works fine),
* think that it failed, and then resorted to the fallback file.

-- 
                    ((x=>x(x))(x=>x(x)))                   Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!

bug#21699: 24.5; Bug in backup-buffer-copy and/or set-file-extended-attributes etc [set-file-extended-attributes]

[Eli Zaretskii]

> Date: Mon, 19 Oct 2015 03:50:04 -0400
> From: Eli Barzilay <eli@barzilay.org>
> Cc: 21699@debbugs.gnu.org
> 
> >> Actually, your suggested variant also returns nil for me.
> 
> Well, it would return nil since the result of `file-extended-attributes'
> would include (selinux-context nil nil nil nil)

It always does, AFAICT.

> > I installed a slightly different variant of this (which always invokes
> > the corresponding low-level primitive for each type of extended
> > attributes, instead of skipping all those after the first success), to
> > keep the original semantics.
> 
> Well, you added this in the docstring:
> 
>     Value is t if the function succeeds in setting the attributes.
> 
> which is a little vague -- I think that it would be better to say
> 
>     Value is t if the function succeeds in setting at least one of the
>     attributes.

We don't have enough information from the low-level APIs to tell
that.  So yes, it's slightly vague, but intentionally so.

> *BUT* I doubt that this is a good idea, since on a system that supports
> both acl and selinux-context you probably want a t result to indicate
> that all of the extended settings worked.

I don't think this is true.  Many (maybe most) systems support either
ACLs or SELinux, and for them the function will incorrectly return
nil.

A better way might be to have a test of "null" attributes, and avoid
calling the low-level APIs when the attributes are "null".  A separate
issue, I think.

bug#21699: 24.5; Bug in backup-buffer-copy and/or set-file-extended-attributes etc

[Eli Zaretskii]

> Date: Mon, 19 Oct 2015 03:57:13 -0400
> From: Eli Barzilay <eli@barzilay.org>
> Cc: 21699@debbugs.gnu.org
> 
> > Also, I need the results of calling file-attributes and file-acl on
> > the file for which backup fails and on the backup directory.
> 
> Here they are, running from "emacs -Q" too.  I also included the results
> on the backup file itself (which *does* get created, as I said earlier):
> 
>     (file-attributes "l:/tmp/x")
>       (nil 1 1000 513 (22052 36758 0 0) (22052 36761 0 0) (22052 36758
> 0 0) 21 "-rw-rw-rw-" t 0 (29879 . 49391))
>     (file-acl "l:/tmp/x")
>       "O:S-1-5-21-255753047-2700399607-2208692068-1000G:S-1-22-2-1000D:P(A;;0x12019f;;;S-1-5-21-255753047-2700399607-2208692068-1000)(A;;FR;;;S-1-22-2-1000)(A;;FR;;;WD)"
> 
>     (file-attributes "c:/eli/.backups")
>       (t 1 1000 513 (22052 36737 0 0) (22052 36737 0 0) (21144 22619 0
> 0) 524288 "drwxrwxrwx" t (3584 0 . 63301) (58391 . 43832))
>     (file-acl "c:/eli/.backups")
>       "O:S-1-5-21-1288650996-1850016226-2761200207-1000G:S-1-5-21-1288650996-1850016226-2761200207-513D:P(A;;FA;;;S-1-5-21-1288650996-1850016226-2761200207-1000)(A;;0x120080;;;S-1-5-21-1288650996-1850016226-2761200207-513)(A;;0x120080;;;WD)(A;OICIIOID;GA;;;BA)(A;ID;FA;;;SY)(A;OICIIOID;GA;;;SY)(A;OICIID;0x1200a9;;;BU)(A;ID;0x1301bf;;;AU)(A;OICIIOID;SDGXGWGR;;;AU)"
> 
>     (file-attributes "c:/eli/.backups/!drive_l!tmp!x~")
>       (nil 1 1000 513 (22052 36737 0 0) (22052 36735 0 0) (21757 58044
> 0 0) 17 "-rw-rw-rw-" t (384512 1 . 20580) (58391 . 43832))
>     (file-acl "c:/eli/.backups/!drive_l!tmp!x~")
>       "O:S-1-5-21-255753047-2700399607-2208692068-1000G:S-1-22-2-1000D:P(A;;0x12019f;;;S-1-5-21-255753047-2700399607-2208692068-1000)(A;;FR;;;S-1-22-2-1000)(A;;FR;;;WD)"

AFAIU, this means you don't have full access ("FA") to the file you
are editing, and I guess that's why chmod fails.  If so, there's not
much we can do about that.

However, with set-file-extended-attributes fixed, backing up works
again for you, is that right?

> >> 4. The last problem of chmod-ing failing after setting the windows
> >>    acl is probably better to defer after resolving the above.
> >
> > That's a big surprise: chmod is largely a no-op on Windows.  If you
> > invoke set-file-modes exactly like backup-buffer-copy does, but from
> > the "M-:" prompt, what error does it report?
> 
> I suspect that some of those ACL entries mean that my user is not
> allowed to change the file.  In any case, the error that I got when I
> played with it is:
> 
> Debugger entered--Lisp error: (file-error "Doing chmod" "permission
> denied" "c:/eli/.backups/!drive_l!lambda!tmp!x~")
>   set-file-modes("c:/eli/.backups/!drive_l!lambda!tmp!x~" 438)
>   eval((set-file-modes "c:/eli/.backups/!drive_l!lambda!tmp!x~" 438) nil)

Yes, I think your suspicion is correct.  (In general, I'd suggest to
take ownership, from the Windows side, on all of the files on that
Linux volume you are using from Windows.  IME, this avoids many
problems with access denial from the Windows side.)

bug#21699: 24.5; Bug in backup-buffer-copy and/or set-file-extended-attributes etc

[Eli Barzilay]

On Mon, Oct 19, 2015 at 4:23 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>
> AFAIU, this means you don't have full access ("FA") to the file you
> are editing, and I guess that's why chmod fails.  If so, there's not
> much we can do about that.
>
> However, with set-file-extended-attributes fixed, backing up works
> again for you, is that right?

Yes.  (On both.)


>> I suspect that some of those ACL entries mean that my user is not
>> allowed to change the file.  In any case, the error that I got when I
>> played with it is:
>>
>> Debugger entered--Lisp error: (file-error "Doing chmod" "permission
>> denied" "c:/eli/.backups/!drive_l!lambda!tmp!x~")
>>   set-file-modes("c:/eli/.backups/!drive_l!lambda!tmp!x~" 438)
>>   eval((set-file-modes "c:/eli/.backups/!drive_l!lambda!tmp!x~" 438) nil)
>
> Yes, I think your suspicion is correct.  (In general, I'd suggest to
> take ownership, from the Windows side, on all of the files on that
> Linux volume you are using from Windows.  IME, this avoids many
> problems with access denial from the Windows side.)

Yeah, I didn't get to deal with that yet, and maybe will never since
this is a Win7 machine, and I think that on 10 these things changed.

-- 
                    ((x=>x(x))(x=>x(x)))                   Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!

bug#21699: 24.5; Bug in backup-buffer-copy and/or set-file-extended-attributes etc

[Eli Zaretskii]

> Date: Mon, 19 Oct 2015 05:03:19 -0400
> From: Eli Barzilay <eli@barzilay.org>
> Cc: 21699@debbugs.gnu.org
> 
> On Mon, Oct 19, 2015 at 4:23 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> >
> > AFAIU, this means you don't have full access ("FA") to the file you
> > are editing, and I guess that's why chmod fails.  If so, there's not
> > much we can do about that.
> >
> > However, with set-file-extended-attributes fixed, backing up works
> > again for you, is that right?
> 
> Yes.  (On both.)

OK, thanks.  Can we close this bug?

> >> I suspect that some of those ACL entries mean that my user is not
> >> allowed to change the file.  In any case, the error that I got when I
> >> played with it is:
> >>
> >> Debugger entered--Lisp error: (file-error "Doing chmod" "permission
> >> denied" "c:/eli/.backups/!drive_l!lambda!tmp!x~")
> >>   set-file-modes("c:/eli/.backups/!drive_l!lambda!tmp!x~" 438)
> >>   eval((set-file-modes "c:/eli/.backups/!drive_l!lambda!tmp!x~" 438) nil)
> >
> > Yes, I think your suspicion is correct.  (In general, I'd suggest to
> > take ownership, from the Windows side, on all of the files on that
> > Linux volume you are using from Windows.  IME, this avoids many
> > problems with access denial from the Windows side.)
> 
> Yeah, I didn't get to deal with that yet, and maybe will never since
> this is a Win7 machine, and I think that on 10 these things changed.

I don't have a Windows 10 system nearby to see if anything's changed
in that regard.  Any pointers?

Thanks.

bug#21699: 24.5; Bug in backup-buffer-copy and/or set-file-extended-attributes etc [set-file-extended-attributes]

[Eli Barzilay]

On Mon, Oct 19, 2015 at 4:04 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> Date: Mon, 19 Oct 2015 03:50:04 -0400
>> From: Eli Barzilay <eli@barzilay.org>
>> Cc: 21699@debbugs.gnu.org
>>
>> *BUT* I doubt that this is a good idea, since on a system that
>> supports both acl and selinux-context you probably want a t result to
>> indicate that all of the extended settings worked.
>
> I don't think this is true.  Many (maybe most) systems support either
> ACLs or SELinux, and for them the function will incorrectly return
> nil.
>
> A better way might be to have a test of "null" attributes, and avoid
> calling the low-level APIs when the attributes are "null".  A separate
> issue, I think.

Did you have a look at my `file-extended-attributes' fix?  It does just
that: when the low-level functions return "null" (four nils in the
selinux case), then they won't get included in the result.  This frees
`set-file-extended-attributes' to require that all settings succeed.


And I think that there's one case where things would fail with your fix:
a linux machine that has selinux disabled will have this as the extended
attributes:

    ((acl . nil) (selinux-context . (nil nil nil nil)))

and your version of `set-file-extended-attributes' would fail when both
of these fail.  With my fix, `file-extended-attributes' would just
return nil in that case, and `set-file-extended-attributes' will succeed
trivially.

-- 
                    ((x=>x(x))(x=>x(x)))                   Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!

bug#21699: 24.5; Bug in backup-buffer-copy and/or set-file-extended-attributes etc

[Eli Barzilay]

On Mon, Oct 19, 2015 at 5:09 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> Date: Mon, 19 Oct 2015 05:03:19 -0400
>> From: Eli Barzilay <eli@barzilay.org>
>> Cc: 21699@debbugs.gnu.org
>>
>> On Mon, Oct 19, 2015 at 4:23 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> >
>> > AFAIU, this means you don't have full access ("FA") to the file you
>> > are editing, and I guess that's why chmod fails.  If so, there's not
>> > much we can do about that.
>> >
>> > However, with set-file-extended-attributes fixed, backing up works
>> > again for you, is that right?
>>
>> Yes.  (On both.)
>
> OK, thanks.  Can we close this bug?

There's the issue of the `file-extended-attributes' fix (in the other
thread), and I think a possible leftover bad case.


>> >> I suspect that some of those ACL entries mean that my user is not
>> >> allowed to change the file.  In any case, the error that I got when I
>> >> played with it is:
>> >>
>> >> Debugger entered--Lisp error: (file-error "Doing chmod" "permission
>> >> denied" "c:/eli/.backups/!drive_l!lambda!tmp!x~")
>> >>   set-file-modes("c:/eli/.backups/!drive_l!lambda!tmp!x~" 438)
>> >>   eval((set-file-modes "c:/eli/.backups/!drive_l!lambda!tmp!x~" 438) nil)
>> >
>> > Yes, I think your suspicion is correct.  (In general, I'd suggest to
>> > take ownership, from the Windows side, on all of the files on that
>> > Linux volume you are using from Windows.  IME, this avoids many
>> > problems with access denial from the Windows side.)
>>
>> Yeah, I didn't get to deal with that yet, and maybe will never since
>> this is a Win7 machine, and I think that on 10 these things changed.
>
> I don't have a Windows 10 system nearby to see if anything's changed
> in that regard.  Any pointers?

I didn't try to verify it yet, but I vaguely remember somewhere that
talked about making things less restricted by default, resolving the
problem of putting files on an external HD and changing the owner when
you want to use the drive in a different machine.  (Which is what I do
now, with a drive that gets populated on the win7 machine.)

-- 
                    ((x=>x(x))(x=>x(x)))                   Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!

bug#21699: 24.5; Bug in backup-buffer-copy and/or set-file-extended-attributes etc [set-file-extended-attributes]

[Eli Zaretskii]

> Date: Mon, 19 Oct 2015 05:10:58 -0400
> From: Eli Barzilay <eli@barzilay.org>
> Cc: 21699@debbugs.gnu.org
> 
> On Mon, Oct 19, 2015 at 4:04 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> >> Date: Mon, 19 Oct 2015 03:50:04 -0400
> >> From: Eli Barzilay <eli@barzilay.org>
> >> Cc: 21699@debbugs.gnu.org
> >>
> >> *BUT* I doubt that this is a good idea, since on a system that
> >> supports both acl and selinux-context you probably want a t result to
> >> indicate that all of the extended settings worked.
> >
> > I don't think this is true.  Many (maybe most) systems support either
> > ACLs or SELinux, and for them the function will incorrectly return
> > nil.
> >
> > A better way might be to have a test of "null" attributes, and avoid
> > calling the low-level APIs when the attributes are "null".  A separate
> > issue, I think.
> 
> Did you have a look at my `file-extended-attributes' fix?  It does just
> that: when the low-level functions return "null" (four nils in the
> selinux case), then they won't get included in the result.  This frees
> `set-file-extended-attributes' to require that all settings succeed.

Yes, I've seen that.  But I'm not sure that we want such a change,
because it loses some information: namely, that the failed interfaces
did fail, or, equivalently, that information about the other kinds of
attributes is not available.

I don't know if this is important, but it does change the semantics of
an interface that was already released.  So I preferred a fix that
didn't involve such changes.

> And I think that there's one case where things would fail with your fix:
> a linux machine that has selinux disabled will have this as the extended
> attributes:
> 
>     ((acl . nil) (selinux-context . (nil nil nil nil)))
> 
> and your version of `set-file-extended-attributes' would fail when both
> of these fail.

You mean, a GNU/Linux machine that supports neither ACLs nor SELinux,
I suppose.  Do such machines exist, and if so, are they popular?

> With my fix, `file-extended-attributes' would just return nil in
> that case, and `set-file-extended-attributes' will succeed
> trivially.

Why should set-file-extended-attributes succeed in this case?  It
didn't set any extended attributes, right?  And if neither ACLs nor
SELinux is supported, we should definitely fall back on chmod for the
backup files, shouldn't we?

bug#21699: 24.5; Bug in backup-buffer-copy and/or set-file-extended-attributes etc [set-file-extended-attributes]

[Eli Barzilay]

On Mon, Oct 19, 2015 at 5:22 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> Date: Mon, 19 Oct 2015 05:10:58 -0400
>> From: Eli Barzilay <eli@barzilay.org>
>> Cc: 21699@debbugs.gnu.org
>>
>> On Mon, Oct 19, 2015 at 4:04 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> >> Date: Mon, 19 Oct 2015 03:50:04 -0400
>> >> From: Eli Barzilay <eli@barzilay.org>
>> >> Cc: 21699@debbugs.gnu.org
>> >>
>> >> *BUT* I doubt that this is a good idea, since on a system that
>> >> supports both acl and selinux-context you probably want a t result to
>> >> indicate that all of the extended settings worked.
>> >
>> > I don't think this is true.  Many (maybe most) systems support either
>> > ACLs or SELinux, and for them the function will incorrectly return
>> > nil.
>> >
>> > A better way might be to have a test of "null" attributes, and avoid
>> > calling the low-level APIs when the attributes are "null".  A separate
>> > issue, I think.
>>
>> Did you have a look at my `file-extended-attributes' fix?  It does just
>> that: when the low-level functions return "null" (four nils in the
>> selinux case), then they won't get included in the result.  This frees
>> `set-file-extended-attributes' to require that all settings succeed.
>
> Yes, I've seen that.  But I'm not sure that we want such a change,
> because it loses some information: namely, that the failed interfaces
> did fail, or, equivalently, that information about the other kinds of
> attributes is not available.

OK.


> I don't know if this is important, but it does change the semantics of
> an interface that was already released.  So I preferred a fix that
> didn't involve such changes.

OK, here's a version that does the decision in
`set-file-extended-attributes', making it succeed if all of the given
attributes were set but ignoring the "null" values.  (Again, I verified
that it works in my case.)

-------------------------------------------------------------------------------
(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'.  Value is t if the function succeeds
in setting all of the given attributes excluding ones that
indicate \"no information\"."
  (let ((result t))
    (dolist (elt attributes)
      (let ((attr (car elt))
            (val (cdr elt)))
        (unless (cond ((eq attr 'acl)
                       (or (equal val nil)
                           (set-file-acl filename val)))
                      ((eq attr 'selinux-context)
                       (or (equal val '(nil nil nil nil))
                           (set-file-selinux-context filename val))))
          (setq result nil))))
    result))
-------------------------------------------------------------------------------


>> And I think that there's one case where things would fail with your fix:
>> a linux machine that has selinux disabled will have this as the extended
>> attributes:
>>
>>     ((acl . nil) (selinux-context . (nil nil nil nil)))
>>
>> and your version of `set-file-extended-attributes' would fail when both
>> of these fail.
>
> You mean, a GNU/Linux machine that supports neither ACLs nor SELinux,
> I suppose.  Do such machines exist, and if so, are they popular?

I think that there are still administrators of servers that disable
selinux since dealing with it can be a PITA.  It's at least something
that was popular a few years ago, so I'm guessing that such setups are
still used.  Also, some quick web grepping got me to

    https://wiki.debian.org/SELinux

which says

    The Debian packaged Linux kernels have SELinux support compiled in,
    but disabled by default.
    ...
    Please note that SELinux is a Linux-specific feature and Debian
    packages shouldn't assume it is present


>> With my fix, `file-extended-attributes' would just return nil in
>> that case, and `set-file-extended-attributes' will succeed
>> trivially.
>
> Why should set-file-extended-attributes succeed in this case?  It
> didn't set any extended attributes, right?

Well, it did set all of the specified attributes, since there were none
of them.  My new fix above will succeed in this case because it will
ignore all of them.


> And if neither ACLs nor SELinux is supported, we should definitely
> fall back on chmod for the backup files, shouldn't we?

But chmod is not done on backup files other than copy the original bits
to the backup.  (The failure I had with that was for the fallback file.)

-- 
                    ((x=>x(x))(x=>x(x)))                   Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!

bug#21699: 24.5; Bug in backup-buffer-copy and/or set-file-extended-attributes etc [set-file-extended-attributes]

[Eli Zaretskii]

> Date: Mon, 19 Oct 2015 05:47:36 -0400
> From: Eli Barzilay <eli@barzilay.org>
> Cc: 21699@debbugs.gnu.org
> 
> > I don't know if this is important, but it does change the semantics of
> > an interface that was already released.  So I preferred a fix that
> > didn't involve such changes.
> 
> OK, here's a version that does the decision in
> `set-file-extended-attributes', making it succeed if all of the given
> attributes were set but ignoring the "null" values.  (Again, I verified
> that it works in my case.)
> 
> -------------------------------------------------------------------------------
> (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'.  Value is t if the function succeeds
> in setting all of the given attributes excluding ones that
> indicate \"no information\"."
>   (let ((result t))
>     (dolist (elt attributes)
>       (let ((attr (car elt))
>             (val (cdr elt)))
>         (unless (cond ((eq attr 'acl)
>                        (or (equal val nil)
>                            (set-file-acl filename val)))
>                       ((eq attr 'selinux-context)
>                        (or (equal val '(nil nil nil nil))
>                            (set-file-selinux-context filename val))))
>           (setq result nil))))
>     result))
> -------------------------------------------------------------------------------

Thanks.  I'll let others to express opinions on this alternative vs
the one I committed.  The difference is what happens when all the
attribute values are "null" values: your version returns t in that
case, and I'm not sure that's correct, see below.

> >> With my fix, `file-extended-attributes' would just return nil in
> >> that case, and `set-file-extended-attributes' will succeed
> >> trivially.
> >
> > Why should set-file-extended-attributes succeed in this case?  It
> > didn't set any extended attributes, right?
> 
> Well, it did set all of the specified attributes, since there were none
> of them.  My new fix above will succeed in this case because it will
> ignore all of them.
> 
> 
> > And if neither ACLs nor SELinux is supported, we should definitely
> > fall back on chmod for the backup files, shouldn't we?
> 
> But chmod is not done on backup files other than copy the original bits
> to the backup.

Yes, I'm talking specifically about that scenario.  We should fall
back on chmod in that scenario, shouldn't we?  And if chmod fails, as
it did for you, shouldn't we tell the user about that?

bug#21699: 24.5; Bug in backup-buffer-copy and/or set-file-extended-attributes etc [set-file-extended-attributes]

[Eli Barzilay]

On Mon, Oct 19, 2015 at 6:14 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>
> Thanks.  I'll let others to express opinions on this alternative vs
> the one I committed.  The difference is what happens when all the
> attribute values are "null" values: your version returns t in that
> case, and I'm not sure that's correct, see below.

Ah, you're talking about this code from `backup-buffer-copy':

    (unless (and extended-attributes
                 (with-demoted-errors
                   (set-file-extended-attributes to-name extended-attributes)))
      ...)

In that case, I think that my slightly earlier fix which made
`file-extended-attributes' drop "null" values is actually fine: it means
that in the above snipped `extended-attributes' will be nil, and the
chmod code will run.  There is another use of a similar pattern (look
for an "If set-file-extended-attributes fails" comment which appears in
both places) where this second one should also have the same `and'.

(The current state is messy anyway, since with your current fix, the
`and' in the above is not needed, and anyway, `extended-attributes' is
never nil.)

FWIW, there is no real loss of information for doing that:
`extended-attributes' currently adds acl and selinux entries always,
with my fix (of dropping the no-info values) you can tell when there was
no information for acl/selinux just by the fact that there is no such
element in the `extended-attributes' result.

-- 
                    ((x=>x(x))(x=>x(x)))                   Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!

bug#21699: 24.5; Bug in backup-buffer-copy and/or set-file-extended-attributes etc [set-file-extended-attributes]

[Eli Zaretskii]

> Date: Thu, 22 Oct 2015 01:43:35 -0400
> From: Eli Barzilay <eli@barzilay.org>
> Cc: 21699@debbugs.gnu.org
> 
> On Mon, Oct 19, 2015 at 6:14 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> >
> > Thanks.  I'll let others to express opinions on this alternative vs
> > the one I committed.  The difference is what happens when all the
> > attribute values are "null" values: your version returns t in that
> > case, and I'm not sure that's correct, see below.
> 
> Ah, you're talking about this code from `backup-buffer-copy':
> 
>     (unless (and extended-attributes
>                  (with-demoted-errors
>                    (set-file-extended-attributes to-name extended-attributes)))
>       ...)
> 
> In that case, I think that my slightly earlier fix which made
> `file-extended-attributes' drop "null" values is actually fine: it means
> that in the above snipped `extended-attributes' will be nil, and the
> chmod code will run.  There is another use of a similar pattern (look
> for an "If set-file-extended-attributes fails" comment which appears in
> both places) where this second one should also have the same `and'.
> 
> (The current state is messy anyway, since with your current fix, the
> `and' in the above is not needed, and anyway, `extended-attributes' is
> never nil.)
> 
> FWIW, there is no real loss of information for doing that:
> `extended-attributes' currently adds acl and selinux entries always,
> with my fix (of dropping the no-info values) you can tell when there was
> no information for acl/selinux just by the fact that there is no such
> element in the `extended-attributes' result.

Paul (or anyone else), any second opinions about this?

Thanks.

bug#21699: 24.5; Bug in backup-buffer-copy and/or set-file-extended-attributes etc

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> FWIW, there is no real loss of information for doing that:
>> `extended-attributes' currently adds acl and selinux entries always,
>> with my fix (of dropping the no-info values) you can tell when there was
>> no information for acl/selinux just by the fact that there is no such
>> element in the `extended-attributes' result.
>
> Paul (or anyone else), any second opinions about this?

(I'm going through old bug reports that unfortunately weren't resolved
at the time.)

This was six years ago, and it doesn't seem like anybody had an opinion.
Skimming this bug report, it seems like the fix in f1575763c0d fixed the
reported problem, so I'm closing this bug report.  If I misunderstood,
please respond to the debbugs address and we'll reopen.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no