> On Oct 24, 2015, at 10:52 AM, Eric Biggers <ebiggers3@xxxxxxxxx> wrote:
>
> A few comments:
>
>> if (!(file_in->f_mode & FMODE_READ) ||
>> !(file_out->f_mode & FMODE_WRITE) ||
>> (file_out->f_flags & O_APPEND) ||
>> !file_out->f_op)
>> return -EBADF;
>
> Isn't 'f_op' always non-NULL?
>
> If the destination file cannot be append-only, shouldn't this be documented?
Actually, wouldn't O_APPEND only be a problem if the target file wasn't
being appended to? In other words, if the target i_size == start offset
then it should be possible to use the copy syscall on an O_APPEND file.
Cheers, Andreas
>> if (inode_in->i_sb != inode_out->i_sb ||
>> file_in->f_path.mnt != file_out->f_path.mnt)
>> return -EXDEV;
>
> Doesn't the same mount already imply the same superblock?
>
>> /*
>> * copy_file_range() differs from regular file read and write in that it
>> * specifically allows return partial success. When it does so is up to
>> * the copy_file_range method.
>> */
>
> What does this mean? I thought that read() and write() can also return partial
> success.
>
>> f_out = fdget(fd_out);
>> if (!f_in.file || !f_out.file) {
>> ret = -EBADF;
>> goto out;
>> }
>
> This looked wrong at first because it may call fdput() on a 'struct fd' that was
> not successfully acquired, but it looks like it works currently because of how
> the FDPUT_FPUT flag is used. It may be a good idea to write it the "obvious"
> way, though (use separate labels depending on which fdput()s need to happen).
>
>
> Other questions:
>
> Should FMODE_PREAD or FMODE_PWRITE access be checked if the user specifies their
> own 'off_in' or 'off_out', respectively?
>
> What is supposed to happen if the user passes provides a file descriptor to a
> non-regular file, such as a block device or char device?
>
> If the 'in' file has fewer than 'len' bytes remaining until EOF, what is the
> expected behavior? It looks like the btrfs implementation has different
> behavior from the pagecache implementation.
>
> It appears the btrfs implementation has alignment restrictions --- where is this
> documented and how will users know what alignment to use?
>
> Are copies within the same file permitted and can the ranges overlap? The man
> page doesn't say.
>
> It looks like the initial patch defines __NR_copy_file_range for the ARM
> architecture but doesn't actually hook that system call up for ARM; why is that?
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Cheers, Andreas
Attachment:
signature.asc
Description: Message signed with OpenPGP using GPGMail
