Re: [RFC] unify the file-closing stuff in fs/file.c

[Jens Axboe <axboe@kernel.dk>]

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.

-- 
Jens Axboe