[LTP] [PATCH v4] syscalls/umount2: Convert to new API and use SAFE_ACCESS

[daisl.fnst@fujitsu.com]

Hi Petr,

Thanks for review, I will send v5 later.

在 2022/3/24 17:11, Petr Vorel 写道:
> Hi Dai,
>
> We're nearly there, IMHO 2 minor changes are required.
>
> nit: you can help reviewers to list changelog from previous version (for your
> next patches), see:
>
> https://patchwork.ozlabs.org/project/ltp/patch/20220324022455.245300-1-zhaogongyi@huawei.com/
> https://patchwork.ozlabs.org/project/ltp/cover/20211103120233.20728-1-chrubis@suse.cz/
>
>> 1. use TST_EXP_FAIL and TST_EXP_PASS macro
>> 2. use SAFE macro
>> 3. simplify verify operations
>> 4. merge umount2_03 to umount2_02
> very nit: more important that "use SAFE macro" (that's one of the main reasons
> why we use new API) is to mention the reason why to merge tests.
>
> ...
>> +/*\
>> + * [Description]
>> + *
>>    *  Test for feature MNT_EXPIRE of umount2().
> This must be:
>    * Test for feature MNT_EXPIRE of umount2().
> extra space after '* ' leads text to be formatted as <pre> or <code>.
>
>> - *  "Mark the mount point as expired.If a mount point is not currently
>> - *   in use, then an initial call to umount2() with this flag fails with
>> - *   the error EAGAIN, but marks the mount point as expired. The mount
>> - *   point remains expired as long as it isn't accessed by any process.
>> - *   A second umount2() call specifying MNT_EXPIRE unmounts an expired
>> - *   mount point. This flag cannot be specified with either MNT_FORCE or
>> - *   MNT_DETACH. (fails with the error EINVAL)"
>> + *
>> + * - EINVAL when flag is specified with either MNT_FORCE or MNT_DETACH
>> + * - EAGAIN when initial call to umount2(2) with MNT_EXPIRE
>> + * - EAGAIN when umount2(2) with MNT_EXPIRE after access(2)
>> + * - succeed when second call to umount2(2) with MNT_EXPIRE
>> + *
>> + *  Test for feature UMOUNT_NOFOLLOW of umount2().
> And here as well:
> * Test for feature UMOUNT_NOFOLLOW of umount2().
>
> run make in metadata/ and then check docparse/metadata.html
> (or docparse/metadata.pdf).
>
> (1st thing to change)
>
>> + *
>> + * - EINVAL when target is a symbolic link
>> + * - succeed when target is a mount point
>>    */
>> -#include <errno.h>
>>   #include <sys/mount.h>
>> -
>> -#include "test.h"
>> -#include "safe_macros.h"
>>   #include "lapi/mount.h"
>> -
>> +#include "tst_test.h"
>>   #include "umount2.h"
> umount2.h is now used only in this test. Please put the content into the test
> and delete the file (2nd change to do).
>
> ...
>> +} tcases[] = {
>> +	{MNTPOINT, MNT_EXPIRE | MNT_FORCE, EINVAL, 0,
>> +		"umount2() with MNT_EXPIRE | MNT_FORCE expected EINVAL"},
> nit: I was thinking about using macro to not repeat to flag and exp_errno,
> (something similar to POLICY_DESC_TEXT(x, y) in
> testcases/kernel/syscalls/mbind/mbind01.c), but as description varies a lot it's
> probably better to keep it as is (unless you rephrase it).
>
> ...
>
>> +	{SYMLINK, UMOUNT_NOFOLLOW, EINVAL, 0,
>> +		"umount2('symlink', UMOUNT_NOFOLLOW) expected EINVAL"},
> ...
>> +	{MNTPOINT, UMOUNT_NOFOLLOW, 0, 0,
>> +		"umount2('mntpoint', UMOUNT_NOFOLLOW) expected success"},
>> +};
> ...
>> +	if (tc->do_access)
>> +		SAFE_ACCESS(MNTPOINT, F_OK);
>> -	if (TEST_ERRNO != test_cases[i].exp_errno) {
>> -		tst_resm(TFAIL | TTERRNO, "%s failed unexpectedly",
>> -			 test_cases[i].desc);
>> -		return;
>> +	if (tc->exp_errno) {
>> +		TST_EXP_FAIL(umount2_retry(tc->mntpoint, tc->flag), tc->exp_errno,
>> +			"umount2_retry(%s, %d)", tc->mntpoint, tc->flag);
>> +		if (!TST_PASS)
>> +			mount_flag = 0;
>> +	} else {
>> +		TST_EXP_PASS(umount2_retry(tc->mntpoint, tc->flag),
>> +			"umount2_retry(%s, %d)", tc->mntpoint, tc->flag);
>> +		if (TST_PASS)
>> +			mount_flag = 0;
>>   	}
> nit: this would be more compact:
>
> 	if (tc->exp_errno)
> 		TST_EXP_FAIL(umount2_retry(tc->mntpoint, tc->flag), tc->exp_errno,
> 			"umount2_retry(%s, %d)", tc->mntpoint, tc->flag);
> 	else
> 		TST_EXP_PASS(umount2_retry(tc->mntpoint, tc->flag),
> 			"umount2_retry(%s, %d)", tc->mntpoint, tc->flag);
>
> 	if (!!tc->exp_errno ^ !!TST_PASS)
> 		mount_flag = 0;
>
> Kind regards,
> Petr