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

[Petr Vorel <pvorel@suse.cz>]

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

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