On 2/10/23 13:35, George Valkov wrote:
Since the source and it’s clone have separate metadata,
it should be possible to change it on the clone, to comply with standards.
Attached is a hacky patch to do that. It also uses the new CLONE_ACL
flag. The old code apparently mishandled ACLs; the new code tries to do
a better job.
This whole area is a mess, but the patch should improve correctness on
macOS, while also being efficient in the usual case.
It feels more natural when the metadata is kept intact for the two files.
That depends on the application. The longstanding tradition for cp is to
preserve metadata if you use 'cp -p', and to not preserve with plain cp.
For interactive use you can use an alias or a little script if you
prefer -p to be enabled by default.
Here is a good example: I create a tarball on my OpenWRT router. Extract on the
Mac.
That's fine, because 'tar' historically has preserved the timestamp and
(if you're root) permissions. Plain cp has not.
I found this link in one of the mailing lists you sent, it explains what was
wrong
with the original SEEK_DATA and SEEK_HOLE approach on macOS
GNU cp does a pass over the file with SEEK_HOLE and SEEK_DATA, so if I
understand things correctly it shouldn't run into the problem mentioned
there.
In other words, that email doesn't appear to explain our current problem.
From 9f2c0ab64925a70e287a92e9928100c7cd6368c2 Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Fri, 10 Feb 2023 13:34:54 -0800
Subject: [PATCH] cp: improve use of fclonefileat
* src/copy.c (copy_reg): Use CLONE_ACL if available.
If the only problem with fclonefileat is that it would generate
the wrong timestamp, or would create the file with too few
permissions, use it and fix the mode and timestamp afterwards,
rather than falling back on a traditional copy.
---
src/copy.c | 48 +++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 41 insertions(+), 7 deletions(-)
diff --git a/src/copy.c b/src/copy.c
index dfbb557de..04f71eb2f 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1146,6 +1146,7 @@ copy_reg (char const *src_name, char const *dst_name,
union scan_inference scan_inference;
bool return_val = true;
bool data_copy_required = x->data_copy_required;
+ bool mode_already_preserved = false;
bool preserve_xattr = USE_XATTR & x->preserve_xattr;
source_desc = open (src_name,
@@ -1244,17 +1245,45 @@ copy_reg (char const *src_name, char const *dst_name,
if (*new_dst)
{
#if HAVE_FCLONEFILEAT && !USE_XATTR
-/* CLONE_NOOWNERCOPY only available on macos >= 10.13. */
+# ifndef CLONE_ACL
+# define CLONE_ACL 0 /* Added in macOS 12.6. */
+# endif
# ifndef CLONE_NOOWNERCOPY
-# define CLONE_NOOWNERCOPY 0
+# define CLONE_NOOWNERCOPY 0 /* Added in macOS 10.13. */
# endif
- int fc_flags = x->preserve_ownership ? 0 : CLONE_NOOWNERCOPY;
+ mode_t cloned_mode_bits = S_ISVTX | S_IRWXUGO;
+ mode_t cloned_mode = src_mode & cloned_mode_bits;
+ int fc_flags = ((x->preserve_mode ? CLONE_ACL : 0)
+ | (x->preserve_ownership ? 0 : CLONE_NOOWNERCOPY));
if (data_copy_required && x->reflink_mode
- && x->preserve_mode && x->preserve_timestamps
+ && (x->preserve_mode || ! (cloned_mode & ~dst_mode))
&& (x->preserve_ownership || CLONE_NOOWNERCOPY))
{
+ fprintf(stderr, "calling fclonefileat...\n");
if (fclonefileat (source_desc, dst_dirfd, dst_relname, fc_flags) == 0)
- goto close_src_desc;
+ {
+ fprintf(stderr, "fclonefileat succeeded\n");
+ if (!x->preserve_timestamps)
+ {
+ struct timespec timespec[2];
+ timespec[0].tv_nsec = timespec[1].tv_nsec = UTIME_NOW;
+ fprintf(stderr, "calling utimensat...\n");
+ if (utimensat (dst_dirfd, dst_relname, timespec,
+ AT_SYMLINK_NOFOLLOW)
+ != 0)
+ {
+ error (0, errno, _("updating times for %s"),
+ quoteaf (dst_name));
+ return_val = false;
+ }
+ fprintf(stderr, "utimensat succeeded\n");
+ }
+ mode_already_preserved = (fc_flags & CLONE_ACL) != 0;
+ dest_desc = -1;
+ omitted_permissions = dst_mode & ~cloned_mode;
+ extra_permissions = 0;
+ goto set_dest_mode;
+ }
else if (! handle_clone_fail (dst_dirfd, dst_relname, src_name,
dst_name,
-1, false /* We didn't create dst */,
@@ -1485,9 +1514,14 @@ copy_reg (char const *src_name, char const *dst_name,
set_author (dst_name, dest_desc, src_sb);
+#if HAVE_FCLONEFILEAT && !USE_XATTR
+set_dest_mode:
+#endif
if (x->preserve_mode || x->move_mode)
{
- if (copy_acl (src_name, source_desc, dst_name, dest_desc, src_mode) != 0
+ if (!mode_already_preserved
+ && (copy_acl (src_name, source_desc, dst_name, dest_desc, src_mode)
+ != 0)
&& x->require_preserve)
return_val = false;
}
@@ -1517,7 +1551,7 @@ copy_reg (char const *src_name, char const *dst_name,
}
close_src_and_dst_desc:
- if (close (dest_desc) < 0)
+ if (0 <= dest_desc && close (dest_desc) < 0)
{
error (0, errno, _("failed to close %s"), quoteaf (dst_name));
return_val = false;
--
2.39.1
