Reviewed-by: Willem de Bruijn <[email protected]>
Jakub Kicinski wrote:
> On Sat, 26 Apr 2025 11:15:34 -0400 Willem de Bruijn wrote:
> > > @@ -193,6 +198,19 @@ KSFT_DISRUPTIVE = True
> > > return env
> > >
> > >
> > > +term_cnt = 0
> > > +
> >
> > A bit ugly to initialize this here. Also, it already is initialized
> > below.
>
> We need a global so that the signal handler can access it.
> Python doesn't have syntax to define a variable without a value.
> Or do you suggest term_cnt = None ?
I meant that the "global term_cnt" in ksft_run below already creates
the global var, and is guaranteed to do so before _ksft_intr, so no
need to also define it outside a function.
Obviously not very important, don't mean to ask for a respin. LGTM.
> The whole term_cnt dance is super ugly, couldn't think of a cleaner way.
> It's really annoying that ksft infra sends 2 terminating signals one
> immediately after the other :|
>
> > > +def _ksft_intr(signum, frame):
> > > + # ksft runner.sh sends 2 SIGTERMs in a row on a timeout
> > > + # if we don't ignore the second one it will stop us from handling
> > > cleanup
> > > + global term_cnt
> > > + term_cnt += 1
> > > + if term_cnt == 1:
> > > + raise KsftTerminate()
> > > + else:
> > > + ksft_pr(f"Ignoring SIGTERM (cnt: {term_cnt}), already
> > > exiting...")
> > > +
> > > +
> > > def ksft_run(cases=None, globs=None, case_pfx=None, args=()):
> > > cases = cases or []
> > >
> > > @@ -205,6 +223,10 @@ KSFT_DISRUPTIVE = True
> > > cases.append(value)
> > > break
> > >
> > > + global term_cnt
> > > + term_cnt = 0
> > > + prev_sigterm = signal.signal(signal.SIGTERM, _ksft_intr)
> > > +
> > > totals = {"pass": 0, "fail": 0, "skip": 0, "xfail": 0}
> > >
> > > print("TAP version 13")
> > > @@ -229,11 +251,12 @@ KSFT_DISRUPTIVE = True
> > > cnt_key = 'xfail'
> > > except BaseException as e:
> > > stop |= isinstance(e, KeyboardInterrupt)
> > > + stop |= isinstance(e, KsftTerminate)
> > > tb = traceback.format_exc()
> > > for line in tb.strip().split('\n'):
> > > ksft_pr("Exception|", line)
> > > if stop:
> > > - ksft_pr("Stopping tests due to KeyboardInterrupt.")
> > > + ksft_pr(f"Stopping tests due to {type(e).__name__}.")
> > > KSFT_RESULT = False
> > > cnt_key = 'fail'
> > >
> > > @@ -248,6 +271,8 @@ KSFT_DISRUPTIVE = True
> > > if stop:
> > > break
> > >
> > > + signal.signal(signal.SIGTERM, prev_sigterm)
> > > +
> >
> > Why is prev_sigterm saved and reassigned as handler here?
>
> Because we ignore all signals when cnt > 2 I didn't want to keep our
> handler installed. Just in case something after ksft_run() hangs.
> It should be equivalent to
>
> signal.signal(signal.SIGTERM, signal.SIG_DLF)
>
> if the prev is of concern. Then again keeping prev doesn't change #LOC
Oh I see. Ok.
