xfstests: simplify TRIM_OFF_LEN() in "ltp/fsx.c"]

Review?

-------- Forwarded Message --------
From: Alex Elder <[email protected]>
To: [email protected]
Subject: [PATCH,v2] xfstests: simplify TRIM_OFF_LEN() in "ltp/fsx.c"
Date: Wed, 21 Sep 2011 15:17:51 -0500

A recent commit added a TRIM_OFF_LEN() macro in "ltp/fsx.c":
5843147e xfstests: fsx fallocate support is b0rked
A later commit fixed a problem with that macro:
c47d7a51 xfstests: fix modulo-by-zero error in fsx

There is an extra flag parameter in that macro that I didn't like in either version. When looking at it the second time around I concluded that there was no need for the flag after all.

Going back to the first commit, the code that TRIM_OFF_LEN() replaced had one of two forms:
- For OP_READ and OP_MAP_READ:

	if (file_size)
		offset %= file_size;
	else
		offset = 0;
	if (offset + size > file_size)
		size = file_size - offset;

- For all other cases (except OP_TRUNCATE):

	offset %= maxfilelen;
	if (offset + size > maxfilelen)
		size = maxfilelen - offset;

There's no harm in ensuring maxfilelen is non-zero (and doing so is safer than what's done above). So both of the above can be generalized this way:

	if (SIZE_LIMIT)
		offset %= SIZE_LIMIT;
	else
		offset = 0;
	if (offset + size > SIZE_LIMIT)
		size = SIZE_LIMIT - offset;

In other words, there is no need for the extra flag in the macro.

The following patch just does away with it. It uses the value of the "size" parameter directly in avoiding a divide-by-zero, and in the process avoids referencing the global "file_size" within the macro expansion.

---
Changed in v2:
- Fixed the dangerous use of "offset" rather than parameter "off" in the expansion of the TRIM_OFF_LEN() macro, and parenthesized each place the parameters are used.

 ltp/fsx.c |   28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

Index: b/ltp/fsx.c
===================================================================
@@ -987,14 +987,14 @@ docloseopen(void)
 	}
 }
 
-#define TRIM_OFF_LEN(off, len, size, allow_zero_file_size)	\
-do {						\
-	if (allow_zero_file_size || file_size)	\
-		offset %= size;			\
-	else					\
-		offset = 0;			\
-	if (offset + len > size)		\
-		len = size - offset;		\
+#define TRIM_OFF_LEN(off, len, size)	\
+do {					\
+	if (size)			\
+		(off) %= (size);	\
+	else				\
+		(off) = 0;		\
+	if ((off) + (len) > (size))	\
+		(len) = (size) - (off);	\
 } while (0)
 
 void

@@ -1054,22 +1054,22 @@ test(void)
 
 	switch (op) {
 	case OP_READ:
-		TRIM_OFF_LEN(offset, size, file_size, 0);
+		TRIM_OFF_LEN(offset, size, file_size);
 		doread(offset, size);
 		break;
 
 	case OP_WRITE:
-		TRIM_OFF_LEN(offset, size, maxfilelen, 1);
+		TRIM_OFF_LEN(offset, size, maxfilelen);
 		dowrite(offset, size);
 		break;
 
 	case OP_MAPREAD:
-		TRIM_OFF_LEN(offset, size, file_size, 0);
+		TRIM_OFF_LEN(offset, size, file_size);
 		domapread(offset, size);
 		break;
 
 	case OP_MAPWRITE:
-		TRIM_OFF_LEN(offset, size, maxfilelen, 1);
+		TRIM_OFF_LEN(offset, size, maxfilelen);
 		domapwrite(offset, size);
 		break;
 

@@ -1080,12 +1080,12 @@ test(void)
 		break;
 
 	case OP_FALLOCATE:
-		TRIM_OFF_LEN(offset, size, maxfilelen, 1);
+		TRIM_OFF_LEN(offset, size, maxfilelen);
 		do_preallocate(offset, size);
 		break;
 
 	case OP_PUNCH_HOLE:
-		TRIM_OFF_LEN(offset, size, maxfilelen, 1);
+		TRIM_OFF_LEN(offset, size, maxfilelen);
 		do_punch_hole(offset, size);
 		break;
 	default:

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

This looks good to me, but I'm rather reluctant to do any cosmetic work until we have fixed the regression in 091 introduced by the last round of fsx changes.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs