[LTP] [PATCH] mmap14: Convert to new API
Ricardo B. Marliere
rbm@suse.com
Wed Oct 30 20:37:13 CET 2024
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,
> };
More information about the ltp
mailing list