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

[Petr Vorel <pvorel@suse.cz>]

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