From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:48548) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Su79b-0001Ql-EL for qemu-devel@nongnu.org; Wed, 25 Jul 2012 15:21:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Su79a-0003Q5-8b for qemu-devel@nongnu.org; Wed, 25 Jul 2012 15:21:51 -0400 Received: from e9.ny.us.ibm.com ([32.97.182.139]:45716) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Su79a-0003Pv-2e for qemu-devel@nongnu.org; Wed, 25 Jul 2012 15:21:50 -0400 Received: from /spool/local by e9.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 25 Jul 2012 13:57:02 -0400 Received: from d01relay04.pok.ibm.com (d01relay04.pok.ibm.com [9.56.227.236]) by d01dlp02.pok.ibm.com (Postfix) with ESMTP id 3C61C6E8AA1 for ; Wed, 25 Jul 2012 13:34:18 -0400 (EDT) Received: from d03av05.boulder.ibm.com (d03av05.boulder.ibm.com [9.17.195.85]) by d01relay04.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q6PHYEFC191356 for ; Wed, 25 Jul 2012 13:34:15 -0400 Received: from d03av05.boulder.ibm.com (loopback [127.0.0.1]) by d03av05.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q6PHXtuK023723 for ; Wed, 25 Jul 2012 11:33:55 -0600 From: Anthony Liguori In-Reply-To: References: <1343233543-18561-1-git-send-email-aliguori@us.ibm.com> <1343233543-18561-2-git-send-email-aliguori@us.ibm.com> Date: Wed, 25 Jul 2012 12:33:44 -0500 Message-ID: <87y5m721af.fsf@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: Re: [Qemu-devel] [PATCH 1/2] qemu-opts: introduce a function to compare option names List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Luiz Capitulino , qemu-devel@nongnu.org, Markus Armbruster Peter Maydell writes: > On 25 July 2012 17:25, Anthony Liguori wrote: >> We don't use the standard C functions for conversion because we don't want to >> depend on the user's locale. All option names in QEMU are en_US in plain ASCII. >> >> Signed-off-by: Anthony Liguori >> --- >> qemu-option.c | 53 +++++++++++++++++++++++++++++++++++++++++++++-------- >> qemu-option.h | 2 ++ >> 2 files changed, 47 insertions(+), 8 deletions(-) >> >> diff --git a/qemu-option.c b/qemu-option.c >> index 8334190..6494c99 100644 >> --- a/qemu-option.c >> +++ b/qemu-option.c >> @@ -89,6 +89,43 @@ const char *get_opt_value(char *buf, int buf_size, const char *p) >> return p; >> } >> >> +static int opt_tolower(int ch) > > This isn't actually doing a pure tolower() operation. > opt_canonicalize() is a bit long though, perhaps. > I guess it's static so not a big deal. Yeah. > > +{ >> + if (ch >= 'A' && ch <= 'Z') { >> + return 'a' + (ch - 'A'); >> + } else if (ch == '_') { >> + return '-'; >> + } >> + >> + return ch; >> +} >> + >> +int qemu_opt_name_cmp(const char *lhs, const char *rhs) >> +{ >> + int i; >> + >> + g_assert(lhs && rhs); >> + >> + for (i = 0; lhs[i] && rhs[i]; i++) { >> + int ret; >> + >> + ret = opt_tolower(lhs[i]) - opt_tolower(rhs[i]); > > This is not in line with the return value that the C library > strcmp() would return. C99 7.21.4 says "The sign of a nonzero > value returned by the comparison functions memcmp, strcmp, > and strncmp is determined by the sign of the difference between > the values of the first pair of characters (both interpreted > as unsigned char) that differ in the objects being compared." > This code will use whatever the signed/unsignedess of plain > 'char' is. > > (None of the callers use the sign of the return value so this > is something of a nitpick.) Sorry, how is this wrong? This returns: strcmp("a", "b") -> -1 qemu_opt_name_cmp("a", "b") -> -1 >> + if (ret < 0) { >> + return -1; >> + } else if (ret > 0) { >> + return 1; >> + } >> + } >> + >> + if (!lhs[i] && rhs[i]) { >> + return -1; >> + } else if (lhs[i] && !rhs[i]) { >> + return 1; >> + } > > If you made the for() loop into a do..while so that we > execute the loop body for the 'final NUL' case you could > avoid having this pair of checks, I think (and you can > make the loop termination case just '!lhs[i]' since if > we get past the 'mismatch' exits we've definitely got > to the end of both strings and can return true). I can poke around but not convinced it will result in better code. I must admit that I prefer explicit handling of edge cases like this. > >> + >> + return 0; >> +} > >> --- a/qemu-option.h >> +++ b/qemu-option.h >> @@ -141,4 +141,6 @@ int qemu_opts_print(QemuOpts *opts, void *dummy); >> int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void *opaque, >> int abort_on_failure); >> >> +int qemu_opt_name_cmp(const char *lhs, const char *rhs); > > No documentation comment? Good point. Regards, Anthony Liguori > > -- PMM