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

[Shirisha ganta]

On Tue, 2023-11-28 at 12:10 +0100, Petr Vorel wrote:
> 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?

Will take care of this in V4
> 
> 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.
This is the general practice followed by all the test cases in this
folder and I followed the same.
> 
> > 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.
Will take care of this in V4
> > + *
> > + * 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)

Will take care of this in V4
> 
> > +{
> > +	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.
Will take care of this in V4
> 
> > +	if (err)
> > +		tst_res(TFAIL, "ftruncate failed");
> Also here.
Will take care of this in V4

> > +	if (sigsetjmp(sig_escape, 1) == 0)
> > +		*q;
> > +	else
> > +		sigbus_count++;
> And here.
Will take care of this in V4
> > +	if (sigbus_count != 1)
> > +		tst_res(TFAIL, "Didn't SIGBUS");
> And here.
Will take care of this in V4
> > +	if (test_pass == 1)
> > +		tst_res(TPASS, "Expected SIGBUS triggered");
> And here.
Will take care of this in V4
> > +	SAFE_MUNMAP(p, hpage_size);
> > +}
> 
> Kind regards,
> Petr