[PATCH v1 0/7] hexdump: update test suite

[PATCH v1 0/7] hexdump: update test suite

[Andy Shevchenko]

The test suite currently doesn't cover many corner cases when
hex_dump_to_buffer() runs into overflow. Refactor and amend test suite to cover
most of the cases.

Andy Shevchenko (7):
  test_hexdump: rename to test_hexdump
  test_hexdump: introduce test_hexdump_prepare_test() helper
  test_hexdump: go through all possible lengths of buffer
  test_hexdump: replace magic numbers by their meaning
  test_hexdump: check all bytes in real buffer
  test_hexdump: test all possible group sizes for overflow
  test_hexdump: print statistics at the end

 lib/Makefile                           |   2 +-
 lib/{test-hexdump.c => test_hexdump.c} | 120 ++++++++++++++++++++++-----------
 2 files changed, 83 insertions(+), 39 deletions(-)
 rename lib/{test-hexdump.c => test_hexdump.c} (57%)

-- 
2.6.2

[PATCH v1 1/7] test_hexdump: rename to test_hexdump

[Andy Shevchenko]

Just to follow the scheme that most of the test modules are using.

There is no fuctional change.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/Makefile                           | 2 +-
 lib/{test-hexdump.c => test_hexdump.c} | 0
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename lib/{test-hexdump.c => test_hexdump.c} (100%)

diff --git a/lib/Makefile b/lib/Makefile
index 5f4a9e8..64f834a 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -32,7 +32,7 @@ obj-y += bcd.o div64.o sort.o parser.o halfmd4.o debug_locks.o random32.o \
 obj-y += string_helpers.o
 obj-$(CONFIG_TEST_STRING_HELPERS) += test-string_helpers.o
 obj-y += hexdump.o
-obj-$(CONFIG_TEST_HEXDUMP) += test-hexdump.o
+obj-$(CONFIG_TEST_HEXDUMP) += test_hexdump.o
 obj-y += kstrtox.o
 obj-$(CONFIG_TEST_BPF) += test_bpf.o
 obj-$(CONFIG_TEST_FIRMWARE) += test_firmware.o
diff --git a/lib/test-hexdump.c b/lib/test_hexdump.c
similarity index 100%
rename from lib/test-hexdump.c
rename to lib/test_hexdump.c
-- 
2.6.2

[PATCH v1 2/7] test_hexdump: introduce test_hexdump_prepare_test() helper

[Andy Shevchenko]

The function prepares the expected result in the provided buffer.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/test_hexdump.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
index 5241df3..ed7c6a7 100644
--- a/lib/test_hexdump.c
+++ b/lib/test_hexdump.c
@@ -42,19 +42,16 @@ static const char * const test_data_8_le[] __initconst = {
 	"e9ac0f9cad319ca6", "0cafb1439919d14c",
 };
 
