[v2] dio_truncate.c test refactory with LTP API

From: Andrea Cervesato <andrea.cervesato@suse.com>

---
 testcases/kernel/io/ltp-aiodio/dio_truncate.c | 231 +++++++-----------
 1 file changed, 91 insertions(+), 140 deletions(-)

Hi!
> +	int fd = 0;
> +	size_t i = 0;
> +	char *buf = NULL;

It's pointless to initalize variables that are not read back before they
are set.

> -int dio_read(char *filename)
> -{
> -	int fd;
> -	int r;
> -	void *bufptr = NULL;
> +	fd = SAFE_OPEN(path, O_CREAT|O_WRONLY|O_DIRECT, 0666);
> -	TEST(posix_memalign(&bufptr, 4096, 64 * 1024));
> -	if (TEST_RETURN) {
> -		tst_resm(TBROK | TRERRNO, "cannot malloc aligned memory");
> +	buf = (char *)SAFE_MEMALIGN(getpagesize(), bs);
                ^
		Useless cast

> +	if (buf == 0) {

The most common way to check for NULL pointers in LKML C is just:

	if (!buf) {
		...
	}

> +		SAFE_CLOSE(fd);
>  		return -1;
>  	}
>  
> -	while ((fd = open(filename, O_DIRECT | O_RDONLY)) < 0) {
> -	}
> -	fprintf(stderr, "dio_truncate: child reading file\n");
> -	while (1) {
> -		off_t offset;
> -		char *bufoff;
> -
> -		/* read the file, checking for zeros */
> -		offset = lseek(fd, SEEK_SET, 0);
> -		do {
> -			r = read(fd, bufptr, 64 * 1024);
> -			if (r > 0) {
> -				if ((bufoff = check_zero(bufptr, r))) {
> -					fprintf(stderr,
> -						"non-zero read at offset %p\n",
> -						offset + bufoff);
> -					exit(1);
> -				}
> -				offset += r;
> -			}
> -		} while (r > 0);
> +	for (i = 0; i < bs; i++)
> +		buf[i] = pattern;

Why not just memset(buf, pattern, bs) ?

> +	for (i = 0; i < bcount; i++) {
> +		if (write(fd, buf, bs) != (ssize_t)bs) {
> +			free(buf);

			SAFE_CLOSE(fd) here?

> +			return -1;
> +		}
>  	}
> +
> +	SAFE_CLOSE(fd);
> +
>  	return 0;
>  }
>  
> -void dio_append(char *filename, int fill)
> +int dio_get_zeros(const char *path, size_t bs)

Should be static.

>  {
> -	int fd;
> +	int i = 0;
> +	int fd = 0;
> +	int zeros = 0;
>  	void *bufptr = NULL;
> -	int i;
> -	int w;
>  
> -	fd = open(filename, O_DIRECT | O_WRONLY | O_CREAT, 0666);
> +	fd = SAFE_OPEN(path, O_RDONLY|O_DIRECT, 0666);
                                    ^
				    We usually prefer spaces between bit
				    or as it's easier to read

> -	if (fd < 0) {
> -		perror("cannot create file");
> -		return;
> -	}
> +	bufptr = SAFE_MALLOC(bs * sizeof(void));
                                  ^
				  Is sizeof(void) even defined?!

I guess that in this case it was supposed to be byte array so
sizeof(char) but sizeof(char) is 1 by definition.

> +	SAFE_READ(0, fd, bufptr, bs);

If you pass 0 as the first argument of the SAFE_READ() it can return
less than the requested amount of data.

> -	TEST(posix_memalign(&bufptr, 4096, 64 * 1024));
> -	if (TEST_RETURN) {
> -		tst_resm(TBROK | TRERRNO, "cannot malloc aligned memory");
> -		close(fd);
> -		return;
> +	for (i = 0; i < (int)bs; i++) {
> +		if (*(char *)bufptr == 0)

Can we just define the bufptr as char * instead and use bufptr[i] here?

> +			zeros++;
> +		bufptr++;
>  	}
>  
> -	memset(bufptr, fill, 64 * 1024);
> +	SAFE_CLOSE(fd);
>  
> -	for (i = 0; i < 1000; i++) {
> -		if ((w = write(fd, bufptr, 64 * 1024)) != 64 * 1024) {
> -			fprintf(stderr, "write %d returned %d\n", i, w);
> -		}
> -	}
> -	close(fd);
> +	return zeros;
>  }
>  
> -int main(void)
> +off_t getsize(const char *filename)

Here as well should be static.

>  {
> -	char filename[PATH_MAX];
> -	int pid[NUM_CHILDREN];
> -	int num_children = 1;
> -	int i;
> -
> -	snprintf(filename, sizeof(filename), "%s/aiodio/file",
> -		 getenv("TMP") ? getenv("TMP") : "/tmp");
> -
> -	for (i = 0; i < num_children; i++) {
> -		if ((pid[i] = fork()) == 0) {
> -			/* child */
> -			return dio_read(filename);
> -		} else if (pid[i] < 0) {
> -			/* error */
> -			perror("fork error");
> -			break;
> -		} else {
> -			/* Parent */
> -			continue;
> -		}
> -	}
> +	struct stat st;
>  
> -	/*
> -	 * Parent creates a zero file using DIO.
> -	 * Truncates it to zero
> -	 * Create another file with '0xaa'
> -	 */
> -	for (i = 0; i < 100; i++) {
> -		dio_append(filename, 0);
> -		truncate(filename, 0);
> -		dio_append("junkfile", 0xaa);
> -		truncate("junkfile", 0);
> -	}
> +	if (SAFE_STAT(filename, &st) == 0)

SAFE_MACROS() exit the test on failure, that is the whole purpose of
them.

> +		return st.st_size;
> +
> +	return -1;
> +}
> +
> +static void run(void)
> +{
> +	int charnum = 0;
> +	long empty_ch = FILESIZE - STARTING_CHARS;
> +
> +	tst_res(TINFO, "Create %s filled with %d chars", FILENAME, STARTING_CHARS);
> +	dio_append(FILENAME, 'a', STARTING_CHARS, 1);
>  
> -	for (i = 0; i < num_children; i++) {
> -		kill(pid[i], SIGTERM);
> +	/* Truncate to a bigger file and check if it's filled with empty chars */
> +	tst_res(TINFO, "Truncate to %ld", FILESIZE);
> +	TST_EXP_POSITIVE(truncate(FILENAME, FILESIZE), "truncate(%s, %lu)", FILENAME, FILESIZE);

SAFE_TRUNCATE() here?

> +	if (!TST_PASS)
> +		return;
> +
> +	TEST(getsize(FILENAME));

The TEST() macro is supposed to be used in cases where we need to store
errno in order to make sure that it's not rewritten in subsequent system
calls. It's pointeles to use it here.

> +	if (TST_RET == FILESIZE) {
> +		tst_res(TPASS, "Truncated file has %ld length", TST_RET);
> +
> +		charnum = dio_get_zeros(FILENAME, FILESIZE);
> +
> +		if (charnum == empty_ch)
> +			tst_res(TPASS, "Truncated file provides %ld empty chars at the end", empty_ch);
> +		else
> +			tst_res(TFAIL, "Truncated file isn't filled with %i chars", charnum);
> +	} else {
> +		tst_res(TFAIL, "Truncated file doesn't have %ld length but %ld", FILESIZE, TST_RET);
>  	}
>  
> -	return 0;
> +	/* Truncate to zero: file must be empty */
> +	tst_res(TINFO, "Truncate to zero");
> +	TST_EXP_POSITIVE(truncate(FILENAME, 0), "truncate(%s, 0)", FILENAME);
> +	if (!TST_PASS)
> +		return;
> +
> +	TEST(getsize(FILENAME));
> +	if (TST_RET == 0)
> +		tst_res(TPASS, "Truncated file has zero length");
> +	else
> +		tst_res(TFAIL, "Truncated file doesn't have zero length");


Where are the children that do dio_read(FILENAME) on background while
the parent does this?

Also why don't we do N iterations of the test as we did before?

Also where is the part where we write to an unrelated file?

The point of this test is to check that the child processes read zeroes
using direct I/O when the parent does:

	in a loop:

	1. writes zeroes to the file being read
	2. truncates the file being read
	3. writes non-zero in an unrelated file
	4. truncates unrelated file

So the pass/success should be determined by the children and not the
parent here.

There are couple of solution there, but I guess that ideally:

* The child process will exit with 1 once it has found anything that is
  not zero

* The parent will check with waitpid() and WNOHANG if there is a child
  that has exitted in each loop iteration, if so kill children and fail the test

* If the parent is out of loops, kill the children and pass the test

>  }
> +
> +static struct tst_test test = {
> +	.needs_tmpdir = 1,
> +	.test_all = run,
> +};
> -- 
> 2.33.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp