| Message ID | patch-1.3-2e4665b625b-20210413T090603Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c00c7382ddc1e3f2d773b14366023abf53ecce2c |
| Headers | show |
| Series | trace2 docs: note that BUG() sends an "error" event | expand |
On Tue, Apr 13, 2021 at 11:08:19AM +0200, Ævar Arnfjörð Bjarmason wrote: > In ee4512ed481 (trace2: create new combined trace facility, > 2019-02-22) we started with two copies of this comment, > 0ee10fd1296 (usage: add trace2 entry upon warning(), 2020-11-23) added > a third. Let's instead add an earlier comment that applies to all > these mostly-the-same functions. I'm sometimes wary of this aggregating comments like this, because somebody who is just reading the third function may not think to look further up to find the comment. But this comment in particular does not seem dangerous if somebody misses it (unlike comments that are warning people about bad things happening). > + */ > static NORETURN void die_builtin(const char *err, 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(err, params); > > vreportf("fatal: ", err, params); TBH, I am not sure it adds all that much value in the first place. It is only telling the reader that the code is not broken. But I kind of wonder if it should simply be doing the defensive thing anyway: va_list cp; va_copy(cp, params); trace2_cmd_error_va(err, params); va_end(cp); We are relying on a subtle contract with the trace2 code, and there is no compile-time check that it will be upheld (and indeed, on many platforms we might not even notice if it isn't, depending on how va_list is implemented). Looking at the trace2 code, we do not even enforce this centrally. It is up to each target to remember to va_copy()! Anyway, that is somewhat outside the scope of your series (though I would not be sad to see this comment go away entirely in favor of the more defensive code above). -Peff
diff --git a/usage.c b/usage.c index 1b206de36d6..c7d233b0de9 100644 --- a/usage.c +++ b/usage.c @@ -55,12 +55,13 @@ static NORETURN void usage_builtin(const char *err, va_list params) exit(129); } +/* + * We call trace2_cmd_error_va() in the below functions first and + * expect it to va_copy 'params' before using it (because an 'ap' can + * only be walked once). + */ static NORETURN void die_builtin(const char *err, 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(err, params); vreportf("fatal: ", err, params); @@ -70,10 +71,6 @@ static NORETURN void die_builtin(const char *err, va_list params) static void error_builtin(const char *err, 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(err, params); vreportf("error: ", err, params); @@ -81,10 +78,6 @@ 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);
In ee4512ed481 (trace2: create new combined trace facility, 2019-02-22) we started with two copies of this comment, 0ee10fd1296 (usage: add trace2 entry upon warning(), 2020-11-23) added a third. Let's instead add an earlier comment that applies to all these mostly-the-same functions. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- usage.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)