Re: [Qemu-devel] [PATCH 03/16] qemu-option: parse_option_bool(): use error_set()

[Laszlo Ersek <lersek@redhat.com>]

On 05/17/12 16:33, Luiz Capitulino wrote:
> Note that set_option_parameter() callers still expect automatic error
> reporting with QError, so set_option_parameter() calls
> qerror_report_err() to keep the same semantics.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  qemu-option.c |   22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/qemu-option.c b/qemu-option.c
> index 72dcb56..a8b50af 100644
> --- a/qemu-option.c
> +++ b/qemu-option.c
> @@ -169,7 +169,8 @@ QEMUOptionParameter *get_option_parameter(QEMUOptionParameter *list,
>      return NULL;
>  }
>  
> -static int parse_option_bool(const char *name, const char *value, bool *ret)
> +static void parse_option_bool(const char *name, const char *value, bool *ret,
> +                              Error **errp)
>  {
>      if (value != NULL) {
>          if (!strcmp(value, "on")) {
> @@ -177,13 +178,11 @@ static int parse_option_bool(const char *name, const char *value, bool *ret)
>          } else if (!strcmp(value, "off")) {
>              *ret = 0;
>          } else {
> -            qerror_report(QERR_INVALID_PARAMETER_VALUE, name, "'on' or 'off'");
> -            return -1;
> +            error_set(errp,QERR_INVALID_PARAMETER_VALUE, name, "'on' or 'off'");
>          }
>      } else {
>          *ret = 1;
>      }
> -    return 0;
>  }
>  
>  static void parse_option_number(const char *name, const char *value,
> @@ -263,6 +262,7 @@ int set_option_parameter(QEMUOptionParameter *list, const char *name,
>      const char *value)
>  {
>      bool flag;
> +    Error *local_err = NULL;
>  
>      // Find a matching parameter
>      list = get_option_parameter(list, name);
> @@ -274,8 +274,10 @@ int set_option_parameter(QEMUOptionParameter *list, const char *name,
>      // Process parameter
>      switch (list->type) {
>      case OPT_FLAG:
> -        if (parse_option_bool(name, value, &flag) == -1)
> -            return -1;
> +        parse_option_bool(name, value, &flag, &local_err);
> +        if (error_is_set(&local_err)) {
> +            goto exit_err;
> +        }
>          list->value.n = flag;
>          break;
>  
> @@ -299,6 +301,11 @@ int set_option_parameter(QEMUOptionParameter *list, const char *name,
>      }
>  
>      return 0;
> +
> +exit_err:
> +    qerror_report_err(local_err);
> +    error_free(local_err);
> +    return -1;
>  }

I think this may have been simpler (= goto-less) like this:

    case OPT_FLAG:
        parse_option_bool(name, value, &flag, &local_err);
        if (!error_is_set(&local_err)) {
            list->value.n = flag;
        }
        break;

and in the end, before the "return 0", another check with error_is_set().

But it appears correct this way too, of course.

Laszlo