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

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]
Related show

Commit Message

H.J. Lu Aug. 31, 2018, 6:33 p.m.
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?

-- 
H.J.

Comments

Carlos O'Donell Aug. 31, 2018, 6:47 p.m. | #1
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



-- 
Cheers,
Carlos.

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 \
   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