Re: [Qemu-devel] [PATCH 1/2] qemu-opts: introduce a function to compare option names

[Anthony Liguori <aliguori@us.ibm.com>]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 25 July 2012 17:25, Anthony Liguori <aliguori@us.ibm.com> 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 <aliguori@us.ibm.com>
>> ---
>>  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