12:24 p.m.
On Tue, Feb 21, 2023 at 01:53:25PM +0100, Laszlo Ersek wrote:
On 2/16/23 00:03, Eric Blake wrote:
> On Wed, Feb 15, 2023 at 03:11:39PM +0100, Laszlo Ersek wrote:
>> Don't try to test async-signal-safety, but strive to exercise as many as
>> possible paths through nbd_internal_execvpe_init() and
>> nbd_internal_fork_safe_execvpe().
>>
>> Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
>> ---
>> lib/test-fork-safe-execvpe.c | 117 +++++++++
>> lib/Makefile.am | 10 +
>> lib/test-fork-safe-execvpe.sh | 270 ++++++++++++++++++++
>> .gitignore | 1 +
>> 4 files changed, 398 insertions(+)
>>
Skipping ahead...
>> +
>> +# Create a temporary directory and change the working directory to it.
>> +tmpd=$(mktemp -d)
>> +trap 'rm -r -- "$tmpd"' EXIT
>
> Our testsuite has cleanup_fn in functions.sh to make this sort of
> cleanup work more robust (more than just on EXIT).
(1) cleanup_fn is in "tests/functions.sh", but this unit test is in lib/.
Hm... still, I see lots of
. ../tests/functions.sh
lines across various test cases. So, based on that, and for uniformity's
sake, I can use cleanup_fn here, indeed.
Yep, the relative reach out-of-directory to ../tests/functions.sh is
common in both libnbd and nbdkit. The two projects' functions.sh are
not quite identical at the moment, but if we do refactor things into
providing common code in a git submodule, that would be another
candidate for unification efforts. (Not this series, though)
(2) But, what do you mean by "more than just on EXIT"? In
"functions.sh", I see:
trap _run_cleanup_hooks INT QUIT TERM EXIT ERR
From these, INT, TERM, EXIT and ERR should *already* be covered by just
EXIT; ERR in particular because of "set -e" in this script.
Bash happens to trigger EXIT after handling other signals, but this is
not guaranteed by POSIX:
https://www.austingroupbugs.net/view.php?id=621#c5938
If you want cleanup to be uniformly applied, regardless of whether the
script itself exited or an external process injected a signal (other
than the impossible-to-catch SIGKILL), you have to list both EXIT and
the various signals of interest in your trap handler. Hence putting
it in a common library, so we don't have to reimplement it correctly
every time.
And regarding QUIT, I disagree that the cleanup handlers should run upon
QUIT. QUIT ("Terminal quit signal") is a signal specifically designed
for interactive abnormal termination, without cleaning up, in practice
even including a core dump -- that is, for debugging purposes. I do use
Ctrl-\ on occasion, for debugging.
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap01.html...
Any temporary file created by the implementation shall be removed by
the implementation upon a utility's successful exit, exit because of
errors, or before termination by any of the SIGHUP, SIGINT, or
SIGTERM signals, unless specified otherwise by the utility
description.
Receipt of the SIGQUIT signal should generally cause termination
(unless in some debugging mode) that would bypass any attempted
recovery actions.
Interesting take on things. That would be a separate patch to
functions.sh, with this argument as reasoning for why we intentionally
want to change things. It may even have a positive impact to CI
behavior, if CI sends SIGQUIT on a test timeout, if it lets us leave
more artifacts behind (but here, I'm not sure enough about what CIs do
on timeouts to know if this is an actual benefit, or just wishful
thinking on my part).
>> +# Show that, if the last candidate fails execve() with an
error number that
>> +# would not be fatal otherwise, we do get that error number.
>> +run empty:fifo:nxregf:symlink f
>> +execve_fail empty/f,fifo/f,nxregf/f,symlink/f ELOOP
>> +
>> +# Put a single prefix in PATH, such that it lead to a successful execution.
This
>
> leads
I disagree (as a non-native speaker); I meant to express a singular
third person imperative, with subjunctive mood:
https://en.wikipedia.org/wiki/English_subjunctive
Without context, the word 'lead' can be pronounced /LED/ (generally a
noun, meaning the metal) or /LEED/ (generally a verb, meaning to
demonstrate a path). The context here makes it obvious we are using
the verb, so I'm not misunderstanding you as having used the wrong
homophone to /LED/, where that sound would work fine in a purely
past-tense construction: "I did XYZ, such that it led /LED/ to a
successful execution". But as you pointed out (and I agree), you are
not going for past tense, but subjunctive. At which point, we can
compare to a more explicit conditional wording "I will do XYZ, so that
_pronoun verb_ to a successful execution", where we then change the
first clause into a more imperative form forming the desired
antecedant. The alternatives are either singular form ("it leads") or
plural form ("they lead"). Since our antecdant "[you] Put a single
prefix in PATH" is acting as just one imperative instruction (just one
action, singular), rather than a set of instructions (plural), "it
leads" sounds better than "they lead".
Yes, English is weird.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org