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

[Shirisha ganta <shirisha@linux.ibm.com>]

On Fri, 2023-09-08 at 14:15 +0200, Cyril Hrubis wrote:
> Hi!
> > +/*\
> > + * [Description]
> > + *
> > + * Test Name: truncate
> > + * 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"
> > +
> > +#define RANDOM_CONSTANT	0x1234ABCD
>              ^
> 	     THis is not used at all.
Will take care of this in v3.
> 
> > +#define MNTPOINT "hugetlbfs/"
> > +long hpage_size;
> > +int fd;
> 
> These two should be static.
Will take care of this in v3.
> 
> > +
> > +
> > +static void sigbus_handler(int signum LTP_ATTRIBUTE_UNUSED)
> > +{
> > +	tst_res(TPASS, "Test Passed");
> > +	exit(0);
> 
> It's not safe to call exit(0) from a signal handler.
> 
> What should be done instead is to:
> 
> - add global static volatile int variable
> - reset it before we attempt to access the truncated memory
> - set it in the signal handler
> - print TPASS/TFAIL based on the value of the variable in the
> run_test()
>   function
> 
Will take care of this in v3.
> > +}
> > +
> > +static void run_test(void)
> > +{
> > +	void *p;
> > +	volatile unsigned int *q;
>          ^
> 	I do not think that this has to be volatile.
> 
> 	All in all this can be just:
> 
> 	unsigned int *p;
> 
> 	...
> 
> 	p = SAFE_MMAP();
> 
> 	...
> 
> 	*p = 0;

This has to be volatile. The test logic is hunting for a bug
that changes the value of q.
> 
> > +	struct sigaction my_sigaction;
> > +	my_sigaction.sa_handler = sigbus_handler;
> > +	p = SAFE_MMAP(NULL, hpage_size, PROT_READ|PROT_WRITE,
> > MAP_SHARED,
> > +			fd, 0);
> > +	if (p == MAP_FAILED)
> > +		tst_res(TFAIL, "mmap failed..!!");
> 
> SAFE_MMAP() cannot fail, it does exit the test with a failure if the
> it
> fails to map the memory.

Will take care of this in v3.
> 
> > +	q = p;
> > +	*q = 0;
> > +	SAFE_SIGACTION(SIGBUS, &my_sigaction, NULL);
> 
> I guess that we can set up the handler in the setup instead.

Will take care of this in v3.
> 
> > +	SAFE_FTRUNCATE(fd, 0);
> > +	*q;
> > +        tst_res(TFAIL, "Didn't SIGBUS");
> 
> And we should SAFE_UNMAP() the memory here.

Sure will take care of this in v3.
> 
> Also does the test work with -i 2 ?
The new Version ie., v3 works
> 
> > +}
> > +
> > +
> > +void setup(void)
> > +{
> > +	hpage_size = tst_get_hugepage_size();
> > +    	fd = tst_creat_unlinked(MNTPOINT, 0);
>    ^
>    Wrong indentation, please make sure to run 'make check' and fix
> all
>    the reported problems.

Done in v3.
> 
> > +}
> > +
> > +void cleanup(void)
> > +{
> > +    	if (fd > 0)
> > +	    SAFE_CLOSE(fd);
> > +}
> > +
> > +static struct tst_test test = {
> > +	.needs_root = 1,
> > +	.mntpoint = MNTPOINT,
> > +	.needs_hugetlbfs = 1,
> > +	.needs_tmpdir = 1,
> > +	.setup = setup,
> > +	.cleanup = cleanup,
> > +	.test_all = run_test,
> > +	.hugepages = {1, TST_NEEDS},
> > +};
> > -- 
> > 2.39.3
> > 
> > 
> > -- 
> > Mailing list info: https://lists.linux.it/listinfo/ltp


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