[kvm-unit-tests PATCH 0/2] lib,arm,powerpc: change command line parsing

[kvm-unit-tests PATCH 0/2] lib,arm,powerpc: change command line parsing

[Radim Krčmář]

This series depends on http://www.spinics.net/lists/kvm/msg131243.html.

Most of the series is drop-in replacement without any visible change, but I
couldn't help to change powerpc/rtas.  I didn't see the reason why it was split
into exclusive sub-tests and the solution where one execution could test both
get and set paths was easier to code.

What do you think about that?

(Sorry for the tangled patch.)


Radim Krčmář (2):
  lib/string: add strncmp
  lib/util,arm,powerpc: replace parse_keyval with better helpers

 arm/selftest.c        | 36 +++++++++++++++---------------------
 lib/string.c          | 17 ++++++++++-------
 lib/string.h          |  1 +
 lib/util.c            | 44 ++++++++++++++++++++++++++++++++++++--------
 lib/util.h            | 28 ++++++++++++++++++----------
 powerpc/emulator.c    |  9 ++-------
 powerpc/rtas.c        | 33 ++++++++++-----------------------
 powerpc/selftest.c    | 40 +++++++++++++++-------------------------
 powerpc/unittests.cfg |  2 +-
 9 files changed, 108 insertions(+), 102 deletions(-)

-- 
2.8.2

[kvm-unit-tests PATCH 1/2] lib/string: add strncmp

[Radim Krčmář]

Reviewed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 lib/string.c | 17 ++++++++++-------
 lib/string.h |  1 +
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/lib/string.c b/lib/string.c
index e7bcfe945fcf..ee93e25e9821 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -26,15 +26,18 @@ char *strcpy(char *dest, const char *src)
     return strcat(dest, src);
 }
 
+int strncmp(const char *a, const char *b, size_t n)
+{
+    for (; n--; ++a, ++b)
+        if (*a != *b || *a == '\0')
+            return *a - *b;
+
+    return 0;
+}
+
 int strcmp(const char *a, const char *b)
 {
-    while (*a == *b) {
-	if (*a == '\0') {
-	    break;
-	}
-	++a, ++b;
-    }
-    return *a - *b;
+    return strncmp(a, b, SIZE_MAX);
 }
 
 char *strchr(const char *s, int c)
diff --git a/lib/string.h b/lib/string.h
index 4e24f54d9e23..2391013ad2b1 100644
--- a/lib/string.h
+++ b/lib/string.h
@@ -5,6 +5,7 @@ extern unsigned long strlen(const char *buf);
 extern char *strcat(char *dest, const char *src);
 extern char *strcpy(char *dest, const char *src);
 extern int strcmp(const char *a, const char *b);
+extern int strncmp(const char *a, const char *b, size_t n);
 extern char *strchr(const char *s, int c);
 extern char *strstr(const char *haystack, const char *needle);
 extern void *memset(void *s, int c, size_t n);
-- 
2.8.2

Re: [kvm-unit-tests PATCH 1/2] lib/string: add strncmp

[Laurent Vivier]

On 11/05/2016 22:55, Radim Krčmář wrote:
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

> ---
>  lib/string.c | 17 ++++++++++-------
>  lib/string.h |  1 +
>  2 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/string.c b/lib/string.c
> index e7bcfe945fcf..ee93e25e9821 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -26,15 +26,18 @@ char *strcpy(char *dest, const char *src)
>      return strcat(dest, src);
>  }
>  
> +int strncmp(const char *a, const char *b, size_t n)
> +{
> +    for (; n--; ++a, ++b)
> +        if (*a != *b || *a == '\0')
> +            return *a - *b;
> +
> +    return 0;
> +}
> +
>  int strcmp(const char *a, const char *b)
>  {
> -    while (*a == *b) {
> -	if (*a == '\0') {
> -	    break;
> -	}
> -	++a, ++b;
> -    }
> -    return *a - *b;
> +    return strncmp(a, b, SIZE_MAX);
>  }
>  
>  char *strchr(const char *s, int c)
> diff --git a/lib/string.h b/lib/string.h
> index 4e24f54d9e23..2391013ad2b1 100644
> --- a/lib/string.h
> +++ b/lib/string.h
> @@ -5,6 +5,7 @@ extern unsigned long strlen(const char *buf);
>  extern char *strcat(char *dest, const char *src);
>  extern char *strcpy(char *dest, const char *src);
>  extern int strcmp(const char *a, const char *b);
> +extern int strncmp(const char *a, const char *b, size_t n);
>  extern char *strchr(const char *s, int c);
>  extern char *strstr(const char *haystack, const char *needle);
>  extern void *memset(void *s, int c, size_t n);
> 

[kvm-unit-tests PATCH 2/2] lib/util,arm,powerpc: replace parse_keyval with better helpers

[Radim Krčmář]

A common pattern was to scan through all argv strings to find key or
key=val.  parse_keyval used to return val as long, but another function
needed to check the key.  New functions do both at once.

parse_keyval was replaced with different parse_keyval, so callers needed
to be updated.  While at it, I changed the meaning of arguments to
powerpc/rtas.c to make the code simpler.  And removing aborts is a
subtle preparation for a patch that reports SKIP when no tests were run
and they weren't necessary even now.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arm/selftest.c        | 36 +++++++++++++++---------------------
 lib/util.c            | 44 ++++++++++++++++++++++++++++++++++++--------
 lib/util.h            | 28 ++++++++++++++++++----------
 powerpc/emulator.c    |  9 ++-------
 powerpc/rtas.c        | 33 ++++++++++-----------------------
 powerpc/selftest.c    | 40 +++++++++++++++-------------------------
 powerpc/unittests.cfg |  2 +-
 7 files changed, 97 insertions(+), 95 deletions(-)

