Peter Maydell <peter.mayd...@linaro.org> writes:
> On 25 July 2012 17:25, Anthony Liguori <aligu...@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 <aligu...@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
