On Fri, Mar 15, 2019 at 11:51:56AM -0500, Eric Sandeen wrote:
>
>
> 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)
I think I've still got a patch sitting around that fixes the
inconsistent return values across most of xfs_io. Oh, I posted it at
one point, too.
https://marc.info/?l=linux-xfs&m=152644986309024&w=2
I guess I should resurrect it at some point.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
