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