On Thu, May 12, 2022 at 4:48 PM Jens Axboe <axboe@xxxxxxxxx> wrote:
>
> On 5/12/22 3:20 PM, Al Viro wrote:
> > Right now we have two places that do such removals - pick_file()
> > and {__,}close_fd_get_file().
> >
> > They are almost identical - the only difference is in calling
> > conventions (well, and the fact that __... is called with descriptor
> > table locked).
> >
> > Calling conventions are... interesting.
> >
> > 1) pick_file() - returns file or ERR_PTR(-EBADF) or ERR_PTR(-EINVAL).
> > The latter is for "descriptor is greater than size of descriptor table".
> > One of the callers treats all ERR_PTR(...) as "return -EBADF"; another
> > uses ERR_PTR(-EINVAL) as "end the loop now" indicator.
> >
> > 2) {__,}close_fd_get_file() returns 0 or -ENOENT (huh?), with file (or NULL)
> > passed to caller by way of struct file ** argument. One of the callers
> > (binder) ignores the return value completely and checks if the file is NULL.
> > Another (io_uring) checks for return value being negative, then maps
> > -ENOENT to -EBADF, not that any other value would be possible.
> >
> > ERR_PTR(-EINVAL) magic in case of pick_file() is borderline defensible;
> > {__,}close_fd_get_file() conventions are insane. The older caller
> > (in binder) had never even looked at return value; the newer one
> > patches the bogus -ENOENT to what it wants to report, with strange
> > "defensive" BS logics just in case __close_fd_get_file() would somehow
> > find a different error to report.
> >
> > At the very least, {__,}close_fd_get_file() callers would've been happier
> > if it just returned file or NULL. What's more, I'm seriously tempted
> > to make pick_file() do the same thing. close_fd() won't care (checking
> > for NULL is just as easy as for IS_ERR) and __range_close() could just
> > as well cap the max_fd argument with last_fd(files_fdtable(current->files)).
> >
> > Does anybody see problems with the following?
>
> Looks good to me, and much better than passing in the pointer to the
> file pointer imho.
I agree. This looks good.
>
> --
> Jens Axboe
>