diff --git a/arm/selftest.c b/arm/selftest.c
index 5656f2bb1cc8..3ecfb637d673 100644
--- a/arm/selftest.c
+++ b/arm/selftest.c
@@ -21,33 +21,27 @@
 
 static void check_setup(int argc, char **argv)
 {
-	int nr_tests = 0, len, i;
+	int nr_tests = 0;
 	long val;
 
-	for (i = 0; i < argc; ++i) {
+	if (parse_keyval(argc, argv, "mem", &val)) {
+		report_prefix_push("mem");
 
-		len = parse_keyval(argv[i], &val);
-		if (len == -1)
-			continue;
+		phys_addr_t memsize = PHYS_END - PHYS_OFFSET;
+		phys_addr_t expected = ((phys_addr_t)val)*1024*1024;
 
-		argv[i][len] = '\0';
-		report_prefix_push(argv[i]);
+		report("size = %d MB", memsize == expected,
+						memsize/1024/1024);
+		++nr_tests;
+		report_prefix_pop();
+	}
 
-		if (strcmp(argv[i], "mem") == 0) {
+	if (parse_keyval(argc, argv, "smp", &val)) {
+		report_prefix_push("smp");
 
-			phys_addr_t memsize = PHYS_END - PHYS_OFFSET;
-			phys_addr_t expected = ((phys_addr_t)val)*1024*1024;
-
-			report("size = %d MB", memsize == expected,
-							memsize/1024/1024);
-			++nr_tests;
-
-		} else if (strcmp(argv[i], "smp") == 0) {
-
-			report("nr_cpus = %d", nr_cpus == (int)val, nr_cpus);
-			++nr_tests;
-		}
+		report("nr_cpus = %d", nr_cpus == (int)val, nr_cpus);
 
+		++nr_tests;
 		report_prefix_pop();
 	}
 
@@ -331,7 +325,7 @@ int main(int argc, char **argv)
 
 	if (strcmp(argv[1], "setup") == 0) {
 
-		check_setup(argc-2, &argv[2]);
+		check_setup(argc-1, &argv[1]);
 
 	} else if (strcmp(argv[1], "vectors-kernel") == 0) {
 
diff --git a/lib/util.c b/lib/util.c
index 69b18100c972..157138060d8c 100644
--- a/lib/util.c
+++ b/lib/util.c
@@ -4,15 +4,43 @@
  * This work is licensed under the terms of the GNU LGPL, version 2.
  */
 #include <libcflat.h>
+#include <util.h>
 
-int parse_keyval(char *s, long *val)
+bool parse_keyval(int argc, char **argv, char *key, long *val)
 {
-	char *p;
+	char *str = find_keyval(argc, argv, key);
+	bool is_keyval = str && str != key;
 
-	p = strchr(s, '=');
-	if (!p)
-		return -1;
-
-	*val = atol(p+1);
-	return p - s;
+	if (is_keyval)
+		*val = atol(str);
+	return is_keyval;
+}
+
+long atol_keyval(int argc, char **argv, char *key)
+{
+	long val;
+	bool is_keyval = parse_keyval(argc, argv, key, &val);
+
+	return is_keyval ? val : 0;
+}
+
+bool find_key(int argc, char **argv, char *key) {
+	return find_keyval(argc, argv, key) == key;
+}
+
+char * find_keyval(int argc, char **argv, char *key)
+{
+	int i;
+	size_t keylen = strlen(key);
+
+	for (i = 1; i < argc; i++) {
+		if (keylen > 0 && strncmp(argv[i], key, keylen - 1))
+			continue;
+		if (argv[i][keylen] == '\0')
+			return key;
+		if (argv[i][keylen] == '=')
+			return argv[i] + keylen + 1;
+	}
+
+	return NULL;
 }
diff --git a/lib/util.h b/lib/util.h
index 4c4b44132277..1eb0067dc8d7 100644
--- a/lib/util.h
+++ b/lib/util.h
@@ -8,16 +8,24 @@
  * This work is licensed under the terms of the GNU LGPL, version 2.
  */
 
-/*
- * parse_keyval extracts the integer from a string formatted as
- * string=integer. This is useful for passing expected values to
- * the unit test on the command line, i.e. it helps parse QEMU
- * command lines that include something like -append var1=1 var2=2
- * @s is the input string, likely a command line parameter, and
- * @val is a pointer to where the integer will be stored.
- *
- * Returns the offset of the '=', or -1 if no keyval pair is found.
+/* @argc and @argv are standard arguments to C main.  */
+
+/* parse_keyval returns true if @argv[i] has @key=val format and parse @val;
+ * returns false otherwise.
  */
-extern int parse_keyval(char *s, long *val);
+bool parse_keyval(int argc, char **argv, char *key, long *val);
+
+/* atol_keyval returns parsed val if @argv[i] has @key=val format; returns 0
+ * otherwise.
+ */
+long atol_keyval(int argc, char **argv, char *key);
+
+/* find_key decides whether @key is equal @argv[i]. */
+bool find_key(int argc, char **argv, char *key);
+
+/* find_keyval returns key if @key is equal to @argv[i]; returns pointer to val
+ * if @argv[i] has @key=val format; returns NULL otherwise.
+ */
+char * find_keyval(int argc, char **argv, char *key);
 
 #endif
diff --git a/powerpc/emulator.c b/powerpc/emulator.c
index 25018a57463b..384f927f4f71 100644
--- a/powerpc/emulator.c
+++ b/powerpc/emulator.c
@@ -4,6 +4,7 @@
 
 #include <libcflat.h>
 #include <asm/processor.h>
+#include <util.h>
 
 static int verbose;
 static int volatile is_invalid;
@@ -219,16 +220,10 @@ static void test_lswx(void)
 
 int main(int argc, char **argv)
 {
-	int i;
-
 	handle_exception(0x700, program_check_handler, (void *)&is_invalid);
 	handle_exception(0x600, alignment_handler, (void *)&alignment);
 
-	for (i = 1; i < argc; i++) {
-		if (strcmp(argv[i], "-v") == 0) {
-			verbose = 1;
-		}
-	}
+	verbose = find_key(argc, argv, "-v");
 
 	report_prefix_push("emulator");
 
diff --git a/powerpc/rtas.c b/powerpc/rtas.c
index 1b1e9c753ef1..431adf54f0af 100644
--- a/powerpc/rtas.c
+++ b/powerpc/rtas.c
@@ -44,6 +44,8 @@ static void check_get_time_of_day(unsigned long start)
 	int now[8];
 	unsigned long t1, t2, count;
 
+	report_prefix_push("get-time-of-day");
+
 	token = rtas_token("get-time-of-day");
 	report("token available", token != RTAS_UNKNOWN_SERVICE);
 	if (token == RTAS_UNKNOWN_SERVICE)
@@ -70,6 +72,8 @@ static void check_get_time_of_day(unsigned long start)
 		count++;
 	} while (t1 + DELAY > t2 && count < MAX_LOOP);
 	report("running", t1 + DELAY <= t2);
+
+	report_prefix_pop();
 }
 
 static void check_set_time_of_day(void)
@@ -79,6 +83,8 @@ static void check_set_time_of_day(void)
 	int date[8];
 	unsigned long t1, t2, count;
 
+	report_prefix_push("set-time-of-day");
+
 	token = rtas_token("set-time-of-day");
 	report("token available", token != RTAS_UNKNOWN_SERVICE);
 	if (token == RTAS_UNKNOWN_SERVICE)
@@ -106,6 +112,8 @@ static void check_set_time_of_day(void)
 		count++;
 	} while (t1 + DELAY > t2 && count < MAX_LOOP);
 	report("running", t1 + DELAY <= t2);
+
+	report_prefix_pop();
 }
 
 int main(int argc, char **argv)
