Page MenuHomeFreeBSD

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

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

Details

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
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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?

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.Fri, Feb 12, 6:07 PM
This revision was automatically updated to reflect the committed changes.