[LTP] [PATCH] mmap14: Convert to new API

[LTP] [PATCH] mmap14: Convert to new API

[Ricardo B. Marliere via ltp]

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>
  *
- * 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)."
@@ -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);
-static void setup(void);
-static void cleanup(void);
 
-int main(int argc, char *argv[])
+static void run(unsigned int n)
 {
-	int lc;
 	unsigned int sz_before;
 	unsigned int sz_after;
 	unsigned int sz_ch;
 
-	tst_parse_opts(argc, argv, NULL, NULL);
-
-	setup();
-
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-
-		tst_count = 0;
-
-		getvmlck(&sz_before);
+	getvmlck(&sz_before);
 
-		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);
 
-		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);
 
-		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");
+	} else {
+		tst_res(TFAIL, "Expected %luK locked, "
+				"get %uK locked",
+				MMAPSIZE / 1024, sz_ch);
 	}
 
-	cleanup();
-	tst_exit();
+	if (munmap(addr, MMAPSIZE) != 0)
+		tst_res(TFAIL | TERRNO, "munmap failed");
 }
 
 void getvmlck(unsigned int *lock_sz)
@@ -97,7 +67,7 @@ void getvmlck(unsigned int *lock_sz)
 
 	fstatus = fopen("/proc/self/status", "r");
 	if (fstatus == NULL)
-		tst_brkm(TFAIL | TERRNO, NULL, "Open dev status failed");
+		tst_res(TFAIL | TERRNO, "Open dev status failed");
 
 	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");
 
 	fclose(fstatus);
 }
 
-static void setup(void)
-{
-	tst_require_root();
-
-	tst_sig(FORK, DEF_HANDLER, cleanup);
-
-	TEST_PAUSE;
-}
-
-static void cleanup(void)
-{
-}
+static struct tst_test test = {
+	.needs_root = 1,
+	.test = run,
+	.tcnt = 1,
+};

---
base-commit: 34e6dd2d233abcebae435d34d793b6a49d08c190
change-id: 20241025-convert_mmap14-93b0a247741e

Best regards,
-- 
Ricardo B. Marliere <rbm@suse.com>


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

Re: [LTP] [PATCH] mmap14: Convert to new API

[Petr Vorel]

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,
 };

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

Re: [LTP] [PATCH] mmap14: Convert to new API

[Ricardo B. Marliere via ltp]

Hi Petr!

On 30 Oct 24 17:21, Petr Vorel wrote:
> 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

Nice!

> 
> > @@ -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")

Ah, makes sense.

> 
> 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.

Ouch, indeed..

> 
> >  	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.

Many thanks for reviewing and guidance. I'll make sure to properly read
the documentation for next time so as to avoid wasting review cycles :)

I'll send a v2.

> 
> 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,
>  };

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