On Tue, Jan 28, 2025 at 12:59:09PM -0500, Brian Foster wrote:
> But that raises another question. I'd want bytes to be s64 here to
> support the current factoring, but iomap_length() returns a u64. In
> poking around a bit I _think_ this is practically safe because the high
> level operations are bound by loff_t (int64_t), so IIUC that means we
> shouldn't actually see a length that doesn't fit in s64.
>
> That said, that still seems a bit grotty. Perhaps one option could be to
> tweak iomap_length() to return something like this:
>
> min_t(u64, SSIZE_MAX, end);
>
> ... to at least makes things explicit.
Yeah. I'm actually not sure why went want to support 64-bit ranges.
I don't even remember if this comes from Dave's really first version
or was my idea, but in hindsight just sticking to ssize_t bounds
would have been smarter.
> I'd guess the (i.e. iomap_file_unshare()) loop logic would look more
> like:
>
> do {
> ...
> ret = iomap_iter_advance(iter, &bytes);
> } while (!ret && bytes > 0);
>
> return ret;
>
> Hmm.. now that I write it out that doesn't seem so bad. It does clean up
> the return path a bit. I think I'll play around with that, but let me
> know if there are other thoughts or ideas..
Given that all the kernel read/write code mixes up bytes and negative
return values I think doing that in iomap is also fine. But you are
deeper into the code right now, and if you think splitting the errno
and bytes is cleaner that sounds perfectly fine to me as well. In
general not overloading a single return value with two things tends
to lead to cleaner code.
Although the above sniplet (I´m not sure how representative it is
anyway) would be a bit nicer as the slightly more verbose version
below:
do {
...
ret = iomap_iter_advance(iter, &bytes);
if (ret)
return ret;
} while (bytes > 0);
