Message ID | CAMe9rOodYBvH6j-J8MKGq06t0KDg+gMpVyXWsR_bqK9OMO0+Ow@mail.gmail.com |
---|---|
State | New |
Headers | show |
Series | test-container: Use xcopy_file_range for cross-device copy [BZ #23597] | expand |
On 08/31/2018 02:33 PM, H.J. Lu wrote: > On Fri, Aug 31, 2018 at 9:23 AM, Carlos O'Donell <carlos@redhat.com> wrote: >>> That is what io/copy_file_range-compat.c does. Are you suggesting >>> cut and paste, instead of #include? >> Not quite. >> >> We are suggesting a unique and simplified copy of those sources to >> be placed into the support/ directory as a *.c file that can be >> used for support purposes. >> >> Coupling support behaviour and runtime behaviour is a bad choice, >> so we should not do it. Even if it means some code duplication. >> > Like this? Almost. Look at the notes below and I'll review a v3. > -- H.J. > > > 0001-test-container-Use-xcopy_file_range-for-cross-device.patch > > > From f08d598bcc973c4747d6baa1a832d683f2c917a8 Mon Sep 17 00:00:00 2001 > From: "H.J. Lu" <hjl.tools@gmail.com> > 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 to use it for cross-device copy. > > [BZ #23597] > * support/Makefile (libsupport-routines): Add xcopy_file_range. > * support/test-container.c (copy_one_file): Call xcopy_file_rang > instead of copy_file_range. > * support/xcopy_file_range.c: New file. Copied and modified > from io/copy_file_range-compat.c. > * support/xunistd.h (xcopy_file_range): New prototype. > --- > support/Makefile | 1 + > support/test-container.c | 2 +- > support/xcopy_file_range.c | 142 +++++++++++++++++++++++++++++++++++++ > support/xunistd.h | 3 + > 4 files changed, 147 insertions(+), 1 deletion(-) > create mode 100644 support/xcopy_file_range.c > > diff --git a/support/Makefile b/support/Makefile > index b528f538a6..7758465bb7 100644 > --- a/support/Makefile > +++ b/support/Makefile > @@ -74,6 +74,7 @@ libsupport-routines = \ > xchroot \ > xclose \ > xconnect \ > + xcopy_file_range \ OK. > xdlfcn \ > xdup2 \ > xfclose \ > diff --git a/support/test-container.c b/support/test-container.c > index 2e91bdf9ec..476a5574e6 100644 > --- a/support/test-container.c > +++ b/support/test-container.c > @@ -383,7 +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) > + if (xcopy_file_range (sfd, 0, dfd, 0, st.st_size, 0) != st.st_size) No. This should just be: xcopy_file_range( ... ); You expect xcopy_file_range to handle all the errors. Rather than hack up the function itself though you might want to: - Create support_copy_file_range which is the real function and returns errors, and is documented as doing that. - Create a xcopy_file_range which is a wrapper that detects errors and calls support_copy_file_range, and for each error it calls FAIL_EXIT with an appropriate error message. So a caller knows it always succeeds (simplifies tests). > FAIL_EXIT1 ("cannot copy file %s to %s\n", sname, dname); > > xclose (sfd); > diff --git a/support/xcopy_file_range.c b/support/xcopy_file_range.c > new file mode 100644 > index 0000000000..e6ba141eee > --- /dev/null > +++ b/support/xcopy_file_range.c > @@ -0,0 +1,142 @@ > +/* 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 > + <http://www.gnu.org/licenses/>. */ > + > +#include <errno.h> > +#include <fcntl.h> > +#include <inttypes.h> > +#include <limits.h> > +#include <sys/stat.h> > +#include <sys/types.h> > +#include <unistd.h> > + > +ssize_t > +xcopy_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; > +} There are 6 error return paths here that should be handled by the xcopy_file_range wrapper. > 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); OK. > + > __END_DECLS > > #endif /* SUPPORT_XUNISTD_H */ > -- 2.17.1
From f08d598bcc973c4747d6baa1a832d683f2c917a8 Mon Sep 17 00:00:00 2001 From: "H.J. Lu" <hjl.tools@gmail.com> 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 to use it for cross-device copy. [BZ #23597] * support/Makefile (libsupport-routines): Add xcopy_file_range. * support/test-container.c (copy_one_file): Call xcopy_file_rang instead of copy_file_range. * support/xcopy_file_range.c: New file. Copied and modified from io/copy_file_range-compat.c. * support/xunistd.h (xcopy_file_range): New prototype. --- support/Makefile | 1 + support/test-container.c | 2 +- support/xcopy_file_range.c | 142 +++++++++++++++++++++++++++++++++++++ support/xunistd.h | 3 + 4 files changed, 147 insertions(+), 1 deletion(-) create mode 100644 support/xcopy_file_range.c diff --git a/support/Makefile b/support/Makefile index b528f538a6..7758465bb7 100644 --- a/support/Makefile +++ b/support/Makefile @@ -74,6 +74,7 @@ libsupport-routines = \ xchroot \ xclose \ xconnect \ + xcopy_file_range \ xdlfcn \ xdup2 \ xfclose \ diff --git a/support/test-container.c b/support/test-container.c index 2e91bdf9ec..476a5574e6 100644 --- a/support/test-container.c +++ b/support/test-container.c @@ -383,7 +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) + if (xcopy_file_range (sfd, 0, dfd, 0, st.st_size, 0) != st.st_size) FAIL_EXIT1 ("cannot copy file %s to %s\n", sname, dname); xclose (sfd); diff --git a/support/xcopy_file_range.c b/support/xcopy_file_range.c new file mode 100644 index 0000000000..e6ba141eee --- /dev/null +++ b/support/xcopy_file_range.c @@ -0,0 +1,142 @@ +/* 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 + <http://www.gnu.org/licenses/>. */ + +#include <errno.h> +#include <fcntl.h> +#include <inttypes.h> +#include <limits.h> +#include <sys/stat.h> +#include <sys/types.h> +#include <unistd.h> + +ssize_t +xcopy_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/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