-static void __init test_hexdump(size_t len, int rowsize, int groupsize,
-				bool ascii)
+static void __init test_hexdump_prepare_test(size_t len, int rowsize,
+					     int groupsize, char *test,
+					     size_t testlen, bool ascii)
 {
-	char test[32 * 3 + 2 + 32 + 1];
-	char real[32 * 3 + 2 + 32 + 1];
 	char *p;
 	const char * const *result;
 	size_t l = len;
 	int gs = groupsize, rs = rowsize;
 	unsigned int i;
 
-	hex_dump_to_buffer(data_b, l, rs, gs, real, sizeof(real), ascii);
-
 	if (rs != 16 && rs != 32)
 		rs = 16;
 
@@ -73,7 +70,7 @@ static void __init test_hexdump(size_t len, int rowsize, int groupsize,
 	else
 		result = test_data_1_le;
 
-	memset(test, ' ', sizeof(test));
+	memset(test, ' ', testlen);
 
 	/* hex dump */
 	p = test;
@@ -95,6 +92,21 @@ static void __init test_hexdump(size_t len, int rowsize, int groupsize,
 	}
 
 	*p = '\0';
+}
+
+#define TEST_HEXDUMP_BUF_SIZE		(32 * 3 + 2 + 32 + 1)
+
+static void __init test_hexdump(size_t len, int rowsize, int groupsize,
+				bool ascii)
+{
+	char test[TEST_HEXDUMP_BUF_SIZE];
+	char real[TEST_HEXDUMP_BUF_SIZE];
+
+	hex_dump_to_buffer(data_b, len, rowsize, groupsize, real, sizeof(real),
+			   ascii);
+
+	test_hexdump_prepare_test(len, rowsize, groupsize, test, sizeof(test),
+				  ascii);
 
 	if (strcmp(test, real)) {
 		pr_err("Len: %zu row: %d group: %d\n", len, rowsize, groupsize);
-- 
2.6.2

[PATCH v1 3/7] test_hexdump: go through all possible lengths of buffer

[Andy Shevchenko]

When test for overflow do iterate the buffer length in a range
0 .. BUF_SIZE.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/test_hexdump.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
index ed7c6a7..15a6440 100644
--- a/lib/test_hexdump.c
+++ b/lib/test_hexdump.c
@@ -126,17 +126,17 @@ static void __init test_hexdump_set(int rowsize, bool ascii)
 	test_hexdump(len, rowsize, 1, ascii);
 }
 
-static void __init test_hexdump_overflow(bool ascii)
+static void __init test_hexdump_overflow(size_t buflen, bool ascii)
 {
-	char buf[56];
+	char buf[TEST_HEXDUMP_BUF_SIZE];
 	const char *t = test_data_1_le[0];
-	size_t l = get_random_int() % sizeof(buf);
+	size_t l = buflen;
 	bool a;
 	int e, r;
 
 	memset(buf, ' ', sizeof(buf));
 
-	r = hex_dump_to_buffer(data_b, 1, 16, 1, buf, l, ascii);
+	r = hex_dump_to_buffer(data_b, 1, 16, 1, buf, buflen, ascii);
 
 	if (ascii)
 		e = 50;
@@ -144,7 +144,7 @@ static void __init test_hexdump_overflow(bool ascii)
 		e = 2;
 	buf[e + 2] = '\0';
 
-	if (!l) {
+	if (!buflen) {
 		a = r == e && buf[0] == ' ';
 	} else if (l < 3) {
 		a = r == e && buf[0] == '\0';
@@ -160,7 +160,7 @@ static void __init test_hexdump_overflow(bool ascii)
 	}
 
 	if (!a) {
-		pr_err("Len: %zu rc: %u strlen: %zu\n", l, r, strlen(buf));
+		pr_err("Len: %zu rc: %u strlen: %zu\n", buflen, r, strlen(buf));
 		pr_err("Result: '%s'\n", buf);
 	}
 }
@@ -180,11 +180,11 @@ static int __init test_hexdump_init(void)
 	for (i = 0; i < 16; i++)
 		test_hexdump_set(rowsize, true);
 
-	for (i = 0; i < 16; i++)
-		test_hexdump_overflow(false);
+	for (i = 0; i <= TEST_HEXDUMP_BUF_SIZE; i++)
+		test_hexdump_overflow(i, false);
 
-	for (i = 0; i < 16; i++)
-		test_hexdump_overflow(true);
+	for (i = 0; i <= TEST_HEXDUMP_BUF_SIZE; i++)
+		test_hexdump_overflow(i, true);
 
 	return -EINVAL;
 }
-- 
2.6.2

[PATCH v1 4/7] test_hexdump: replace magic numbers by their meaning

[Andy Shevchenko]

The magic numbers of the length are converted to their actual meaning, such as
end of the buffer with and without ASCII part.

We don't touch the rest magic constants that will be removed in the following
commits.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/test_hexdump.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
index 15a6440..a3e3b01 100644
--- a/lib/test_hexdump.c
+++ b/lib/test_hexdump.c
@@ -130,18 +130,23 @@ static void __init test_hexdump_overflow(size_t buflen, bool ascii)
 {
 	char buf[TEST_HEXDUMP_BUF_SIZE];
 	const char *t = test_data_1_le[0];
+	size_t len = 1;
 	size_t l = buflen;
+	int rs = 16, gs = 1;
+	int ae, he, e, r;
 	bool a;
-	int e, r;
 
 	memset(buf, ' ', sizeof(buf));
 
-	r = hex_dump_to_buffer(data_b, 1, 16, 1, buf, buflen, ascii);
+	r = hex_dump_to_buffer(data_b, len, rs, gs, buf, buflen, ascii);
+
+	ae = rs * 2 /* hex */ + rs / gs /* spaces */ + 1 /* space */ + len /* ascii */;
+	he = (gs * 2 /* hex */ + 1 /* space */) * len / gs - 1 /* no trailing space */;
 
 	if (ascii)
-		e = 50;
+		e = ae;
 	else
-		e = 2;
+		e = he;
 	buf[e + 2] = '\0';
 
 	if (!buflen) {
-- 
2.6.2

[PATCH v1 5/7] test_hexdump: check all bytes in real buffer

[Andy Shevchenko]

After processing by hex_dump_to_buffer() check all the parts to be expected.

Part 1. The actual expected hex dump with or without ASCII part.
	This is provided by plain strcmp() call including check for the
	terminating NUL.

Part 2. Check if the buffer is dirty beyond needed.
	We fill the buffer by ' ' (space) characters, so, we expect to have the
	tail of buffer will be left untouched. Check all bytes in the tail of
	the buffer.

Part 3. Return code should be as expected.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/test_hexdump.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
index a3e3b01..9b95b67 100644
--- a/lib/test_hexdump.c
+++ b/lib/test_hexdump.c
@@ -128,10 +128,9 @@ static void __init test_hexdump_set(int rowsize, bool ascii)
 
 static void __init test_hexdump_overflow(size_t buflen, bool ascii)
 {
+	char test[TEST_HEXDUMP_BUF_SIZE];
 	char buf[TEST_HEXDUMP_BUF_SIZE];
-	const char *t = test_data_1_le[0];
 	size_t len = 1;
-	size_t l = buflen;
 	int rs = 16, gs = 1;
 	int ae, he, e, r;
 	bool a;
@@ -147,26 +146,27 @@ static void __init test_hexdump_overflow(size_t buflen, bool ascii)
 		e = ae;
 	else
 		e = he;
-	buf[e + 2] = '\0';
 
 	if (!buflen) {
-		a = r == e && buf[0] == ' ';
-	} else if (l < 3) {
-		a = r == e && buf[0] == '\0';
-	} else if (l < 4) {
-		a = r == e && !strcmp(buf, t);
-	} else if (ascii) {
-		if (l < 51)
-			a = r == e && buf[l - 1] == '\0' && buf[l - 2] == ' ';
-		else
-			a = r == e && buf[50] == '\0' && buf[49] == '.';
+		memset(test, ' ', sizeof(test));
+		test[sizeof(buf) - 1] = '\0';
+
+		a = r == e && !memchr_inv(buf, ' ', sizeof(buf));
 	} else {
-		a = r == e && buf[e] == '\0';
+		int f = min_t(int, e + 1, buflen);
+
+		test_hexdump_prepare_test(len, rs, gs, test, sizeof(test), ascii);
+		test[f - 1] = '\0';
+
+		a = r == e && !memchr_inv(buf + f, ' ', sizeof(buf) - f) && !strcmp(buf, test);
 	}
 
+	buf[sizeof(buf) - 1] = '\0';
+
 	if (!a) {
-		pr_err("Len: %zu rc: %u strlen: %zu\n", buflen, r, strlen(buf));
-		pr_err("Result: '%s'\n", buf);
+		pr_err("Len: %zu buflen: %zu strlen: %zu\n", len, buflen, strlen(buf));
+		pr_err("Result: %d '%s'\n", r, buf);
+		pr_err("Expect: %d '%s'\n", e, test);
 	}
 }
 
-- 
2.6.2

[PATCH v1 6/7] test_hexdump: test all possible group sizes for overflow

[Andy Shevchenko]

Currently only one combination is tested for overflow, i.e. rowsize = 16,
groupsize = 1, len = 1.  Do various test to go all possible branches.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/test_hexdump.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
index 9b95b67..fa2f4af 100644
--- a/lib/test_hexdump.c
+++ b/lib/test_hexdump.c
@@ -126,12 +126,13 @@ static void __init test_hexdump_set(int rowsize, bool ascii)
 	test_hexdump(len, rowsize, 1, ascii);
 }
 
-static void __init test_hexdump_overflow(size_t buflen, bool ascii)
+static void __init test_hexdump_overflow(size_t buflen, size_t len,
+					 int rowsize, int groupsize,
+					 bool ascii)
 {
 	char test[TEST_HEXDUMP_BUF_SIZE];
 	char buf[TEST_HEXDUMP_BUF_SIZE];
-	size_t len = 1;
-	int rs = 16, gs = 1;
+	int rs = rowsize, gs = groupsize;
 	int ae, he, e, r;
 	bool a;
 
@@ -170,6 +171,18 @@ static void __init test_hexdump_overflow(size_t buflen, bool ascii)
 	}
 }
 
+static void __init test_hexdump_overflow_set(size_t buflen, bool ascii)
+{
+	unsigned int i = 0;
+
+	do {
+		int gs = 1 << i;
+		size_t len = get_random_int() % 16 + gs;
+
+		test_hexdump_overflow(buflen, rounddown(len, gs), 16, gs, ascii);
+	} while (i++ < 3);
+}
+
 static int __init test_hexdump_init(void)
 {
 	unsigned int i;
@@ -186,10 +199,10 @@ static int __init test_hexdump_init(void)
 		test_hexdump_set(rowsize, true);
 
 	for (i = 0; i <= TEST_HEXDUMP_BUF_SIZE; i++)
-		test_hexdump_overflow(i, false);
+		test_hexdump_overflow_set(i, false);
 
 	for (i = 0; i <= TEST_HEXDUMP_BUF_SIZE; i++)
-		test_hexdump_overflow(i, true);
+		test_hexdump_overflow_set(i, true);
 
 	return -EINVAL;
 }
-- 
2.6.2

[PATCH v1 7/7] test_hexdump: print statistics at the end

[Andy Shevchenko]

Like others test are doing print the gathered statistics after test module is
finished. Return from the module based on the result.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/test_hexdump.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
index fa2f4af..b69341b 100644
--- a/lib/test_hexdump.c
+++ b/lib/test_hexdump.c
@@ -42,6 +42,9 @@ static const char * const test_data_8_le[] __initconst = {
 	"e9ac0f9cad319ca6", "0cafb1439919d14c",
 };
 
+static unsigned total_tests __initdata;
+static unsigned failed_tests __initdata;
+
 static void __init test_hexdump_prepare_test(size_t len, int rowsize,
 					     int groupsize, char *test,
 					     size_t testlen, bool ascii)
@@ -102,6 +105,8 @@ static void __init test_hexdump(size_t len, int rowsize, int groupsize,
 	char test[TEST_HEXDUMP_BUF_SIZE];
 	char real[TEST_HEXDUMP_BUF_SIZE];
 
+	total_tests++;
+
 	hex_dump_to_buffer(data_b, len, rowsize, groupsize, real, sizeof(real),
 			   ascii);
 
@@ -112,6 +117,7 @@ static void __init test_hexdump(size_t len, int rowsize, int groupsize,
 		pr_err("Len: %zu row: %d group: %d\n", len, rowsize, groupsize);
 		pr_err("Result: '%s'\n", real);
 		pr_err("Expect: '%s'\n", test);
+		failed_tests++;
 	}
 }
 
@@ -136,6 +142,8 @@ static void __init test_hexdump_overflow(size_t buflen, size_t len,
 	int ae, he, e, r;
 	bool a;
 
+	total_tests++;
+
 	memset(buf, ' ', sizeof(buf));
 
 	r = hex_dump_to_buffer(data_b, len, rs, gs, buf, buflen, ascii);
@@ -168,6 +176,7 @@ static void __init test_hexdump_overflow(size_t buflen, size_t len,
 		pr_err("Len: %zu buflen: %zu strlen: %zu\n", len, buflen, strlen(buf));
 		pr_err("Result: %d '%s'\n", r, buf);
 		pr_err("Expect: %d '%s'\n", e, test);
+		failed_tests++;
 	}
 }
 
@@ -188,8 +197,6 @@ static int __init test_hexdump_init(void)
 	unsigned int i;
 	int rowsize;
 
-	pr_info("Running tests...\n");
-
 	rowsize = (get_random_int() % 2 + 1) * 16;
 	for (i = 0; i < 16; i++)
 		test_hexdump_set(rowsize, false);
@@ -204,7 +211,14 @@ static int __init test_hexdump_init(void)
 	for (i = 0; i <= TEST_HEXDUMP_BUF_SIZE; i++)
 		test_hexdump_overflow_set(i, true);
 
-	return -EINVAL;
+	if (failed_tests == 0)
+		pr_info("all %u tests passed\n", total_tests);
+	else
+		pr_err("failed %u out of %u tests\n", failed_tests, total_tests);
+
+	return failed_tests ? -EINVAL : 0;
 }
 module_init(test_hexdump_init);
+
+MODULE_AUTHOR("Andy Shevchenko <andriy.shevchenko@linux.intel.com>");
 MODULE_LICENSE("Dual BSD/GPL");
-- 
2.6.2

Re: [PATCH v1 1/7] test_hexdump: rename to test_hexdump

[Rasmus Villemoes]

On Wed, Nov 11 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Just to follow the scheme that most of the test modules are using.
>
> There is no fuctional change.

Yeah, this has also been bugging me slightly.

Acked-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

Re: [PATCH v1 2/7] test_hexdump: introduce test_hexdump_prepare_test() helper

[Rasmus Villemoes]

On Wed, Nov 11 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> The function prepares the expected result in the provided buffer.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  lib/test_hexdump.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)

