This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] copy_file_range: New function to copy file data
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.