[LTP] [PATCH 2/3] diotest6: test readv/writev not read/write

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> -	if (!buf1 || !buf2) {
> -		tst_resm(TBROK | TERRNO, "valloc() failed");
> -		free(buf1);
> -		free(buf2);
> +	/* allocate read/write io vectors */
> +	iov_r = calloc(nvector, sizeof(*iov_r));
> +	if (!iov_r) {
> +		tst_resm(TBROK | TERRNO, "calloc failed for iov_r array");
> +		return -1;
> +	}
> +	iov_w = calloc(nvector, sizeof(*iov_w));
> +	if (!iov_w) {
> +		tst_resm(TBROK | TERRNO, "calloc failed for iov_w array");
> +		free(iov_r);
>  		return -1;
>  	}

Doing free(NULL) is no-op. So we can simplify this code by allocating
both arrays at once and then checking if !iow_r || !iow_w just like we
did with the normal buffers.

> -	/* Allocate for buffers and data pointers */
> -	seekoff = offset + bufsize * childnum;
> +	/* allocate buffers and setup read/write io vectors */
> +	for (i = 0, iovp = iov_r; i < nvector; iovp++, i++) {
> +		iovp->iov_base = valloc(bufsize);
> +		if (!iovp->iov_base) {
> +			tst_resm(TBROK | TERRNO, "valloc error iov_r[%d]", i);
> +			goto free_out;
> +		}
> +		iovp->iov_len = bufsize;
> +	}

Uh, why do we increment both iovp and i here?

When we can simply do:

for (i = 0; i < nvector; i++) {
	iovp[i].iov_base = valloc(bufsize);
	...
}

> +	for (i = 0, iovp = iov_w; i < nvector; iovp++, i++) {
> +		iovp->iov_base = valloc(bufsize);
> +		if (!iovp->iov_base) {
> +			tst_resm(TBROK | TERRNO, "valloc error iov_w[%d]", i);
> +			goto free_out;
> +		}
> +		iovp->iov_len = bufsize;
> +	}

Here as well.

>  	/* seek, write, read and verify */
> +	seekoff = offset + bufsize * childnum * nvector;
>  	for (i = 0; i < iter; i++) {
> -		fillbuf(buf1, bufsize, childnum+i);
> +		vfillbuf(iov_w, nvector, childnum+i);
>  
>  		if (lseek(fd_w, seekoff, SEEK_SET) < 0) {
>  			tst_resm(TFAIL, "lseek before write failed: %s",
>  				 strerror(errno));
> -			return (-1);
> +			goto free_out;
>  		}
> -		if (write(fd_w, buf1, bufsize) < bufsize) {
> -			tst_resm(TFAIL, "write failed: %s", strerror(errno));
> -			return (-1);
> +		if (writev(fd_w, iov_w, nvector) < 0) {
> +			tst_resm(TFAIL, "writev failed: %s", strerror(errno));
> +			goto free_out;
>  		}

We have to check here that the whole vector was written as well.

>  		if (action == READ_DIRECT) {
>  			/* Make sure data is on to disk before read */
>  			if (fsync(fd_w) < 0) {
>  				tst_resm(TFAIL, "fsync failed: %s",
>  					 strerror(errno));
> -				return (-1);
> +				goto free_out;
>  			}
>  		}
>  		if (lseek(fd_r, seekoff, SEEK_SET) < 0) {
>  			tst_resm(TFAIL, "lseek before read failed: %s",
>  				 strerror(errno));
> -			return (-1);
> +			goto free_out;
>  		}
> -		int ret;
> -		if ((ret = read(fd_r, buf2, bufsize)) < bufsize) {
> -			tst_resm(TFAIL, "read failed: %s", strerror(errno));
> -			return (-1);
> +		if (readv(fd_r, iov_r, nvector) < 0) {
> +			tst_resm(TFAIL, "readv failed: %s", strerror(errno));
> +			goto free_out;
>  		}

Here as well.

> -		if (bufcmp(buf1, buf2, bufsize) != 0) {
> +		if (vbufcmp(iov_w, iov_r, nvector) != 0) {
>  			tst_resm(TFAIL, "comparsion failed. Child=%d offset=%d",
>  				 childnum, (int)seekoff);
> -			return (-1);
> +			goto free_out;
>  		}
>  	}
> -	return 0;
> +	ret = 0;
> +
> +free_out:

I would call this label err: or just out: but that is minor.

> +	for (i = 0, iovp = iov_r; i < nvector; iovp++, i++)
> +		free(iovp->iov_base);
> +	for (i = 0, iovp = iov_w; i < nvector; iovp++, i++)
> +		free(iovp->iov_base);
> +	free(iov_r);
> +	free(iov_w);
> +	return ret;
>  }
>  
>  /*
> -- 
> 2.9.3
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz