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
