Thanks for looking into this.
On 3/23/23 07:55, Pádraig Brady wrote:
Perhaps we should use the above __ANDROID__ logic in all cases,
so that we fallback unless there is a specific reason not to.
Yes, that sounds better.
+/* 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. */
I got confused by the terminology here, as "transient failure" has the
opposite connotation from terminal failure. I suggest that we remove the
function (since it's used only once and its name is confusing) and
replace its calling code as suggested below.
- if (errno == EPERM && *total_n_read == 0)
+ if (is_CLONENOTSUP (errno, *total_n_read))
The *total_n_read should be *total_n_read != 0 partly for style and
partly to be portable to ancient compilers that don't properly support
bool. More important, I suggest leaving is_CLONENOTSUP's API as-is, and
changing the 'if' to:
if (*total_n_read == 0 && is_CLONENOTSUP (errno))
as I don't see why we should treat EPERM differently from ENOSYS etc.
/* 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)
Since the code no longer calls is_CLONENOTSUP when FICLONE fails, the
comment should be changed to not mention FICLONE.
+ /* 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);
I had trouble with those two "transient_failure"s lining up, plus the
terminology thing mentioned above. I suggest changing this to:
/* When FICLONE fails, report failure only with errno values known
to mean trouble when FICLONE is supported and called properly.
Do do not report failure merely because !is_CLONENOTSUP (errno),
as too many systems yield oddball errno values here. */
bool report_failure = (errno == EIO || errno == ENOMEM
|| errno == ENOSPC || errno == EDQUOT);
and then replace all occurrences of "transient_failure" with
"report_failure".