[LTP] [PATCH] mmap14: Convert to new API
Petr Vorel
pvorel@suse.cz
Wed Oct 30 17:21:14 CET 2024
Hi Ricardo,
very nice, few minor things below.
> Signed-off-by: Ricardo B. Marliere <rbm@suse.com>
> ---
> testcases/kernel/syscalls/mmap/mmap14.c | 97 ++++++++++-----------------------
> 1 file changed, 30 insertions(+), 67 deletions(-)
> diff --git a/testcases/kernel/syscalls/mmap/mmap14.c b/testcases/kernel/syscalls/mmap/mmap14.c
> index 31632601bbed85b60f1dfee7044bf961b1f2a756..63a2f9e4cab2f7ab12b1ad982bc690f9fabcc252 100644
> --- a/testcases/kernel/syscalls/mmap/mmap14.c
> +++ b/testcases/kernel/syscalls/mmap/mmap14.c
> @@ -1,22 +1,12 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> /*
> * Copyright (c) 2013 FNST, DAN LI <li.dan@cn.fujitsu.com>
> *
nit: blank space above (ideally to remove). You could also add your/SUSE/LTP copyright.
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License as published by
> - * the Free Software Foundation; either version 2 of the License, or
> - * (at your option) any later version.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See
> - * the GNU General Public License for more details.
> - *
> - * You should have received a copy of the GNU General Public License
> - * along with this program; if not, write to the Free Software
> - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> */
> -/*
> +/*\
> + * [Description]
> + *
> * Test Description:
> * Verify MAP_LOCKED works fine.
> * "Lock the pages of the mapped region into memory in the manner of mlock(2)."
I would use something like:
/*\
* [Description]
*
* Verify basic functionality of mmap(2) with MAP_LOCKED.
*
* mmap(2) should succeed returning the address of the mapped region,
* and this region should be locked into memory.
*/
Also, having more than one space after '*' causes different formatting in
auto-generated doc (it will become <pre>...</pre>)
You can see it if you have asciidoctor (or asciidoc) installed and run
cd metadata && make
resulting docs is in ../docparse/*.html
We publish the docs in releases, e.g.:
https://github.com/linux-test-project/ltp/releases/download/20240930/metadata.20240930.html
> @@ -28,65 +18,45 @@
> #include <stdio.h>
> #include <sys/mman.h>
> -#include "test.h"
> +#include "tst_test.h"
> #define TEMPFILE "mmapfile"
> #define MMAPSIZE (1UL<<20)
> #define LINELEN 256
> -char *TCID = "mmap14";
> -int TST_TOTAL = 1;
> -
> static char *addr;
> static void getvmlck(unsigned int *lock_sz);
Ideally you just move getvmlck() code above instead using function signature.
> +static void run(unsigned int n)
Here n is unused. That's why you should use void
static void run(void)
And below in the struct tst_test test just:
.test_all = run,
See our old doc (not yet converted to the new format):
https://github.com/linux-test-project/ltp/blob/master/doc/old/C-Test-API.asciidoc#11-basic-test-structure
> {
> unsigned int sz_before;
> unsigned int sz_after;
> unsigned int sz_ch;
> - addr = mmap(NULL, MMAPSIZE, PROT_READ | PROT_WRITE,
> - MAP_PRIVATE | MAP_LOCKED | MAP_ANONYMOUS,
> - -1, 0);
> + addr = mmap(NULL, MMAPSIZE, PROT_READ | PROT_WRITE,
> + MAP_PRIVATE | MAP_LOCKED | MAP_ANONYMOUS,
> + -1, 0);
very nit (just formatting - feel free to reformat the old test):
addr = mmap(NULL, MMAPSIZE, PROT_READ | PROT_WRITE,
MAP_PRIVATE | MAP_LOCKED | MAP_ANONYMOUS, -1, 0);
> - if (addr == MAP_FAILED) {
> - tst_resm(TFAIL | TERRNO, "mmap of %s failed", TEMPFILE);
> - continue;
> - }
> + if (addr == MAP_FAILED)
> + tst_res(TFAIL | TERRNO, "mmap of %s failed", TEMPFILE);
You need to have return here (like in mmap05.c) to skip the rest of the test.
In the old API this was achieved by continue.
> - getvmlck(&sz_after);
> + getvmlck(&sz_after);
> - sz_ch = sz_after - sz_before;
> - if (sz_ch == MMAPSIZE / 1024) {
> - tst_resm(TPASS, "Functionality of mmap() "
> - "successful");
> - } else {
> - tst_resm(TFAIL, "Expected %luK locked, "
> - "get %uK locked",
> - MMAPSIZE / 1024, sz_ch);
> - }
> -
> - if (munmap(addr, MMAPSIZE) != 0)
> - tst_brkm(TFAIL | TERRNO, NULL, "munmap failed");
> + sz_ch = sz_after - sz_before;
> + if (sz_ch == MMAPSIZE / 1024) {
> + tst_res(TPASS, "Functionality of mmap() "
> + "successful");
This is not much useful information. Also IMHO it's better to avoid splitting
string (not ideal for grepping)
> + } else {
> + tst_res(TFAIL, "Expected %luK locked, "
> + "get %uK locked",
> + MMAPSIZE / 1024, sz_ch);
> }
Maybe something like:
if (sz_ch == MMAPSIZE / 1024) {
tst_res(TPASS, "mmap() locked %uK", sz_ch);
} else {
tst_res(TFAIL, "Expected %luK locked, get %uK locked",
MMAPSIZE / 1024, sz_ch);
}
> + if (munmap(addr, MMAPSIZE) != 0)
> + tst_res(TFAIL | TERRNO, "munmap failed");
The above should be replaced with:
SAFE_MUNMAP((char *)addr, sz_ch);
> }
> void getvmlck(unsigned int *lock_sz)
Here is missing static.
> @@ -97,7 +67,7 @@ void getvmlck(unsigned int *lock_sz)
> fstatus = fopen("/proc/self/status", "r");
Please, use here:
fstatus = SAFE_FOPEN("/proc/self/status", "r");
(it needs #include "tst_safe_stdio.h")
See old (not yet converted docs):
https://github.com/linux-test-project/ltp/blob/master/doc/old/C-Test-API.asciidoc#a-word-about-the-cleanup-callback
Or in our old tutorial (already converted to the new API):
https://linux-test-project.readthedocs.io/en/latest/developers/test_case_tutorial.html
> if (fstatus == NULL)
> - tst_brkm(TFAIL | TERRNO, NULL, "Open dev status failed");
> + tst_res(TFAIL | TERRNO, "Open dev status failed");
Then the check above is not needed. Also beware, tst_brk() (or tst_brkm()) quits
testing. But tst_res() usually not (tst_res(TBROK, ...) still quits testing, but
that's kind of misusing the API), thus you changed a test workflow.
> while (fgets(line, LINELEN, fstatus) != NULL)
> if (strstr(line, "VmLck") != NULL)
> @@ -105,20 +75,13 @@ void getvmlck(unsigned int *lock_sz)
> ret = sscanf(line, "%*[^0-9]%d%*[^0-9]", lock_sz);
> if (ret != 1)
> - tst_brkm(TFAIL | TERRNO, NULL, "Get lock size failed");
> + tst_res(TFAIL | TERRNO, "Get lock size failed");
Again, SAFE_SSCANF() instead of check, also remember tst_brk() tst_res() difference.
int ret is then not needed.
Also, we usually use in tests SAFE_FCLOSE(fstatus) instead of fclose(fstatus)
(it checks the return value and yells if unexpected).
...
> + .needs_root = 1,
As I noted above, instead of these two:
> + .test = run,
> + .tcnt = 1,
please use just:
.test_all = run,
If you want, I can merge with the following diff below
(+ the copyright you prefer me to add).
Or just send another version.
Kind regards,
Petr
+++ testcases/kernel/syscalls/mmap/mmap14.c
@@ -1,23 +1,20 @@
// SPDX-License-Identifier: GPL-2.0-or-later
/*
* Copyright (c) 2013 FNST, DAN LI <li.dan@cn.fujitsu.com>
- *
*/
/*\
* [Description]
*
- * Test Description:
- * Verify MAP_LOCKED works fine.
- * "Lock the pages of the mapped region into memory in the manner of mlock(2)."
+ * Verify basic functionality of mmap(2) with MAP_LOCKED.
*
- * Expected Result:
- * mmap() should succeed returning the address of the mapped region,
- * and this region should be locked into memory.
+ * mmap(2) should succeed returning the address of the mapped region,
+ * and this region should be locked into memory.
*/
+
#include <stdio.h>
#include <sys/mman.h>
-
+#include "tst_safe_stdio.h"
#include "tst_test.h"
#define TEMPFILE "mmapfile"
@@ -26,9 +23,23 @@
static char *addr;
-static void getvmlck(unsigned int *lock_sz);
+static void getvmlck(unsigned int *lock_sz)
+{
+ char line[LINELEN];
+ FILE *fstatus = NULL;
+
+ fstatus = SAFE_FOPEN("/proc/self/status", "r");
+
+ while (fgets(line, LINELEN, fstatus) != NULL)
+ if (strstr(line, "VmLck") != NULL)
+ break;
+
+ SAFE_SSCANF(line, "%*[^0-9]%d%*[^0-9]", lock_sz);
+
+ SAFE_FCLOSE(fstatus);
+}
-static void run(unsigned int n)
+static void run(void)
{
unsigned int sz_before;
unsigned int sz_after;
@@ -37,8 +48,7 @@ static void run(unsigned int n)
getvmlck(&sz_before);
addr = mmap(NULL, MMAPSIZE, PROT_READ | PROT_WRITE,
- MAP_PRIVATE | MAP_LOCKED | MAP_ANONYMOUS,
- -1, 0);
+ MAP_PRIVATE | MAP_LOCKED | MAP_ANONYMOUS, -1, 0);
if (addr == MAP_FAILED)
tst_res(TFAIL | TERRNO, "mmap of %s failed", TEMPFILE);
@@ -47,41 +57,16 @@ static void run(unsigned int n)
sz_ch = sz_after - sz_before;
if (sz_ch == MMAPSIZE / 1024) {
- tst_res(TPASS, "Functionality of mmap() "
- "successful");
+ tst_res(TPASS, "mmap() locked %uK", sz_ch);
} else {
- tst_res(TFAIL, "Expected %luK locked, "
- "get %uK locked",
+ tst_res(TFAIL, "Expected %luK locked, get %uK locked",
MMAPSIZE / 1024, sz_ch);
}
- if (munmap(addr, MMAPSIZE) != 0)
- tst_res(TFAIL | TERRNO, "munmap failed");
-}
-
-void getvmlck(unsigned int *lock_sz)
-{
- int ret;
- char line[LINELEN];
- FILE *fstatus = NULL;
-
- fstatus = fopen("/proc/self/status", "r");
- if (fstatus == NULL)
- tst_res(TFAIL | TERRNO, "Open dev status failed");
-
- while (fgets(line, LINELEN, fstatus) != NULL)
- if (strstr(line, "VmLck") != NULL)
- break;
-
- ret = sscanf(line, "%*[^0-9]%d%*[^0-9]", lock_sz);
- if (ret != 1)
- tst_res(TFAIL | TERRNO, "Get lock size failed");
-
- fclose(fstatus);
+ SAFE_MUNMAP((char *)addr, sz_ch);
}
static struct tst_test test = {
.needs_root = 1,
- .test = run,
- .tcnt = 1,
+ .test_all = run,
};
More information about the ltp
mailing list