[PATCH] copy_file_range: New function to copy file data

[Adhemerval Zanella]

On 21/12/2017 16:07, Florian Weimer wrote:
> 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.

I think for former it will hit the oom scenario where kernel will randomly
killing a process (assuming it is what Linux still does) which result the
process to continue execution or being killed.  Anyway, I think I am think
I am over engineering things here, so your approach should be ok.