[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:

I would rewrite this as follows, I think that makes it easier to understand and we duplicate very little code:

```c++
#if defined(_LIBCPP_FILESYSTEM_USE_COPY_FILE_RANGE)

bool copy_file_impl(FileDescriptor& read_fd, FileDescriptor& write_fd, error_code& ec) {
  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();

  return copy_file_impl_fstream(read_fd, write_fd, ec);
}

#elif defined(_LIBCPP_FILESYSTEM_USE_SENDFILE)

bool copy_file_impl(FileDescriptor& read_fd, FileDescriptor& write_fd, error_code& ec) {
  if (copy_file_impl_sendfile(read_fd, write_fd, ec)) {
    return true;
  }
  // EINVAL: unsupported file type
  if (ec.value() != EINVAL) {
    return false;
  }
  ec.clear();

  return copy_file_impl_fstream(read_fd, write_fd, ec);
}
 
#elif defined(_LIBCPP_FILESYSTEM_USE_COPYFILE)

...
```

And once you rewrite like that, you could also consider inlining `copy_file_impl_copy_file_range` and `copy_file_impl_sendfile` into their respective `copy_file_impl`. IDK if that would simplify the code but it might.

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