Re: [PATCH 1/6] tests/qemu-iotests: Improve the check for GNU sed

[Thomas Huth]

On 15/02/2022 14.51, Daniel P. Berrangé wrote:

On Tue, Feb 15, 2022 at 02:28:24PM +0100, Thomas Huth wrote:

On 11/02/2022 17.48, Thomas Huth wrote:

On 11/02/2022 17.14, Eric Blake wrote:

On Tue, Feb 08, 2022 at 03:52:19PM +0100, Thomas Huth wrote:

The current code with $SED has been introduced almost three years
ago already...


    Can’t we just do `alias sed=gsed`?


Maybe ... but let's ask Philippe and Kevin first, who Signed-off
commit bde36af1ab4f476 that introduced the current way with $SED:
What's your opinion about this?


This commit was to have check-block working on the OpenBSD VM image.


Sure. The question was whether using an alias as suggested by Hanna would be
nicer instead of using $SED ?


Scripting with aliases becomes a nightmare to debug, since it is
relatively uncommon.  In particular, in bash, you have to explicitly
opt in to using aliases (contrary to POSIX sh where aliases are
available to scripts at startup).


shopt -s expand_aliases
... as I just learnt the hard way ;-)


Using $SED everywhere may require
more hunting, but it is more obvious when reading a test that "oh
yeah, I might be using extensions that the default 'sed' can't
support" than a script that blindly uses 'sed' and depends on it
aliasing to a more-capable sed at a distance.

The other question is how many GNU sed features are we actually
depending on?  Which tests break if we have BSD sed or busybox sed?
Can we rewrite those sed scripts to avoid GNU extensions?  But
auditing for s/sed/$SED/ seems easier than auditing for which
non-portable sed extensions we depend on.


The most obvious part are the filter functions in common.filter - we're
using "-r" here that is not part of the POSIX sed as far as I can see.


Now that I stepped through the list, the other major part that is failing on
non-GNU seds are the statements that use "\r" or "\n" or "\e" to replace
special characters. That seems to be a non-POSIX extension, too.

But for running with Alpine, there is also the additional problems that the
libc uses slightly different error strings, e.g. "I/O error" instead of
"Input/output error", see e.g.:

  https://gitlab.com/thuth/qemu/-/jobs/2094869856

Maybe it could be fixed with some extensions to the filters, but I'm not
sure whether we really want to go down that road...?


AFAIK, errno strings are not standardized by POSIX, so I presume this
problem will apply to running I/O tests on any non-Linux system too.

With this in mind I think we should consider what a portable solution
looks like. We can't simply match the Alpine strings and turn them
into GLibC strings, as that does nothing to help portability on *BSD,
macOS, Windows, etc.
Luckily, the strings did not cause that much problems on *BSDs and macOS 
yet... Most of the current set of tests works fine there. It's really just 
that libc from Alpine that is causing this trouble...


 Thomas

Re: [PATCH 1/6] tests/qemu-iotests: Improve the check for GNU sed

[Daniel P . Berrangé]

