diff mbox series

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

Message ID 6dfa210d-1c9a-4888-9408-eccc3068cd2a@redhat.com
State Changes Requested
Headers show
Series [v2] munlockall: add test case that verifies memory has been unlocked | expand

Commit Message

Dennis Brendel March 5, 2024, 5:26 p.m. UTC 
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
that (legacy?) syntax and why uclinux needs special handling.

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

Comments

Petr Vorel March 5, 2024, 9:30 p.m. UTC | #1 
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,
> +};
Dennis Brendel March 6, 2024, 8:49 a.m. UTC | #2 
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,
>> +};
>
Dennis Brendel March 6, 2024, 9 a.m. UTC | #3 
On 3/5/24 22:30, Petr Vorel wrote:
>> +	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?

Forgot to reply to that.

< 0 should not happen due to unsigned type.

If the size is 0 then it is safe to quit. In general it should always be safe
to exit even if there was memory locked, i.e. when the parsing of
/proc/self/status is faulty or when wrong numbers are reported.

Dennis
Petr Vorel March 6, 2024, 10:07 a.m. UTC | #4 
> On 3/5/24 22:30, Petr Vorel wrote:
> >> +	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?

> Forgot to reply to that.

> < 0 should not happen due to unsigned type.

> If the size is 0 then it is safe to quit. In general it should always be safe
> to exit even if there was memory locked, i.e. when the parsing of
> /proc/self/status is faulty or when wrong numbers are reported.

Good point, thanks!

Kind regards,
Petr

> Dennis
diff mbox series

Patch

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);
+
+	if (mlockall(MCL_CURRENT | MCL_FUTURE) != 0)
+		tst_brk(TBROK, "Could not lock memory using mlockall()");
+
+	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);
+
+	if (munlockall() != 0)
+		tst_brk(TBROK, "Could not unlock memory using munlockall()");
+
+	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);
+	} else {
+		tst_res(TPASS, "Test passed");
+	}
+}
+
+static struct tst_test test = {
+	.test_all = verify_munlockall,
+};