Re: [PATCH] copy_file_range: New function to copy file data

[Florian Weimer <fweimer at redhat dot com>]

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