On Fri, May 19, 2023, at 16:51, Lorenzo Stoakes wrote:
> Given you are sharply criticising the code I authored here, is it too much
> to ask for you to cc- me, the author on commentaries like this? Thanks.
My mistake, I expected this to get added automatically based on
the "Fixes:" tag, I probably dropped you by accident in the end.
> On Fri, May 19, 2023 at 11:39:13AM +0200, Arnd Bergmann wrote:
>> From: Arnd Bergmann <arnd@xxxxxxxx>
>>
>> While looking at an unused-variable warning, I noticed a new interface coming
>> in that requires the use of IS_ERR_OR_NULL(), which tends to indicate bad
>> interface design and is usually surprising to users.
>
> I am not sure I understand your reasoning, why does it 'tend to indicate
> bad interface design'? You say that as if it is an obvious truth. Not
> obvious to me at all.
>
> There are 3 possible outcomes from the function - an error, the function
> failing to pin a page, or it succeeding in doing so. For some of the
> callers that results in an error, for others it is not an error.
>
> Overloading EIO on the assumption that gup will never, ever return this
> indicating an error seems to me a worse solution.
The problem is that we have inconsistent error handling in functions
that return an object, about half of them use NULL to indicate an error,
and the other half use ERR_PTR(), and users frequently get those
wrong by picking the wrong one. Functions that can return both make
this worse because whichever of the two normal ways a user expects,
they still get it wrong.
> Not a fan at all of this patch, it doesn't achieve anything useful, is in
> service of some theoretical improvement, and actually introduces a new
> class of bug (differentiating EIO and failing to pin).
Having another -EIO return code is a problem, so I agree that
my patch wouldn't be good either. Maybe separating the error return
from the page pointer by passing a 'struct page **p' argument that
gets filled would help?
Arnd
