🚀 go-pugleaf

On 2023-08-26 01:22, Po Lu wrote:
> Given that, I'll resort to disrearding such errors from set-file-times
> only on the pertinent filesystems instead.

Will this be done by extending emacs/src/androidvfs.c to add vops for
futimens and utimensat, or by some other means? I'm a bit unclear about
the division of labor between Gnulib and androidvfs.c in this area.

cc'ing to bug-gnulib. For those following there, the original thread
starts at:

https://lists.gnu.org/r/emacs-devel/2023-08/msg01004.html

and is talking about this patch to Emacs:

https://git.savannah.gnu.org/cgit/emacs.git/commit/?id49798a9b81fb4f7f8e1e1963ea9039a4a68a471

> From: Po Lu <luangruo@yahoo.com>
> Cc: Bruno Haible <bruno@clisp.org>,  emacs-devel@gnu.org,  Paul Eggert
>  <eggert@cs.ucla.edu>
> Date: Sat, 26 Aug 2023 16:22:32 +0800
>
> Eli Zaretskii <eliz@gnu.org> writes:
>
> >> diff --git a/lisp/files.el b/lisp/files.el
> >> index 1803eb9..a015dd3 100644
> >> --- a/lisp/files.el
> >> +++ b/lisp/files.el
> >> @@ -6622,7 +6622,11 @@ copy-directory
> >>                                      (file-attributes directory))))
> >>               (follow-flag (unless follow 'nofollow)))
> >>           (if modes (set-file-modes newname modes follow-flag))
> >> -         (if times (set-file-times newname times follow-flag)))))))
> >> +         (when times
> >> +            ;; Don't didactically fail if file times can't be set, as
> >> +            ;; some file systems forbid modifying them.
> >> +            (with-demoted-errors "Setting file times: %s"
> >> +              (set-file-times newname times follow-flag))))))))
> >
> > I think we should only demote these errors on Android, not on other
> > systems.  Setting correct file times when copying/modifying files is
> > an important feature, and users should be alerted when it somehow
> > fails, unless the failure is expected.  And it only is expected on
> > Android, AFAIU.
>
> Given that, I'll resort to disrearding such errors from set-file-times
> only on the pertinent filesystems instead.

Thanks.

> > Alternatively, the Gnulib folks (CC'd) should modify their fdutimens
> > replacement to return ENOSYS/ENOTSUP on Android filesystems.
>
> Gnulib is not relevant here, as the ``filesystems'' which fail are
> implemented within androidvfs.c.

Maybe I'm misunderstanding, but the comment in fileio.c seems to imply
that the problem is with Gnulib's replacement for futimens:

      if (futimens (ofd, ts) != 0
	  /* Various versions of the Android C library are missing
	     futimens, prompting Gnulib to install a fallback that
	     uses fdutimens instead.  However, fdutimens is not
	     supported on many Android kernels, so just silently fail
	     if errno is ENOTSUP or ENOSYS.  */

Eli Zaretskii <eliz@gnu.org> writes:

>> diff --git a/lisp/files.el b/lisp/files.el
>> index 1803eb9..a015dd3 100644
>> --- a/lisp/files.el
>> +++ b/lisp/files.el
>> @@ -6622,7 +6622,11 @@ copy-directory
>>                                      (file-attributes directory))))
>>               (follow-flag (unless follow 'nofollow)))
>>           (if modes (set-file-modes newname modes follow-flag))
>> -         (if times (set-file-times newname times follow-flag)))))))
>> +         (when times
>> +            ;; Don't didactically fail if file times can't be set, as
>> +            ;; some file systems forbid modifying them.
>> +            (with-demoted-errors "Setting file times: %s"
>> +              (set-file-times newname times follow-flag))))))))
>
> I think we should only demote these errors on Android, not on other
> systems.  Setting correct file times when copying/modifying files is
> an important feature, and users should be alerted when it somehow
> fails, unless the failure is expected.  And it only is expected on
> Android, AFAIU.

