diff mbox series

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

Message ID 20220113132421.865002-1-brauner@kernel.org (mailing list archive)
State New, archived
Headers show
Series [1/2] idmapped-mounts: add fs_allow_idmap() helper | expand

Commit Message

Christian Brauner Jan. 13, 2022, 1:24 p.m. UTC 
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(-)


base-commit: c61c1e2f4cad89d4f900eaef173616b309228110

Comments

Eryu Guan Jan. 16, 2022, 4:51 a.m. UTC | #1 
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
Christian Brauner Jan. 18, 2022, 1:33 p.m. UTC | #2 
On Sun, Jan 16, 2022 at 12:51:32PM +0800, Eryu Guan wrote:
> 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.

Agreed.
diff mbox series

Patch

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);
+
+	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);
 	}