Re: [Qemu-devel] [PATCH v6 2/7] option: make parse_option_bool/number non-static

[Marc-André Lureau]

Hi

On Tue, Jun 14, 2016 at 6:07 PM, Daniel P. Berrange <address@hidden> wrote:
> The opts-visitor.c opts_type_bool() method has code for
> parsing a string to set a bool value, as does the
> qemu-option.c parse_option_bool() method, except it
> handles fewer cases.
>
> To enable consistency across the codebase, extend
> parse_option_bool() to handle "yes", "no", "y" and
> "n", and make it non-static. Convert the opts
> visitor to call this method directly.
>
> Also make parse_option_number() non-static to allow
> for similar reuse later.
>
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
>  include/qemu/option.h |  4 ++++
>  qapi/opts-visitor.c   | 19 +------------------
>  util/qemu-option.c    | 27 ++++++++++++++++-----------
>  3 files changed, 21 insertions(+), 29 deletions(-)
>
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index 8542d2d..c37d8e9 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -37,6 +37,10 @@ int get_param_value(char *buf, int buf_size,
>                      const char *tag, const char *str);
>
>
> +void parse_option_bool(const char *name, const char *value, bool *ret,
> +                       Error **errp);
> +void parse_option_number(const char *name, const char *value,
> +                         uint64_t *ret, Error **errp);
>  void parse_option_size(const char *name, const char *value,
>                         uint64_t *ret, Error **errp);
>  bool has_help_option(const char *param);
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index 4cf1cf8..b3757ca 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -334,7 +334,6 @@ opts_type_str(Visitor *v, const char *name, char **obj, 
> Error **errp)
>  }
>
>
> -/* mimics qemu-option.c::parse_option_bool() */
>  static void
>  opts_type_bool(Visitor *v, const char *name, bool *obj, Error **errp)
>  {
> @@ -346,23 +345,7 @@ opts_type_bool(Visitor *v, const char *name, bool *obj, 
> Error **errp)
>          return;
>      }
>
> -    if (opt->str) {
> -        if (strcmp(opt->str, "on") == 0 ||
> -            strcmp(opt->str, "yes") == 0 ||
> -            strcmp(opt->str, "y") == 0) {
> -            *obj = true;
> -        } else if (strcmp(opt->str, "off") == 0 ||
> -            strcmp(opt->str, "no") == 0 ||
> -            strcmp(opt->str, "n") == 0) {
> -            *obj = false;
> -        } else {
> -            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
> -                       "on|yes|y|off|no|n");
> -            return;
> -        }
> -    } else {
> -        *obj = true;
> -    }
> +    parse_option_bool(opt->name, opt->str, obj, errp);
>
>      processed(ov, name);
>  }
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 3467dc2..6fc9ccb 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -125,25 +125,30 @@ int get_param_value(char *buf, int buf_size,
>      return get_next_param_value(buf, buf_size, tag, &str);
>  }
>
> -static void parse_option_bool(const char *name, const char *value, bool *ret,
> -                              Error **errp)
> +void parse_option_bool(const char *name, const char *value, bool *ret,
> +                       Error **errp)
>  {
>      if (value != NULL) {
> -        if (!strcmp(value, "on")) {
> -            *ret = 1;
> -        } else if (!strcmp(value, "off")) {
> -            *ret = 0;
> +        if (strcmp(value, "on") == 0 ||
> +            strcmp(value, "yes") == 0 ||
> +            strcmp(value, "y") == 0) {
> +            *ret = true;
> +        } else if (strcmp(value, "off") == 0 ||
> +            strcmp(value, "no") == 0 ||
> +            strcmp(value, "n") == 0) {
> +            *ret = false;
>          } else {
> -            error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> -                       name, "'on' or 'off'");
> +            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name,
> +                       "on|yes|y|off|no|n");
> +            return;
>          }
>      } else {
> -        *ret = 1;
> +        *ret = true;
>      }
>  }
>
> -static void parse_option_number(const char *name, const char *value,
> -                                uint64_t *ret, Error **errp)
> +void parse_option_number(const char *name, const char *value,
> +                         uint64_t *ret, Error **errp)
>  {
>      char *postfix;
>      uint64_t number;
> --
> 2.5.5
>
>

Reviewed-by: Marc-André Lureau <address@hidden>



-- 
Marc-André Lureau