qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Peter Maydell
Subject: Re: [Qemu-devel] [PATCH 1/2] qemu-opts: introduce a function to compare option names
Date: Wed, 25 Jul 2012 17:53:36 +0100

On 25 July 2012 17:25, Anthony Liguori <address@hidden> 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 <address@hidden>
> ---
>  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.

 +{
> +    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.)

> +        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).

> +
> +    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?

-- PMM



reply via email to

[Prev in Thread] Current Thread [Next in Thread]