From patchwork Fri Aug 31 20:07:01 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "H.J. Lu" X-Patchwork-Id: 29151 Received: (qmail 69537 invoked by alias); 31 Aug 2018 20:07:07 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 69520 invoked by uid 89); 31 Aug 2018 20:07:07 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.3 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-oi0-f68.google.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=9KOXBdannUnoIR9Ktn/iDgudPw7LhgftWqH1a60yP3Q=; b=YMxci2WExxOZ1V0gGLOguaiYV5qP2VVJ1VUMCHVK23LrMhMJvGfczHzV4yAcySIQ3y 6CVkKlw/gg0s8q3HbqyECGnVj0RqoMFTR30zJKA+RaDb14tZ7lbeRTAhhXkktle9i/TB 1WCUm/MX+0JmU8PrpnQHom9k3Sib0VR90D2KdHCJYJM7E+GGrSkBp6qagga2V8ut8Fa1 7WSxUUxbrqvTtcboxGUka/Fmo2KdQSLOauNzODrhtNsSPdqk/9D2Wy10YaZ0dwuwQd2e b6AHcXxeCENYPSylaBA55WdRsfObQ0E6Wcv85CKT0UD19nO5cwK2KM3K/T1mXYs1Vazo y0Bw== MIME-Version: 1.0 In-Reply-To: <01ea0257-473f-d32b-3079-a466bc63462b@redhat.com> References: <01ea0257-473f-d32b-3079-a466bc63462b@redhat.com> From: "H.J. Lu" Date: Fri, 31 Aug 2018 13:07:01 -0700 Message-ID: Subject: Re: V3 [PATCH] test-container: Use xcopy_file_range for cross-device copy [BZ #23597] To: "Carlos O'Donell" Cc: Adhemerval Zanella , GNU C Library On Fri, Aug 31, 2018 at 12:56 PM, Carlos O'Donell wrote: > On 08/31/2018 03:30 PM, H.J. Lu wrote: >> From 00e519806af9a7381b1f10d823bcaea5a799916b Mon Sep 17 00:00:00 2001 >> From: "H.J. Lu" >> Date: Fri, 31 Aug 2018 11:26:16 -0700 >> Subject: [PATCH] test-container: Use xcopy_file_range for cross-device copy >> [BZ #23597] >> >> copy_file_range can't be used to copy a file from glibc source directory >> to glibc build directory since they may be on different filesystems. >> This patch adds xcopy_file_range for cross-device copy. >> >> [BZ #23597] >> * support/Makefile (libsupport-routines): Add >> support_copy_file_range and xcopy_file_range. >> * support/support.h: Include . >> (support_copy_file_range): New prototype. >> * support/support_copy_file_range.c: New file. Copied and >> modified from io/copy_file_range-compat.c. >> * support/test-container.c (copy_one_file): Call xcopy_file_rang >> instead of copy_file_range. >> * support/xcopy_file_range.c: New file. >> * support/xunistd.h (xcopy_file_range): New prototype. > > OK if you fix the following: > > - xcopy_file_range should continue to return ssize_t (the number of bytes > copied). > > Reviewed-by: Carlos O'Donell > >> --- >> support/Makefile | 2 + >> support/support.h | 5 ++ >> support/support_copy_file_range.c | 143 ++++++++++++++++++++++++++++++ >> support/test-container.c | 3 +- >> support/xcopy_file_range.c | 31 +++++++ >> support/xunistd.h | 3 + >> 6 files changed, 185 insertions(+), 2 deletions(-) >> create mode 100644 support/support_copy_file_range.c >> create mode 100644 support/xcopy_file_range.c >> >> diff --git a/support/Makefile b/support/Makefile >> index b528f538a6..545bfa2727 100644 >> --- a/support/Makefile >> +++ b/support/Makefile >> @@ -43,6 +43,7 @@ libsupport-routines = \ >> support_capture_subprocess \ >> support_capture_subprocess_check \ >> support_chroot \ >> + support_copy_file_range \ > > OK. > >> support_descriptor_supports_holes \ >> support_enter_mount_namespace \ >> support_enter_network_namespace \ >> @@ -74,6 +75,7 @@ libsupport-routines = \ >> xchroot \ >> xclose \ >> xconnect \ >> + xcopy_file_range \ > > OK. > >> xdlfcn \ >> xdup2 \ >> xfclose \ >> diff --git a/support/support.h b/support/support.h >> index c6ff4bafb4..d0e15bca1d 100644 >> --- a/support/support.h >> +++ b/support/support.h >> @@ -27,6 +27,8 @@ >> #include >> /* For mode_t. */ >> #include >> +/* For ssize_t and off64_t. */ >> +#include > > OK. > >> >> __BEGIN_DECLS >> >> @@ -94,6 +96,9 @@ extern const char support_install_prefix[]; >> /* Corresponds to the install's lib/ or lib64/ directory. */ >> extern const char support_libdir_prefix[]; >> >> +extern ssize_t support_copy_file_range (int, off64_t *, int, off64_t *, >> + size_t, unsigned int); > > OK. > >> + >> __END_DECLS >> >> #endif /* SUPPORT_H */ >> diff --git a/support/support_copy_file_range.c b/support/support_copy_file_range.c >> new file mode 100644 >> index 0000000000..9a1e39773e >> --- /dev/null >> +++ b/support/support_copy_file_range.c >> @@ -0,0 +1,143 @@ >> +/* Simplified copy_file_range with cross-device copy. >> + Copyright (C) 2018 Free Software Foundation, Inc. >> + This file is part of the GNU C Library. >> + >> + The GNU C Library is free software; you can redistribute it and/or >> + modify it under the terms of the GNU Lesser General Public >> + License as published by the Free Software Foundation; either >> + version 2.1 of the License, or (at your option) any later version. >> + >> + The GNU C Library is distributed in the hope that it will be useful, >> + but WITHOUT ANY WARRANTY; without even the implied warranty of >> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + Lesser General Public License for more details. >> + >> + You should have received a copy of the GNU Lesser General Public >> + License along with the GNU C Library; if not, see >> + . */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +ssize_t >> +support_copy_file_range (int infd, __off64_t *pinoff, >> + int outfd, __off64_t *poutoff, >> + size_t length, unsigned int flags) > > OK. > >> +{ >> + if (flags != 0) >> + { >> + errno = EINVAL; >> + return -1; >> + } >> + >> + struct stat64 instat; >> + struct stat64 outstat; >> + if (fstat64 (infd, &instat) != 0 || fstat64 (outfd, &outstat) != 0) >> + return -1; >> + if (S_ISDIR (instat.st_mode) || S_ISDIR (outstat.st_mode)) >> + { >> + errno = EISDIR; >> + return -1; >> + } >> + if (!S_ISREG (instat.st_mode) || !S_ISREG (outstat.st_mode)) >> + { >> + /* We need a regular input file so that the we can seek >> + backwards in case of a write failure. */ >> + errno = EINVAL; >> + return -1; >> + } >> + >> + /* The output descriptor must not have O_APPEND set. */ >> + if (fcntl (outfd, F_GETFL) & O_APPEND) >> + { >> + errno = EBADF; >> + return -1; >> + } >> + >> + /* Avoid an overflow in the result. */ >> + if (length > SSIZE_MAX) >> + length = SSIZE_MAX; >> + >> + /* Main copying loop. The buffer size is arbitrary and is a >> + trade-off between stack size consumption, cache usage, and >> + amortization of system call overhead. */ >> + size_t copied = 0; >> + char buf[8192]; >> + while (length > 0) >> + { >> + size_t to_read = length; >> + if (to_read > sizeof (buf)) >> + to_read = sizeof (buf); >> + >> + /* Fill the buffer. */ >> + ssize_t read_count; >> + if (pinoff == NULL) >> + read_count = read (infd, buf, to_read); >> + else >> + read_count = pread64 (infd, buf, to_read, *pinoff); >> + if (read_count == 0) >> + /* End of file reached prematurely. */ >> + return copied; >> + if (read_count < 0) >> + { >> + if (copied > 0) >> + /* Report the number of bytes copied so far. */ >> + return copied; >> + return -1; >> + } >> + if (pinoff != NULL) >> + *pinoff += read_count; >> + >> + /* Write the buffer part which was read to the destination. */ >> + char *end = buf + read_count; >> + for (char *p = buf; p < end; ) >> + { >> + ssize_t write_count; >> + if (poutoff == NULL) >> + write_count = write (outfd, p, end - p); >> + else >> + write_count = pwrite64 (outfd, p, end - p, *poutoff); >> + if (write_count < 0) >> + { >> + /* Adjust the input read position to match what we have >> + written, so that the caller can pick up after the >> + error. */ >> + size_t written = p - buf; >> + /* NB: This needs to be signed so that we can form the >> + negative value below. */ >> + ssize_t overread = read_count - written; >> + if (pinoff == NULL) >> + { >> + if (overread > 0) >> + { >> + /* We are on an error recovery path, so we >> + cannot deal with failure here. */ >> + int save_errno = errno; >> + (void) lseek64 (infd, -overread, SEEK_CUR); >> + errno = save_errno; >> + } >> + } >> + else /* pinoff != NULL */ >> + *pinoff -= overread; >> + >> + if (copied + written > 0) >> + /* Report the number of bytes copied so far. */ >> + return copied + written; >> + return -1; >> + } >> + p += write_count; >> + if (poutoff != NULL) >> + *poutoff += write_count; >> + } /* Write loop. */ >> + >> + copied += read_count; >> + length -= read_count; >> + } >> + return copied; > > OK. > >> +} >> diff --git a/support/test-container.c b/support/test-container.c >> index 2e91bdf9ec..c56b53ed81 100644 >> --- a/support/test-container.c >> +++ b/support/test-container.c >> @@ -383,8 +383,7 @@ copy_one_file (const char *sname, const char *dname) >> if (dfd < 0) >> FAIL_EXIT1 ("unable to open %s for writing\n", dname); >> >> - if (copy_file_range (sfd, 0, dfd, 0, st.st_size, 0) != st.st_size) >> - FAIL_EXIT1 ("cannot copy file %s to %s\n", sname, dname); >> + xcopy_file_range (sfd, 0, dfd, 0, st.st_size, 0); > > OK. Exactly, just the bare copy which we expect to succeed. > >> >> xclose (sfd); >> xclose (dfd); >> diff --git a/support/xcopy_file_range.c b/support/xcopy_file_range.c >> new file mode 100644 >> index 0000000000..2df6b1cc16 >> --- /dev/null >> +++ b/support/xcopy_file_range.c >> @@ -0,0 +1,31 @@ >> +/* copy_file_range with error checking. >> + Copyright (C) 2018 Free Software Foundation, Inc. >> + This file is part of the GNU C Library. >> + >> + The GNU C Library is free software; you can redistribute it and/or >> + modify it under the terms of the GNU Lesser General Public >> + License as published by the Free Software Foundation; either >> + version 2.1 of the License, or (at your option) any later version. >> + >> + The GNU C Library is distributed in the hope that it will be useful, >> + but WITHOUT ANY WARRANTY; without even the implied warranty of >> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + Lesser General Public License for more details. >> + >> + You should have received a copy of the GNU Lesser General Public >> + License along with the GNU C Library; if not, see >> + . */ >> + >> +#include >> +#include >> +#include >> + >> +void > > No, this should return ssize_t. > >> +xcopy_file_range (int infd, off64_t *pinoff, int outfd, off64_t *poutoff, >> + size_t length, unsigned int flags) >> +{ >> + >> + if (support_copy_file_range (infd, pinoff, outfd, poutoff, length, >> + flags) != length) >> + FAIL_EXIT1 ("cannot copy file: %m\n"); > > This should check for return == -1 and FAIL_EXIT1, otherwise pass the > result back to the caller. > > xcopy_file_range should have the semantics that it FAIL_EXIT1 on error, > but otherwise works as intended. > >> +} >> diff --git a/support/xunistd.h b/support/xunistd.h >> index cdd4e8d92d..6da6525a1f 100644 >> --- a/support/xunistd.h >> +++ b/support/xunistd.h >> @@ -64,6 +64,9 @@ void *xmmap (void *addr, size_t length, int prot, int flags, int fd); >> void xmprotect (void *addr, size_t length, int prot); >> void xmunmap (void *addr, size_t length); >> >> +void xcopy_file_range(int fd_in, loff_t *off_in, int fd_out, >> + loff_t *off_out, size_t len, unsigned int flags); > > Not OK, should result ssize_t. > >> + >> __END_DECLS >> >> #endif /* SUPPORT_XUNISTD_H */ >> -- 2.17.1 > This is the updated patch I am checking in. Thanks. From 68836e5e0ca7227f2477784149d10cda0f8aa81d Mon Sep 17 00:00:00 2001 From: "H.J. Lu" Date: Fri, 31 Aug 2018 11:26:16 -0700 Subject: [PATCH] test-container: Use xcopy_file_range for cross-device copy [BZ #23597] copy_file_range can't be used to copy a file from glibc source directory to glibc build directory since they may be on different filesystems. This patch adds xcopy_file_range for cross-device copy. Reviewed-by: Carlos O'Donell [BZ #23597] * support/Makefile (libsupport-routines): Add support_copy_file_range and xcopy_file_range. * support/support.h: Include . (support_copy_file_range): New prototype. * support/support_copy_file_range.c: New file. Copied and modified from io/copy_file_range-compat.c. * support/test-container.c (copy_one_file): Call xcopy_file_rang instead of copy_file_range. * support/xcopy_file_range.c: New file. * support/xunistd.h (xcopy_file_range): New prototype. --- support/Makefile | 2 + support/support.h | 5 ++ support/support_copy_file_range.c | 143 ++++++++++++++++++++++++++++++ support/test-container.c | 3 +- support/xcopy_file_range.c | 32 +++++++ support/xunistd.h | 3 + 6 files changed, 186 insertions(+), 2 deletions(-) create mode 100644 support/support_copy_file_range.c create mode 100644 support/xcopy_file_range.c diff --git a/support/Makefile b/support/Makefile index b528f538a6..545bfa2727 100644 --- a/support/Makefile +++ b/support/Makefile @@ -43,6 +43,7 @@ libsupport-routines = \ support_capture_subprocess \ support_capture_subprocess_check \ support_chroot \ + support_copy_file_range \ support_descriptor_supports_holes \ support_enter_mount_namespace \ support_enter_network_namespace \ @@ -74,6 +75,7 @@ libsupport-routines = \ xchroot \ xclose \ xconnect \ + xcopy_file_range \ xdlfcn \ xdup2 \ xfclose \ diff --git a/support/support.h b/support/support.h index c6ff4bafb4..d0e15bca1d 100644 --- a/support/support.h +++ b/support/support.h @@ -27,6 +27,8 @@ #include /* For mode_t. */ #include +/* For ssize_t and off64_t. */ +#include __BEGIN_DECLS @@ -94,6 +96,9 @@ extern const char support_install_prefix[]; /* Corresponds to the install's lib/ or lib64/ directory. */ extern const char support_libdir_prefix[]; +extern ssize_t support_copy_file_range (int, off64_t *, int, off64_t *, + size_t, unsigned int); + __END_DECLS #endif /* SUPPORT_H */ diff --git a/support/support_copy_file_range.c b/support/support_copy_file_range.c new file mode 100644 index 0000000000..9a1e39773e --- /dev/null +++ b/support/support_copy_file_range.c @@ -0,0 +1,143 @@ +/* Simplified copy_file_range with cross-device copy. + Copyright (C) 2018 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + . */ + +#include +#include +#include +#include +#include +#include +#include +#include + +ssize_t +support_copy_file_range (int infd, __off64_t *pinoff, + int outfd, __off64_t *poutoff, + size_t length, unsigned int flags) +{ + if (flags != 0) + { + errno = EINVAL; + return -1; + } + + struct stat64 instat; + struct stat64 outstat; + if (fstat64 (infd, &instat) != 0 || fstat64 (outfd, &outstat) != 0) + return -1; + if (S_ISDIR (instat.st_mode) || S_ISDIR (outstat.st_mode)) + { + errno = EISDIR; + return -1; + } + if (!S_ISREG (instat.st_mode) || !S_ISREG (outstat.st_mode)) + { + /* We need a regular input file so that the we can seek + backwards in case of a write failure. */ + errno = EINVAL; + return -1; + } + + /* The output descriptor must not have O_APPEND set. */ + if (fcntl (outfd, F_GETFL) & O_APPEND) + { + errno = EBADF; + return -1; + } + + /* Avoid an overflow in the result. */ + if (length > SSIZE_MAX) + length = SSIZE_MAX; + + /* Main copying loop. The buffer size is arbitrary and is a + trade-off between stack size consumption, cache usage, and + amortization of system call overhead. */ + size_t copied = 0; + char buf[8192]; + while (length > 0) + { + size_t to_read = length; + if (to_read > sizeof (buf)) + to_read = sizeof (buf); + + /* Fill the buffer. */ + ssize_t read_count; + if (pinoff == NULL) + read_count = read (infd, buf, to_read); + else + read_count = pread64 (infd, buf, to_read, *pinoff); + if (read_count == 0) + /* End of file reached prematurely. */ + return copied; + if (read_count < 0) + { + if (copied > 0) + /* Report the number of bytes copied so far. */ + return copied; + return -1; + } + if (pinoff != NULL) + *pinoff += read_count; + + /* Write the buffer part which was read to the destination. */ + char *end = buf + read_count; + for (char *p = buf; p < end; ) + { + ssize_t write_count; + if (poutoff == NULL) + write_count = write (outfd, p, end - p); + else + write_count = pwrite64 (outfd, p, end - p, *poutoff); + if (write_count < 0) + { + /* Adjust the input read position to match what we have + written, so that the caller can pick up after the + error. */ + size_t written = p - buf; + /* NB: This needs to be signed so that we can form the + negative value below. */ + ssize_t overread = read_count - written; + if (pinoff == NULL) + { + if (overread > 0) + { + /* We are on an error recovery path, so we + cannot deal with failure here. */ + int save_errno = errno; + (void) lseek64 (infd, -overread, SEEK_CUR); + errno = save_errno; + } + } + else /* pinoff != NULL */ + *pinoff -= overread; + + if (copied + written > 0) + /* Report the number of bytes copied so far. */ + return copied + written; + return -1; + } + p += write_count; + if (poutoff != NULL) + *poutoff += write_count; + } /* Write loop. */ + + copied += read_count; + length -= read_count; + } + return copied; +} diff --git a/support/test-container.c b/support/test-container.c index 2e91bdf9ec..c56b53ed81 100644 --- a/support/test-container.c +++ b/support/test-container.c @@ -383,8 +383,7 @@ copy_one_file (const char *sname, const char *dname) if (dfd < 0) FAIL_EXIT1 ("unable to open %s for writing\n", dname); - if (copy_file_range (sfd, 0, dfd, 0, st.st_size, 0) != st.st_size) - FAIL_EXIT1 ("cannot copy file %s to %s\n", sname, dname); + xcopy_file_range (sfd, 0, dfd, 0, st.st_size, 0); xclose (sfd); xclose (dfd); diff --git a/support/xcopy_file_range.c b/support/xcopy_file_range.c new file mode 100644 index 0000000000..b3501a4d5e --- /dev/null +++ b/support/xcopy_file_range.c @@ -0,0 +1,32 @@ +/* copy_file_range with error checking. + Copyright (C) 2018 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + . */ + +#include +#include +#include + +ssize_t +xcopy_file_range (int infd, off64_t *pinoff, int outfd, off64_t *poutoff, + size_t length, unsigned int flags) +{ + ssize_t status = support_copy_file_range (infd, pinoff, outfd, + poutoff, length, flags); + if (status == -1) + FAIL_EXIT1 ("cannot copy file: %m\n"); + return status; +} diff --git a/support/xunistd.h b/support/xunistd.h index cdd4e8d92d..f99f362cb4 100644 --- a/support/xunistd.h +++ b/support/xunistd.h @@ -64,6 +64,9 @@ void *xmmap (void *addr, size_t length, int prot, int flags, int fd); void xmprotect (void *addr, size_t length, int prot); void xmunmap (void *addr, size_t length); +ssize_t xcopy_file_range(int fd_in, loff_t *off_in, int fd_out, + loff_t *off_out, size_t len, unsigned int flags); + __END_DECLS #endif /* SUPPORT_XUNISTD_H */ -- 2.17.1