Re: [PATCH 1/2] idmapped-mounts: add fs_allow_idmap() helper

[Eryu Guan <guan@eryu.me>]

On Thu, Jan 13, 2022 at 02:24:20PM +0100, Christian Brauner wrote:
> Move the check whether the underlying filesystem supports idmapped
> mounts into a separate helper. We will use it in the following patch to
> make it possible to always run all tests that don't require idmapped
> mounts.
> 
> Cc: Seth Forshee <sforshee@digitalocean.com>
> Cc: Eryu Guan <guaneryu@gmail.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: fstests@vger.kernel.org
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---
>  src/idmapped-mounts/idmapped-mounts.c | 57 ++++++++++++++-------------
>  1 file changed, 30 insertions(+), 27 deletions(-)
> 
> diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
> index da690779..a78a901f 100644
> --- a/src/idmapped-mounts/idmapped-mounts.c
> +++ b/src/idmapped-mounts/idmapped-mounts.c
> @@ -13907,6 +13907,35 @@ static bool run_test(struct t_idmapped_mounts suite[], size_t suite_size)
>  	return true;
>  }
>  
> +static bool fs_allow_idmap(void)
> +{
> +	int ret;
> +	int open_tree_fd = -EBADF;
> +	struct mount_attr attr = {
> +		.attr_set	= MOUNT_ATTR_IDMAP,
> +		.userns_fd	= -EBADF,
> +	};
> +
> +	/* Changing mount properties on a detached mount. */
> +	attr.userns_fd = get_userns_fd(0, 1000, 1);
> +	if (attr.userns_fd < 0)
> +		exit(EXIT_FAILURE);

IMHO, it'd be better to return false here and let caller decide if we
should exit or not.

Thanks,
Eryu

> +
> +	open_tree_fd = sys_open_tree(t_mnt_fd, "",
> +				     AT_EMPTY_PATH | AT_NO_AUTOMOUNT |
> +				     AT_SYMLINK_NOFOLLOW | OPEN_TREE_CLOEXEC |
> +				     OPEN_TREE_CLONE);
> +	if (open_tree_fd < 0)
> +		ret = -1;
> +	else
> +		ret = sys_mount_setattr(open_tree_fd, "", AT_EMPTY_PATH, &attr,
> +					sizeof(attr));
> +	close(open_tree_fd);
> +	close(attr.userns_fd);
> +
> +	return ret == 0;
> +}
> +
>  int main(int argc, char *argv[])
>  {
>  	int fret, ret;
> @@ -13987,34 +14016,8 @@ int main(int argc, char *argv[])
>  	 * idmapped mounts.
>  	 */
>  	if (supported) {
> -		int open_tree_fd = -EBADF;
> -		struct mount_attr attr = {
> -			.attr_set	= MOUNT_ATTR_IDMAP,
> -			.userns_fd	= -EBADF,
> -		};
> -
> -		/* Changing mount properties on a detached mount. */
> -		attr.userns_fd	= get_userns_fd(0, 1000, 1);
> -		if (attr.userns_fd < 0)
> +		if (!fs_allow_idmap())
>  			exit(EXIT_FAILURE);
> -
> -		open_tree_fd = sys_open_tree(t_mnt_fd, "",
> -					     AT_EMPTY_PATH |
> -					     AT_NO_AUTOMOUNT |
> -					     AT_SYMLINK_NOFOLLOW |
> -					     OPEN_TREE_CLOEXEC |
> -					     OPEN_TREE_CLONE);
> -		if (open_tree_fd < 0)
> -			ret = -1;
> -		else
> -			ret = sys_mount_setattr(open_tree_fd, "", AT_EMPTY_PATH, &attr, sizeof(attr));
> -
> -		close(open_tree_fd);
> -		close(attr.userns_fd);
> -
> -		if (ret)
> -			exit(EXIT_FAILURE);
> -
>  		exit(EXIT_SUCCESS);
>  	}
>  
> 
> base-commit: c61c1e2f4cad89d4f900eaef173616b309228110
> -- 
> 2.32.0