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

[H.J. Lu]

On Fri, Aug 31, 2018 at 11:47 AM, Carlos O'Donell <carlos@redhat.com> wrote:
>
> Almost. Look at the notes below and I'll review a v3.
>

>> 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( ... );
>

Done.

> 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).

Done.

>>      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
>
>> +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.

Done.

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

I changed return type to void.

Here is the updated patch.  OK for master?

Thanks.

-- 
H.J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-test-container-Use-xcopy_file_range-for-cross-device.patch
Type: text/x-patch
Size: 9319 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20180831/a6d3d712/attachment.bin>