[libcxx-commits] [PATCH] D60880: [libc++] Use COPYFILE_CLONE from the macOS copyfile(3) API to support APFS clones

Thu Apr 18 11:33:08 PDT 2019

ldionne marked an inline comment as done.
ldionne added a comment.

In D60880#1472059 <https://reviews.llvm.org/D60880#1472059>, @aprantl wrote:

> According to the documentation COPYFILE_CLONE will clone a symlink rather than its contents, whereas COPY_FILE_DATA will follow the link. That's why I'm checking for symlinks in https://reviews.llvm.org/D60802. I don't know whether copy_file_impl_copyfile makes any guarantees.

I looked at the code that calls `copyfile_impl`, and it gives an error if the source file is not a regular file here <https://github.com/llvm-mirror/libcxx/blob/master/src/filesystem/operations.cpp#L753>. I assumed it meant it wasn't a symlink, but maybe I made a mistake.

> I assume the availability check isn't necessary because libcxx won't back-deploy to 10.11?

This _is_ the dylib, so we're not back-deploying it to 10.11. If it were in the headers, then we'd need to check that the underlying system supports this functionality, yes.

================
Comment at: libcxx/src/filesystem/operations.cpp:681
   CopyFileState cfs;
-  if (fcopyfile(read_fd.fd, write_fd.fd, cfs.state, COPYFILE_DATA) < 0) {
+  if (fcopyfile(read_fd.fd, write_fd.fd, cfs.state, COPYFILE_CLONE) < 0) {
     ec = capture_errno();
----------------
aprantl wrote:
> It looks like you may be able to pass `nullptr` as the third parameter if you don't need the state afterwards.
Hmm, I think that's correct:

> If the state parameter is the return value from `copyfile_state_alloc()`, then `copyfile()` and `fcopyfile()` will use the information from the state object; if it is `NULL`, then both functions will work normally, but less control will be available to the caller.

So we could get rid of the `CopyFileState` helper and instead just pass `nullptr` -- is that your suggestion?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60880/new/

https://reviews.llvm.org/D60880