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@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
