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 <ler...@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#tag_17_04 > > 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 _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