Given that, I'll resort to disrearding such errors from set-file-times
only on the pertinent filesystems instead.

> Alternatively, the Gnulib folks (CC'd) should modify their fdutimens
> replacement to return ENOSYS/ENOTSUP on Android filesystems.

Gnulib is not relevant here, as the ``filesystems'' which fail are
implemented within androidvfs.c.

Eli Zaretskii <eliz@gnu.org> writes:

> Maybe I'm misunderstanding, but the comment in fileio.c seems to imply
> that the problem is with Gnulib's replacement for futimens:
>
>       if (futimens (ofd, ts) != 0
> 	  /* Various versions of the Android C library are missing
> 	     futimens, prompting Gnulib to install a fallback that
> 	     uses fdutimens instead.  However, fdutimens is not
> 	     supported on many Android kernels, so just silently fail
> 	     if errno is ENOTSUP or ENOSYS.  */

The comment in fileio.c I revised is orthogonal to the change to
files.el; I just fixed a typo while reviewing code adjascent to
set-file-times.

Paul Eggert wrote:
> https://git.savannah.gnu.org/cgit/emacs.git/commit/?id49798a9b81fb4f7f8e1e1963ea9039a4a68a471

Isn't there something missing in this function 'copy-directory',
around emacs/lisp/files.el line 6624? I see code for preserving the modes
and the times, but not for preserving the owner.

For comparison, 'mv' from GNU coreutils 9.3.147-d553ab contains also code
for preserving the owner, and this code sometimes gives diagnostics that
are ignored, in the sense that 'mv' terminates with exit code 0.

See:
$ ldd mv
        linux-vdso.so.1 (0x00007fff51f73000)
        libselinux.so.1 => /lib/x86_64-linux-gnu/libselinux.so.1 (0x00007f856994a000)
        libacl.so.1 => /lib/x86_64-linux-gnu/libacl.so.1 (0x00007f8569940000)
        libattr.so.1 => /lib/x86_64-linux-gnu/libattr.so.1 (0x00007f8569938000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f8569710000)
        libpcre2-8.so.0 => /lib/x86_64-linux-gnu/libpcre2-8.so.0 (0x00007f8569679000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f8569998000)
$ mkdir dir1
$ echo foo > dir1/file1
$ mv dir1 /media/nas/bruno/dir1
mv: failed to preserve ownership for '/media/nas/bruno/dir1': Permission denied
$ echo $?
0

Here /media/nas/bruno/dir1 is on a CIFS version 1 file system, and the
'strace' log from the 'mv' command shows the essential syscalls:

utimensat(AT_FDCWD, "/media/nas/bruno/dir1", [{tv_sec93067148, tv_nsec5342620} /* 2023-08-26T18:25:48.145342620+0200 */, {tv_sec93067154, tv_nsecq7388257} /* 2023-08-26T18:25:54.717388257+0200 */], 0) = 0
fchownat(AT_FDCWD, "/media/nas/bruno/dir1", 1000, 1000, AT_SYMLINK_NOFOLLOW) = -1 EACCES (Permission denied)

Note: In earlier versions of coreutils 'mv', instead of an fchownat call, we
saw an equivalent lchown call:
lchown("/media/nas/bruno/dir1", 1000, 1000) = -1 EACCES (Permission denied)

Bruno

> From: Bruno Haible <bruno@clisp.org>
> Cc: emacs-devel@gnu.org, bug-gnulib@gnu.org
> Date: Sat, 26 Aug 2023 18:40:20 +0200
>
> Paul Eggert wrote:
> > https://git.savannah.gnu.org/cgit/emacs.git/commit/?id49798a9b81fb4f7f8e1e1963ea9039a4a68a471
>
> Isn't there something missing in this function 'copy-directory',
> around emacs/lisp/files.el line 6624? I see code for preserving the modes
> and the times, but not for preserving the owner.

See

  https://lists.gnu.org/archive/html/emacs-devel/2009-10/msg00094.html

and the ensuing discussion, where we eventually decided not to do
that.