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.
--D
>
> > ---
> > io/copy_file_range.c | 17 +++++++++++++++--
> > 1 file changed, 15 insertions(+), 2 deletions(-)
> >
> >
> > diff --git a/io/copy_file_range.c b/io/copy_file_range.c
> > index 4e2969c9..d069e5bb 100644
> > --- a/io/copy_file_range.c
> > +++ b/io/copy_file_range.c
> > @@ -120,11 +120,24 @@ copy_range_f(int argc, char **argv)
> > return 0;
> >
> > if (src == 0 && dst == 0 && len == 0) {
> > - len = copy_src_filesize(fd);
> > - copy_dst_truncate();
> > + off64_t sz;
> > +
> > + sz = copy_src_filesize(fd);
> > + if (sz < 0 || (unsigned long long)sz > SIZE_MAX) {
> > + ret = 1;
> > + goto out;
> > + }
> > + len = sz;
> > +
> > + ret = copy_dst_truncate();
> > + if (ret < 0) {
> > + ret = 1;
> > + goto out;
> > + }
> > }
> >
> > ret = copy_file_range_cmd(fd, &src, &dst, len);
> > +out:
> > close(fd);
> > return ret;
> > }
> >