On Tue, Feb 15, 2022 at 02:28:24PM +0100, Thomas Huth wrote:
> On 11/02/2022 17.48, Thomas Huth wrote:
> > On 11/02/2022 17.14, Eric Blake wrote:
> > > On Tue, Feb 08, 2022 at 03:52:19PM +0100, Thomas Huth wrote:
> > > > > > The current code with $SED has been introduced almost three years
> > > > > > ago already...
> > > > > > 
> > > > > > >    Can’t we just do `alias sed=gsed`?
> > > > > > 
> > > > > > Maybe ... but let's ask Philippe and Kevin first, who Signed-off
> > > > > > commit bde36af1ab4f476 that introduced the current way with $SED:
> > > > > > What's your opinion about this?
> > > > > 
> > > > > This commit was to have check-block working on the OpenBSD VM image.
> > > > 
> > > > Sure. The question was whether using an alias as suggested by Hanna 
> > > > would be
> > > > nicer instead of using $SED ?
> > > 
> > > Scripting with aliases becomes a nightmare to debug, since it is
> > > relatively uncommon.  In particular, in bash, you have to explicitly
> > > opt in to using aliases (contrary to POSIX sh where aliases are
> > > available to scripts at startup).
> > 
> > shopt -s expand_aliases
> > ... as I just learnt the hard way ;-)
> > 
> > > Using $SED everywhere may require
> > > more hunting, but it is more obvious when reading a test that "oh
> > > yeah, I might be using extensions that the default 'sed' can't
> > > support" than a script that blindly uses 'sed' and depends on it
> > > aliasing to a more-capable sed at a distance.
> > > 
> > > The other question is how many GNU sed features are we actually
> > > depending on?  Which tests break if we have BSD sed or busybox sed?
> > > Can we rewrite those sed scripts to avoid GNU extensions?  But
> > > auditing for s/sed/$SED/ seems easier than auditing for which
> > > non-portable sed extensions we depend on.
> > 
> > The most obvious part are the filter functions in common.filter - we're
> > using "-r" here that is not part of the POSIX sed as far as I can see.
> 
> Now that I stepped through the list, the other major part that is failing on
> non-GNU seds are the statements that use "\r" or "\n" or "\e" to replace
> special characters. That seems to be a non-POSIX extension, too.
> 
> But for running with Alpine, there is also the additional problems that the
> libc uses slightly different error strings, e.g. "I/O error" instead of
> "Input/output error", see e.g.:
> 
>  https://gitlab.com/thuth/qemu/-/jobs/2094869856
> 
> Maybe it could be fixed with some extensions to the filters, but I'm not
> sure whether we really want to go down that road...?

AFAIK, errno strings are not standardized by POSIX, so I presume this
problem will apply to running I/O tests on any non-Linux system too.

With this in mind I think we should consider what a portable solution
looks like. We can't simply match the Alpine strings and turn them
into GLibC strings, as that does nothing to help portability on *BSD,
macOS, Windows, etc. We would need to figure out how to blank out
arbitrary input error message strings.

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 :|

Re: [PATCH 1/6] tests/qemu-iotests: Improve the check for GNU sed

[Thomas Huth]

On 11/02/2022 17.48, Thomas Huth wrote:

On 11/02/2022 17.14, Eric Blake wrote:

On Tue, Feb 08, 2022 at 03:52:19PM +0100, Thomas Huth wrote:

The current code with $SED has been introduced almost three years
ago already...


   Can’t we just do `alias sed=gsed`?


Maybe ... but let's ask Philippe and Kevin first, who Signed-off
commit bde36af1ab4f476 that introduced the current way with $SED:
What's your opinion about this?


This commit was to have check-block working on the OpenBSD VM image.


Sure. The question was whether using an alias as suggested by Hanna would be
nicer instead of using $SED ?


Scripting with aliases becomes a nightmare to debug, since it is
relatively uncommon.  In particular, in bash, you have to explicitly
opt in to using aliases (contrary to POSIX sh where aliases are
available to scripts at startup).


shopt -s expand_aliases
... as I just learnt the hard way ;-)


Using $SED everywhere may require
more hunting, but it is more obvious when reading a test that "oh
yeah, I might be using extensions that the default 'sed' can't
support" than a script that blindly uses 'sed' and depends on it
aliasing to a more-capable sed at a distance.

The other question is how many GNU sed features are we actually
depending on?  Which tests break if we have BSD sed or busybox sed?
Can we rewrite those sed scripts to avoid GNU extensions?  But
auditing for s/sed/$SED/ seems easier than auditing for which
non-portable sed extensions we depend on.


The most obvious part are the filter functions in common.filter - we're 
using "-r" here that is not part of the POSIX sed as far as I can see.


Now that I stepped through the list, the other major part that is failing on 
non-GNU seds are the statements that use "\r" or "\n" or "\e" to replace 
special characters. That seems to be a non-POSIX extension, too.


But for running with Alpine, there is also the additional problems that the 
libc uses slightly different error strings, e.g. "I/O error" instead of 
"Input/output error", see e.g.:


 https://gitlab.com/thuth/qemu/-/jobs/2094869856

Maybe it could be fixed with some extensions to the filters, but I'm not 
sure whether we really want to go down that road...?


 Thomas

Re: [PATCH 1/6] tests/qemu-iotests: Improve the check for GNU sed

[Thomas Huth]

On 11/02/2022 17.14, Eric Blake wrote:

On Tue, Feb 08, 2022 at 03:52:19PM +0100, Thomas Huth wrote:

The current code with $SED has been introduced almost three years
ago already...


   Can’t we just do `alias sed=gsed`?


Maybe ... but let's ask Philippe and Kevin first, who Signed-off
commit bde36af1ab4f476 that introduced the current way with $SED:
What's your opinion about this?


This commit was to have check-block working on the OpenBSD VM image.


Sure. The question was whether using an alias as suggested by Hanna would be
nicer instead of using $SED ?


Scripting with aliases becomes a nightmare to debug, since it is
relatively uncommon.  In particular, in bash, you have to explicitly
opt in to using aliases (contrary to POSIX sh where aliases are
available to scripts at startup).


shopt -s expand_aliases
... as I just learnt the hard way ;-)


Using $SED everywhere may require
more hunting, but it is more obvious when reading a test that "oh
yeah, I might be using extensions that the default 'sed' can't
support" than a script that blindly uses 'sed' and depends on it
aliasing to a more-capable sed at a distance.

The other question is how many GNU sed features are we actually
depending on?  Which tests break if we have BSD sed or busybox sed?
Can we rewrite those sed scripts to avoid GNU extensions?  But
auditing for s/sed/$SED/ seems easier than auditing for which
non-portable sed extensions we depend on.


The most obvious part are the filter functions in common.filter - we're 
using "-r" here that is not part of the POSIX sed as far as I can see.


Not sure whether anybody really wants to rewrite all sed statements for full 
portability, but maybe we could also introduce a wrapper for GNU sed like this:


if ! command -v gsed >/dev/null 2>&1; then
if sed --version | grep -v 'not GNU sed' | grep 'GNUx sed' \
   > /dev/null 2>&1;
then
gsed()
{
sed $*
}
else
gsed()
{
_notrun "GNU sed not available"
}
fi
fi

... then we could simply use "gsed" everywhere we depend on the GNU 
behavior, and the tests don't look as ugly as with the $SED ?


 Thomas

Re: [PATCH 1/6] tests/qemu-iotests: Improve the check for GNU sed

[Eric Blake]

On Tue, Feb 08, 2022 at 03:52:19PM +0100, Thomas Huth wrote:
> > > The current code with $SED has been introduced almost three years
> > > ago already...
> > > 
> > > >   Can’t we just do `alias sed=gsed`?
> > > 
> > > Maybe ... but let's ask Philippe and Kevin first, who Signed-off
> > > commit bde36af1ab4f476 that introduced the current way with $SED:
> > > What's your opinion about this?
> > 
> > This commit was to have check-block working on the OpenBSD VM image.
> 
> Sure. The question was whether using an alias as suggested by Hanna would be
> nicer instead of using $SED ?

Scripting with aliases becomes a nightmare to debug, since it is
relatively uncommon.  In particular, in bash, you have to explicitly
opt in to using aliases (contrary to POSIX sh where aliases are
available to scripts at startup).  Using $SED everywhere may require
more hunting, but it is more obvious when reading a test that "oh
yeah, I might be using extensions that the default 'sed' can't
support" than a script that blindly uses 'sed' and depends on it
aliasing to a more-capable sed at a distance.

The other question is how many GNU sed features are we actually
depending on?  Which tests break if we have BSD sed or busybox sed?
Can we rewrite those sed scripts to avoid GNU extensions?  But
auditing for s/sed/$SED/ seems easier than auditing for which
non-portable sed extensions we depend on.

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

Re: [PATCH 1/6] tests/qemu-iotests: Improve the check for GNU sed

[Thomas Huth]

On 08/02/2022 14.11, Philippe Mathieu-Daudé wrote:

On 8/2/22 13:38, Thomas Huth wrote:

On 08/02/2022 13.28, Hanna Reitz wrote:

On 08.02.22 13:13, Thomas Huth wrote:

On 08/02/2022 12.46, Hanna Reitz wrote:

On 08.02.22 11:13, Thomas Huth wrote:

Instead of failing the iotests if GNU sed is not available (or skipping
them completely in the check-block.sh script), it would be better to
simply skip the bash-based tests, so that the python-based tests could
still be run. Thus add the check for BusyBox sed to common.rc and mark
the tests as "not run" if GNU sed is not available. Then we can also
remove the sed checks from the check-block.sh script.

Signed-off-by: Thomas Huth 
---
  tests/check-block.sh | 12 
  tests/qemu-iotests/common.rc | 26 +-
  2 files changed, 13 insertions(+), 25 deletions(-)

diff --git a/tests/check-block.sh b/tests/check-block.sh
index 720a46bc36..af0c574812 100755
--- a/tests/check-block.sh
+++ b/tests/check-block.sh
@@ -52,18 +52,6 @@ if LANG=C bash --version | grep -q 'GNU bash, 
version [123]' ; then

  skip "bash version too old ==> Not running the qemu-iotests."
  fi
-if ! (sed --version | grep 'GNU sed') > /dev/null 2>&1 ; then


This specifically tests for `sed`, whereas...


There was a check for "gsed" one line later:

 if ! command -v gsed >/dev/null 2>&1; then

... so the check-block.sh script ran the iotests also if "sed" was not 
GNU, but gsed was available.


Oh, right.


[...]


diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 9885030b43..9ea504810c 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -17,17 +17,27 @@
  # along with this program.  If not, see .
  #
+# bail out, setting up .notrun file
+_notrun()
+{
+    echo "$*" >"$OUTPUT_DIR/$seq.notrun"
+    echo "$seq not run: $*"
+    status=0
+    exit
+}
+
+# We need GNU sed for the iotests. Make sure to not use BusyBox sed
+# which says that "This is not GNU sed version 4.0"
  SED=
  for sed in sed gsed; do
-    ($sed --version | grep 'GNU sed') > /dev/null 2>&1
+    ($sed --version | grep -v "not GNU sed" | grep 'GNU sed') > 
/dev/null 2>&1


...this will accept `gsed`, too.  The problem is that many bash iotests 
just use `sed` instead of `$SED`, so I think if we let this do the 
gatekeeping, then we should change this to just check for `sed`.


I think we should be fine - at least for the tests in the "auto" group. 
Otherwise we would have seen test failures on non-Linux systems like 
*BSD earlier already.


Makes sense, but I’m quite uncomfortable with this.


The current code with $SED has been introduced almost three years ago 
already...



  Can’t we just do `alias sed=gsed`?


Maybe ... but let's ask Philippe and Kevin first, who Signed-off commit 
bde36af1ab4f476 that introduced the current way with $SED: What's your 
opinion about this?


This commit was to have check-block working on the OpenBSD VM image.


Sure. The question was whether using an alias as suggested by Hanna would be 
nicer instead of using $SED ?


 Thomas

Re: [PATCH 1/6] tests/qemu-iotests: Improve the check for GNU sed

[Philippe Mathieu-Daudé via]

On 8/2/22 13:38, Thomas Huth wrote:

On 08/02/2022 13.28, Hanna Reitz wrote:

On 08.02.22 13:13, Thomas Huth wrote:

On 08/02/2022 12.46, Hanna Reitz wrote:

On 08.02.22 11:13, Thomas Huth wrote:
Instead of failing the iotests if GNU sed is not available (or 
skipping

them completely in the check-block.sh script), it would be better to
simply skip the bash-based tests, so that the python-based tests could
still be run. Thus add the check for BusyBox sed to common.rc and mark
the tests as "not run" if GNU sed is not available. Then we can also
remove the sed checks from the check-block.sh script.

Signed-off-by: Thomas Huth 
---
  tests/check-block.sh | 12 
  tests/qemu-iotests/common.rc | 26 +-
  2 files changed, 13 insertions(+), 25 deletions(-)

diff --git a/tests/check-block.sh b/tests/check-block.sh
index 720a46bc36..af0c574812 100755
--- a/tests/check-block.sh
+++ b/tests/check-block.sh
@@ -52,18 +52,6 @@ if LANG=C bash --version | grep -q 'GNU bash, 
version [123]' ; then

  skip "bash version too old ==> Not running the qemu-iotests."
  fi
-if ! (sed --version | grep 'GNU sed') > /dev/null 2>&1 ; then


This specifically tests for `sed`, whereas...


There was a check for "gsed" one line later:

 if ! command -v gsed >/dev/null 2>&1; then

... so the check-block.sh script ran the iotests also if "sed" was 
not GNU, but gsed was available.


Oh, right.


[...]

diff --git a/tests/qemu-iotests/common.rc 
b/tests/qemu-iotests/common.rc

index 9885030b43..9ea504810c 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -17,17 +17,27 @@
  # along with this program.  If not, see 
.

  #
+# bail out, setting up .notrun file
+_notrun()
+{
+    echo "$*" >"$OUTPUT_DIR/$seq.notrun"
+    echo "$seq not run: $*"
+    status=0
+    exit
+}
+
+# We need GNU sed for the iotests. Make sure to not use BusyBox sed
+# which says that "This is not GNU sed version 4.0"
  SED=
  for sed in sed gsed; do
-    ($sed --version | grep 'GNU sed') > /dev/null 2>&1
+    ($sed --version | grep -v "not GNU sed" | grep 'GNU sed') > 
/dev/null 2>&1


...this will accept `gsed`, too.  The problem is that many bash 
iotests just use `sed` instead of `$SED`, so I think if we let this 
do the gatekeeping, then we should change this to just check for `sed`.


I think we should be fine - at least for the tests in the "auto" 
group. Otherwise we would have seen test failures on non-Linux 
systems like *BSD earlier already.


Makes sense, but I’m quite uncomfortable with this.


The current code with $SED has been introduced almost three years ago 
already...



  Can’t we just do `alias sed=gsed`?


Maybe ... but let's ask Philippe and Kevin first, who Signed-off commit 
bde36af1ab4f476 that introduced the current way with $SED: What's your 
opinion about this?


This commit was to have check-block working on the OpenBSD VM image.

Re: [PATCH 1/6] tests/qemu-iotests: Improve the check for GNU sed

[Hanna Reitz]

On 08.02.22 13:13, Thomas Huth wrote:

On 08/02/2022 12.46, Hanna Reitz wrote:

On 08.02.22 11:13, Thomas Huth wrote:

Instead of failing the iotests if GNU sed is not available (or skipping
them completely in the check-block.sh script), it would be better to
simply skip the bash-based tests, so that the python-based tests could
still be run. Thus add the check for BusyBox sed to common.rc and mark
the tests as "not run" if GNU sed is not available. Then we can also
remove the sed checks from the check-block.sh script.

Signed-off-by: Thomas Huth 
---
  tests/check-block.sh | 12 
  tests/qemu-iotests/common.rc | 26 +-
  2 files changed, 13 insertions(+), 25 deletions(-)

diff --git a/tests/check-block.sh b/tests/check-block.sh
index 720a46bc36..af0c574812 100755
--- a/tests/check-block.sh
+++ b/tests/check-block.sh
@@ -52,18 +52,6 @@ if LANG=C bash --version | grep -q 'GNU bash, 
version [123]' ; then

  skip "bash version too old ==> Not running the qemu-iotests."
  fi
-if ! (sed --version | grep 'GNU sed') > /dev/null 2>&1 ; then


This specifically tests for `sed`, whereas...


There was a check for "gsed" one line later:

 if ! command -v gsed >/dev/null 2>&1; then

... so the check-block.sh script ran the iotests also if "sed" was not 
GNU, but gsed was available.


Oh, right.


[...]

diff --git a/tests/qemu-iotests/common.rc 
b/tests/qemu-iotests/common.rc

index 9885030b43..9ea504810c 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -17,17 +17,27 @@
  # along with this program.  If not, see 
.

  #
+# bail out, setting up .notrun file
+_notrun()
+{
+    echo "$*" >"$OUTPUT_DIR/$seq.notrun"
+    echo "$seq not run: $*"
+    status=0
+    exit
+}
+
+# We need GNU sed for the iotests. Make sure to not use BusyBox sed
+# which says that "This is not GNU sed version 4.0"
  SED=
  for sed in sed gsed; do
-    ($sed --version | grep 'GNU sed') > /dev/null 2>&1
+    ($sed --version | grep -v "not GNU sed" | grep 'GNU sed') > 
/dev/null 2>&1


...this will accept `gsed`, too.  The problem is that many bash 
iotests just use `sed` instead of `$SED`, so I think if we let this 
do the gatekeeping, then we should change this to just check for `sed`.


I think we should be fine - at least for the tests in the "auto" 
group. Otherwise we would have seen test failures on non-Linux systems 
like *BSD earlier already.


Makes sense, but I’m quite uncomfortable with this.  Can’t we just do 
`alias sed=gsed`?


Hanna

Re: [PATCH 1/6] tests/qemu-iotests: Improve the check for GNU sed

[Hanna Reitz]

On 08.02.22 11:13, Thomas Huth wrote:

Instead of failing the iotests if GNU sed is not available (or skipping
them completely in the check-block.sh script), it would be better to
simply skip the bash-based tests, so that the python-based tests could
still be run. Thus add the check for BusyBox sed to common.rc and mark
the tests as "not run" if GNU sed is not available. Then we can also
remove the sed checks from the check-block.sh script.

Signed-off-by: Thomas Huth 
---
  tests/check-block.sh | 12 
  tests/qemu-iotests/common.rc | 26 +-
  2 files changed, 13 insertions(+), 25 deletions(-)

diff --git a/tests/check-block.sh b/tests/check-block.sh
index 720a46bc36..af0c574812 100755
--- a/tests/check-block.sh
+++ b/tests/check-block.sh
@@ -52,18 +52,6 @@ if LANG=C bash --version | grep -q 'GNU bash, version [123]' 
; then
  skip "bash version too old ==> Not running the qemu-iotests."
  fi
  
-if ! (sed --version | grep 'GNU sed') > /dev/null 2>&1 ; then


This specifically tests for `sed`, whereas...

[...]


diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 9885030b43..9ea504810c 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -17,17 +17,27 @@
  # along with this program.  If not, see .
  #
  
+# bail out, setting up .notrun file

+_notrun()
+{
+echo "$*" >"$OUTPUT_DIR/$seq.notrun"
+echo "$seq not run: $*"
+status=0
+exit
+}
+
+# We need GNU sed for the iotests. Make sure to not use BusyBox sed
+# which says that "This is not GNU sed version 4.0"
  SED=
  for sed in sed gsed; do
-($sed --version | grep 'GNU sed') > /dev/null 2>&1
+($sed --version | grep -v "not GNU sed" | grep 'GNU sed') > /dev/null 2>&1


...this will accept `gsed`, too.  The problem is that many bash iotests 
just use `sed` instead of `$SED`, so I think if we let this do the 
gatekeeping, then we should change this to just check for `sed`.


Hanna


  if [ "$?" -eq 0 ]; then
  SED=$sed
  break
  fi
  done
  if [ -z "$SED" ]; then
-echo "$0: GNU sed not found"
-exit 1
+_notrun "GNU sed not found"
  fi

Re: [PATCH 1/6] tests/qemu-iotests: Improve the check for GNU sed

[Thomas Huth]

On 08/02/2022 13.28, Hanna Reitz wrote:

On 08.02.22 13:13, Thomas Huth wrote:

On 08/02/2022 12.46, Hanna Reitz wrote:

On 08.02.22 11:13, Thomas Huth wrote:

Instead of failing the iotests if GNU sed is not available (or skipping
them completely in the check-block.sh script), it would be better to
simply skip the bash-based tests, so that the python-based tests could
still be run. Thus add the check for BusyBox sed to common.rc and mark
the tests as "not run" if GNU sed is not available. Then we can also
remove the sed checks from the check-block.sh script.

Signed-off-by: Thomas Huth 
---
  tests/check-block.sh | 12 
  tests/qemu-iotests/common.rc | 26 +-
  2 files changed, 13 insertions(+), 25 deletions(-)

diff --git a/tests/check-block.sh b/tests/check-block.sh
index 720a46bc36..af0c574812 100755
--- a/tests/check-block.sh
+++ b/tests/check-block.sh
@@ -52,18 +52,6 @@ if LANG=C bash --version | grep -q 'GNU bash, version 
[123]' ; then

  skip "bash version too old ==> Not running the qemu-iotests."
  fi
-if ! (sed --version | grep 'GNU sed') > /dev/null 2>&1 ; then


This specifically tests for `sed`, whereas...


There was a check for "gsed" one line later:

 if ! command -v gsed >/dev/null 2>&1; then

... so the check-block.sh script ran the iotests also if "sed" was not 
GNU, but gsed was available.


Oh, right.


[...]


diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 9885030b43..9ea504810c 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -17,17 +17,27 @@
  # along with this program.  If not, see .
  #
+# bail out, setting up .notrun file
+_notrun()
+{
+    echo "$*" >"$OUTPUT_DIR/$seq.notrun"
+    echo "$seq not run: $*"
+    status=0
+    exit
+}
+
+# We need GNU sed for the iotests. Make sure to not use BusyBox sed
+# which says that "This is not GNU sed version 4.0"
  SED=
  for sed in sed gsed; do
-    ($sed --version | grep 'GNU sed') > /dev/null 2>&1
+    ($sed --version | grep -v "not GNU sed" | grep 'GNU sed') > 
/dev/null 2>&1


...this will accept `gsed`, too.  The problem is that many bash iotests 
just use `sed` instead of `$SED`, so I think if we let this do the 
gatekeeping, then we should change this to just check for `sed`.


I think we should be fine - at least for the tests in the "auto" group. 
Otherwise we would have seen test failures on non-Linux systems like *BSD 
earlier already.


Makes sense, but I’m quite uncomfortable with this.


The current code with $SED has been introduced almost three years ago already...


  Can’t we just do `alias sed=gsed`?


Maybe ... but let's ask Philippe and Kevin first, who Signed-off commit 
bde36af1ab4f476 that introduced the current way with $SED: What's your 
opinion about this?


 Thomas

Re: [PATCH 1/6] tests/qemu-iotests: Improve the check for GNU sed

[Thomas Huth]

On 08/02/2022 12.46, Hanna Reitz wrote:

On 08.02.22 11:13, Thomas Huth wrote:

Instead of failing the iotests if GNU sed is not available (or skipping
them completely in the check-block.sh script), it would be better to
simply skip the bash-based tests, so that the python-based tests could
still be run. Thus add the check for BusyBox sed to common.rc and mark
the tests as "not run" if GNU sed is not available. Then we can also
remove the sed checks from the check-block.sh script.

Signed-off-by: Thomas Huth 
---
  tests/check-block.sh | 12 
  tests/qemu-iotests/common.rc | 26 +-
  2 files changed, 13 insertions(+), 25 deletions(-)

diff --git a/tests/check-block.sh b/tests/check-block.sh
index 720a46bc36..af0c574812 100755
--- a/tests/check-block.sh
+++ b/tests/check-block.sh
@@ -52,18 +52,6 @@ if LANG=C bash --version | grep -q 'GNU bash, version 
[123]' ; then

  skip "bash version too old ==> Not running the qemu-iotests."
  fi
-if ! (sed --version | grep 'GNU sed') > /dev/null 2>&1 ; then


This specifically tests for `sed`, whereas...


There was a check for "gsed" one line later:

 if ! command -v gsed >/dev/null 2>&1; then

... so the check-block.sh script ran the iotests also if "sed" was not GNU, 
but gsed was available.



[...]


diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 9885030b43..9ea504810c 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -17,17 +17,27 @@
  # along with this program.  If not, see .
  #
