[libcxx-commits] [libcxx] [libc++] use copy_file_range for fs::copy (PR #109211)

[Louis Dionne via libcxx-commits]

Jannik =?utf-8?q?Glückert?= <jannik.glueckert at gmail.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/109211 at github.com>


================
@@ -194,6 +271,46 @@ bool copy_file_impl(FileDescriptor& read_fd, FileDescriptor& write_fd, error_cod
 
   return true;
 }
+#endif
+
+#if defined(_LIBCPP_FILESYSTEM_USE_COPY_FILE_RANGE) || defined(_LIBCPP_FILESYSTEM_USE_SENDFILE)
+bool copy_file_impl(FileDescriptor& read_fd, FileDescriptor& write_fd, error_code& ec) {
+#  if defined(_LIBCPP_FILESYSTEM_USE_COPY_FILE_RANGE)
+  if (copy_file_impl_copy_file_range(read_fd, write_fd, ec)) {
+    return true;
+  }
+  // EINVAL: src and dst are the same file (this is not cheaply
+  // detectable from userspace)
+  // EINVAL: copy_file_range is unsupported for this file type by the
+  // underlying filesystem
+  // ENOTSUP: undocumented, can arise with old kernels and NFS
+  // EOPNOTSUPP: filesystem does not implement copy_file_range
+  // ETXTBSY: src or dst is an active swapfile (nonsensical, but allowed
+  // with normal copying)
+  // EXDEV: src and dst are on different filesystems that do not support
+  // cross-fs copy_file_range
+  // ENOENT: undocumented, can arise with CIFS
+  // ENOSYS: unsupported by kernel or blocked by seccomp
+  if (ec.value() != EINVAL && ec.value() != ENOTSUP && ec.value() != EOPNOTSUPP && ec.value() != ETXTBSY &&
+      ec.value() != EXDEV && ec.value() != ENOENT && ec.value() != ENOSYS) {
+    return false;
+  }
+  ec.clear();
+#  endif
+
+#  if defined(_LIBCPP_FILESYSTEM_USE_SENDFILE)
+  if (copy_file_impl_sendfile(read_fd, write_fd, ec)) {
+    return true;
+  }
+  // EINVAL: unsupported file type
+  if (ec.value() != EINVAL) {
+    return false;
+  }
+  ec.clear();
+#  endif
+
+  return copy_file_impl_fstream(read_fd, write_fd, ec);
+}
----------------
ldionne wrote:

Ah, I see what I missed now.

I would suggest something like this (which is basically what you wrote above IIUC):

```c++
#if HAS_SENDFILE
copy_file_impl_sendfile() { ... }
#endif

#if HAS_COPY_FILE_RANGE
copy_file_impl_copy_file_range() { ... }
#endif

#if HAS_COPY_FILE
copy_file_impl_copyfile() { ... }
#endif

copy_file_impl_fstream() { ... }


copy_file_impl() {
#if something
  full implementation
#elif something
  full implementation
#else
  full implementation
#endif
}
```

Does that make sense? Decouple the implementation of the different functions and the place where you compose them together, so that we can see at a glance what `copy_file_impl` does when looking at it.

https://github.com/llvm/llvm-project/pull/109211