Re: V3 [PATCH] test-container: Use xcopy_file_range for cross-device copy [BZ #23597]

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Fri, Aug 31, 2018 at 12:56 PM, Carlos O'Donell <carlos@redhat.com> 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" <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 for cross-device copy.
>>
>>       [BZ #23597]
>>       * support/Makefile (libsupport-routines): Add
>>       support_copy_file_range and xcopy_file_range.
>>       * support/support.h: Include <sys/types.h>.
>>       (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 <carlos@redhat.com>
>
>> ---
>>  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 <sys/cdefs.h>
>>  /* For mode_t.  */
>>  #include <sys/stat.h>
>> +/* For ssize_t and off64_t.  */
>> +#include <sys/types.h>
>
> 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
>> +   <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>
>> +#include <support/support.h>
>> +
>> +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
>> +   <http://www.gnu.org/licenses/>.  */
>> +
>> +#include <support/support.h>
>> +#include <support/xunistd.h>
>> +#include <support/check.h>
>> +
>> +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.

-- 
H.J.

From 68836e5e0ca7227f2477784149d10cda0f8aa81d 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 for cross-device copy.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

	[BZ #23597]
	* support/Makefile (libsupport-routines): Add
	support_copy_file_range and xcopy_file_range.
	* support/support.h: Include <sys/types.h>.
	(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 <sys/cdefs.h>
 /* For mode_t.  */
 #include <sys/stat.h>
+/* For ssize_t and off64_t.  */
+#include <sys/types.h>
 
 __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
+   <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>
+#include <support/support.h>
+
+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
+   <http://www.gnu.org/licenses/>.  */
+
+#include <support/support.h>
+#include <support/xunistd.h>
+#include <support/check.h>
+
+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