Re: [Libguestfs] [libnbd PATCH v3 10/29] lib/utils: add unit tests for async-signal-safe execvpe()

On 2/16/23 00:33, Eric Blake wrote: > On Wed, Feb 15, 2023 at 05:03:48PM -0600, Eric Blake wrote: >>> + >>> +# Create subdirectories for triggering non-fatal internal error conditions of >>> +# execvpe(). (Almost) every subdirectory will contain one entry, called "f". >>> +# >>> +# Create a directory that's empty. >>> +mkdir empty >>> + >>> +# Create a directory with a named pipe (FIFO) in it. >>> +mkdir fifo >>> +mkfifo fifo/f >>> + >>> +# Create a directory with a non-executable file in it. >>> +mkdir nxregf >>> +touch nxregf/f >>> + >>> +# Create a symlink loop. >>> +ln -s symlink symlink > > Another interesting thing you might want to add to the test: > > mkdir -p subdir/f > > then show that PATH=...:subdir:... does not get tripped up by > subdir/f/ having the execute bit set (aka search bit since it's a > directory) but not being an executable. > Hmm. There could be two spots to test this (because what we're triggering here is EACCES). One, *between* the following two tests: run "" fifo/f; execve_fail fifo/f EACCES run "" nxregf/f; execve_fail nxregf/f EACCES because "fifo/f" is effectively the same test as "subdir/f" -- not a regular file in the first place. Two, inside: run empty:fifo:nxregf:symlink f execve_fail empty/f,fifo/f,nxregf/f,symlink/f ELOOP (again inserted between the fifo and the nxregf cases). Now, while developing the test cases, I did (somewhat unwittingly) end up testing what you propose, but then I decided that trying to execute a directory was not much different from trying to execute a fifo -- the x file mode bit shouldn't matter because the file type wouldn't be right in the first place (the x bit should only matter on regular files).

So... Do you think "subdir/f" improves code coverage? If so, do you agree that I should extend one (or both!) of the above two tests -- or else, should I add something entirely new?