On 23/03/2023 13:39, Pádraig Brady wrote:
On 23/03/2023 13:03, Pádraig Brady wrote:
Details at
https://github.com/termux/termux-packages/issues/15706#issuecomment-1481144831
But in summary, the new --reflink errno checking in coreutils 9.2
is not working appropriately on android 4.9 kernels at least.
Proposed patch attached.
It may be better to key on kernel version rather than __ANDROID__
(along the lines of https://github.com/coreutils/coreutils/commit/ba5e6885d),
but that needs more investigation.
Actually it seems the errno checking may be too brittle in general,
https://www.reddit.com/r/Proxmox/comments/11zieha/cpmvinstall_are_broken_in_unprivileged_containers/
Perhaps we should use the above __ANDROID__ logic in all cases,
so that we fallback unless there is a specific reason not to.
The attached does that.
cheers,
Pádraig
From ab90dd977e3ac3ccf91c1e154e24a09ca3729207 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <p...@draigbrady.com>
Date: Thu, 23 Mar 2023 13:19:04 +0000
Subject: [PATCH] copy: --reflink=auto fallback to standard copy in more cases
* src/copy.c (is_transient_failure): A new function refactored
from handle_clone_fail().
(is_CLONENOTSUP): Merge in the handling of EPERM as that also
pertains to determination of whether cloning is supported
if we ever use this function in that context.
(handle_clone_fail): Use is_transient_failure() in all cases,
so that we assume a terminal failure in less errno cases.
* NEWS: Mention the bug fix.
Addresses https://bugs.gnu.org/62404
---
NEWS | 8 ++++++++
src/copy.c | 41 +++++++++++++++++++++++++----------------
2 files changed, 33 insertions(+), 16 deletions(-)
diff --git a/NEWS b/NEWS
index 030f0e543..73801d4bf 100644
--- a/NEWS
+++ b/NEWS
@@ -2,6 +2,14 @@ GNU coreutils NEWS -*- outline -*-
* Noteworthy changes in release ?.? (????-??-??) [?]
+** Bug fixes
+
+ cp --relink=auto (the default), mv, and install will
+ fall back to a standard copy in more cases.
+ Previously copies could fail with "Permission denied" errors etc.
+ on older kernels or unprivileged containers on ZFS etc.
+ [issue introduced in coreutils-9.2]
+
* Noteworthy changes in release 9.2 (2023-03-20) [stable]
diff --git a/src/copy.c b/src/copy.c
index 39197872c..19a605bd5 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -278,15 +278,28 @@ create_hole (int fd, char const *name, bool punch_holes, off_t size)
}
+/* Whether the errno indicates operation is a transient failure.
+ I.e., a failure that would indicate the operation _is_ supported,
+ but has failed in a terminal way. */
+
+static bool
+is_transient_failure (int err)
+{
+ return err == EIO || err == ENOMEM
+ || err == ENOSPC || err == EDQUOT;
+}
+
+
/* Whether the errno from FICLONE, or copy_file_range
indicates operation is not supported for this file or file system. */
static bool
-is_CLONENOTSUP (int err)
+is_CLONENOTSUP (int err, bool partial)
{
return err == ENOSYS || is_ENOTSUP (err)
|| err == EINVAL || err == EBADF
- || err == EXDEV || err == ETXTBSY;
+ || err == EXDEV || err == ETXTBSY
+ || (err == EPERM && ! partial);
}
@@ -339,15 +352,13 @@ sparse_copy (int src_fd, int dest_fd, char **abuf, size_t buf_size,
{
copy_debug.offload = COPY_DEBUG_UNSUPPORTED;
- if (is_CLONENOTSUP (errno))
- break;
-
- /* copy_file_range might not be enabled in seccomp filters,
+ /* Consider EPERM if not copied any data yet. EPERM could occur
+ if copy_file_range not be enabled in seccomp filters,
so retry with a standard copy. EPERM can also occur
for immutable files, but that would only be in the edge case
where the file is made immutable after creating/truncating,
in which case the (more accurate) error is still shown. */
- if (errno == EPERM && *total_n_read == 0)
+ if (is_CLONENOTSUP (errno, *total_n_read))
break;
/* ENOENT was seen sometimes across CIFS shares, resulting in
@@ -1172,15 +1183,13 @@ handle_clone_fail (int dst_dirfd, char const* dst_relname,
char const* src_name, char const* dst_name,
int dest_desc, bool new_dst, enum Reflink_type reflink_mode)
{
- /* If the clone operation is creating the destination,
- then don't try and cater for all non transient file system errors,
- and instead only cater for specific transient errors. */
- bool transient_failure;
- if (dest_desc < 0) /* currently for fclonefileat(). */
- transient_failure = errno == EIO || errno == ENOMEM
- || errno == ENOSPC || errno == EDQUOT;
- else /* currently for FICLONE. */
- transient_failure = ! is_CLONENOTSUP (errno);
+ /* If the clone operation is not creating the destination (i.e., FICLONE)
+ then we could possibly be more restrictive with errors deemed non terminal.
+ However doing that like in the following
+ would be more coupled to disparate errno handling on various systems.
+ if (0 <= dest_desc)
+ transient_failure = ! is_CLONENOTSUP (errno, false); */
+ bool transient_failure = is_transient_failure (errno);
if (reflink_mode == REFLINK_ALWAYS || transient_failure)
error (0, errno, _("failed to clone %s from %s"),
--
2.26.2
