| Message ID | 20201124200527.890563-1-jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] usage: add trace2 entry upon warning() | expand |
Jonathan Tan <jonathantanmy@google.com> writes: > Emit a trace2 error event whenever warning() is called, just like when > die(), error(), or usage() is called. > > This helps debugging issues that would trigger warnings but not errors. > In particular, this might have helped debugging an issue I encountered > with commit graphs at $DAYJOB [1]. > > There is a tradeoff between including potentially relevant messages and > cluttering up the trace output produced. I think that warning() messages > should be included in traces, because by its nature, Git is used over > multiple invocations of the Git tool, and a failure (currently traced) > in a Git invocation might be caused by an unexpected interaction in a > previous Git invocation that only has a warning (currently untraced) as > a symptom - as is the case in [1]. > > [1] https://lore.kernel.org/git/20200629220744.1054093-1-jonathantanmy@google.com/ > > Signed-off-by: Jonathan Tan <jonathantanmy@google.com> > --- > Thanks, Junio. That comment looks good. Here is the version with Junio's > suggested comment included, for everyone's reference. Heh, I meant it as more of a #leftoverbit, not directly applicable to this particular patch, but would be a good follow-up topic, as I would have expected that die/warn/error should lose their own comments where they call trace2_cmd_error_va() in the same patch that adds the comment for callers near the function. Let's use v2 if the difference between v2 and v3 is only the addition of the comment before trace2_cmd_error_va() function decl to help the callers.
diff --git a/Documentation/technical/api-trace2.txt b/Documentation/technical/api-trace2.txt index 6b6085585d..c65ffafc48 100644 --- a/Documentation/technical/api-trace2.txt +++ b/Documentation/technical/api-trace2.txt @@ -466,7 +466,7 @@ completed.) `"error"`:: This event is emitted when one of the `error()`, `die()`, - or `usage()` functions are called. + `warning()`, or `usage()` functions are called. + ------------ { diff --git a/trace2.h b/trace2.h index b18bc5529c..2b5c5c4ba0 100644 --- a/trace2.h +++ b/trace2.h @@ -116,6 +116,11 @@ int trace2_cmd_exit_fl(const char *file, int line, int code); * Emit an 'error' event. * * Write an error message to the TRACE2 targets. + * + * Note that the "va_list ap" the caller has is not touched, because + * it is fed to each of the TRACE2 targets, which must use va_copy() + * and use va_end() on the copy. The caller can still use ap it uses + * to call this function and and call va_end() on it when it is done. */ void trace2_cmd_error_va_fl(const char *file, int line, const char *fmt, va_list ap); diff --git a/usage.c b/usage.c index 06665823a2..1868a24f7a 100644 --- a/usage.c +++ b/usage.c @@ -81,6 +81,12 @@ static void error_builtin(const char *err, va_list params) static void warn_builtin(const char *warn, va_list params) { + /* + * We call this trace2 function first and expect it to va_copy 'params' + * before using it (because an 'ap' can only be walked once). + */ + trace2_cmd_error_va(warn, params); + vreportf("warning: ", warn, params); }
Emit a trace2 error event whenever warning() is called, just like when die(), error(), or usage() is called. This helps debugging issues that would trigger warnings but not errors. In particular, this might have helped debugging an issue I encountered with commit graphs at $DAYJOB [1]. There is a tradeoff between including potentially relevant messages and cluttering up the trace output produced. I think that warning() messages should be included in traces, because by its nature, Git is used over multiple invocations of the Git tool, and a failure (currently traced) in a Git invocation might be caused by an unexpected interaction in a previous Git invocation that only has a warning (currently untraced) as a symptom - as is the case in [1]. [1] https://lore.kernel.org/git/20200629220744.1054093-1-jonathantanmy@google.com/ Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- Thanks, Junio. That comment looks good. Here is the version with Junio's suggested comment included, for everyone's reference. --- Documentation/technical/api-trace2.txt | 2 +- trace2.h | 5 +++++ usage.c | 6 ++++++ 3 files changed, 12 insertions(+), 1 deletion(-)