bin/pkill: Fix {pgrep,pkill}-j_test.sh
ClosedPublic
Actions

Authored by arichardson on Feb 4 2021, 3:54 PM.

Details

Reviewers

jamie
ngie
imp

Commits

rGa24b7455e869: bin/pkill: Fix {pgrep,pkill}-j_test.sh
rG5ff2e55e0071: bin/pkill: Fix {pgrep,pkill}-j_test.sh

Summary

The POSIX sh case statement does not allow for pattern matching using the
regex + qualifier so this case statement never matches. Instead just check
for a string starting with a digit followed by any character.

While touching these files also fix various shellcheck warnings.

Test Plan

kyua -v parallelism=4 test failed before, succeeds now.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 36700
Build 33589: arc lint + arc unit

Event Timeline

arichardson created this revision.Feb 4 2021, 3:54 PM

Herald added a subscriber: imp. · View Herald TranscriptFeb 4 2021, 3:54 PM

arichardson requested review of this revision.Feb 4 2021, 3:54 PM

Harbormaster completed remote builds in B36700: Diff 83352.Feb 4 2021, 3:54 PM

The shellcheck warning fixes are great (I'll happily stamp that diff)!

The other part I'm more concerned about, since the logic isn't equivalent. I would like to know more about what happened and what is triggering this change, since this might introduce other issues.

Oh, I see now based on the description.

Given that they're mutating system state, the easy fix might be to say this isn't concurrency safe, but the better way to handle this might be to use a slightly less deterministic jail name using something like mktemp -u.

In D28480#637440, @ngie wrote:

The shellcheck warning fixes are great (I'll happily stamp that diff)!

The other part I'm more concerned about, since the logic isn't equivalent. I would like to know more about what happened and what is triggering this change, since this might introduce other issues.

What previously happened was that the loop would terminate after 10 iterations and the last value was used (which was generally correct).
I'm not quite sure why this caused things to fail when running tests in parallel, but my guess is that due to delayed scheduling on QEMU, the jails with id $jid had already exited so the pkill -f -j "$jid" fails?

ping? Is this okay to commit?

My quick eyeball check says this looks good and everything that's not simply quote protecting for spaces is the same (module the + -> * change, which is good enough).

This revision is now accepted and ready to land.Feb 12 2021, 6:07 PM

Closed by commit rG5ff2e55e0071: bin/pkill: Fix {pgrep,pkill}-j_test.sh (authored by arichardson). · Explain WhyFeb 13 2021, 2:13 PM

This revision was automatically updated to reflect the committed changes.

arichardson added a commit: rG5ff2e55e0071: bin/pkill: Fix {pgrep,pkill}-j_test.sh.

arichardson added a commit: rGa24b7455e869: bin/pkill: Fix {pgrep,pkill}-j_test.sh.Mar 17 2021, 10:31 AM