From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Subject: Re: [kvm-unit-tests PATCH 2/2] lib/util,arm,powerpc: replace parse_keyval with better helpers Date: Thu, 12 May 2016 15:00:51 +0200 Message-ID: <20160512130050.GB5864@potion> References: <1463000114-3572-1-git-send-email-rkrcmar@redhat.com> <1463000114-3572-3-git-send-email-rkrcmar@redhat.com> <20160512071908.537ltvwkiiekqvri@hawk.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: kvm@vger.kernel.org, Paolo Bonzini , Thomas Huth , Laurent Vivier To: Andrew Jones Return-path: Received: from mx1.redhat.com ([209.132.183.28]:53653 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751623AbcELNAz (ORCPT ); Thu, 12 May 2016 09:00:55 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5E6883DD47 for ; Thu, 12 May 2016 13:00:54 +0000 (UTC) Content-Disposition: inline In-Reply-To: <20160512071908.537ltvwkiiekqvri@hawk.localdomain> Sender: kvm-owner@vger.kernel.org List-ID: 2016-05-12 09:19+0200, Andrew Jones: > On Wed, May 11, 2016 at 10:55:14PM +0200, Radim Kr=C4=8Dm=C3=A1=C5=99= wrote: >> A common pattern was to scan through all argv strings to find key or >> key=3Dval. parse_keyval used to return val as long, but another fun= ction >> needed to check the key. New functions do both at once. >>=20 >> parse_keyval was replaced with different parse_keyval, so callers ne= eded >> 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. >>=20 >> Signed-off-by: Radim Kr=C4=8Dm=C3=A1=C5=99 >> --- >> 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 =3D parse_keyval(argc, argv, key, &val); >> + >> + return is_keyval ? val : 0; >=20 > 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 '=3D'. 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) { >=20 > Mr. {, please come down. How dare you colloquially address the King of Curly Brackets. >> + return find_keyval(argc, argv, key) =3D=3D key; >> +} >> + >> +char * find_keyval(int argc, char **argv, char *key) >=20 > 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 =3D strlen(key); >> + >> + for (i =3D 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= =2E >> */ >> =20 >> -/* >> - * parse_keyval extracts the integer from a string formatted as >> - * string=3Dinteger. 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=3D1 var2=3D= 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 '=3D', or -1 if no keyval pair is foun= d. >> +/* @argc and @argv are standard arguments to C main. */ >=20 > 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 unmodifie= d > 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=3Dval format and = parse @val; >> + * returns false otherwise. >> */ >=20 > How about this instead >=20 > /* > * parse_keyval searches @argv members for strings of the form @key=3D= val > * Returns > * true when a @key=3Dval string is found; val is parsed and stored = in @val. > * false otherwise > */ >=20 >> -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=3Dval format= ; returns 0 >=20 > same comment rewrite suggestion as above >=20 >> + * otherwise. >> + */ >> +long atol_keyval(int argc, char **argv, char *key); >> + >> +/* find_key decides whether @key is equal @argv[i]. */ >=20 > s/equal/equal to/, but I'd rewrite this one too :-) >=20 > /* > * 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 po= inter to val >> + * if @argv[i] has @key=3Dval format; returns NULL otherwise. >> + */ >=20 > Another rewrite suggestion. Please list the return possibilities > Returns > - @key when ... > - A pointer to the start of val when ... > - NULL otherwise >=20 > or something like that Sure, thanks.