Makes sense. It does improve readability slightly.

Acked-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

Re: [PATCH v1 3/7] test_hexdump: go through all possible lengths of buffer

[Rasmus Villemoes]

On Wed, Nov 11 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> When test for overflow do iterate the buffer length in a range
> 0 .. BUF_SIZE.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  lib/test_hexdump.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
> index ed7c6a7..15a6440 100644
> --- a/lib/test_hexdump.c
> +++ b/lib/test_hexdump.c
> @@ -126,17 +126,17 @@ static void __init test_hexdump_set(int rowsize, bool ascii)
>  	test_hexdump(len, rowsize, 1, ascii);
>  }
>  
> -static void __init test_hexdump_overflow(bool ascii)
> +static void __init test_hexdump_overflow(size_t buflen, bool ascii)
>  {
> -	char buf[56];
> +	char buf[TEST_HEXDUMP_BUF_SIZE];
>  	const char *t = test_data_1_le[0];
> -	size_t l = get_random_int() % sizeof(buf);
> +	size_t l = buflen;
>  	bool a;
>  	int e, r;
>  
>  	memset(buf, ' ', sizeof(buf));
>  
> -	r = hex_dump_to_buffer(data_b, 1, 16, 1, buf, l, ascii);
> +	r = hex_dump_to_buffer(data_b, 1, 16, 1, buf, buflen, ascii);
>  
>  	if (ascii)
>  		e = 50;
> @@ -144,7 +144,7 @@ static void __init test_hexdump_overflow(bool ascii)
>  		e = 2;
>  	buf[e + 2] = '\0';
>  
> -	if (!l) {
> +	if (!buflen) {
>  		a = r == e && buf[0] == ' ';
>  	} else if (l < 3) {
>  		a = r == e && buf[0] == '\0';


Why keep the variable l when it is just a synonym for the new
parameter buflen? It is quite confusing that you change some but not
all occurrences of l to buflen. If you want to make the diff minimal
but still have a descriptive parameter name, just keep the 'size_t l =
buflen;' assignment and don't otherwise refer to buflen. But I think
it's better to eliminate 'l' and just change everything to
buflen. Don't mix the two approaches, though.

Rasmus

Re: [PATCH v1 4/7] test_hexdump: replace magic numbers by their meaning

[Rasmus Villemoes]

On Wed, Nov 11 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> The magic numbers of the length are converted to their actual meaning, such as
> end of the buffer with and without ASCII part.
>
> We don't touch the rest magic constants that will be removed in the following
> commits.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  lib/test_hexdump.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
> index 15a6440..a3e3b01 100644
> --- a/lib/test_hexdump.c
> +++ b/lib/test_hexdump.c
> @@ -130,18 +130,23 @@ static void __init test_hexdump_overflow(size_t buflen, bool ascii)
>  {
>  	char buf[TEST_HEXDUMP_BUF_SIZE];
>  	const char *t = test_data_1_le[0];
> +	size_t len = 1;
>  	size_t l = buflen;
> +	int rs = 16, gs = 1;
> +	int ae, he, e, r;
>  	bool a;
> -	int e, r;
>  
>  	memset(buf, ' ', sizeof(buf));
>  
> -	r = hex_dump_to_buffer(data_b, 1, 16, 1, buf, buflen, ascii);
> +	r = hex_dump_to_buffer(data_b, len, rs, gs, buf, buflen, ascii);
> +
> +	ae = rs * 2 /* hex */ + rs / gs /* spaces */ + 1 /* space */ + len /* ascii */;
> +	he = (gs * 2 /* hex */ + 1 /* space */) * len / gs - 1 /* no trailing space */;

This computation seems to rely on len being an exact multiple of gs -
which of course it is in this case. hex_dump_to_buffer also enforces
it by setting groupsize to 1 when it's not the case, but that doesn't
really help us here. So maybe an extra comment would be justified.

Rasmus

Re: [PATCH v1 5/7] test_hexdump: check all bytes in real buffer

[Rasmus Villemoes]

On Wed, Nov 11 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> After processing by hex_dump_to_buffer() check all the parts to be expected.
>
> Part 1. The actual expected hex dump with or without ASCII part.
> 	This is provided by plain strcmp() call including check for the
> 	terminating NUL.
>
> Part 2. Check if the buffer is dirty beyond needed.
> 	We fill the buffer by ' ' (space) characters, so, we expect to have the
> 	tail of buffer will be left untouched. Check all bytes in the tail of
> 	the buffer.

First of all, ' ' is one of the characters which hexdump is certainly supposed
to spit out, so I think it's better to use some other character for
prefilling. Otherwise we wouldn't be able to detect a stray write of a
space which wasn't properly guarded by a size check. I'd suggest
'\xff' or any other non-ascii character (and make it a #define so that
it's less magic).


> Part 3. Return code should be as expected.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  lib/test_hexdump.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
> index a3e3b01..9b95b67 100644
> --- a/lib/test_hexdump.c
> +++ b/lib/test_hexdump.c
> @@ -128,10 +128,9 @@ static void __init test_hexdump_set(int rowsize, bool ascii)
>  
>  static void __init test_hexdump_overflow(size_t buflen, bool ascii)
>  {
> +	char test[TEST_HEXDUMP_BUF_SIZE];
>  	char buf[TEST_HEXDUMP_BUF_SIZE];
> -	const char *t = test_data_1_le[0];
>  	size_t len = 1;
> -	size_t l = buflen;
>  	int rs = 16, gs = 1;
>  	int ae, he, e, r;
>  	bool a;
> @@ -147,26 +146,27 @@ static void __init test_hexdump_overflow(size_t buflen, bool ascii)
>  		e = ae;
>  	else
>  		e = he;
> -	buf[e + 2] = '\0';
>  
>  	if (!buflen) {
> -		a = r == e && buf[0] == ' ';
> -	} else if (l < 3) {
> -		a = r == e && buf[0] == '\0';
> -	} else if (l < 4) {
> -		a = r == e && !strcmp(buf, t);
> -	} else if (ascii) {
> -		if (l < 51)
> -			a = r == e && buf[l - 1] == '\0' && buf[l - 2] == ' ';
> -		else
> -			a = r == e && buf[50] == '\0' && buf[49] == '.';
> +		memset(test, ' ', sizeof(test));
> +		test[sizeof(buf) - 1] = '\0';
> +
> +		a = r == e && !memchr_inv(buf, ' ', sizeof(buf));

test and buf happen to have the same size, but
"test[sizeof(buf) - 1] = '\0'" is rather odd. But you don't even seem
to use test in this branch?

>  	} else {
> -		a = r == e && buf[e] == '\0';
> +		int f = min_t(int, e + 1, buflen);
> +
> +		test_hexdump_prepare_test(len, rs, gs, test, sizeof(test), ascii);
> +		test[f - 1] = '\0';
> +
> +		a = r == e && !memchr_inv(buf + f, ' ', sizeof(buf) - f) && !strcmp(buf, test);
>  	}

There's also a bit of duplication in the !buflen and buflen
branches. Why not pull the computation of f (the number of expected
bytes written) outside and do

  f = min_t(int, e + 1, buflen);
  a = r == e && !memchr_inv(buf + f, ' ', sizeof(buf) - f);
  if (buflen) {
    test_hexdump_prepare_test(len, rs, gs, test, sizeof(test), ascii);
    test[f - 1] = '\0';
    a = a && !memcmp(buf, test, f);
  }

(I think it's better to use memcmp for "untrusted" buffers - if
hexdump didn't make buf into a proper C string, it's a little fragile
passing it to strcmp). This makes it obvious that the entire contents
of buf is being tested.

Rasmus

Re: [PATCH v1 6/7] test_hexdump: test all possible group sizes for overflow

[Rasmus Villemoes]

On Wed, Nov 11 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Currently only one combination is tested for overflow, i.e. rowsize = 16,
> groupsize = 1, len = 1.  Do various test to go all possible branches.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  lib/test_hexdump.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
> index 9b95b67..fa2f4af 100644
> --- a/lib/test_hexdump.c
> +++ b/lib/test_hexdump.c
> @@ -126,12 +126,13 @@ static void __init test_hexdump_set(int rowsize, bool ascii)
>  	test_hexdump(len, rowsize, 1, ascii);
>  }
>  
> -static void __init test_hexdump_overflow(size_t buflen, bool ascii)
> +static void __init test_hexdump_overflow(size_t buflen, size_t len,
> +					 int rowsize, int groupsize,
> +					 bool ascii)
>  {
>  	char test[TEST_HEXDUMP_BUF_SIZE];
>  	char buf[TEST_HEXDUMP_BUF_SIZE];
> -	size_t len = 1;
> -	int rs = 16, gs = 1;
> +	int rs = rowsize, gs = groupsize;
>  	int ae, he, e, r;
>  	bool a;
>  
> @@ -170,6 +171,18 @@ static void __init test_hexdump_overflow(size_t buflen, bool ascii)
>  	}
>  }
>  
> +static void __init test_hexdump_overflow_set(size_t buflen, bool ascii)
> +{
> +	unsigned int i = 0;
> +
> +	do {
> +		int gs = 1 << i;
> +		size_t len = get_random_int() % 16 + gs;
> +
> +		test_hexdump_overflow(buflen, rounddown(len, gs), 16, gs, ascii);
> +	} while (i++ < 3);
> +}


aren't you missing a

  test_hexdump_overflow(buflen, rounddown(len, gs), 32, gs, ascii);

here to also exercise the rowsize==32 code?


> +
>  static int __init test_hexdump_init(void)
>  {
>  	unsigned int i;
> @@ -186,10 +199,10 @@ static int __init test_hexdump_init(void)
>  		test_hexdump_set(rowsize, true);
>  
>  	for (i = 0; i <= TEST_HEXDUMP_BUF_SIZE; i++)
> -		test_hexdump_overflow(i, false);
> +		test_hexdump_overflow_set(i, false);
>  
>  	for (i = 0; i <= TEST_HEXDUMP_BUF_SIZE; i++)
> -		test_hexdump_overflow(i, true);
> +		test_hexdump_overflow_set(i, true);

It seems neater to do one loop:

for (i = 0; i <= TEST_HEXDUMP_BUF_SIZE; i++) {
  test_hexdump_overflow_set(i, false);
  test_hexdump_overflow_set(i, true);
}

Rasmus

Re: [PATCH v1 7/7] test_hexdump: print statistics at the end

[Rasmus Villemoes]

On Wed, Nov 11 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Like others test are doing print the gathered statistics after test module is
> finished. Return from the module based on the result.
>

Homogeneity between test modules is good.

Acked-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

Re: [PATCH v1 6/7] test_hexdump: test all possible group sizes for overflow

[Andy Shevchenko]

On Thu, 2015-11-19 at 11:14 +0100, Rasmus Villemoes wrote:
> On Wed, Nov 11 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.c
> om> wrote:
> 
> > Currently only one combination is tested for overflow, i.e. rowsize
> > = 16,
> > groupsize = 1, len = 1.  Do various test to go all possible
> > branches.

[]

> > +	do {
> > +		int gs = 1 << i;
> > +		size_t len = get_random_int() % 16 + gs;
> > +
> > +		test_hexdump_overflow(buflen, rounddown(len, gs),
> > 16, gs, ascii);
> > +	} while (i++ < 3);
> > +}
> 
> 
> aren't you missing a
> 
>   test_hexdump_overflow(buflen, rounddown(len, gs), 32, gs, ascii);
> 
> here to also exercise the rowsize==32 code?

I could add that as well, though it seems minor since the idea is to go
for all branches, which 16 covers anyway.

>  static int __init test_hexdump_init(void)
> >  {
> >  	unsigned int i;
> > @@ -186,10 +199,10 @@ static int __init test_hexdump_init(void)
> >  		test_hexdump_set(rowsize, true);
> >  
> >  	for (i = 0; i <= TEST_HEXDUMP_BUF_SIZE; i++)
> > -		test_hexdump_overflow(i, false);
> > +		test_hexdump_overflow_set(i, false);
> >  
> >  	for (i = 0; i <= TEST_HEXDUMP_BUF_SIZE; i++)
> > -		test_hexdump_overflow(i, true);
> > +		test_hexdump_overflow_set(i, true);
> 
> It seems neater to do one loop:
> 
> for (i = 0; i <= TEST_HEXDUMP_BUF_SIZE; i++) {
>   test_hexdump_overflow_set(i, false);
>   test_hexdump_overflow_set(i, true);
> }

I would like to keep them separately, though I'm also okay to do it in
one loop.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 5/7] test_hexdump: check all bytes in real buffer

[Andy Shevchenko]

On Thu, 2015-11-19 at 11:11 +0100, Rasmus Villemoes wrote:
> On Wed, Nov 11 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.c
> om> wrote:
> 
> > After processing by hex_dump_to_buffer() check all the parts to be
> > expected.
> > 
> > Part 1. The actual expected hex dump with or without ASCII part.
> > 	This is provided by plain strcmp() call including check for the
> > 	terminating NUL.
> > 
> > Part 2. Check if the buffer is dirty beyond needed.
> > 	We fill the buffer by ' ' (space) characters, so, we expect to
> > have the
> > 	tail of buffer will be left untouched. Check all bytes in the
> > tail of
> > 	the buffer.
> 
> First of all, ' ' is one of the characters which hexdump is certainly
> supposed
> to spit out, so I think it's better to use some other character for
> prefilling. Otherwise we wouldn't be able to detect a stray write of
> a
> space which wasn't properly guarded by a size check. I'd suggest
> '\xff' or any other non-ascii

Okay, I may change the ' ' to something, but somehow printable. See
also below.

>  character (and make it a #define so that
> it's less magic).
> 
> 
> > Part 3. Return code should be as expected.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  lib/test_hexdump.c | 32 ++++++++++++++++----------------
> >  1 file changed, 16 insertions(+), 16 deletions(-)
> > 
> > diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
> > index a3e3b01..9b95b67 100644
> > --- a/lib/test_hexdump.c
> > +++ b/lib/test_hexdump.c
> > @@ -128,10 +128,9 @@ static void __init test_hexdump_set(int
> > rowsize, bool ascii)
> >  
> >  static void __init test_hexdump_overflow(size_t buflen, bool
> > ascii)
> >  {
> > +	char test[TEST_HEXDUMP_BUF_SIZE];
> >  	char buf[TEST_HEXDUMP_BUF_SIZE];
> > -	const char *t = test_data_1_le[0];
> >  	size_t len = 1;
> > -	size_t l = buflen;
> >  	int rs = 16, gs = 1;
> >  	int ae, he, e, r;
> >  	bool a;
> > @@ -147,26 +146,27 @@ static void __init
> > test_hexdump_overflow(size_t buflen, bool ascii)
> >  		e = ae;
> >  	else
> >  		e = he;
> > -	buf[e + 2] = '\0';
> >  
> >  	if (!buflen) {
> > -		a = r == e && buf[0] == ' ';
> > -	} else if (l < 3) {
> > -		a = r == e && buf[0] == '\0';
> > -	} else if (l < 4) {
> > -		a = r == e && !strcmp(buf, t);
> > -	} else if (ascii) {
> > -		if (l < 51)
> > -			a = r == e && buf[l - 1] == '\0' && buf[l
> > - 2] == ' ';
> > -		else
> > -			a = r == e && buf[50] == '\0' && buf[49]
> > == '.';
> > +		memset(test, ' ', sizeof(test));
> > +		test[sizeof(buf) - 1] = '\0';
> > +
> > +		a = r == e && !memchr_inv(buf, ' ', sizeof(buf));
> 
> test and buf happen to have the same size, but
> "test[sizeof(buf) - 1] = '\0'" is rather odd. But you don't even seem
> to use test in this branch?

Here I feel the test buffer just to print below if any error happens
when buflen == 0.

That's why I would like to have a somehow printable character.

> 
> >  	} else {
> > -		a = r == e && buf[e] == '\0';
> > +		int f = min_t(int, e + 1, buflen);
> > +
> > +		test_hexdump_prepare_test(len, rs, gs, test,
> > sizeof(test), ascii);
> > +		test[f - 1] = '\0';
> > +
> > +		a = r == e && !memchr_inv(buf + f, ' ',
> > sizeof(buf) - f) && !strcmp(buf, test);
> >  	}
> 
> There's also a bit of duplication in the !buflen and buflen
> branches. Why not pull the computation of f (the number of expected
> bytes written) outside and do

See above. buflen == 0 is a special case where buffer shouldn't be
touched at all.

> 
>   f = min_t(int, e + 1, buflen);
>   a = r == e && !memchr_inv(buf + f, ' ', sizeof(buf) - f);
>   if (buflen) {
>     test_hexdump_prepare_test(len, rs, gs, test, sizeof(test),
> ascii);
>     test[f - 1] = '\0';
>     a = a && !memcmp(buf, test, f);
>   }
> 
> (I think it's better to use memcmp for "untrusted" buffers - if
> hexdump didn't make buf into a proper C string, it's a little fragile
> passing it to strcmp). This makes it obvious that the entire contents
> of buf is being tested.

Can do that.

> 
> Rasmus

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 4/7] test_hexdump: replace magic numbers by their meaning

[Andy Shevchenko]

On Thu, 2015-11-19 at 11:08 +0100, Rasmus Villemoes wrote:
> On Wed, Nov 11 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.c
> om> wrote:
> 
> > The magic numbers of the length are converted to their actual
> > meaning, such as
> > end of the buffer with and without ASCII part.
> > 
> > We don't touch the rest magic constants that will be removed in the
> > following
> > commits.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  lib/test_hexdump.c | 13 +++++++++----
> >  1 file changed, 9 insertions(+), 4 deletions(-)
> > 
> > diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
> > index 15a6440..a3e3b01 100644
> > --- a/lib/test_hexdump.c
> > +++ b/lib/test_hexdump.c
> > @@ -130,18 +130,23 @@ static void __init
> > test_hexdump_overflow(size_t buflen, bool ascii)
> >  {
> >  	char buf[TEST_HEXDUMP_BUF_SIZE];
> >  	const char *t = test_data_1_le[0];
> > +	size_t len = 1;
> >  	size_t l = buflen;
> > +	int rs = 16, gs = 1;
> > +	int ae, he, e, r;
> >  	bool a;
> > -	int e, r;
> >  
> >  	memset(buf, ' ', sizeof(buf));
> >  
> > -	r = hex_dump_to_buffer(data_b, 1, 16, 1, buf, buflen,
> > ascii);
> > +	r = hex_dump_to_buffer(data_b, len, rs, gs, buf, buflen,
> > ascii);
> > +
> > +	ae = rs * 2 /* hex */ + rs / gs /* spaces */ + 1 /* space
> > */ + len /* ascii */;
> > +	he = (gs * 2 /* hex */ + 1 /* space */) * len / gs - 1 /*
> > no trailing space */;
> 
> This computation seems to rely on len being an exact multiple of gs -
> which of course it is in this case. hex_dump_to_buffer also enforces
> it by setting groupsize to 1 when it's not the case, but that doesn't
> really help us here. So maybe an extra comment would be justified.

Agree. A comment would be nice to have.

> 
> Rasmus

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 3/7] test_hexdump: go through all possible lengths of buffer

[Andy Shevchenko]

On Thu, 2015-11-19 at 11:07 +0100, Rasmus Villemoes wrote:
> On Wed, Nov 11 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.c
> om> wrote:
> 
> > When test for overflow do iterate the buffer length in a range
> > 0 .. BUF_SIZE.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  lib/test_hexdump.c | 20 ++++++++++----------
> >  1 file changed, 10 insertions(+), 10 deletions(-)
> > 
> > diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
> > index ed7c6a7..15a6440 100644
> > --- a/lib/test_hexdump.c
> > +++ b/lib/test_hexdump.c
> > @@ -126,17 +126,17 @@ static void __init test_hexdump_set(int
> > rowsize, bool ascii)
> >  	test_hexdump(len, rowsize, 1, ascii);
> >  }
> >  
> > -static void __init test_hexdump_overflow(bool ascii)
> > +static void __init test_hexdump_overflow(size_t buflen, bool
> > ascii)
> >  {
> > -	char buf[56];
> > +	char buf[TEST_HEXDUMP_BUF_SIZE];
> >  	const char *t = test_data_1_le[0];
> > -	size_t l = get_random_int() % sizeof(buf);
> > +	size_t l = buflen;
> >  	bool a;
> >  	int e, r;
> >  
> >  	memset(buf, ' ', sizeof(buf));
> >  
> > -	r = hex_dump_to_buffer(data_b, 1, 16, 1, buf, l, ascii);
> > +	r = hex_dump_to_buffer(data_b, 1, 16, 1, buf, buflen,
> > ascii);
> >  
> >  	if (ascii)
> >  		e = 50;
> > @@ -144,7 +144,7 @@ static void __init test_hexdump_overflow(bool
> > ascii)
> >  		e = 2;
> >  	buf[e + 2] = '\0';
> >  
> > -	if (!l) {
> > +	if (!buflen) {
> >  		a = r == e && buf[0] == ' ';
> >  	} else if (l < 3) {
> >  		a = r == e && buf[0] == '\0';
> 
> 
> Why keep the variable l when it is just a synonym for the new
> parameter buflen? It is quite confusing that you change some but not
> all occurrences of l to buflen. If you want to make the diff minimal
> but still have a descriptive parameter name, just keep the 'size_t l
> =
> buflen;' assignment and don't otherwise refer to buflen. But I think
> it's better to eliminate 'l' and just change everything to
> buflen. Don't mix the two approaches, though.

Okay, I got it for the future, though the series is already in linux-
next, so do we really need to re-hack half of it because of that?

I suppose everything else you noticed I may send as one follow up
patch.

> 
> Rasmus

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 3/7] test_hexdump: go through all possible lengths of buffer

[Rasmus Villemoes]

On Fri, Nov 20 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Thu, 2015-11-19 at 11:07 +0100, Rasmus Villemoes wrote:
>> 
>> 
>> Why keep the variable l when it is just a synonym for the new
>> parameter buflen? It is quite confusing that you change some but not
>> all occurrences of l to buflen. If you want to make the diff minimal
>> but still have a descriptive parameter name, just keep the 'size_t l
>> =
>> buflen;' assignment and don't otherwise refer to buflen. But I think
>> it's better to eliminate 'l' and just change everything to
>> buflen. Don't mix the two approaches, though.
>
> Okay, I got it for the future, though the series is already in linux-
> next, so do we really need to re-hack half of it because of that?

Stuff in -next isn't set in stone. I'm pretty sure Andrew can replace
one set of patches with another.

Rasmus

Re: [PATCH v1 5/7] test_hexdump: check all bytes in real buffer

[Rasmus Villemoes]

On Fri, Nov 20 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Thu, 2015-11-19 at 11:11 +0100, Rasmus Villemoes wrote:
>> 
>> First of all, ' ' is one of the characters which hexdump is certainly
>> supposed to spit out, so I think it's better to use some other
>> character for prefilling. Otherwise we wouldn't be able to detect a
>> stray write of a space which wasn't properly guarded by a size
>> check. I'd suggest '\xff' or any other non-ascii
>
> Okay, I may change the ' ' to something, but somehow printable. See
> also below.
>
>> > -			a = r == e && buf[l - 1] == '\0' && buf[l
>> > - 2] == ' ';
>> > -		else
>> > -			a = r == e && buf[50] == '\0' && buf[49]
>> > == '.';
>> > +		memset(test, ' ', sizeof(test));
>> > +		test[sizeof(buf) - 1] = '\0';
>> > +
>> > +		a = r == e && !memchr_inv(buf, ' ', sizeof(buf));
>> 
>> test and buf happen to have the same size, but
>> "test[sizeof(buf) - 1] = '\0'" is rather odd. But you don't even seem
>> to use test in this branch?
>
> Here I feel the test buffer just to print below if any error happens
> when buflen == 0.
>
> That's why I would like to have a somehow printable character.

Oh, but that's wrong! That is, printing the filled test buffer does not
actually show what was "expected", whether you were filling with spaces,
$ or \xff. I really can't think of a situation where you'd want to print
the supposedly unused part of the buffer, so the character used for
filling shouldn't matter.

In any case, the most important thing is to stay away from [ a-f0-9]
which hexdump is certainly expected to produce - the reason I suggested
a non-ascii character was because hexdump might produce any printable
ascii character in the ascii part (but I suppose one could use e.g. '#'
which isn't among the characters we feed it).

>> >  	} else {
>> > -		a = r == e && buf[e] == '\0';
>> > +		int f = min_t(int, e + 1, buflen);
>> > +
>> > +		test_hexdump_prepare_test(len, rs, gs, test,
>> > sizeof(test), ascii);
>> > +		test[f - 1] = '\0';
>> > +
>> > +		a = r == e && !memchr_inv(buf + f, ' ',
>> > sizeof(buf) - f) && !strcmp(buf, test);
>> >  	}
>> 
>> There's also a bit of duplication in the !buflen and buflen
>> branches. Why not pull the computation of f (the number of expected
>> bytes written) outside and do
>
> See above. buflen == 0 is a special case where buffer shouldn't be
> touched at all.

No, buflen==0 is not that special. We expect hexdump to touch exactly
the first buflen bytes (or only e+1, whichever is smaller), and the
remaining bytes should be untouched. A memcmp handles the first part, a
memchr_inv the second.

Rasmus

Re: [PATCH v1 6/7] test_hexdump: test all possible group sizes for overflow

[Rasmus Villemoes]

On Fri, Nov 20 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Thu, 2015-11-19 at 11:14 +0100, Rasmus Villemoes wrote:
>> 
>> aren't you missing a
>> 
>>   test_hexdump_overflow(buflen, rounddown(len, gs), 32, gs, ascii);
>> 
>> here to also exercise the rowsize==32 code?
>
> I could add that as well, though it seems minor since the idea is to go
> for all branches, which 16 covers anyway.

Well, I didn't look into the implementation when I wrote that; it just
seemed like an obvious thing to check all allowed combinations of
rowsize, groupsize and ascii.

>>  static int __init test_hexdump_init(void)
>> >  {
>> >  	unsigned int i;
>> > @@ -186,10 +199,10 @@ static int __init test_hexdump_init(void)
>> >  		test_hexdump_set(rowsize, true);
>> >  
>> >  	for (i = 0; i <= TEST_HEXDUMP_BUF_SIZE; i++)
>> > -		test_hexdump_overflow(i, false);
>> > +		test_hexdump_overflow_set(i, false);
>> >  
>> >  	for (i = 0; i <= TEST_HEXDUMP_BUF_SIZE; i++)
>> > -		test_hexdump_overflow(i, true);
>> > +		test_hexdump_overflow_set(i, true);
>> 
>> It seems neater to do one loop:
>> 
>> for (i = 0; i <= TEST_HEXDUMP_BUF_SIZE; i++) {
>>   test_hexdump_overflow_set(i, false);
>>   test_hexdump_overflow_set(i, true);
>> }
>
> I would like to keep them separately, though I'm also okay to do it in
> one loop.

Your code, your call.

Rasmus

Re: [PATCH v1 3/7] test_hexdump: go through all possible lengths of buffer

[Andy Shevchenko]

On Mon, Nov 23, 2015 at 10:59 AM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> On Fri, Nov 20 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>
>> On Thu, 2015-11-19 at 11:07 +0100, Rasmus Villemoes wrote:
>>>
>>>
>>> Why keep the variable l when it is just a synonym for the new
>>> parameter buflen? It is quite confusing that you change some but not
>>> all occurrences of l to buflen. If you want to make the diff minimal
>>> but still have a descriptive parameter name, just keep the 'size_t l
>>> =
>>> buflen;' assignment and don't otherwise refer to buflen. But I think
>>> it's better to eliminate 'l' and just change everything to
>>> buflen. Don't mix the two approaches, though.
>>
>> Okay, I got it for the future, though the series is already in linux-
>> next, so do we really need to re-hack half of it because of that?
>
> Stuff in -next isn't set in stone. I'm pretty sure Andrew can replace
> one set of patches with another.

Thanks for your review. I;m busy currently with some other stuff, you
might have noticed, I will return back to the series maybe next week.


-- 
With Best Regards,
Andy Shevchenko