From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54301) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VTTym-0000Ep-TD for qemu-devel@nongnu.org; Tue, 08 Oct 2013 05:53:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VTTyg-0006Nj-Pv for qemu-devel@nongnu.org; Tue, 08 Oct 2013 05:53:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56489) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VTTyg-0006Nc-Ha for qemu-devel@nongnu.org; Tue, 08 Oct 2013 05:53:18 -0400 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r989rHRD015029 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 8 Oct 2013 05:53:17 -0400 Message-ID: <5253D60A.2060305@redhat.com> Date: Tue, 08 Oct 2013 11:53:14 +0200 From: Max Reitz MIME-Version: 1.0 References: <1378734793-17818-1-git-send-email-mreitz@redhat.com> In-Reply-To: <1378734793-17818-1-git-send-email-mreitz@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC] qapi/error: Optional error propagation backtrace List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi 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 Ping =E2=80=93 any comments on this? > --- > Since this obviously breaks existing tests (and cannot really be used f= or > 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 rig= ht > 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=3D"" > gtkabi=3D"2.0" > tpm=3D"no" > libssh2=3D"" > +error_backtrace=3D"no" > =20 > # parse CC options first > for opt do > @@ -949,6 +950,10 @@ for opt do > ;; > --enable-libssh2) libssh2=3D"yes" > ;; > + --enable-error-bt) error_backtrace=3D"yes" > + ;; > + --disable-error-bt) error_backtrace=3D"no" > + ;; > *) echo "ERROR: unknown option $opt"; show_help=3D"yes" > ;; > esac > @@ -1168,6 +1173,8 @@ echo " --gcov=3DGCOV 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 i= s 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" > =20 > @@ -4044,6 +4052,10 @@ if test "$libssh2" =3D "yes" ; then > echo "CONFIG_LIBSSH2=3Dy" >> $config_host_mak > fi > =20 > +if test "$error_backtrace" =3D "yes" ; then > + echo "CONFIG_ERROR_BACKTRACE=3Dy" >> $config_host_mak > +fi > + > if test "$virtio_blk_data_plane" =3D "yes" ; then > echo 'CONFIG_VIRTIO_BLK_DATA_PLANE=3D$(CONFIG_VIRTIO)' >> $config_h= ost_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 i= nformation > + * 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__) > =20 > /** > * 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_clas= s, > + const char *fmt, ...) GCC_FMT_ATTR(7, 8); > + > +#define error_set_errno(...) \ > + error_set_errno_bt(__FILE__, __func__, __LINE__, __VA_ARGS__) > =20 > /** > * Same as error_set(), but sets a generic error > @@ -47,7 +59,11 @@ void error_set_errno(Error **err, int os_error, Erro= rClass err_class, const char > /** > * Helper for open() errors > */ > -void error_setg_file_open(Error **errp, int os_errno, const char *file= name); > +void error_setg_file_open_bt(const char *file, const char *func, int l= ine, > + Error **errp, int os_errno, const char *f= ilename); > + > +#define error_setg_file_open(...) \ > + error_setg_file_open_bt(__FILE__, __func__, __LINE__, __VA_ARGS__) > =20 > /** > * Returns true if an indirect pointer to an error is pointing to a v= alid > @@ -71,11 +87,17 @@ Error *error_copy(const Error *err); > const char *error_get_pretty(Error *err); > =20 > /** > - * Propagate an error to an indirect pointer to an error. This functi= on will > - * always transfer ownership of the error reference and handles the ca= se where > - * dst_err is NULL correctly. Errors after the first are discarded. > + * Propagate an error to an indirect pointer to an error, appending th= e given > + * file/function/line information to the message (as a backtrace) if > + * CONFIG_ERROR_BACKTRACE is defined. This function will always trans= fer > + * 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__) > =20 > /** > * 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; > }; > =20 > -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; > =20 > if (errp =3D=3D NULL) { > @@ -36,15 +38,22 @@ void error_set(Error **errp, ErrorClass err_class, = const char *fmt, ...) > err =3D g_malloc0(sizeof(*err)); > =20 > va_start(ap, fmt); > - err->msg =3D g_strdup_vprintf(fmt, ap); > + msg1 =3D g_strdup_vprintf(fmt, ap); > +#ifdef CONFIG_ERROR_BACKTRACE > + err->msg =3D g_strdup_printf("%s:%i (in %s): %s", file, line, func= , msg1); > + g_free(msg1); > +#else > + err->msg =3D msg1; > +#endif > va_end(ap); > err->err_class =3D err_class; > =20 > *errp =3D err; > } > =20 > -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_cla= ss, > + const char *fmt, ...) > { > Error *err; > char *msg1; > @@ -59,21 +68,34 @@ void error_set_errno(Error **errp, int os_errno, Er= rorClass err_class, > =20 > va_start(ap, fmt); > msg1 =3D g_strdup_vprintf(fmt, ap); > +#ifdef CONFIG_ERROR_BACKTRACE > + if (os_errno !=3D 0) { > + err->msg =3D g_strdup_printf("%s:%i (in %s): %s: %s", file, li= ne, func, > + msg1, strerror(os_errno)); > + } else { > + err->msg =3D g_strdup_printf("%s:%i (in %s): %s", file, line, = func, msg1); > + } > + g_free(msg1); > +#else > if (os_errno !=3D 0) { > err->msg =3D g_strdup_printf("%s: %s", msg1, strerror(os_errn= o)); > g_free(msg1); > } else { > err->msg =3D msg1; > } > +#endif > va_end(ap); > err->err_class =3D err_class; > =20 > *errp =3D err; > } > =20 > -void error_setg_file_open(Error **errp, int os_errno, const char *file= name) > +void error_setg_file_open_bt(const char *file, const char *func, int l= ine, > + Error **errp, int os_errno, const char *f= ilename) > { > - 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); > } > =20 > Error *error_copy(const Error *err) > @@ -110,11 +132,26 @@ void error_free(Error *err) > } > } > =20 > -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 =3D local_err; > - } else if (local_err) { > + if (local_err) { > +#ifdef CONFIG_ERROR_BACKTRACE > + if (dst_err && !*dst_err) { > + Error *err =3D g_malloc0(sizeof(*err)); > + err->msg =3D g_strdup_printf("%s\n from %s:%i (in %s)", > + local_err->msg, file, line, fun= c); > + err->err_class =3D local_err->err_class; > + *dst_err =3D err; > + } > error_free(local_err); > +#else > + if (dst_err && !*dst_err) { > + *dst_err =3D local_err; > + } else { > + error_free(local_err); > + } > +#endif > } > }