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

[Eli Barzilay <eli@barzilay.org>]

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!