Re: [LTP] [PATCH v2] munlockall: add test case that verifies memory has been unlocked

[Petr Vorel <pvorel@suse.cz>]

Hi Dennis,

Generally LGTM, thanks!

Reviewed-by: Petr Vorel <pvorel@suse.cz>

you should also add test into .gitignore and to some file in runtest/ (in this
case into runtest/syscalls), if we don't simply replace your test with munlockall01.c
as Cyril suggested (I would also vote for replacing it).

FYI you can have look for what we check:

https://github.com/linux-test-project/ltp/wiki/Maintainer-Patch-Review-Checklist

> Changes to v1:

> - use a docparse comment
> - use tabs for indentation
> - report broken test and exit if any preparation/confirmation fails
>   by using tst_brk(TBROK, ...)
> - fix further violations reported by `make check`

> I did not yet replace munlockall01.c because I am not familiar with
I guess Cyril meant just to replace munlockall01.c with what you wrote.
+1 for this, there is no point to keep original munlockall01.c.
And because you replace, you can delete the original copyright and can use
GPL-2.0-or-later (original test was GPL v2 only).

> that (legacy?) syntax and why uclinux needs special handling.

uclinux is for nommu, we don't support it in new API (and nobody from the
community standup for the support), it can be just deleted.

> ---
>  .../kernel/syscalls/munlockall/munlockall02.c | 51 +++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 testcases/kernel/syscalls/munlockall/munlockall02.c

> diff --git a/testcases/kernel/syscalls/munlockall/munlockall02.c b/testcases/kernel/syscalls/munlockall/munlockall02.c
> new file mode 100644
> index 000000000..06f781d86
> --- /dev/null
> +++ b/testcases/kernel/syscalls/munlockall/munlockall02.c
> @@ -0,0 +1,51 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright Red Hat
> + * Author: Dennis Brendel <dbrendel@redhat.com>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * Verify that munlockall(2) unlocks all previously locked memory
> + */
> +
> +#include <sys/mman.h>
> +
> +#include "tst_test.h"
> +
> +static void verify_munlockall(void)
> +{
> +	size_t size = 0;
> +
> +	SAFE_FILE_LINES_SCANF("/proc/self/status", "VmLck: %ld", &size);
> +
> +	if (size != 0UL)
> +		tst_brk(TBROK, "Locked memory after init should be 0 but is "
> +			       "%ld", size);
nit: I would not split string (kernel source code also relaxed to 100 chars
instead of 8O).

> +
> +	if (mlockall(MCL_CURRENT | MCL_FUTURE) != 0)
> +		tst_brk(TBROK, "Could not lock memory using mlockall()");
Maybe use tst_brk(TBROK | TERRNO, to get errno printed?
> +
> +	SAFE_FILE_LINES_SCANF("/proc/self/status", "VmLck: %ld", &size);
> +
> +	if (size == 0UL)
> +		tst_brk(TBROK, "Locked memory after mlockall() should be "
> +			       "greater than 0, but is %ld", size);
I suppose < 0 really means no memory locked, thus really safe to quit before
munlockall(), right?
> +
> +	if (munlockall() != 0)
> +		tst_brk(TBROK, "Could not unlock memory using munlockall()");
Also here use TBROK | TERRNO ?
> +
> +	SAFE_FILE_LINES_SCANF("/proc/self/status", "VmLck: %ld", &size);
> +
> +	if (size != 0UL) {
> +		tst_res(TFAIL, "Locked memory after munlockall() should be 0 "
> +			       "but is %ld", size);
nit: also here I would join string to single line (still below 100 chars).
> +	} else {
> +		tst_res(TPASS, "Test passed");
nit maybe:
		tst_res(TPASS, "Memory successfully locked and unlocked");

Kind regards,
Petr

> +	}
> +}
> +
> +static struct tst_test test = {
> +	.test_all = verify_munlockall,
> +};

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