Re: [LTP] [PATCH v4] move_mount03: check allow to mount beneath top mount

[Petr Vorel <pvorel@suse.cz>]

Hi Wei, Marting,

> Hi,
> one note below but this can be merged as is.

> Reviewed-by: Martin Doucha <mdoucha@suse.cz>
Thanks!

...
> > diff --git a/testcases/kernel/syscalls/move_mount/move_mount03.c b/testcases/kernel/syscalls/move_mount/move_mount03.c
> > new file mode 100644

...
> +/*\
> + * [Description]
> + *
> + * Test allow to mount beneath top mount feature
very nit: missing dot.
But more important the docparse docs are very sparse now, when everything is in
the documentation visible only when looking into source code.

I would also mention the commit and the original test in docparse, e.g.
something like:

/*\
 * [Description]
 *
 * Test allow to mount beneath top mount feature added in kernel 6.5:
 * 6ac392815628 ("fs: allow to mount beneath top mount")
 *
 * Test based on:
 * https://github.com/brauner/move-mount-beneath
 *
 * See also:
 * https://lore.kernel.org/all/20230202-fs-move-mount-replace-v4-0-98f3d80d7eaa@kernel.org/
 */

> + */
...
> + * See also:
> + * https://lwn.net/Articles/930591/
I would move these two into docparse as in example above.

This cover letter of v3. Wouldn't it be better to link the final version (v4)
and from lwn.net (as in example above)?
https://lore.kernel.org/all/20230202-fs-move-mount-replace-v4-0-98f3d80d7eaa@kernel.org/

> + * https://github.com/brauner/move-mount-beneath
...
> > +static void run(void)
> > +{
> > +	SAFE_MOUNT("none", DIRA, "tmpfs", 0, 0);
> > +	SAFE_MOUNT("none", DIRB, "tmpfs", 0, 0);
> > +	SAFE_TOUCH(DIRA "/A", 0, NULL);
> > +	SAFE_TOUCH(DIRB "/B", 0, NULL);
> > +
> > +	fda = open_tree(AT_FDCWD, DIRA, OPEN_TREE_CLOEXEC | OPEN_TREE_CLONE);
> > +	if (fda == -1)
> > +		tst_brk(TBROK | TERRNO, "open_tree() failed");
> > +
> > +	fdb = SAFE_OPEN(DIRB, O_PATH | O_NOFOLLOW, 0666);
> > +	TST_EXP_PASS(move_mount(fda, "", fdb, "",
> > +				MOVE_MOUNT_BENEATH | MOVE_MOUNT_F_EMPTY_PATH |
> > +				MOVE_MOUNT_T_EMPTY_PATH));
> > +	SAFE_CLOSE(fda);
> > +	SAFE_CLOSE(fdb);
> > +
> > +	TST_EXP_PASS(access(DIRB "/B", F_OK));

> That extra check I've asked for in v3 would still be nice here.

Would you mind to send v5 with checks Martin suggested?

https://lore.kernel.org/ltp/8798c323-8298-49b1-9950-09f2a7c309cb@suse.cz/

> > +	SAFE_UMOUNT(DIRB);
> > +	TST_EXP_PASS(access(DIRB "/A", F_OK));
> > +
> > +	SAFE_UMOUNT(DIRB);
> > +	SAFE_UMOUNT(DIRA);
> > +}
...
> +static void cleanup(void)
> +{
very nit: remove extra space here:
> +
> +	if (fda >= 0)
> +		SAFE_CLOSE(fda);
> +
> +	if (fdb >= 0)
> +		SAFE_CLOSE(fdb);
....

> +static struct tst_test test = {
> +	.test_all = run,
> +	.needs_root = 1,
> +	.min_kver = "6.5.0",
nit: .min_kver = "6.5", would be enough

Otherwise LGTM.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

BTW briefly looking into the Christian's testing source code, these flags are
not yet covered by LTP:
MOVE_MOUNT_SET_GROUP, MOUNT_ATTR_RDONLY, AT_RECURSIVE

Kind regards,
Petr

> +	.needs_tmpdir = 1,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +};

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp