Laszlo Ersek
2023-Mar-21 14:56 UTC
[Libguestfs] [libnbd PATCH v4 0/2] lib/utils: introduce async-signal-safe execvpe()
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
Eric Blake
2023-Mar-21 17:28 UTC
[Libguestfs] [libnbd PATCH v4 0/2] lib/utils: introduce async-signal-safe execvpe()
On Tue, Mar 21, 2023 at 03:56:22PM +0100, Laszlo Ersek wrote:> > $ 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.)ci/README.rst is informative on that regards; I'm impressed at what lcitool (from the libvirt-ci) project gives us for local reproducibility of repeating various CI build failures.> > 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.Yeah, I remember the debates back when GNU coreutils was offered patches to choose to build a multiname binary - it fits with Alpine's goals and matches busybox's minimalist footprint, but it violates GNU Coding Standards (which state that in general, an application's behavior should not depend on argv[0], precisely so that renaming it doesn't change its behavior).> > 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.That works, but is not always my favorite: it fixes CI, but does not help any developer on a similar platform. I probably would have tried this in the test itself (untested here): cp /bin/expr f cleanup_fn rm f requires ./f 1 + 1 # We insist on expr behavior independent of argv[0] which will then skip the test (exit status 77) if a renamed /bin/expr doesn't behave the same as the original. But since we haven't heard complaints from anyone using Alpine as their primary development box (or any other non-Alpine but otherwise-busybox setup), we can make that change when such a complaint arises, rather than adding even more churn to your latest series of patches. Another thought - instead of skipping the test when /bin/expr is underwhelming when renamed, we could rewrite the test to avoid that dependency altogether. After all, we are building a test-fork-safe-execvpe binary that does exactly what we say it should; we could hack up test-fork-safe-execvpe.c to depend on argv[0] (I know - using the very thing I don't like about busybox) so that when run by its usual name it performs the unit test, but when run by the basename of f, it does just enough for us to prove that we indeed executed our desired binary (whether that be emulating a subset of /bin/expr, or something else). But again, that's more effort to code up than just skipping the test on a platform that's less likely to be a primary development station in the first place.> > 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 platformsI didn't even pay attention to this one before you pointed it out; but it is indeed a bug in busybox now that POSIX is moving towards standardizing realpath, so I've filed it: https://bugs.busybox.net/show_bug.cgi?id=15466> 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 >-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Daniel P. Berrangé
2023-Mar-22 10:24 UTC
[Libguestfs] [libnbd PATCH v4 0/2] lib/utils: introduce async-signal-safe execvpe()
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 LinuxIIUC, 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. 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 :|
Maybe Matching Threads
- [libnbd PATCH v4 0/2] lib/utils: introduce async-signal-safe execvpe()
- [libnbd PATCH v4 0/2] lib/utils: introduce async-signal-safe execvpe()
- [libnbd PATCH v4 0/2] lib/utils: introduce async-signal-safe execvpe()
- [libnbd PATCH v4 0/2] lib/utils: introduce async-signal-safe execvpe()
- [libnbd PATCH v4 0/2] lib/utils: introduce async-signal-safe execvpe()