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

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].

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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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

Thanks for investigating!
Laszlo

On Tue, Mar 21, 2023 at 09:05:12AM -0500, Eric Blake
wrote:
> 
> $ podman build -f ci/containers/alpine-edge.Dockerfile -t
libnbd-alpine-edge
You usually shouldn't need this step when reproducing a problem
on CI. Instead just use the container that gitlab has published
under the project's registry

For any job eg

https://gitlab.com/nbdkit/libnbd/-/jobs/3973002673

you'll see at about line 7/8 in the log a line like

 Using Docker executor with image
registry.gitlab.com/nbdkit/libnbd/ci-alpine-edge:latest

That image URL is accessible remotely on your local dev machine eg

  podman run -it registry.gitlab.com/nbdkit/libnbd/ci-alpine-edge:latest

You should only need to do 'podman build' if you're debugging a
suspected
problem with the container image itself.
> $ 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.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|