@@ -115,33 +123,12 @@ int main(int argc, char **argv)
 
 	report_prefix_push("rtas");
 
-	if (argc < 2)
-		report_abort("no test specified");
-
-	report_prefix_push(argv[1]);
-
-	if (strcmp(argv[1], "get-time-of-day") == 0) {
-
-		len = parse_keyval(argv[2], &val);
-		if (len == -1) {
-			printf("Missing parameter \"date\"\n");
-			abort();
-		}
-		argv[2][len] = '\0';
-
+	if (parse_keyval(argc, argv, "get-time-of-day", &val))
 		check_get_time_of_day(val);
 
-	} else if (strcmp(argv[1], "set-time-of-day") == 0) {
-
+	if (find_key(argc, argv, "set-time-of-day"))
 		check_set_time_of_day();
 
-	} else {
-		printf("Unknown subtest\n");
-		abort();
-	}
-
-	report_prefix_pop();
-
 	report_prefix_pop();
 
 	return report_summary();
diff --git a/powerpc/selftest.c b/powerpc/selftest.c
index 8c5ff0ac889d..09856486bac5 100644
--- a/powerpc/selftest.c
+++ b/powerpc/selftest.c
@@ -11,33 +11,28 @@
 
 static void check_setup(int argc, char **argv)
 {
-	int nr_tests = 0, len, i;
+	int nr_tests = 0;
 	long val;
 
-	for (i = 0; i < argc; ++i) {
+	if (parse_keyval(argc, argv, "mem", &val)) {
+		report_prefix_push("mem");
 
-		len = parse_keyval(argv[i], &val);
-		if (len == -1)
-			continue;
+		phys_addr_t memsize = PHYSICAL_END - PHYSICAL_START;
+		phys_addr_t expected = ((phys_addr_t)val)*1024*1024;
 
-		argv[i][len] = '\0';
-		report_prefix_push(argv[i]);
+		report("size = %d MB", memsize == expected,
+						memsize/1024/1024);
 
-		if (strcmp(argv[i], "mem") == 0) {
+		++nr_tests;
+		report_prefix_pop();
+	}
 
-			phys_addr_t memsize = PHYSICAL_END - PHYSICAL_START;
-			phys_addr_t expected = ((phys_addr_t)val)*1024*1024;
+	if (parse_keyval(argc, argv, "smp", &val)) {
+		report_prefix_push("smp");
 
-			report("size = %d MB", memsize == expected,
-							memsize/1024/1024);
-			++nr_tests;
-
-		} else if (strcmp(argv[i], "smp") == 0) {
-
-			report("nr_cpus = %d", nr_cpus == (int)val, nr_cpus);
-			++nr_tests;
-		}
+		report("nr_cpus = %d", nr_cpus == (int)val, nr_cpus);
 
+		++nr_tests;
 		report_prefix_pop();
 	}
 
@@ -49,14 +44,9 @@ int main(int argc, char **argv)
 {
 	report_prefix_push("selftest");
 
-	if (argc < 2)
-		report_abort("no test specified");
-
-	report_prefix_push(argv[1]);
-
 	if (strcmp(argv[1], "setup") == 0) {
 
-		check_setup(argc-2, &argv[2]);
+		check_setup(argc-1, &argv[1]);
 
 	}
 
diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
index ed4fdbe64909..fe5db7e302bb 100644
--- a/powerpc/unittests.cfg
+++ b/powerpc/unittests.cfg
@@ -39,7 +39,7 @@ file = spapr_hcall.elf
 [rtas-get-time-of-day]
 file = rtas.elf
 timeout = 5
-extra_params = -append "get-time-of-day date=$(date +%s)"
+extra_params = -append "get-time-of-day=$(date +%s)"
 groups = rtas
 
 [rtas-set-time-of-day]
-- 
2.8.2

Re: [kvm-unit-tests PATCH 2/2] lib/util,arm,powerpc: replace parse_keyval with better helpers

[Andrew Jones]

On Wed, May 11, 2016 at 10:55:14PM +0200, Radim Krčmář wrote:
> A common pattern was to scan through all argv strings to find key or
> key=val.  parse_keyval used to return val as long, but another function
> needed to check the key.  New functions do both at once.
> 
> parse_keyval was replaced with different parse_keyval, so callers needed
> to be updated.  While at it, I changed the meaning of arguments to
> powerpc/rtas.c to make the code simpler.  And removing aborts is a
> subtle preparation for a patch that reports SKIP when no tests were run
> and they weren't necessary even now.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  arm/selftest.c        | 36 +++++++++++++++---------------------
>  lib/util.c            | 44 ++++++++++++++++++++++++++++++++++++--------
>  lib/util.h            | 28 ++++++++++++++++++----------
>  powerpc/emulator.c    |  9 ++-------
>  powerpc/rtas.c        | 33 ++++++++++-----------------------
>  powerpc/selftest.c    | 40 +++++++++++++++-------------------------
>  powerpc/unittests.cfg |  2 +-
>  7 files changed, 97 insertions(+), 95 deletions(-)
> 
> diff --git a/arm/selftest.c b/arm/selftest.c
> index 5656f2bb1cc8..3ecfb637d673 100644
> --- a/arm/selftest.c
> +++ b/arm/selftest.c
> @@ -21,33 +21,27 @@
>  
>  static void check_setup(int argc, char **argv)
>  {
> -	int nr_tests = 0, len, i;
> +	int nr_tests = 0;
>  	long val;
>  
> -	for (i = 0; i < argc; ++i) {
> +	if (parse_keyval(argc, argv, "mem", &val)) {
> +		report_prefix_push("mem");
>  
> -		len = parse_keyval(argv[i], &val);
> -		if (len == -1)
> -			continue;
> +		phys_addr_t memsize = PHYS_END - PHYS_OFFSET;
> +		phys_addr_t expected = ((phys_addr_t)val)*1024*1024;
>  
> -		argv[i][len] = '\0';
> -		report_prefix_push(argv[i]);
> +		report("size = %d MB", memsize == expected,
> +						memsize/1024/1024);
> +		++nr_tests;
> +		report_prefix_pop();
> +	}
>  
> -		if (strcmp(argv[i], "mem") == 0) {
> +	if (parse_keyval(argc, argv, "smp", &val)) {
> +		report_prefix_push("smp");
>  
> -			phys_addr_t memsize = PHYS_END - PHYS_OFFSET;
> -			phys_addr_t expected = ((phys_addr_t)val)*1024*1024;
> -
> -			report("size = %d MB", memsize == expected,
> -							memsize/1024/1024);
> -			++nr_tests;
> -
> -		} else if (strcmp(argv[i], "smp") == 0) {
> -
> -			report("nr_cpus = %d", nr_cpus == (int)val, nr_cpus);
> -			++nr_tests;
> -		}
> +		report("nr_cpus = %d", nr_cpus == (int)val, nr_cpus);
>  

extra blank line here

> +		++nr_tests;
>  		report_prefix_pop();
>  	}
>  
> @@ -331,7 +325,7 @@ int main(int argc, char **argv)
>  
>  	if (strcmp(argv[1], "setup") == 0) {
>  
> -		check_setup(argc-2, &argv[2]);
> +		check_setup(argc-1, &argv[1]);
>  
>  	} else if (strcmp(argv[1], "vectors-kernel") == 0) {
>  
> diff --git a/lib/util.c b/lib/util.c
> index 69b18100c972..157138060d8c 100644
> --- a/lib/util.c
> +++ b/lib/util.c
> @@ -4,15 +4,43 @@
>   * This work is licensed under the terms of the GNU LGPL, version 2.
>   */
>  #include <libcflat.h>
> +#include <util.h>
>  
> -int parse_keyval(char *s, long *val)
> +bool parse_keyval(int argc, char **argv, char *key, long *val)
>  {
> -	char *p;
> +	char *str = find_keyval(argc, argv, key);
> +	bool is_keyval = str && str != key;
>  
> -	p = strchr(s, '=');
> -	if (!p)
> -		return -1;
> -
> -	*val = atol(p+1);
> -	return p - s;
> +	if (is_keyval)
> +		*val = atol(str);
> +	return is_keyval;
> +}
> +
> +long atol_keyval(int argc, char **argv, char *key)
> +{
> +	long val;
> +	bool is_keyval = parse_keyval(argc, argv, key, &val);
> +
> +	return is_keyval ? val : 0;

Not sure how this one would be useful, and I don't see any uses
for it below. It may be difficult to use due to its ambiguous
results,  as zero could be the value, or the result because the
key wasn't found, or because the key was found, but was a key
with no '='.

> +}
> +
> +bool find_key(int argc, char **argv, char *key) {

Mr. {, please come down.

> +	return find_keyval(argc, argv, key) == key;
> +}
> +
> +char * find_keyval(int argc, char **argv, char *key)

I prefer the '*' by the name.

> +{
> +	int i;
> +	size_t keylen = strlen(key);
> +
> +	for (i = 1; i < argc; i++) {

We should start i at 0, because we shouldn't assume the user will
always pass in main's &argv[0]. Above arm/setup.c actually uses
&argv[1]; so does arm's setup test work? Anyway, it shouldn't matter
if we always look at the program name while searching for the key,
because, for example, x86/msr.flat would be a strange key name :-)

> +		if (keylen > 0 && strncmp(argv[i], key, keylen - 1))
> +			continue;
> +		if (argv[i][keylen] == '\0')
> +			return key;
> +		if (argv[i][keylen] == '=')
> +			return argv[i] + keylen + 1;
> +	}
> +
> +	return NULL;
>  }
> diff --git a/lib/util.h b/lib/util.h
> index 4c4b44132277..1eb0067dc8d7 100644
> --- a/lib/util.h
> +++ b/lib/util.h
> @@ -8,16 +8,24 @@
>   * This work is licensed under the terms of the GNU LGPL, version 2.
>   */
>  
> -/*
> - * parse_keyval extracts the integer from a string formatted as
> - * string=integer. This is useful for passing expected values to
> - * the unit test on the command line, i.e. it helps parse QEMU
> - * command lines that include something like -append var1=1 var2=2
> - * @s is the input string, likely a command line parameter, and
> - * @val is a pointer to where the integer will be stored.
> - *
> - * Returns the offset of the '=', or -1 if no keyval pair is found.
> +/* @argc and @argv are standard arguments to C main.  */

I agree the only use for a parsing function in kvm-unit-tests is
main()'s inputs, but we shouldn't expect/require them to be unmodified
prior to making parse_keyval calls.

> +
> +/* parse_keyval returns true if @argv[i] has @key=val format and parse @val;
> + * returns false otherwise.
>   */

How about this instead

/*
 * parse_keyval searches @argv members for strings of the form @key=val
 * Returns
 *  true when a @key=val string is found; val is parsed and stored in @val.
 *  false otherwise
 */

> -extern int parse_keyval(char *s, long *val);
> +bool parse_keyval(int argc, char **argv, char *key, long *val);
> +
> +/* atol_keyval returns parsed val if @argv[i] has @key=val format; returns 0

same comment rewrite suggestion as above

> + * otherwise.
> + */
> +long atol_keyval(int argc, char **argv, char *key);
> +
> +/* find_key decides whether @key is equal @argv[i]. */

s/equal/equal to/, but I'd rewrite this one too :-)

/*
 * find_key searches @argv members for the string @key
 * Returns true when found, false otherwise.
 */

> +bool find_key(int argc, char **argv, char *key);
> +
> +/* find_keyval returns key if @key is equal to @argv[i]; returns pointer to val
> + * if @argv[i] has @key=val format; returns NULL otherwise.
> + */

Another rewrite suggestion. Please list the return possibilities
Returns
 - @key when ...
 - A pointer to the start of val when ...
 - NULL otherwise

or something like that

> +char * find_keyval(int argc, char **argv, char *key);
>  
>  #endif
> diff --git a/powerpc/emulator.c b/powerpc/emulator.c
> index 25018a57463b..384f927f4f71 100644
> --- a/powerpc/emulator.c
> +++ b/powerpc/emulator.c
> @@ -4,6 +4,7 @@
>  
>  #include <libcflat.h>
>  #include <asm/processor.h>
> +#include <util.h>
>  
>  static int verbose;
>  static int volatile is_invalid;
> @@ -219,16 +220,10 @@ static void test_lswx(void)
>  
>  int main(int argc, char **argv)
>  {
> -	int i;
> -
>  	handle_exception(0x700, program_check_handler, (void *)&is_invalid);
>  	handle_exception(0x600, alignment_handler, (void *)&alignment);
>  
> -	for (i = 1; i < argc; i++) {
> -		if (strcmp(argv[i], "-v") == 0) {
> -			verbose = 1;
> -		}
> -	}
> +	verbose = find_key(argc, argv, "-v");

That's a nice cleanup. Yay for utility functions :-)

>  
>  	report_prefix_push("emulator");
>  
> diff --git a/powerpc/rtas.c b/powerpc/rtas.c
> index 1b1e9c753ef1..431adf54f0af 100644
> --- a/powerpc/rtas.c
> +++ b/powerpc/rtas.c
> @@ -44,6 +44,8 @@ static void check_get_time_of_day(unsigned long start)
>  	int now[8];
>  	unsigned long t1, t2, count;
>  
> +	report_prefix_push("get-time-of-day");
> +
>  	token = rtas_token("get-time-of-day");
>  	report("token available", token != RTAS_UNKNOWN_SERVICE);
>  	if (token == RTAS_UNKNOWN_SERVICE)
> @@ -70,6 +72,8 @@ static void check_get_time_of_day(unsigned long start)
>  		count++;
>  	} while (t1 + DELAY > t2 && count < MAX_LOOP);
>  	report("running", t1 + DELAY <= t2);
> +
> +	report_prefix_pop();
>  }
>  
>  static void check_set_time_of_day(void)
> @@ -79,6 +83,8 @@ static void check_set_time_of_day(void)
>  	int date[8];
>  	unsigned long t1, t2, count;
>  
> +	report_prefix_push("set-time-of-day");
> +
>  	token = rtas_token("set-time-of-day");
>  	report("token available", token != RTAS_UNKNOWN_SERVICE);
>  	if (token == RTAS_UNKNOWN_SERVICE)
> @@ -106,6 +112,8 @@ static void check_set_time_of_day(void)
>  		count++;
>  	} while (t1 + DELAY > t2 && count < MAX_LOOP);
>  	report("running", t1 + DELAY <= t2);
> +
> +	report_prefix_pop();
>  }
>  
>  int main(int argc, char **argv)
> @@ -115,33 +123,12 @@ int main(int argc, char **argv)
>  
>  	report_prefix_push("rtas");
>  
> -	if (argc < 2)
> -		report_abort("no test specified");
> -
> -	report_prefix_push(argv[1]);
> -
> -	if (strcmp(argv[1], "get-time-of-day") == 0) {
> -
> -		len = parse_keyval(argv[2], &val);
> -		if (len == -1) {
> -			printf("Missing parameter \"date\"\n");
> -			abort();
> -		}
> -		argv[2][len] = '\0';
> -
> +	if (parse_keyval(argc, argv, "get-time-of-day", &val))
>  		check_get_time_of_day(val);
>  
> -	} else if (strcmp(argv[1], "set-time-of-day") == 0) {
> -
> +	if (find_key(argc, argv, "set-time-of-day"))
>  		check_set_time_of_day();
>  
> -	} else {
> -		printf("Unknown subtest\n");
> -		abort();
> -	}
> -
> -	report_prefix_pop();
> -

Also a nice cleanup. We could have kept the missing parameter abort
with something like

 if (find_key(argc, argv, "get-time-of-day")) {
     if (!parse_keyval(argc, argv, "get-time-of-day", &val)) {
         printf("Missing parameter \"date\"\n");
         abort();
     }
     check_get_time_of_day(val);
 }

but I'll leave that to the ppc guys to decide.

I agree a 'SKIP rtas (no tests specified)' sounds like a better
approach than aborting on argc < 2.

>  	report_prefix_pop();
>  
>  	return report_summary();
> diff --git a/powerpc/selftest.c b/powerpc/selftest.c
> index 8c5ff0ac889d..09856486bac5 100644
> --- a/powerpc/selftest.c
> +++ b/powerpc/selftest.c
> @@ -11,33 +11,28 @@
>  
>  static void check_setup(int argc, char **argv)
>  {
> -	int nr_tests = 0, len, i;
> +	int nr_tests = 0;
>  	long val;
>  
> -	for (i = 0; i < argc; ++i) {
> +	if (parse_keyval(argc, argv, "mem", &val)) {
> +		report_prefix_push("mem");
>  
> -		len = parse_keyval(argv[i], &val);
> -		if (len == -1)
> -			continue;
> +		phys_addr_t memsize = PHYSICAL_END - PHYSICAL_START;
> +		phys_addr_t expected = ((phys_addr_t)val)*1024*1024;
>  
> -		argv[i][len] = '\0';
> -		report_prefix_push(argv[i]);
> +		report("size = %d MB", memsize == expected,
> +						memsize/1024/1024);
>  
> -		if (strcmp(argv[i], "mem") == 0) {
> +		++nr_tests;
> +		report_prefix_pop();
> +	}
>  
> -			phys_addr_t memsize = PHYSICAL_END - PHYSICAL_START;
> -			phys_addr_t expected = ((phys_addr_t)val)*1024*1024;
> +	if (parse_keyval(argc, argv, "smp", &val)) {
> +		report_prefix_push("smp");
>  
> -			report("size = %d MB", memsize == expected,
> -							memsize/1024/1024);
> -			++nr_tests;
> -
> -		} else if (strcmp(argv[i], "smp") == 0) {
> -
> -			report("nr_cpus = %d", nr_cpus == (int)val, nr_cpus);
> -			++nr_tests;
> -		}
> +		report("nr_cpus = %d", nr_cpus == (int)val, nr_cpus);
>  
extra blank line

> +		++nr_tests;
>  		report_prefix_pop();
>  	}
>  
> @@ -49,14 +44,9 @@ int main(int argc, char **argv)
>  {
>  	report_prefix_push("selftest");
>  
> -	if (argc < 2)
> -		report_abort("no test specified");
> -
> -	report_prefix_push(argv[1]);
> -
>  	if (strcmp(argv[1], "setup") == 0) {
>  
> -		check_setup(argc-2, &argv[2]);
> +		check_setup(argc-1, &argv[1]);
>  
>  	}
>  
> diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
> index ed4fdbe64909..fe5db7e302bb 100644
> --- a/powerpc/unittests.cfg
> +++ b/powerpc/unittests.cfg
> @@ -39,7 +39,7 @@ file = spapr_hcall.elf
>  [rtas-get-time-of-day]
>  file = rtas.elf
>  timeout = 5
> -extra_params = -append "get-time-of-day date=$(date +%s)"
> +extra_params = -append "get-time-of-day=$(date +%s)"
>  groups = rtas
>  
>  [rtas-set-time-of-day]
> -- 
> 2.8.2

Thanks,
drew

Re: [kvm-unit-tests PATCH 2/2] lib/util,arm,powerpc: replace parse_keyval with better helpers

[Andrew Jones]

On Thu, May 12, 2016 at 09:19:08AM +0200, Andrew Jones wrote:
> On Wed, May 11, 2016 at 10:55:14PM +0200, Radim Krčmář wrote:
> >  int main(int argc, char **argv)
> > @@ -115,33 +123,12 @@ int main(int argc, char **argv)
> >  
> >  	report_prefix_push("rtas");
> >  
> > -	if (argc < 2)
> > -		report_abort("no test specified");
> > -
> > -	report_prefix_push(argv[1]);
> > -
> > -	if (strcmp(argv[1], "get-time-of-day") == 0) {
> > -
> > -		len = parse_keyval(argv[2], &val);
> > -		if (len == -1) {
> > -			printf("Missing parameter \"date\"\n");
> > -			abort();
> > -		}
> > -		argv[2][len] = '\0';
> > -
> > +	if (parse_keyval(argc, argv, "get-time-of-day", &val))
> >  		check_get_time_of_day(val);
> >  
> > -	} else if (strcmp(argv[1], "set-time-of-day") == 0) {
> > -
> > +	if (find_key(argc, argv, "set-time-of-day"))
> >  		check_set_time_of_day();
> >  
> > -	} else {
> > -		printf("Unknown subtest\n");
> > -		abort();
> > -	}
> > -
> > -	report_prefix_pop();
> > -
> 
> Also a nice cleanup. We could have kept the missing parameter abort
> with something like
> 
>  if (find_key(argc, argv, "get-time-of-day")) {
>      if (!parse_keyval(argc, argv, "get-time-of-day", &val)) {
>          printf("Missing parameter \"date\"\n");
>          abort();
>      }
>      check_get_time_of_day(val);
>  }

Hmm, actually the above wouldn't work with the current
find_key implementation. Maybe we should change it to
just check for null?

 bool find_key(int argc, char **argv, char *key)
 {
    return find_keyval(argc, argv, key) != NULL;
 }

And change the documentation to explain it looks for @key
by itself, or with a paired =val, but doesn't parse val.

drew

Re: [kvm-unit-tests PATCH 2/2] lib/util,arm,powerpc: replace parse_keyval with better helpers

[Laurent Vivier]

On 12/05/2016 10:05, Andrew Jones wrote:
> On Thu, May 12, 2016 at 09:19:08AM +0200, Andrew Jones wrote:
>> On Wed, May 11, 2016 at 10:55:14PM +0200, Radim Krčmář wrote:
>>>  int main(int argc, char **argv)
>>> @@ -115,33 +123,12 @@ int main(int argc, char **argv)
>>>  
>>>  	report_prefix_push("rtas");
>>>  
>>> -	if (argc < 2)
>>> -		report_abort("no test specified");
>>> -
>>> -	report_prefix_push(argv[1]);
>>> -
>>> -	if (strcmp(argv[1], "get-time-of-day") == 0) {
>>> -
>>> -		len = parse_keyval(argv[2], &val);
>>> -		if (len == -1) {
>>> -			printf("Missing parameter \"date\"\n");
>>> -			abort();
>>> -		}
>>> -		argv[2][len] = '\0';
>>> -
>>> +	if (parse_keyval(argc, argv, "get-time-of-day", &val))
>>>  		check_get_time_of_day(val);
>>>  
>>> -	} else if (strcmp(argv[1], "set-time-of-day") == 0) {
>>> -
>>> +	if (find_key(argc, argv, "set-time-of-day"))
>>>  		check_set_time_of_day();
>>>  
>>> -	} else {
>>> -		printf("Unknown subtest\n");
>>> -		abort();
>>> -	}
>>> -
>>> -	report_prefix_pop();
>>> -
>>
>> Also a nice cleanup. We could have kept the missing parameter abort
>> with something like
>>
>>  if (find_key(argc, argv, "get-time-of-day")) {
>>      if (!parse_keyval(argc, argv, "get-time-of-day", &val)) {
>>          printf("Missing parameter \"date\"\n");
>>          abort();
>>      }
>>      check_get_time_of_day(val);
>>  }
> 
> Hmm, actually the above wouldn't work with the current
> find_key implementation. Maybe we should change it to
> just check for null?
> 
>  bool find_key(int argc, char **argv, char *key)
>  {
>     return find_keyval(argc, argv, key) != NULL;
>  }
> 
> And change the documentation to explain it looks for @key
> by itself, or with a paired =val, but doesn't parse val.

I don't know if it can help, but instead of creating our own API,
perhaps we can implement getopt() and getopt_long() and use them?

Or is it over-engineered?

Laurent

Re: [kvm-unit-tests PATCH 2/2] lib/util,arm,powerpc: replace parse_keyval with better helpers

[Radim Krčmář]

2016-05-12 10:09+0200, Laurent Vivier:
> I don't know if it can help, but instead of creating our own API,
> perhaps we can implement getopt() and getopt_long() and use them?

A known API is a good idea as long I'm not the one implementing it. :)

> Or is it over-engineered?

Yeah, getopt would make both callers and the library more complicated.

Re: [kvm-unit-tests PATCH 2/2] lib/util,arm,powerpc: replace parse_keyval with better helpers

[Thomas Huth]

On 12.05.2016 15:22, Radim Krčmář wrote:
> 2016-05-12 10:09+0200, Laurent Vivier:
>> I don't know if it can help, but instead of creating our own API,
>> perhaps we can implement getopt() and getopt_long() and use them?
> 
> A known API is a good idea as long I'm not the one implementing it. :)
> 
>> Or is it over-engineered?
> 
> Yeah, getopt would make both callers and the library more complicated.

We could maybe use the implementation from SLOF:

http://git.qemu.org/?p=SLOF.git;a=blob;f=lib/libc/getopt/getopt.c;h=be626ddc290ee84b3605

It's BSD-licensed, so I think it should be ok to use it
in kvm-unit-tests?

 Thomas

Re: [kvm-unit-tests PATCH 2/2] lib/util,arm,powerpc: replace parse_keyval with better helpers

[Radim Krčmář]

2016-05-12 10:05+0200, Andrew Jones:
> On Thu, May 12, 2016 at 09:19:08AM +0200, Andrew Jones wrote:
>> On Wed, May 11, 2016 at 10:55:14PM +0200, Radim Krčmář wrote:
>> >  int main(int argc, char **argv)
>> > @@ -115,33 +123,12 @@ int main(int argc, char **argv)
>> >  
>> >  	report_prefix_push("rtas");
>> >  
>> > -	if (argc < 2)
>> > -		report_abort("no test specified");
>> > -
>> > -	report_prefix_push(argv[1]);
>> > -
>> > -	if (strcmp(argv[1], "get-time-of-day") == 0) {
>> > -
>> > -		len = parse_keyval(argv[2], &val);
>> > -		if (len == -1) {
>> > -			printf("Missing parameter \"date\"\n");
>> > -			abort();
>> > -		}
>> > -		argv[2][len] = '\0';
>> > -
>> > +	if (parse_keyval(argc, argv, "get-time-of-day", &val))
>> >  		check_get_time_of_day(val);
>> >  
>> > -	} else if (strcmp(argv[1], "set-time-of-day") == 0) {
>> > -
>> > +	if (find_key(argc, argv, "set-time-of-day"))
>> >  		check_set_time_of_day();
>> >  
>> > -	} else {
>> > -		printf("Unknown subtest\n");
>> > -		abort();
>> > -	}
>> > -
>> > -	report_prefix_pop();
>> > -
>> 
>> Also a nice cleanup. We could have kept the missing parameter abort
>> with something like
>> 
>>  if (find_key(argc, argv, "get-time-of-day")) {
>>      if (!parse_keyval(argc, argv, "get-time-of-day", &val)) {
>>          printf("Missing parameter \"date\"\n");
>>          abort();
>>      }
>>      check_get_time_of_day(val);
>>  }

If checked for the parameter, I'd rather keep it closer to the original:

  if (argc < 3) // there was a bug in the original
      report_abort("")
  if (find_key(2, argv, "get-time-of-day")) {
      if (!parse_keyval(2, argv+2, "date", &val))
          report_abort("");
      check_get_time_of_day(val);
  }

> Hmm, actually the above wouldn't work with the current
> find_key implementation. Maybe we should change it to
> just check for null?

No, that was the point.

>  bool find_key(int argc, char **argv, char *key)
>  {
>     return find_keyval(argc, argv, key) != NULL;
>  }

This is the same as return find_keyval(argc, argv, key), so you could
just use that.

> And change the documentation to explain it looks for @key
> by itself, or with a paired =val, but doesn't parse val.

They are too similar to deserve a distinction.  The previous find_key is
somewhat useful, because the caller doesn't have to remember *key (= can
use a string literal) to distinguish the case when there is no argument.

Re: [kvm-unit-tests PATCH 2/2] lib/util,arm,powerpc: replace parse_keyval with better helpers

[Radim Krčmář]

2016-05-12 09:19+0200, Andrew Jones:
> On Wed, May 11, 2016 at 10:55:14PM +0200, Radim Krčmář wrote:
>> A common pattern was to scan through all argv strings to find key or
>> key=val.  parse_keyval used to return val as long, but another function
>> needed to check the key.  New functions do both at once.
>> 
>> parse_keyval was replaced with different parse_keyval, so callers needed
>> to be updated.  While at it, I changed the meaning of arguments to
>> powerpc/rtas.c to make the code simpler.  And removing aborts is a
>> subtle preparation for a patch that reports SKIP when no tests were run
>> and they weren't necessary even now.
>> 
>> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
>> ---
>> diff --git a/lib/util.c b/lib/util.c
>> @@ -4,15 +4,43 @@
>> +long atol_keyval(int argc, char **argv, char *key)
>> +{
>> +	long val;
>> +	bool is_keyval = parse_keyval(argc, argv, key, &val);
>> +
>> +	return is_keyval ? val : 0;
> 
> Not sure how this one would be useful, and I don't see any uses
> for it below. It may be difficult to use due to its ambiguous
> results,  as zero could be the value, or the result because the
> key wasn't found, or because the key was found, but was a key
> with no '='.

I don't like parse_keyval, because it forces you to pass 'long' even if
you want a boolean out of it.  I wanted a function that doesn't impose a
type ... it's not needed, I'll get rid of it.

>> +bool find_key(int argc, char **argv, char *key) {
> 
> Mr. {, please come down.

How dare you colloquially address the King of Curly Brackets.

>> +	return find_keyval(argc, argv, key) == key;
>> +}
>> +
>> +char * find_keyval(int argc, char **argv, char *key)
> 
> I prefer the '*' by the name.

Ok, seems like it is the normal way.  I consider the name of returned
variable to be "" (empty string), so the pointer is by it. ;)

>> +{
>> +	int i;
>> +	size_t keylen = strlen(key);
>> +
>> +	for (i = 1; i < argc; i++) {
>
> We should start i at 0, because we shouldn't assume the user will
> always pass in main's &argv[0]. Above arm/setup.c actually uses
> &argv[1]; so does arm's setup test work? Anyway, it shouldn't matter
> if we always look at the program name while searching for the key,
> because, for example, x86/msr.flat would be a strange key name :-)

Sure, I'll rename argc and argv (to nr_strings and strings?), so it's
not confusing.

>> diff --git a/lib/util.h b/lib/util.h
>> @@ -8,16 +8,24 @@
>>   * This work is licensed under the terms of the GNU LGPL, version 2.
>>   */
>>  
>> -/*
>> - * parse_keyval extracts the integer from a string formatted as
>> - * string=integer. This is useful for passing expected values to
>> - * the unit test on the command line, i.e. it helps parse QEMU
>> - * command lines that include something like -append var1=1 var2=2
>> - * @s is the input string, likely a command line parameter, and
>> - * @val is a pointer to where the integer will be stored.
>> - *
>> - * Returns the offset of the '=', or -1 if no keyval pair is found.
>> +/* @argc and @argv are standard arguments to C main.  */
> 
> I agree the only use for a parsing function in kvm-unit-tests is
> main()'s inputs, but we shouldn't expect/require them to be unmodified
> prior to making parse_keyval calls.

I didn't mean they have to be unmodified, just that argc is the length
of argv array and the first element is ignored, because it's not an
argument ...

>> +
>> +/* parse_keyval returns true if @argv[i] has @key=val format and parse @val;
>> + * returns false otherwise.
>>   */
> 
> How about this instead
> 
> /*
>  * parse_keyval searches @argv members for strings of the form @key=val
>  * Returns
>  *  true when a @key=val string is found; val is parsed and stored in @val.
>  *  false otherwise
>  */
> 
>> -extern int parse_keyval(char *s, long *val);
>> +bool parse_keyval(int argc, char **argv, char *key, long *val);
>> +
>> +/* atol_keyval returns parsed val if @argv[i] has @key=val format; returns 0
> 
> same comment rewrite suggestion as above
> 
>> + * otherwise.
>> + */
>> +long atol_keyval(int argc, char **argv, char *key);
>> +
>> +/* find_key decides whether @key is equal @argv[i]. */
> 
> s/equal/equal to/, but I'd rewrite this one too :-)
> 
> /*
>  * find_key searches @argv members for the string @key
>  * Returns true when found, false otherwise.
>  */

I'll add explanation of argc and use them.

>> +bool find_key(int argc, char **argv, char *key);
>> +
>> +/* find_keyval returns key if @key is equal to @argv[i]; returns pointer to val
>> + * if @argv[i] has @key=val format; returns NULL otherwise.
>> + */
> 
> Another rewrite suggestion. Please list the return possibilities
> Returns
>  - @key when ...
>  - A pointer to the start of val when ...
>  - NULL otherwise
> 
> or something like that

Sure, thanks.