[LTP] 回复: [PATCH v2] syscalls/mount_setattr01: Add basic functional test

[chenhx.fnst@fujitsu.com]

Hi,

> -----邮件原件-----
> 发件人: Petr Vorel <pvorel@suse.cz>
> 发送时间: 2022年4月21日 4:43
> <chrubis@suse.cz>
> 主题: Re: [LTP] [PATCH v2] syscalls/mount_setattr01: Add basic functional test
> 
> Hi Chen, Dai,
> 
> > From: Chen Hanxiao <chenhx.fnst@fujitsu.com>
> 
> > diff --git a/include/lapi/fsmount.h b/include/lapi/fsmount.h
> ...
> > +/*
> > + * mount_setattr()
> > + */
> > +struct mount_attr {
> > +	uint64_t attr_set;
> > +	uint64_t attr_clr;
> > +	uint64_t propagation;
> > +	uint64_t userns_fd;
> > +};
> Interesting enough: in kernel
> tools/testing/selftests/mount_setattr/mount_setattr_test.c
> defines it as __u64 (IMHO should be really uint64_t as that test is userspace as
> Cyril pointed out) but real kernel code in fs/namespace.c happily uses "unsigned
> int" :).
> 
> ...
> > diff --git a/testcases/kernel/syscalls/mount_setattr/mount_setattr01.c
> > b/testcases/kernel/syscalls/mount_setattr/mount_setattr01.c
> > new file mode 100644
> > index 000000000..d63db46fa
> > --- /dev/null
> > +++ b/testcases/kernel/syscalls/mount_setattr/mount_setattr01.c
> > @@ -0,0 +1,118 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2022 FUJITSU LIMITED. All rights reserved.
> > + * Author: Dai Shili <daisl.fnst@fujitsu.com>
> > + * Author: Chen Hanxiao <chenhx.fnst@fujitsu.com>  */
> > +
> > +/*\
> > + * [Description]
> > + *
> > + * Basic mount_setattr() test.
> > + * Test whether the basic mount attributes are set correctly.
> > + *
> > + * Verify some MOUNT_SETATTR(2) attributes:
> > + * 1) MOUNT_ATTR_RDONLY - makes the mount read-only
> > + * 2) MOUNT_ATTR_NOSUID - causes the mount not to honor the
> > + *    set-user-ID and set-group-ID mode bits and file capabilities
> > + *    when executing programs.
> > + * 3) MOUNT_ATTR_NODEV - prevents access to devices on this mount
> > + * 4) MOUNT_ATTR_NOEXEC - prevents executing programs on this mount
> > + * 5) MOUNT_ATTR_NOSYMFOLLOW - prevents following symbolic links
> > + *    on this mount
> > + * 6) MOUNT_ATTR_NODIRATIME - prevents updating access time for
> > + *    directories on this mount
> > + * Minimum Linux version required is v5.12.
> Since we don't check for v5.12, it might be better to state "The functionality was
> added in v5.12." (although only enterprise kernels would backport new
> functionality, mainline stable kernels will not).

Fine.
> > + */
> 
> This needs some changes in order to be formatted properly as list and have
> paragraphs. Sigh, nobody really runs:
> 
> cd metadata/ && make && chromium ../docparse/metadata.html
> 
> to have look what output his docs has :( (can be fixed before merge)
> 
Will be fixed and I'll be more careful next time.

> /*\
...
> OPEN_TREE_CLONE));
> Although Cyril mentioned only TST_EXP_FD_SILENT(), IMHO it should be
> followed with:
> 	if (!TST_PASS)
> 		return;
> 
> Or does it make sense to continue testing when open_tree() fails?
> 
> > +	otfd = (int)TST_RET;
> > +
> > +	TST_EXP_PASS_SILENT(mount_setattr(otfd, "", AT_EMPTY_PATH, &attr,
> sizeof(attr)),
> > +		"%s set", tc->name);
> 
> Shouldn't be here also :
> 	if (!TST_PASS)
> 		return;
> 
> check? I guess we need SAFE_ variants for not having to check it all the time.

It's hard for a SAFE_MOUNT_SETATTR:
mount_setattr(2) need:
#include <linux/mount.h> /* Definition of MOUNT_ATTR_* constants */

But linux/mount.h have some conflicts with sys/mount.h, which is widely used in LTP.
If we really want SAFE_MOUNT_SETATTR, we may need a big refactor.

> 
> Or does it make sense to continue testing when mount_setattr() fails?
> 
> > +	TEST(move_mount(otfd, "", AT_FDCWD, OT_MNTPOINT,
> > +MOVE_MOUNT_F_EMPTY_PATH));
> > +
> > +	if (TST_RET == -1) {
> > +		tst_res(TFAIL | TTERRNO, "move_mount() failed");
> > +		return;
> > +	}
> 
> Maybe instead of TEST() and if use this?
> 
> 	TST_EXP_PASS_SILENT(move_mount(otfd, "", AT_FDCWD, OT_MNTPOINT,
> MOVE_MOUNT_F_EMPTY_PATH));
> 
> 	if (!TST_PASS)
> 		return;
> 

Sure, I'll post v3 soon.

Regards,
- Chen

> > +
> > +	SAFE_CLOSE(otfd);
> > +
> > +	TST_EXP_PASS_SILENT(statvfs(OT_MNTPOINT, &buf), "statvfs sucess");
> And here as well.
> > +
> > +	if ((buf.f_flag & tc->expect_attrs) == 0)
> > +		tst_res(TFAIL, "%s is not actually set as expected", tc->name);
> > +	else
> > +		tst_res(TPASS, "%s is actually set as expected", tc->name);
> > +
> > +	if (tst_is_mounted_at_tmpdir(OT_MNTPOINT))
> > +		SAFE_UMOUNT(OT_MNTPOINT);
> > +}
> 
> The rest LGTM.
> 
> Kind regards,
> Petr