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

["Radim Krčmář" <rkrcmar@redhat.com>]

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.