| Message ID | 1463000114-3572-3-git-send-email-rkrcmar@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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]
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(-)