Re: [Qemu-devel] [PATCH 4/8] qemu-option: factor out parse_option_bool

[Kevin Wolf]

Gerd Hoffmann schrieb:
> Signed-off-by: Gerd Hoffmann <address@hidden>
> ---
>  qemu-option.c |   34 ++++++++++++++++++++++------------
>  qemu-option.h |    1 +
>  2 files changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/qemu-option.c b/qemu-option.c
> index 646bbad..45f0c8f 100644
> --- a/qemu-option.c
> +++ b/qemu-option.c
> @@ -101,6 +101,23 @@ QEMUOptionParameter 
> *get_option_parameter(QEMUOptionParameter *list,
>      return NULL;
>  }
>  
> +int parse_option_bool(const char *name, const char *value, int *ret)

I don't really like this being global. In the end, we want to have only
one parsing mechanism anyway and I don't think it needs to have multiple
source files. So is there any reason not to put your QemuOpts code into
qemu-option.c and leave parsing internals static?

> +{
> +    if (value != NULL) {
> +        if (!strcmp(value, "on")) {
> +            *ret = 1;
> +        } else if (!strcmp(value, "off")) {
> +            *ret = 0;
> +        } else {
> +            fprintf(stderr, "Option '%s': Use 'on' or 'off'\n", name);
> +            return -1;
> +        }
> +    } else {
> +        *ret = 1;
> +    }
> +    return 0;
> +}
> +
>  /*
>   * Sets the value of a parameter in a given option list. The parsing of the
>   * value depends on the type of option:
> @@ -121,6 +138,8 @@ QEMUOptionParameter 
> *get_option_parameter(QEMUOptionParameter *list,
>  int set_option_parameter(QEMUOptionParameter *list, const char *name,
>      const char *value)
>  {
> +    int flag;
> +
>      // Find a matching parameter
>      list = get_option_parameter(list, name);
>      if (list == NULL) {
> @@ -131,18 +150,9 @@ int set_option_parameter(QEMUOptionParameter *list, 
> const char *name,
>      // Process parameter
>      switch (list->type) {
>      case OPT_FLAG:
> -        if (value != NULL) {
> -            if (!strcmp(value, "on")) {
> -                list->value.n = 1;
> -            } else if (!strcmp(value, "off")) {
> -                list->value.n = 0;
> -            } else {
> -                fprintf(stderr, "Option '%s': Use 'on' or 'off'\n", name);
> -                return -1;
> -            }
> -        } else {
> -            list->value.n = 1;
> -        }
> +        if (-1 == parse_option_bool(name, value, &flag))

You're still doing it... ;-)

Kevin