From ddcaff9a10a84ed34353dbafd65e3c0693dd0b35 Mon Sep 17 00:00:00 2001 From: Stefan Bender Date: Mon, 14 Oct 2019 17:02:58 +0200 Subject: [PATCH] unix: update uv_fs_copyfile() fallback logic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit fixes uv_fs_copyfile() in cases where an unknown error occurs when copy-on-write is requested by setting UV_FS_COPYFILE_FICLONE. The original approach tried to catch some of the errors raised by the ioctl() call, assuming that sendfile() would also fail in those cases. This is not necessarily true, as some variants of ioctl() also raise EINVAL (some maybe EBADF), but sendfile() works just fine. This patch reverses the logic, falling back to sendfile() in any case where ioctl() returns an error. In other words, it tries much harder to make uv_fs_copyfile() work. Related to that, the original approach returned UV_ENOTSUP unconditionally in cases where ioctl() failed and UV_FS_COPYFILE_FICLONE_FORCE was set. However, ioctl() may have failed for other reasons than being not supported. The function now returns the actual error raised by ioctl(), leaving it to the caller to deal with it. Fixes: https://github.com/libuv/libuv/issues/2483 PR-URL: https://github.com/libuv/libuv/pull/2514 Reviewed-By: Ben Noordhuis Reviewed-By: Saúl Ibarra Corretgé Reviewed-By: Colin Ihrig --- docs/src/fs.rst | 11 +++++++++-- src/unix/fs.c | 20 ++++++++------------ test/test-fs-copyfile.c | 2 +- 3 files changed, 18 insertions(+), 15 deletions(-) diff --git a/docs/src/fs.rst b/docs/src/fs.rst index 2ef49b0a..dc16ff08 100644 --- a/docs/src/fs.rst +++ b/docs/src/fs.rst @@ -359,10 +359,13 @@ API is to overwrite the destination if it exists. - `UV_FS_COPYFILE_FICLONE`: If present, `uv_fs_copyfile()` will attempt to create a copy-on-write reflink. If the underlying platform does not - support copy-on-write, then a fallback copy mechanism is used. + support copy-on-write, or an error occurs while attempting to use + copy-on-write, a fallback copy mechanism based on + :c:func:`uv_fs_sendfile()` is used. - `UV_FS_COPYFILE_FICLONE_FORCE`: If present, `uv_fs_copyfile()` will attempt to create a copy-on-write reflink. If the underlying platform does - not support copy-on-write, then an error is returned. + not support copy-on-write, or an error occurs while attempting to use + copy-on-write, then an error is returned. .. warning:: If the destination path is created, but an error occurs while copying @@ -375,6 +378,10 @@ API .. versionchanged:: 1.20.0 `UV_FS_COPYFILE_FICLONE` and `UV_FS_COPYFILE_FICLONE_FORCE` are supported. + .. versionchanged:: 1.33.0 If an error occurs while using + `UV_FS_COPYFILE_FICLONE_FORCE`, that error is returned. Previously, + all errors were mapped to `UV_ENOTSUP`. + .. c:function:: int uv_fs_sendfile(uv_loop_t* loop, uv_fs_t* req, uv_file out_fd, uv_file in_fd, int64_t in_offset, size_t length, uv_fs_cb cb) Limited equivalent to :man:`sendfile(2)`. diff --git a/src/unix/fs.c b/src/unix/fs.c index f812b24e..b37cfbbc 100644 --- a/src/unix/fs.c +++ b/src/unix/fs.c @@ -1055,18 +1055,14 @@ static ssize_t uv__fs_copyfile(uv_fs_t* req) { #ifdef FICLONE if (req->flags & UV_FS_COPYFILE_FICLONE || req->flags & UV_FS_COPYFILE_FICLONE_FORCE) { - if (ioctl(dstfd, FICLONE, srcfd) == -1) { - /* If an error occurred that the sendfile fallback also won't handle, or - this is a force clone then exit. Otherwise, fall through to try using - sendfile(). */ - if (errno != ENOTTY && errno != EOPNOTSUPP && errno != EXDEV) { - err = UV__ERR(errno); - goto out; - } else if (req->flags & UV_FS_COPYFILE_FICLONE_FORCE) { - err = UV_ENOTSUP; - goto out; - } - } else { + if (ioctl(dstfd, FICLONE, srcfd) == 0) { + /* ioctl() with FICLONE succeeded. */ + goto out; + } + /* If an error occurred and force was set, return the error to the caller; + * fall back to sendfile() when force was not set. */ + if (req->flags & UV_FS_COPYFILE_FICLONE_FORCE) { + err = UV__ERR(errno); goto out; } } diff --git a/test/test-fs-copyfile.c b/test/test-fs-copyfile.c index def3d967..c3e698e5 100644 --- a/test/test-fs-copyfile.c +++ b/test/test-fs-copyfile.c @@ -188,7 +188,7 @@ TEST_IMPL(fs_copyfile) { unlink(dst); r = uv_fs_copyfile(NULL, &req, fixture, dst, UV_FS_COPYFILE_FICLONE_FORCE, NULL); - ASSERT(r == 0 || r == UV_ENOSYS || r == UV_ENOTSUP); + ASSERT(r <= 0); if (r == 0) handle_result(&req);