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

[Carlos O'Donell <carlos at redhat dot com>]

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


-- 
Cheers,
Carlos.