Re: master 349798a9b8: Demote errors from utimensat copying directories

Re: master 349798a9b8: Demote errors from utimensat copying directories

[Eli Zaretskii]

> 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.

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

Re: master 349798a9b8: Demote errors from utimensat copying directories

[Po Lu]

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.

Re: master 349798a9b8: Demote errors from utimensat copying directories

[Eli Zaretskii]

> 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.  */

Re: master 349798a9b8: Demote errors from utimensat copying directories

[Po Lu]

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.

Re: master 349798a9b8: Demote errors from utimensat copying directories

[Paul Eggert]

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/?id=349798a9b81fb4f7f8e1e1963ea9039a4a68a471

Re: errors from fchownat copying directories

[Bruno Haible]

Paul Eggert wrote:
> https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=349798a9b81fb4f7f8e1e1963ea9039a4a68a471

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_sec=1693067148, tv_nsec=145342620} /* 2023-08-26T18:25:48.145342620+0200 */, {tv_sec=1693067154, tv_nsec=717388257} /* 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

Re: errors from fchownat copying directories

[Eli Zaretskii]

> 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/?id=349798a9b81fb4f7f8e1e1963ea9039a4a68a471
> 
> 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.