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@xxxxxxxxxx>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
