Re: [PATCH v17 09/16] util/log: Add Error argument to qemu_str_to_log_mask

[Markus Armbruster]

Richard Henderson <richard.henderson@linaro.org> writes:

> Do not rely on return value of 0 to indicate error,
> pass along an Error pointer to be set.

Not wrong, but goes against error.h's recommendation

 * - Whenever practical, also return a value that indicates success /
 *   failure.  This can make the error checking more concise, and can
 *   avoid useless error object creation and destruction.  Note that
 *   we still have many functions returning void.  We recommend
 *   • bool-valued functions return true on success / false on failure,
 *   • pointer-valued functions return non-null / null pointer, and
 *   • integer-valued functions return non-negative / negative.

>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/qemu/log.h | 2 +-
>  bsd-user/main.c    | 6 ++++--
>  linux-user/main.c  | 7 +++++--
>  monitor/hmp-cmds.c | 5 +++--
>  softmmu/vl.c       | 7 +++++--
>  util/log.c         | 5 +++--
>  6 files changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/include/qemu/log.h b/include/qemu/log.h
> index df59bfabcd..9b92d2663e 100644
> --- a/include/qemu/log.h
> +++ b/include/qemu/log.h
> @@ -87,7 +87,7 @@ bool qemu_set_log_filename(const char *filename, Error 
> **errp);
>  bool qemu_set_log_filename_flags(const char *name, int flags, Error **errp);
>  void qemu_set_dfilter_ranges(const char *ranges, Error **errp);
>  bool qemu_log_in_addr_range(uint64_t addr);
> -int qemu_str_to_log_mask(const char *str);
> +int qemu_str_to_log_mask(const char *str, Error **errp);
>  
>  /* Print a usage message listing all the valid logging categories
>   * to the specified FILE*.
> diff --git a/bsd-user/main.c b/bsd-user/main.c
> index 703f3e2c41..a981239a0b 100644
> --- a/bsd-user/main.c
> +++ b/bsd-user/main.c
> @@ -411,8 +411,10 @@ int main(int argc, char **argv)
>      {
>          int mask = 0;
>          if (log_mask) {
> -            mask = qemu_str_to_log_mask(log_mask);
> -            if (!mask) {
> +            Error *err = NULL;
> +            mask = qemu_str_to_log_mask(log_mask, &err);
> +            if (err) {
> +                error_report_err(err);
>                  qemu_print_log_usage(stdout);
>                  exit(1);
>              }
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 0c23584a96..d0464736cc 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -264,8 +264,11 @@ static void handle_arg_help(const char *arg)
>  
>  static void handle_arg_log(const char *arg)
>  {
> -    last_log_mask = qemu_str_to_log_mask(arg);
> -    if (!last_log_mask) {
> +    Error *err = NULL;
> +
> +    last_log_mask = qemu_str_to_log_mask(arg, &err);
> +    if (err) {
> +        error_report_err(err);
>          qemu_print_log_usage(stdout);
>          exit(EXIT_FAILURE);
>      }
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 6c559b48c8..c4bd97d467 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -291,8 +291,9 @@ void hmp_log(Monitor *mon, const QDict *qdict)
>      if (!strcmp(items, "none")) {
>          mask = 0;
>      } else {
> -        mask = qemu_str_to_log_mask(items);
> -        if (!mask) {
> +        mask = qemu_str_to_log_mask(items, &err);
> +        if (err) {
> +            error_free(err);
>              hmp_help_cmd(mon, "log");
>              return;
>          }
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 98e071e63b..02193696b9 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2486,8 +2486,11 @@ static void qemu_process_early_options(void)
>      {
>          int mask = 0;
>          if (log_mask) {
> -            mask = qemu_str_to_log_mask(log_mask);
> -            if (!mask) {
> +            Error *err = NULL;
> +
> +            mask = qemu_str_to_log_mask(log_mask, &err);
> +            if (err) {
> +                error_report_err(err);
>                  qemu_print_log_usage(stdout);
>                  exit(1);
>              }
> diff --git a/util/log.c b/util/log.c
> index def88a9402..b5f08db202 100644
> --- a/util/log.c
> +++ b/util/log.c
> @@ -500,8 +500,8 @@ const QEMULogItem qemu_log_items[] = {
>      { 0, NULL, NULL },
>  };
>  
> -/* takes a comma separated list of log masks. Return 0 if error. */
> -int qemu_str_to_log_mask(const char *str)
> +/* Takes a comma separated list of log masks. */
> +int qemu_str_to_log_mask(const char *str, Error **errp)
>  {
>      const QEMULogItem *item;
>      int mask = 0;
> @@ -524,6 +524,7 @@ int qemu_str_to_log_mask(const char *str)
       char **parts = g_strsplit(str, ",", 0);
       char **tmp;

When @parts is null or "", the loop runs zero times, and ...

       for (tmp = parts; tmp && *tmp; tmp++) {
           if (g_str_equal(*tmp, "all")) {
               for (item = qemu_log_items; item->mask != 0; item++) {
                   mask |= item->mask;
               }
   #ifdef CONFIG_TRACE_LOG
           } else if (g_str_has_prefix(*tmp, "trace:") && (*tmp)[6] != '\0') {
               trace_enable_events((*tmp) + 6);
               mask |= LOG_TRACE;
   #endif
           } else {
               for (item = qemu_log_items; item->mask != 0; item++) {
                   if (g_str_equal(*tmp, item->name)) {
>                      goto found;
>                  }
>              }
> +            error_setg(errp, "Invalid -d option \"%s\"", *tmp);
>              goto error;
>          found:
>              mask |= item->mask;
           }
       }

... we end up here with mask zero and no error set.

       g_strfreev(parts);
       return mask;

Before the patch, this counted as error.  Afterwards, it doesn't.
Impact?  Worth mentioning in the commit message?

    error:
       g_strfreev(parts);
       return 0;

Returning -1 here would stick to error.h's recommendation.

   }