Re: [PATCH] trace2: intercept all common signals
- From
Emily Shaffer <nasamuffin@google.com>
- Date
- May 10, 2024, 17:57 UTC
- Message-ID
- <CAJoAoZkkDxhJtXfLx-4++9JuaSr5xJ4Da4_ijVZP05DkLhHDcQ@mail.gmail.com>
- In-Reply-To
- <20240510172243.3529851-1-emilyshaffer@google.com>
On Fri, May 10, 2024 at 10:22 AM Emily Shaffer <emilyshaffer@google.com> wrote:
Show 23 quoted lines
> > From: Emily Shaffer <nasamuffin@google.com> > > We already use trace2 to find out about unexpected pipe breakages, which > is nice for detecting bugs or system problems, by adding a handler for > SIGPIPE which simply writes a trace2 line. However, there are a handful > of other common signals which we might want to snoop on: > > - SIGINT, SIGTERM, or SIGQUIT, when a user manually cancels a command in > frustration or mistake (via Ctrl-C, Ctrl-D, or `kill`) > - SIGHUP, when the network closes unexpectedly (indicating there may be > a problem to solve) > > There are lots more signals which we might find useful later, but at > least let's teach trace2 to report these egregious ones. Conveniently, > they're also already covered by the `_common` variants in sigchain.[ch]. > > Sigchain itself is already tested via helper/test-sigchain.c, and trace2 > is tested in a number of places - let's also add tests demonstrating > that sigchain + trace2 works correctly. > > Signed-off-by: Emily Shaffer <nasamuffin@google.com> > ---
Missed including the CI results. They're passing[1] with the exception of the osx-gcc run, which seems to also be failing on the latest 'master'[2] and looks to be failing in setup rather than in test run.
1: https://github.com/nasamuffin/git/actions/runs/9035666915 2: https://github.com/nasamuffin/git/actions/runs/9036080205/job/24832209129
Show 89 quoted lines
> t/helper/test-trace2.c | 17 +++++++++++++++++
> t/t0210-trace2-normal.sh | 22 ++++++++++++++++++++++
> trace2.c | 2 +-
> 3 files changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/t/helper/test-trace2.c b/t/helper/test-trace2.c
> index 1adac29a57..8970956ea8 100644
> --- a/t/helper/test-trace2.c
> +++ b/t/helper/test-trace2.c
> @@ -231,6 +231,22 @@ static int ut_010bug_BUG(int argc UNUSED, const char **argv UNUSED)
> BUG("a %s message", "BUG");
> }
>
> +static int ut_011signal(int argc, const char **argv)
> +{
> + const char *usage_error = "expect <bool common>";
> + int common = 0;
> +
> + if (argc != 1 || get_i(&common, argv[0]))
> + die("%s", usage_error);
> +
> + /*
> + * There is no strong reason SIGSEGV is ignored by trace2 - it's just
> + * not included by sigchain_push_common().
> + */
> + raise(common ? SIGINT : SIGSEGV);
> + return 0; /*unreachable*/
> +}
> +
> /*
> * Single-threaded timer test. Create several intervals using the
> * TEST1 timer. The test script can verify that an aggregate Trace2
> @@ -482,6 +498,7 @@ static struct unit_test ut_table[] = {
> { ut_008bug, "008bug", "" },
> { ut_009bug_BUG, "009bug_BUG","" },
> { ut_010bug_BUG, "010bug_BUG","" },
> + { ut_011signal, "011signal","" },
>
> { ut_100timer, "100timer", "<count> <ms_delay>" },
> { ut_101timer, "101timer", "<count> <ms_delay> <threads>" },
> diff --git a/t/t0210-trace2-normal.sh b/t/t0210-trace2-normal.sh
> index c312657a12..c34ccc518c 100755
> --- a/t/t0210-trace2-normal.sh
> +++ b/t/t0210-trace2-normal.sh
> @@ -244,6 +244,28 @@ test_expect_success 'bug messages followed by BUG() are written to trace2' '
> test_cmp expect actual
> '
>
> +test_expect_success 'trace2 reports common signals' '
> + test_when_finished "rm trace.normal actual" &&
> +
> + # signals are fatal, so expect this to fail
> + ! env GIT_TRACE2="$(pwd)/trace.normal" test-tool trace2 011signal 1 &&
> +
> + perl "$TEST_DIRECTORY/t0210/scrub_normal.perl" <trace.normal >actual &&
> +
> + test_grep "signal elapsed:" actual
> +'
> +
> +test_expect_success 'trace2 ignores uncommon signals' '
> + test_when_finished "rm trace.normal actual" &&
> +
> + # signals are fatal, so expect this to fail
> + ! env GIT_TRACE2="$(pwd)/trace.normal" test-tool trace2 011signal 0 &&
> +
> + perl "$TEST_DIRECTORY/t0210/scrub_normal.perl" <trace.normal >actual &&
> +
> + ! test_grep "signal elapsed:" actual
> +'
> +
> sane_unset GIT_TRACE2_BRIEF
>
> # Now test without environment variables and get all Trace2 settings
> diff --git a/trace2.c b/trace2.c
> index f894532d05..3692010f5d 100644
> --- a/trace2.c
> +++ b/trace2.c
> @@ -231,7 +231,7 @@ void trace2_initialize_fl(const char *file, int line)
> tr2_sid_get();
>
> atexit(tr2main_atexit_handler);
> - sigchain_push(SIGPIPE, tr2main_signal_handler);
> + sigchain_push_common(tr2main_signal_handler);
> tr2tls_init();
>
> /*
> --
> 2.45.0.118.g7fe29c98d7-goog
>