+# bail out, setting up .notrun file
+_notrun()
+{
+    echo "$*" >"$OUTPUT_DIR/$seq.notrun"
+    echo "$seq not run: $*"
+    status=0
+    exit
+}
+
+# We need GNU sed for the iotests. Make sure to not use BusyBox sed
+# which says that "This is not GNU sed version 4.0"
  SED=
  for sed in sed gsed; do
-    ($sed --version | grep 'GNU sed') > /dev/null 2>&1
+    ($sed --version | grep -v "not GNU sed" | grep 'GNU sed') > /dev/null 
2>&1


...this will accept `gsed`, too.  The problem is that many bash iotests just 
use `sed` instead of `$SED`, so I think if we let this do the gatekeeping, 
then we should change this to just check for `sed`.


I think we should be fine - at least for the tests in the "auto" group. 
Otherwise we would have seen test failures on non-Linux systems like *BSD 
earlier already.


 Thomas

[PATCH 1/6] tests/qemu-iotests: Improve the check for GNU sed

[Thomas Huth]

Instead of failing the iotests if GNU sed is not available (or skipping
them completely in the check-block.sh script), it would be better to
simply skip the bash-based tests, so that the python-based tests could
still be run. Thus add the check for BusyBox sed to common.rc and mark
the tests as "not run" if GNU sed is not available. Then we can also
remove the sed checks from the check-block.sh script.

Signed-off-by: Thomas Huth 
---
 tests/check-block.sh | 12 
 tests/qemu-iotests/common.rc | 26 +-
 2 files changed, 13 insertions(+), 25 deletions(-)

diff --git a/tests/check-block.sh b/tests/check-block.sh
index 720a46bc36..af0c574812 100755
--- a/tests/check-block.sh
+++ b/tests/check-block.sh
@@ -52,18 +52,6 @@ if LANG=C bash --version | grep -q 'GNU bash, version [123]' 
; then
 skip "bash version too old ==> Not running the qemu-iotests."
 fi
 
-if ! (sed --version | grep 'GNU sed') > /dev/null 2>&1 ; then
-if ! command -v gsed >/dev/null 2>&1; then
-skip "GNU sed not available ==> Not running the qemu-iotests."
-fi
-else
-# Double-check that we're not using BusyBox' sed which says
-# that "This is not GNU sed version 4.0" ...
-if sed --version | grep -q 'not GNU sed' ; then
-skip "BusyBox sed not supported ==> Not running the qemu-iotests."
-fi
-fi
-
 cd tests/qemu-iotests
 
 # QEMU_CHECK_BLOCK_AUTO is used to disable some unstable sub-tests
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 9885030b43..9ea504810c 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -17,17 +17,27 @@
 # along with this program.  If not, see .
 #
 
+# bail out, setting up .notrun file
+_notrun()
+{
+echo "$*" >"$OUTPUT_DIR/$seq.notrun"
+echo "$seq not run: $*"
+status=0
+exit
+}
+
+# We need GNU sed for the iotests. Make sure to not use BusyBox sed
+# which says that "This is not GNU sed version 4.0"
 SED=
 for sed in sed gsed; do
-($sed --version | grep 'GNU sed') > /dev/null 2>&1
+($sed --version | grep -v "not GNU sed" | grep 'GNU sed') > /dev/null 2>&1
 if [ "$?" -eq 0 ]; then
 SED=$sed
 break
 fi
 done
 if [ -z "$SED" ]; then
-echo "$0: GNU sed not found"
-exit 1
+_notrun "GNU sed not found"
 fi
 
 dd()
@@ -722,16 +732,6 @@ _img_info()
 done
 }
 
-# bail out, setting up .notrun file
-#
-_notrun()
-{
-echo "$*" >"$OUTPUT_DIR/$seq.notrun"
-echo "$seq not run: $*"
-status=0
-exit
-}
-
 # bail out, setting up .casenotrun file
 # The function _casenotrun() is used as a notifier. It is the
 # caller's responsibility to make skipped a particular test.
-- 
2.27.0