Re: [LTP] [PATCH v3] Migrating the libhugetlbfs/testcases/truncate.c test

[Petr Vorel <pvorel@suse.cz>]

Hi,

Maybe I'm slow, but it was not not obvious from the subject "Migrating the
libhugetlbfs/testcases/truncate.c test" that you migrate test from:
https://github.com/libhugetlbfs/libhugetlbfs/blob/master/tests/truncate.c

I'd personally have subject

Add hugemmap33 (very sort description of the test)

And then later mention in the commit message that the test originates from
https://github.com/libhugetlbfs/libhugetlbfs/blob/master/tests/truncate.c

NOTE: if you run test more times (-iN), it fails, e.g.:

./hugemmap33 -i2
tst_hugepage.c:84: TINFO: 1 hugepage(s) reserved
tst_test.c:1037: TINFO: Mounting none to /tmp/LTP_hugiULTJ7/hugetlbfs fstyp=hugetlbfs flags=0
tst_test.c:1690: TINFO: LTP version: 20230929-152-gce87c8de3
tst_test.c:1574: TINFO: Timeout per run is 0h 00m 30s
hugemmap33.c:56: TPASS: Expected SIGBUS triggered
tst_test.c:1634: TBROK: Test killed by SIGBUS!

Could you please fix it?

Also, we use static whenever we can, please fix these:

$ make check-hugemmap33
CHECK testcases/kernel/mem/hugetlb/hugemmap/hugemmap33.c
hugemmap33.c:61:6: warning: Symbol 'setup' has no prototype or library ('tst_') prefix. Should it be static?
hugemmap33.c:71:6: warning: Symbol 'cleanup' has no prototype or library ('tst_') prefix. Should it be static?

> Test Description:
This line is useless.

> Test is used to verify the correct functionality and
> compatibility of the library with the "truncate" system
> call when operating on files residing in a mounted
> huge page filesystem.

> Signed-off-by: Shirisha G <shirisha@linux.ibm.com>
> ---
> v3:
>  -Addressed the below requested changes 
>   1. Removed RANDOM_CONSTANT
>   2. Made hpage_size and fd to static
>   3. Used a volatile variable as a flag
>      to pass test in the run_test()
>   4. Removed the failure condition for SAFE_MMAP()
>   5. Have setup the handler in the setup()
>   6. Added SAFE_MUNMAP()
>   7. Ran make check and fixed all issues
> ---
> v2:
>  -Corrected typo
> ---
>  runtest/hugetlb                               |  1 +
>  testcases/kernel/mem/.gitignore               |  1 +
>  .../kernel/mem/hugetlb/hugemmap/hugemmap33.c  | 88 +++++++++++++++++++
>  3 files changed, 90 insertions(+)
>  create mode 100644 testcases/kernel/mem/hugetlb/hugemmap/hugemmap33.c

> diff --git a/runtest/hugetlb b/runtest/hugetlb
> index 299c07ac9..1300e80fb 100644
> --- a/runtest/hugetlb
> +++ b/runtest/hugetlb
> @@ -35,6 +35,7 @@ hugemmap29 hugemmap29
>  hugemmap30 hugemmap30
>  hugemmap31 hugemmap31
>  hugemmap32 hugemmap32
> +hugemmap33 hugemmap33
>  hugemmap05_1 hugemmap05 -m
>  hugemmap05_2 hugemmap05 -s
>  hugemmap05_3 hugemmap05 -s -m
> diff --git a/testcases/kernel/mem/.gitignore b/testcases/kernel/mem/.gitignore
> index 7258489ed..d130d4dcd 100644
> --- a/testcases/kernel/mem/.gitignore
> +++ b/testcases/kernel/mem/.gitignore
> @@ -34,6 +34,7 @@
>  /hugetlb/hugemmap/hugemmap30
>  /hugetlb/hugemmap/hugemmap31
>  /hugetlb/hugemmap/hugemmap32
> +/hugetlb/hugemmap/hugemmap33
>  /hugetlb/hugeshmat/hugeshmat01
>  /hugetlb/hugeshmat/hugeshmat02
>  /hugetlb/hugeshmat/hugeshmat03
> diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap33.c b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap33.c
> new file mode 100644
> index 000000000..3405627f6
> --- /dev/null
> +++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap33.c
> @@ -0,0 +1,88 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2005-2006 IBM Corporation.
Maybe also your or LTP copyright here, as clearly there is LTP specific code.

> + * Author: David Gibson & Adam Litke
> + */
> +
> +/*\
> + * [Description]
> + *
> + * Test Name: truncate
This is wrong and useless, please remove it.
> + *
> + * Test case is used to verify the correct functionality
> + * and compatibility of the library with the "truncate" system call when
> + * operating on files residing in a mounted huge page filesystem.
> + */
> +
> +#include "hugetlb.h"
> +#include <setjmp.h>
> +#include <signal.h>
> +
> +#define MNTPOINT "hugetlbfs/"
> +
> +static long hpage_size;
> +static int fd;
> +static sigjmp_buf sig_escape;
> +static volatile int test_pass;
> +static int sigbus_count;
> +
> +static void sigbus_handler(int signum)

hugemmap33.c:29:32: warning: unused parameter ‘signum’ [-Wunused-parameter]
   29 | static void sigbus_handler(int signum)

Therefore use
static void sigbus_handler(int signum LTP_ATTRIBUTE_UNUSED)

> +{
> +	test_pass = 1;
> +	siglongjmp(sig_escape, 17);
> +}
> +
> +static void run_test(void)
> +{
> +	void *p;
> +	volatile unsigned int *q;
> +
> +	sigbus_count = 0;
> +	test_pass = 0;
> +	int err;
> +
> +	p = SAFE_MMAP(NULL, hpage_size, PROT_READ|PROT_WRITE, MAP_SHARED,
> +			fd, 0);
> +	q = p;
> +	*q = 0;
> +	err = ftruncate(fd, 0);
Blank line here would help readability.

> +	if (err)
> +		tst_res(TFAIL, "ftruncate failed");
Also here.
> +	if (sigsetjmp(sig_escape, 1) == 0)
> +		*q;
> +	else
> +		sigbus_count++;
And here.
> +	if (sigbus_count != 1)
> +		tst_res(TFAIL, "Didn't SIGBUS");
And here.
> +	if (test_pass == 1)
> +		tst_res(TPASS, "Expected SIGBUS triggered");
And here.
> +	SAFE_MUNMAP(p, hpage_size);
> +}

Kind regards,
Petr

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