[PATCH] copy_file_range: New function to copy file data

[Florian Weimer]

On 12/21/2017 06:04 PM, Adhemerval Zanella wrote:

>> +  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))
>> +    {
>> +      __set_errno (EISDIR);
>> +      return -1;
>> +    }
> 
> To follow the pattern you can put 'instat' and 'outstat' in its own scope.

Agreed.

>> +      if (read_count < 0)
>> +        {
>> +          if (copied > 0)
>> +            /* Report the number of bytes copied so far.  */
>> +            return copied;
>> +          return -1;
>> +        }> +      if (pinoff != 0)
>> +        *pinoff += read_count;
> 
> pinoff != NULL.

Oh, right.

>> +
>> +      /* 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 = __libc_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) __libc_lseek64 (infd, -overread, SEEK_CUR);
>> +                      __set_errno (save_errno);
> 
> Should we really handle errors here? Using current man pages EBADF, ENXIO,
> ESPIPE can't really happen because of previous checks.  EINVAL and EOVERFLOW
> due resulting file offset would be negative or beyond the end of a seekable
> device is also unlikely due the fact we are using the results of a previous
> partial write to calculate the required offset.  I am not sure if it can
> really fail here.

Theoretically, I assume that with enough memory pressure, the seek might 
have to re-read on-disk data structures, and then anything can happen. 
This is why I don't want to assert on the error.  Perhaps more likely is 
a file descriptor race condition which closes the descriptor under us, 
but then the application is screwed anyway.

We cannot report the error in all cases because with a partial write, we 
need to report the number of written bytes (because that effect has 
already happened and is visible by other means).

So I think the code is okay as it is now, all things considering.

Thanks,
Florian