[Libguestfs] [libnbd PATCH v4 0/2] lib/utils: introduce async-signal-safe execvpe()

Wed Mar 22 10:42:00 UTC 2023

On 3/22/23 11:24, Daniel P. Berrangé wrote:
> On Tue, Mar 21, 2023 at 03:56:22PM +0100, Laszlo Ersek wrote:
>> On 3/21/23 15:05, Eric Blake wrote:
>>> On Tue, Mar 21, 2023 at 07:04:59AM +0100, Laszlo Ersek wrote:
>>>> On 3/20/23 20:41, Eric Blake wrote:
>>>>> On Sun, Mar 19, 2023 at 10:41:37AM +0100, Laszlo Ersek wrote:
>>>>>> This is version 4 of the following sub-series:
>>>>>>
>>>>>> [libnbd PATCH v3 09/29] lib/utils: introduce async-signal-safe execvpe()
>>>>>> [libnbd PATCH v3 10/29] lib/utils: add unit tests for async-signal-safe execvpe()
>>>>>>
>>> ...
>>>>>
>>>>
>>>> Series merged as commit range 742cbd8c7adc..0b7172b3cffa.
>>>
>>> I see you already fixed one pipeline failure due to some gcc versions
>>> being more picky about function __attribute__ placement than others.
>>> The remaining failures are with alpine Linux, where /bin/expr comes
>>> from busybox instead of coreutils, and has the unfortunate quality of
>>> having its behavior dependent on argv[0].
>>
>> :)
>>
>> I just wanted to announce on-list that I disabled
>> "lib/test-fork-safe-execvpe.sh" on Alpine Linux because of this :)
>>
>>>
>>> Starting from a clean clone, I reproduced it locally with:
>>>
>>> $ git diff
>>> diff --git i/lib/test-fork-safe-execvpe.sh w/lib/test-fork-safe-execvpe.sh
>>> index 838bac9..4b3700c 100755
>>> --- i/lib/test-fork-safe-execvpe.sh
>>> +++ w/lib/test-fork-safe-execvpe.sh
>>> @@ -18,7 +18,7 @@
>>>
>>>  . ../tests/functions.sh
>>>
>>> -set -e
>>> +set -ex
>>>
>>>  # Determine the absolute pathname of the execvpe helper binary. The "realpath"
>>>  # utility is not in POSIX, but Linux, FreeBSD and OpenBSD all have it.
>>> @@ -155,7 +155,7 @@ success()
>>>
>>>  # Create a temporary directory and change the working directory to it.
>>>  tmpd=$(mktemp -d)
>>> -cleanup_fn rm -r -- "$tmpd"
>>> +#cleanup_fn rm -r -- "$tmpd"
>>>  cd "$tmpd"
>>>
>>>  # If the "file" parameter of execvpe() is an empty string, then we must fail --
>>>
>>> $ podman build -f ci/containers/alpine-edge.Dockerfile -t libnbd-alpine-edge
>>> $ podman run -it --rm --userns=keep-id -v .:/repo:z -w /repo libnbd-alpine-edge bash
>>> $ ./configure
>>> $ make check
>>> $ grep tmpd= lib/test-suite.log
>>> + tmpd=/tmp/tmp.EMgKeF
>>> $ /tmp/tmp.EMgKeF/bin/f 1 + 1
>>> f: applet not found
>>> 0b748c9fe495:~$
>>>
>>> So it looks like we need some way to work around busybox' insistance
>>> that argv[0] determines which applet to run.
>>
>> I couldn't come up with a reproducer like yours. I couldn't figure out
>> how to *quickly* get an interactive Alpine Linux environment with the
>> test failing, and I also couldn't figure out how to trigger "verbose"
>> test runs on gitlab, without polluting the master branch with debug
>> patches. (I tried forking the project in my own space on gitlab, and
>> pushed a debug patch with just "set -x" onto a non-master branch *there*
>> -- but CI didn't start in response to that.)
>>
>> So, I only installed a new Alpine Linux virtual machine -- what a pain
>> *that* was --, and investigated what /usr/bin/expr was. When I found it
>> was a symlink to /bin/busybox, I started looking for Alpine Linux
>> specific tweaks that could replace busybox (in this role) with a real
>> binary executable "expr" utility.
>>
>> I was relieved to find the following wiki article:
>>
>>   https://wiki.alpinelinux.org/wiki/How_to_get_regular_stuff_working
>>
>> which promised -- I thought anyways -- a real coreutils package.
>>
>> Imagine my dismay when I found that, after installing coreutils with apk
>> in the Alpine Linux VM, the symlink stayed in place, only its target
>> binary changed from "/bin/busybox" to "coreutils". Well done, Alpine
>> Linux, well done.
>>
>> So, no, this mess (= Alpine Linux) is not salvageable. The
>> "lib/test-fork-safe-execvpe.sh" script depends on "expr" being
>> functional under the name "f". And yes I want "f" to be a
>> single-character filename; otherwise the nice tabular formatting of the
>> script falls apart (or produces overlong lines).
>>
>> As last step, I learned about ci/skipped_tests, and used it to disable
>> the test on alpine linux.
>>
>> The latest pipeline passed:
>> <https://gitlab.com/nbdkit/libnbd/-/pipelines/813280321>.
>>
>> Either way, I wanted to highlight the following commits on the list:
>>
>> 1  b29ff42e5d00 lib: account for realpath deficiency on some platforms
>> 2  65631e5dfff5 lib/utils: try to placate attribute placement restriction from gcc
>> 3  4cae20ccefaf Revert "lib: account for realpath deficiency on some platforms"
>> 4  f5a065aa3a9c ci: skip "lib/test-fork-safe-execvpe.sh" on Alpine Linux
> 
> IIUC, that skips the test in CI, but if a developer or downstream user
> runs 'make check' won't they still execute test-fork-safe-execvpe.sh ?
> IOW, will the next release of libnbd have a broken test when it gets
> imported into the Alpine distro ?
> 
> 
> FWIW, in terms of CI I'd suggest that skipping Alpine is something that
> is an especially bad idea. Alpine is the only distro in CI that uses
> Musl instead of GLibc, and Alpine also does other relatively unusual
> things compared to other Linux distros, such as the use of busybox that
> impacted us here. Thus despite the pain, testing on Alpine has a pretty
> significant value-add for test scenario coverage, compared to testing
> on lots of other Linux distros which are effectively just different
> versions of the same regular software setup.

Right, I'm working on this. Still struggling with the podman setup. The
UID issue has been sorted (with Giuseppe's help -- I had a very old
binary DB laying around).

Now the "podman build -f ci/containers/alpine-edge.Dockerfile -t
libnbd-alpine-edge" command is failing with a different error message --
the download completes, but the internal relinking etc fails due to
permission errors, which I don't understand. I've asked Martin for comments.

Meanwhile, your other email (= just download the prebuilt container from
gitlab) could help!

Thanks!
Laszlo