Re: [LTP] [PATCH v5] syscalls/mount_setattr01: Add basic functional test

xuyang2018.jy
2022-05-18T14:16:53Z

Hi Petr
> Hi Chen, Dai,
>
> ...
>> +static void setup(void)
>> +{
>> + fsopen_supported_by_kernel();
>> + struct stat st = {0};
>> +
>> + if (stat(OT_MNTPOINT,&st) == -1)
>> + SAFE_MKDIR(OT_MNTPOINT, 0777);
>> +}
>> +
>> +static void run(unsigned int n)
>> +{
>> + struct tcase *tc =&tcases[n];
>> + struct mount_attr attr = {
>> + .attr_set = tc->mount_attrs,
>> + };
>> + struct statvfs buf;
>> +
>> + TST_EXP_FD_SILENT(open_tree(AT_FDCWD, MNTPOINT, AT_EMPTY_PATH |
>> + AT_SYMLINK_NOFOLLOW | OPEN_TREE_CLOEXEC | OPEN_TREE_CLONE));
>> + if (!TST_PASS)
>> + return;
>> +
>> + otfd = (int)TST_RET;
>> +
>> + TST_EXP_PASS_SILENT(mount_setattr(otfd, "", AT_EMPTY_PATH,&attr, sizeof(attr)),
>> + "%s set", tc->name);
>> + if (!TST_PASS)
>> + goto out1;
>> +
>> + TST_EXP_PASS_SILENT(move_mount(otfd, "", AT_FDCWD, OT_MNTPOINT, MOVE_MOUNT_F_EMPTY_PATH));
>> + if (!TST_PASS)
>> + goto out1;
>> + mount_flag = 1;
>> + SAFE_CLOSE(otfd);
>> +
>> + TST_EXP_PASS_SILENT(statvfs(OT_MNTPOINT,&buf), "statvfs sucess");
>> + if (!TST_PASS)
>> + goto out2;
>> +
>> + if (buf.f_flag& tc->expect_attrs)
>> + tst_res(TPASS, "%s is actually set as expected", tc->name);
>> + else
>> + tst_res(TFAIL, "%s is not actually set as expected", tc->name);
>> +
>> + goto out2;
>> +
>> +out1:
>> + SAFE_CLOSE(otfd);
>> +out2:
>> + mount_flag = 0;
>> + SAFE_UMOUNT(OT_MNTPOINT);
> mount_flag needs to be checked before otherwise if it fails on:
> TST_EXP_PASS_SILENT(mount_setattr(otfd, "", AT_EMPTY_PATH,&attr, sizeof(attr)),
> "%s set", tc->name);
>
> It tries to umount() not yet mounted filesystem:
>
> mount_setattr01.c:109: TPASS: MOUNT_ATTR_RDONLY is actually set as expected
> mount_setattr01.c:109: TPASS: MOUNT_ATTR_NOSUID is actually set as expected
> mount_setattr01.c:109: TPASS: MOUNT_ATTR_NODEV is actually set as expected
> mount_setattr01.c:109: TPASS: MOUNT_ATTR_NOEXEC is actually set as expected
> mount_setattr01.c:93: TFAIL: MOUNT_ATTR_NOSYMFOLLOW set failed: EINVAL (22)
> tst_device.c:394: TWARN: umount('ot_mntpoint') failed with EINVAL
> mount_setattr01.c:119: TBROK: umount(ot_mntpoint) failed: EINVAL (22)
>
> Because we mount later than opening otfd with open_tree() but need to close otfd
> before umount() we cannot depend only on labels.
>
> Thus I suggest to have this cleanup code:
>
> out1:
> SAFE_CLOSE(otfd);
> out2:
> if (mount_flag)
> SAFE_UMOUNT(OT_MNTPOINT);
>
> mount_flag = 0;
>
> If Cyril is ok with this I'd merge it before release.
> (I tested it on various systems, found only this issue.)
This fix is obviously correct. Since I have reviewed this patch in
internal, you can also add my reviewed-by for this patch.

ps: I think we should also check functionality like mount03 did instead
of just check flag.

Best Regards
Yang Xu
>
> Kind regards,
> Petr
>
> +++ testcases/kernel/syscalls/mount_setattr/mount_setattr01.c
> @@ -115,8 +115,10 @@ static void run(unsigned int n)
> out1:
> SAFE_CLOSE(otfd);
> out2:
> + if (mount_flag)
> + SAFE_UMOUNT(OT_MNTPOINT);
> +
> mount_flag = 0;
> - SAFE_UMOUNT(OT_MNTPOINT);
> }
>
> static struct tst_test test = {
>