Re: [RFC][PATCH] Change calling conventions for filldir_t

From: Matthew Wilcox <willy@infradead.org>
To: Al Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org,
	Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [RFC][PATCH] Change calling conventions for filldir_t
Date: Tue, 16 Aug 2022 17:24:16 +0100	[thread overview]
Message-ID: <YvvEsGWvarSwW5kh@casper.infradead.org> (raw)
In-Reply-To: <YvvBs+7YUcrzwV1a@ZenIV>

On Tue, Aug 16, 2022 at 05:11:31PM +0100, Al Viro wrote:
> filldir_t instances (directory iterators callbacks) used to return 0 for
> "OK, keep going" or -E... for "stop".  Note that it's *NOT* how the
> error values are reported - the rules for those are callback-dependent
> and ->iterate{,_shared}() instances only care about zero vs. non-zero
> (look at emit_dir() and friends).
> 
> So let's just return bool ("should we keep going?") - it's less confusing
> that way.  The choice between "true means keep going" and "true means
> stop" is bikesheddable; we have two groups of callbacks -
>     do something for everything in directory, until we run into problem
> and
>     find an entry in directory and do something to it.
> 
> The former tended to use 0/-E... conventions - -E<something> on failure.
> The latter tended to use 0/1, 1 being "stop, we are done".
> The callers treated anything non-zero as "stop", ignoring which
> non-zero value did they get.
> 
> "true means stop" would be more natural for the second group; "true
> means keep going" - for the first one.  I tried both variants and
> the things like
> 	if allocation failed
> 		something = -ENOMEM;
> 		return true;
> just looked unnatural and asking for trouble.

I like it the way you have it.  My only suggestion is:

+++ b/include/linux/fs.h
@@ -2039,6 +2039,7 @@ extern bool may_open_dev(const struct path *path);
  * the kernel specify what kind of dirent layout it wants to have.
  * This allows the kernel to read directories into kernel space or
  * to have different dirent layouts depending on the binary type.
+ * Return 'true' to keep going and 'false' if there are no more entries.
  */
 struct dir_context;
 typedef int (*filldir_t)(struct dir_context *, const char *, int, loff_t, u64,

> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 9eced4cc286e..8b8c0c11afec 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2040,7 +2040,7 @@ umode_t mode_strip_sgid(struct user_namespace *mnt_userns,
>   * to have different dirent layouts depending on the binary type.
>   */
>  struct dir_context;
> -typedef int (*filldir_t)(struct dir_context *, const char *, int, loff_t, u64,
> +typedef bool (*filldir_t)(struct dir_context *, const char *, int, loff_t, u64,
>  			 unsigned);
>  
>  struct dir_context {
