Re: [Qemu-devel] [RFC] qapi/error: Optional error propagation backtrace

[Max Reitz <mreitz@redhat.com>]

On 2013-09-09 15:53, Max Reitz wrote:
> Add a configure switch which enables an error propagation backtrace.
> This results in the error_set function prepending every message by the
> source file name, function and line in which it was called, as well as
> error_propagate appending this information to the propagated message,
> resulting in a backtrace.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Ping – any comments on this?

> ---
> Since this obviously breaks existing tests (and cannot really be used for
> new tests since it includes a line number, which is a rather volatile
> output) and is generally not very useful to normal users, the switch is
> disabled by default. This functionality is intended for developers
> tracking down error paths.
>
> Since I do not know whether I am the only one considering it useful
> in the first place, this is just an RFC for now.
>
> Furthermore, I am not sure whether a configure switch is really the right
> way to implement this.
> ---
>   configure            | 12 +++++++++++
>   include/qapi/error.h | 40 +++++++++++++++++++++++++++---------
>   util/error.c         | 57 +++++++++++++++++++++++++++++++++++++++++++---------
>   3 files changed, 90 insertions(+), 19 deletions(-)
>
> diff --git a/configure b/configure
> index e989609..4e43f7b 100755
> --- a/configure
> +++ b/configure
> @@ -243,6 +243,7 @@ gtk=""
>   gtkabi="2.0"
>   tpm="no"
>   libssh2=""
> +error_backtrace="no"
>   
>   # parse CC options first
>   for opt do
> @@ -949,6 +950,10 @@ for opt do
>     ;;
>     --enable-libssh2) libssh2="yes"
>     ;;
> +  --enable-error-bt) error_backtrace="yes"
> +  ;;
> +  --disable-error-bt) error_backtrace="no"
> +  ;;
>     *) echo "ERROR: unknown option $opt"; show_help="yes"
>     ;;
>     esac
> @@ -1168,6 +1173,8 @@ echo "  --gcov=GCOV              use specified gcov [$gcov_tool]"
>   echo "  --enable-tpm             enable TPM support"
>   echo "  --disable-libssh2        disable ssh block device support"
>   echo "  --enable-libssh2         enable ssh block device support"
> +echo "  --disable-error-bt       disable backtraces on internal errors"
> +echo "  --enable-error-bt        enable backtraces on internal errors"
>   echo ""
>   echo "NOTE: The object files are built at the place where configure is launched"
>   exit 1
> @@ -3650,6 +3657,7 @@ echo "gcov              $gcov_tool"
>   echo "gcov enabled      $gcov"
>   echo "TPM support       $tpm"
>   echo "libssh2 support   $libssh2"
> +echo "error backtraces  $error_backtrace"
>   echo "TPM passthrough   $tpm_passthrough"
>   echo "QOM debugging     $qom_cast_debug"
>   
> @@ -4044,6 +4052,10 @@ if test "$libssh2" = "yes" ; then
>     echo "CONFIG_LIBSSH2=y" >> $config_host_mak
>   fi
>   
> +if test "$error_backtrace" = "yes" ; then
> +  echo "CONFIG_ERROR_BACKTRACE=y" >> $config_host_mak
> +fi
> +
>   if test "$virtio_blk_data_plane" = "yes" ; then
>     echo 'CONFIG_VIRTIO_BLK_DATA_PLANE=$(CONFIG_VIRTIO)' >> $config_host_mak
>   fi
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index ffd1cea..d3cfe35 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -25,16 +25,28 @@ typedef struct Error Error;
>   /**
>    * Set an indirect pointer to an error given a ErrorClass value and a
>    * printf-style human message.  This function is not meant to be used outside
> - * of QEMU.
> + * of QEMU.  The message will be prepended by the file/function/line information
> + * if CONFIG_ERROR_BACKTRACE is defined.
>    */
> -void error_set(Error **err, ErrorClass err_class, const char *fmt, ...) GCC_FMT_ATTR(3, 4);
> +void error_set_bt(const char *file, const char *func, int line,
> +                  Error **err, ErrorClass err_class, const char *fmt, ...)
> +        GCC_FMT_ATTR(6, 7);
> +
> +#define error_set(...) \
> +    error_set_bt(__FILE__, __func__, __LINE__, __VA_ARGS__)
>   
>   /**
>    * Set an indirect pointer to an error given a ErrorClass value and a
>    * printf-style human message, followed by a strerror() string if
> - * @os_error is not zero.
> + * @os_error is not zero.  The message will be prepended by the
> + * file/function/line information if CONFIG_ERROR_BACKTRACE is defined.
>    */
> -void error_set_errno(Error **err, int os_error, ErrorClass err_class, const char *fmt, ...) GCC_FMT_ATTR(4, 5);
> +void error_set_errno_bt(const char *file, const char *func, int line,
> +                        Error **err, int os_error, ErrorClass err_class,
> +                        const char *fmt, ...) GCC_FMT_ATTR(7, 8);
> +
> +#define error_set_errno(...) \
> +    error_set_errno_bt(__FILE__, __func__, __LINE__, __VA_ARGS__)
>   
>   /**
>    * Same as error_set(), but sets a generic error
> @@ -47,7 +59,11 @@ void error_set_errno(Error **err, int os_error, ErrorClass err_class, const char
>   /**
>    * Helper for open() errors
>    */
> -void error_setg_file_open(Error **errp, int os_errno, const char *filename);
> +void error_setg_file_open_bt(const char *file, const char *func, int line,
> +                             Error **errp, int os_errno, const char *filename);
> +
> +#define error_setg_file_open(...) \
> +    error_setg_file_open_bt(__FILE__, __func__, __LINE__, __VA_ARGS__)
>   
>   /**
>    * Returns true if an indirect pointer to an error is pointing to a valid
> @@ -71,11 +87,17 @@ Error *error_copy(const Error *err);
>   const char *error_get_pretty(Error *err);
>   
>   /**
> - * Propagate an error to an indirect pointer to an error.  This function will
> - * always transfer ownership of the error reference and handles the case where
> - * dst_err is NULL correctly.  Errors after the first are discarded.
> + * Propagate an error to an indirect pointer to an error, appending the given
> + * file/function/line information to the message (as a backtrace) if
> + * CONFIG_ERROR_BACKTRACE is defined.  This function will always transfer
> + * ownership of the error reference and handles the case where dst_err is NULL
> + * correctly.  Errors after the first are discarded.
>    */
> -void error_propagate(Error **dst_err, Error *local_err);
> +void error_propagate_bt(const char *file, const char *func, int line,
> +                       Error **dest_err, Error *local_err);
> +
> +#define error_propagate(...) \
> +    error_propagate_bt(__FILE__, __func__, __LINE__, __VA_ARGS__)
>   
>   /**
>    * Free an error object.
> diff --git a/util/error.c b/util/error.c
> index 53b0435..a07f4c7 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -23,9 +23,11 @@ struct Error
>       ErrorClass err_class;
>   };
>   
> -void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
> +void error_set_bt(const char *file, const char *func, int line,
> +                  Error **errp, ErrorClass err_class, const char *fmt, ...)
>   {
>       Error *err;
> +    char *msg1;
>       va_list ap;
>   
>       if (errp == NULL) {
> @@ -36,15 +38,22 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
>       err = g_malloc0(sizeof(*err));
>   
>       va_start(ap, fmt);
> -    err->msg = g_strdup_vprintf(fmt, ap);
> +    msg1 = g_strdup_vprintf(fmt, ap);
> +#ifdef CONFIG_ERROR_BACKTRACE
> +    err->msg = g_strdup_printf("%s:%i (in %s): %s", file, line, func, msg1);
> +    g_free(msg1);
> +#else
> +    err->msg = msg1;
> +#endif
>       va_end(ap);
>       err->err_class = err_class;
>   
>       *errp = err;
>   }
>   
> -void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
> -                     const char *fmt, ...)
> +void error_set_errno_bt(const char *file, const char *func, int line,
> +                        Error **errp, int os_errno, ErrorClass err_class,
> +                        const char *fmt, ...)
>   {
>       Error *err;
>       char *msg1;
> @@ -59,21 +68,34 @@ void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
>   
>       va_start(ap, fmt);
>       msg1 = g_strdup_vprintf(fmt, ap);
> +#ifdef CONFIG_ERROR_BACKTRACE
> +    if (os_errno != 0) {
> +        err->msg = g_strdup_printf("%s:%i (in %s): %s: %s", file, line, func,
> +                                   msg1, strerror(os_errno));
> +    } else {
> +        err->msg = g_strdup_printf("%s:%i (in %s): %s", file, line, func, msg1);
> +    }
> +    g_free(msg1);
> +#else
>       if (os_errno != 0) {
>           err->msg = g_strdup_printf("%s: %s", msg1, strerror(os_errno));
>           g_free(msg1);
>       } else {
>           err->msg = msg1;
>       }
> +#endif
>       va_end(ap);
>       err->err_class = err_class;
>   
>       *errp = err;
>   }
>   
> -void error_setg_file_open(Error **errp, int os_errno, const char *filename)
> +void error_setg_file_open_bt(const char *file, const char *func, int line,
> +                             Error **errp, int os_errno, const char *filename)
>   {
> -    error_setg_errno(errp, os_errno, "Could not open '%s'", filename);
> +    error_set_errno_bt(file, func, line, errp, os_errno,
> +                       ERROR_CLASS_GENERIC_ERROR, "Could not open '%s'",
> +                       filename);
>   }
>   
>   Error *error_copy(const Error *err)
> @@ -110,11 +132,26 @@ void error_free(Error *err)
>       }
>   }
>   
> -void error_propagate(Error **dst_err, Error *local_err)
> +
> +void error_propagate_bt(const char *file, const char *func, int line,
> +                        Error **dst_err, Error *local_err)
>   {
> -    if (dst_err && !*dst_err) {
> -        *dst_err = local_err;
> -    } else if (local_err) {
> +    if (local_err) {
> +#ifdef CONFIG_ERROR_BACKTRACE
> +        if (dst_err && !*dst_err) {
> +            Error *err = g_malloc0(sizeof(*err));
> +            err->msg = g_strdup_printf("%s\n    from %s:%i (in %s)",
> +                                       local_err->msg, file, line, func);
> +            err->err_class = local_err->err_class;
> +            *dst_err = err;
> +        }
>           error_free(local_err);
> +#else
> +        if (dst_err && !*dst_err) {
> +            *dst_err = local_err;
> +        } else {
> +            error_free(local_err);
> +        }
> +#endif
>       }
>   }