Re: [PATCH 3/3] vfs: return -EOVERFLOW in generic_remap_checks() when overflow check fails

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, Sep 20, 2024 at 07:58:01AM -0700, Christoph Hellwig wrote:
> On Fri, Sep 20, 2024 at 07:37:27AM -0700, Darrick J. Wong wrote:
> > On Fri, Sep 20, 2024 at 07:19:28AM -0700, Christoph Hellwig wrote:
> > > On Fri, Sep 20, 2024 at 08:30:22PM +0800, Julian Sun wrote:
> > > > Keep it consistent with the handling of the same check within
> > > > generic_copy_file_checks().
> > > > Also, returning -EOVERFLOW in this case is more appropriate.
> > > 
> > > Maybe:
> > > 
> > > Keep the errno value consistent with the equivalent check in
> > > generic_copy_file_checks() that returns -EOVERFLOW, which feels like the
> > > more appropriate value to return compared to the overly generic -EINVAL.
> > 
> > The manpage for clone/dedupe/exchange don't say anything about
> > EOVERFLOW, but they do have this to say about EINVAL:
> > 
> > EINVAL
> > The  filesystem  does  not  support  reflinking the ranges of the given
> > files.
> 
> Which isn't exactly the integer overflow case described here :)

Hm?  This patch is touching the error code you get for failing alignment
checks, not the one you get for failing check_add_overflow.  EOVERFLOW
seems like an odd return code for unaligned arguments.  Though you're
right that EINVAL is verrry vague.

> > Does this errno code change cause any regressions in fstests?
> 
> Given our rather sparse test coverage of it I doubt it, but it
> would be great to have that confirmed by the submitter.

Yes. :)

> While we're talking about that - a simple exerciser for the overflow
> condition for xfstests would be very useful to have.

Yes, there's <cough> supposed to be one that does that.

$ git grep -ci CLONE.*invalid.argument
common/filter.btrfs:1
tests/btrfs/035.out:1
tests/btrfs/052.out:12
tests/btrfs/096.out:1
tests/btrfs/112.out:16
tests/btrfs/113.out:1
tests/btrfs/229:1
tests/generic/157.out:6
tests/generic/303.out:4
tests/generic/518.out:1


--D