[LTP] 回复: [PATCH] mount03: Convert to new API
chenhx.fnst@fujitsu.com
chenhx.fnst@fujitsu.com
Tue Jul 26 08:12:02 CEST 2022
> -----邮件原件-----
>
> Hi Chen,
>
> quite nice work, but more cleanup is needed.
>
> > testcases/kernel/syscalls/mount/mount03.c | 462
> > ++++++++--------------
> ...
> > #define FILE_MODE (S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH)
> > -#define DIR_MODE (S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP| \
> > - S_IXGRP|S_IROTH|S_IXOTH)
> > #define SUID_MODE (S_ISUID|S_IRUSR|S_IXUSR|S_IXGRP|S_IXOTH)
>
> @Richie @Li @Metan: There are checkpatch.pl warnings. Yes, kernel folks does
> not like permission warnings. Do we want to follow? Or should we remove these
> from our checkpatch.pl fork (we use constants in many places)?
>
OK, will be fixed.
> $ make check-mount03
> mount03.c:29: WARNING: Symbolic permissions
> 'S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH' are not preferred. Consider using octal
> permissions '0644'.
> mount03.c:30: WARNING: Symbolic permissions
> 'S_IRUSR|S_IXUSR|S_IXGRP|S_IXOTH' are not preferred. Consider using octal
> permissions '0511'.
> mount03.c:50: WARNING: static char array declaration should probably be
> static const char
> mount03.c:103: WARNING: Symbolic permissions 'S_IRWXU' are not preferred.
> Consider using octal permissions '0700'.
> mount03.c:114: WARNING: Symbolic permissions 'S_IRWXU' are not preferred.
> Consider using octal permissions '0700'.
> mount03.c:125: WARNING: Symbolic permissions 'S_IRWXU' are not preferred.
> Consider using octal permissions '0700'.
> mount03.c:181: WARNING: Symbolic permissions 'S_IRWXU' are not preferred.
> Consider using octal permissions '0700'.
> mount03.c:204: WARNING: Symbolic permissions 'S_IRWXU' are not preferred.
> Consider using octal permissions '0700'.
>
> ...
> > +#define MNTPOINT "mntpoint"
> ...
> > +static char nobody_uid[] = "nobody";
> I'd remove constant and use "nobody" in the setup (the same like creat08.c).
> If we want to have it defined, I'd use #define (like you use it in MNTPOINT).
>
> Also your code produces warning:
> mount03.c:50: WARNING: static char array declaration should probably be
> static const char
>
> ...
> > +static int test_ms_nosuid(void)
> ...
> > + int pid, status;
> pid_t pid; // don't use int for pid please.
>
> This is not needed (-1 is handled by SAFE_FORK()), therefore you don't need
> switch.
>
> For a readability, your code (most of inherited from the original) is:
> static int test_ms_nosuid(void)
> {
> int pid, status;
>
> switch (pid = SAFE_FORK()) {
> case -1:
> tst_brk(TFAIL | TERRNO, "fork failed");
> break;
> case 0:
> SAFE_SETREUID(ltpuser->pw_uid, ltpuser->pw_uid);
>
> execlp(file, basename(file), NULL);
> exit(1);
> default:
> waitpid(pid, &status, 0);
> if (WIFEXITED(status)) {
> /* reset the setup_uid */
> if (status)
> return 0;
> }
> return 1;
> }
> return 0;
> }
>
> * I'm not sure why exit(1),
> * we have SAFE_EXECLP. Therefore something like:
>
>
> static void test_ms_nosuid(void)
> {
> int status;
> pid_t pid;
>
> pid = SAFE_FORK();
>
> if (!pid) {
> SAFE_SETREUID(ltpuser->pw_uid, ltpuser->pw_uid);
> SAFE_EXECLP(file, basename(file), NULL);
> }
>
> SAFE_WAITPID(pid, &status, 0);
> }
>
> But SAFE_WAITPID() would not allow to print TPASS/TFAIL.
Maybe we can use the original waitpid + tst_res logic.
>
> > -/*
> > - * test_rwflag(int i, int cnt)
> > - * Validate the mount system call for rwflags.
> > - */
> > -int test_rwflag(int i, int cnt)
> > +static void test_rwflag(int i)
> Instead of a crazy long switch which is in the original code, we prefer to use
> functions which are put into struct tcase via function pointer. See e.g.
> mq_open01.c for example.
>
Thanks for the advice, will be fixed.
>
> ...
> > + snprintf(file, PATH_MAX, "%s/mount03_setuid_test", MNTPOINT);
> > + TEST(test_ms_nosuid());
> Please don't reuse TEST() macro for normal function. Just call it:
>
> > + if (TST_RET == 0)
> > + tst_res(TPASS, "mount(2) passed with flag MS_NOSUID");
> > + else
> > + tst_res(TFAIL, "mount(2) failed with flag MS_NOSUID");
> There is TST_EXP_PASS() macro:
> TST_EXP_PASS(test_ms_nosuid())
> if (!TST_PASS)
> return;
>
> But, please don't reuse TEST() macro for normal function. These are meant to
> be for syscalls testing, here I'd print TPASS/TFAIL in test_ms_nosuid() and make
> it void.
>
OK.
> ...
> > +static void setup(void)
> > +{
> > + struct stat file_stat = {0};
>
> > + ltpuser = getpwnam(nobody_uid);
> ltpuser = SAFE_GETPWNAM("nobody");
> (or use constant, but SAFE_GETPWNAM is required (no check, see creat08.c)
>
> > + if (ltpuser == NULL)
> > + tst_res(TFAIL, "getpwnam failed");
> > + snprintf(file, PATH_MAX, "%s/mount03_setuid_test", MNTPOINT);
>
> > + SAFE_STAT(MNTPOINT, &file_stat);
> > if (file_stat.st_mode != SUID_MODE &&
> > - chmod(file, SUID_MODE) < 0)
> > - tst_brkm(TBROK, cleanup,
> > - "setuid for setuid_test failed");
> > - SAFE_UMOUNT(cleanup, mntpoint);
> > -
> > - TEST_PAUSE;
> > + chmod(MNTPOINT, SUID_MODE) < 0)
> > + tst_res(TFAIL, "setuid for setuid_test failed");
> Please use tst_brk(TBROK, ...), original version has tst_brkm(TBROK, ...) for a
> reason [1]:
> 1) tst_brk quits testing, which is usually required in setup function
> 2) TBROK is usually used instead of TFAIL in setup function
>
> TBROK: Something has failed in test preparation phase.
> TFAIL: Test has failed.
>
> (This description should have been in LTP Test Writing Guidelines [2], because it
> applies to both C and shell API.)
>
Sure, I'll read the docs more carefully.
>
> ...
> > +static struct tst_test test = {
> > + .tcnt = ARRAY_SIZE(tcases),
> > + .test = run,
> > + .setup = setup,
> > + .cleanup = cleanup,
> > + .needs_root = 1,
> > + .format_device = 1,
> > + .resource_files = resource_files,
>
> Please inline resource files like:
> .resource_files = (const char *const[]) {
> "mount03_setuid_test",
> NULL,
> },
>
> It produces "mount03_setuid_test" in doc [3]:
>
> | .resource_files | mount03_setuid_test |
>
> But using .resource_files = resource_files produces just "resource_files":
> | .resource_files | resource_files |
>
> (to make doc locally: cd metadata && make # result is
> in ../docparse/metadata.html)
>
> > + .forks_child = 1,
> > + .mntpoint = MNTPOINT,
> > + .skip_filesystems = (const char *const []){"fuse", NULL},
> Why you introduced skipping on FUSE? I don't see it in the old code, nothing
> mentioned in commit message).
>
> Also when I test it for all filesystems (which could be a good idea for mount
> test) it fails for vfat, exfat and ntfs:
My test machine(Centos Stream) ntfs is provided by fuse, so it was skipped...
I'll add them to skip list.
Regards,
- Chen
>
> .skip_filesystems = (const char *const []){"vfat", "exfat", "ntfs", NULL},
> .all_filesystems = 1,
>
> Skipping just fuse does not help in my case:
>
> tst_test.c:1599: TINFO: Testing on ntfs
> tst_test.c:1064: TINFO: Formatting /dev/loop3 with ntfs opts='' extra opts=''
> The partition start sector was not specified for /dev/loop3 and it could not be
> obtained automatically. It has been set to 0.
> The number of sectors per track was not specified for /dev/loop3 and it could
> not be obtained automatically. It has been set to 0.
> The number of heads was not specified for /dev/loop3 and it could not be
> obtained automatically. It has been set to 0.
> To boot from a device, Windows needs the 'partition start sector', the 'sectors
> per track' and the 'number of heads' to be set.
> Windows will not be able to boot from this device.
> mount03.c:102: TFAIL: mount(2) passed with flag MS_RDONLY: open fail with
> EROFS as expected succeeded
> tst_device.c:394: TWARN: umount('mntpoint') failed with EINVAL
> mount03.c:237: TBROK: umount(mntpoint) failed: EINVAL (22)
>
> tst_test.c:1599: TINFO: Testing on exfat
> tst_test.c:1064: TINFO: Formatting /dev/loop2 with exfat opts='' extra opts=''
> mount03.c:102: TPASS: mount(2) passed with flag MS_RDONLY: open fail with
> EROFS as expected : EROFS (30)
> mount03.c:112: TBROK: mknod() failed: EPERM (1)
>
> tst_test.c:1599: TINFO: Testing on vfat
> tst_test.c:1064: TINFO: Formatting /dev/loop1 with vfat opts='' extra opts=''
> mount03.c:102: TPASS: mount(2) passed with flag MS_RDONLY: open fail with
> EROFS as expected : EROFS (30)
> mount03.c:112: TBROK: mknod() failed: EPERM (1)
>
> Kind regards,
> Petr
>
> [1]
> https://github.com/linux-test-project/ltp/wiki/C-Test-API#12-basic-test-interfac
> e
> [2] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines
> [3] http://linux-test-project.github.io/metadata/metadata.stable.html
More information about the ltp
mailing list