On 3/14/19 9:56 PM, Darrick J. Wong wrote:
> On Thu, Mar 14, 2019 at 09:12:11PM -0500, Eric Sandeen wrote:
>> On 3/14/19 4:04 PM, Darrick J. Wong wrote:
>>> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>>>
>>> We need to check the return value of copy_src_filesize and
>>> copy_dst_truncate because either could return -1 due to fstat/ftruncate
>>> failure.
>>>
>>> Fixes: 628e112afdd98c5 ("xfs_io: implement 'copy_range' command")
>>> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>>> Reviewed-by: Anna Schumaker <Anna.Schumaker@xxxxxxxxxx>
>>> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
>>
>> I see this has reviews already, but I'm not conviced 1 is the right
>> return on error.
>>
>> i.e. the error condition just prior returns 0:
>>
>> fd = openfile(argv[optind], NULL, IO_READONLY, 0, NULL);
>> if (fd < 0)
>> return 0;
>>
>> but OTOH if copy_file_range_cmd() fails we return nonzero. Argh.
>>
>> Is there a rhyme or reason here that I'm missing? Is it broken now
>> so just do a coinflip on the new return for now?
>
> Pretty much. :(
>
> I don't really care if you change the return value, I just thought it
> was bad form not to check the syscall/helper return values at all.
Yeah no argument there. Ok, so this function already returns inconsistently,
I'll worry about that later (I hope)
Thanks,
-Eric
