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

[Markus Armbruster]

Anthony Liguori <address@hidden> writes:

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

Fails to explain the important part, namely the actual change to option
comparison!

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

Why bother?

> +
> +    for (i = 0; lhs[i] && rhs[i]; i++) {
> +        int ret;
> +
> +        ret = opt_tolower(lhs[i]) - opt_tolower(rhs[i]);
> +        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;
> +    }
> +    
> +    return 0;
> +}
> +

Are you sure you want to make options case-insensitive?  If yes, please
mention it in the commit message.

The implementation is too longwinded for my taste :)

static unsigned char opt_canon_ch(char ch)
{
    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)
{
    unsigned char l, r;

    do {
        l = opt_canon_ch(*lhs++);
        r = opt_canon_ch(*rhs++);
    } while (l && l == r);

    return l - r;
}

Or maybe a bool qemu_opt_name_eq() instead.

>  int get_next_param_value(char *buf, int buf_size,
>                           const char *tag, const char **pstr)
>  {
> @@ -101,7 +138,7 @@ int get_next_param_value(char *buf, int buf_size,
>          if (*p != '=')
>              break;
>          p++;
> -        if (!strcmp(tag, option)) {
> +        if (!qemu_opt_name_cmp(tag, option)) {
>              *pstr = get_opt_value(buf, buf_size, p);
>              if (**pstr == ',') {
>                  (*pstr)++;

Aha, you cover the non-QemuOpts stuff, too.  Fine with me.

> @@ -137,7 +174,7 @@ int check_params(char *buf, int buf_size,
>          }
>          p++;
>          for (i = 0; params[i] != NULL; i++) {
> -            if (!strcmp(params[i], buf)) {
> +            if (!qemu_opt_name_cmp(params[i], buf)) {
>                  break;
>              }
>          }
> @@ -160,7 +197,7 @@ QEMUOptionParameter 
> *get_option_parameter(QEMUOptionParameter *list,
>      const char *name)
>  {
>      while (list && list->name) {
> -        if (!strcmp(list->name, name)) {
> +        if (!qemu_opt_name_cmp(list->name, name)) {
>              return list;
>          }
>          list++;
> @@ -516,7 +553,7 @@ static QemuOpt *qemu_opt_find(QemuOpts *opts, const char 
> *name)
>      QemuOpt *opt;
>  
>      QTAILQ_FOREACH_REVERSE(opt, &opts->head, QemuOptHead, next) {
> -        if (strcmp(opt->name, name) != 0)
> +        if (qemu_opt_name_cmp(opt->name, name) != 0)
>              continue;
>          return opt;
>      }
> @@ -599,7 +636,7 @@ static void opt_set(QemuOpts *opts, const char *name, 
> const char *value,
>      int i;
>  
>      for (i = 0; desc[i].name != NULL; i++) {
> -        if (strcmp(desc[i].name, name) == 0) {
> +        if (qemu_opt_name_cmp(desc[i].name, name) == 0) {
>              break;
>          }
>      }
> @@ -660,7 +697,7 @@ int qemu_opt_set_bool(QemuOpts *opts, const char *name, 
> bool val)
>      int i;
>  
>      for (i = 0; desc[i].name != NULL; i++) {
> -        if (strcmp(desc[i].name, name) == 0) {
> +        if (qemu_opt_name_cmp(desc[i].name, name) == 0) {
>              break;
>          }
>      }
> @@ -709,7 +746,7 @@ QemuOpts *qemu_opts_find(QemuOptsList *list, const char 
> *id)
>              }
>              continue;
>          }
> -        if (strcmp(opts->id, id) != 0) {
> +        if (qemu_opt_name_cmp(opts->id, id) != 0) {
>              continue;
>          }
>          return opts;

There's a strcmp(option, "id") in opts_do_parse(), and a similar one in
qemu_opts_from_qdict_1().  Perhaps they should be compared with
qemu_opt_name_cmp(), too, just for consistency.

> @@ -1062,7 +1099,7 @@ void qemu_opts_validate(QemuOpts *opts, const 
> QemuOptDesc *desc, Error **errp)
>          int i;
>  
>          for (i = 0; desc[i].name != NULL; i++) {
> -            if (strcmp(desc[i].name, opt->name) == 0) {
> +            if (qemu_opt_name_cmp(desc[i].name, opt->name) == 0) {
>                  break;
>              }
>          }
> diff --git a/qemu-option.h b/qemu-option.h
> index 951dec3..ee27a13 100644
> --- 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);
> +
>  #endif

Why external?