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

[Dennis Brendel]

Hi Petr,

Thank you very much for your feedback! I'll send a v3 which considers your
suggestions.

Dennis

On 3/5/24 22:30, Petr Vorel wrote:
> 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,
>> +};
>