Re: [PATCH] tests/qemu-iotests: Rework the checks and spots using GNU sed

[Thomas Huth]

On 15/02/2022 23.10, Eric Blake wrote:
> On Tue, Feb 15, 2022 at 02:20:31PM +0100, 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 that rely on GNU sed, so that the other
> > tests could still be run. Thus we now explicitely use "gsed" (either as
> > direct program or as a wrapper around "sed" if it's the GNU version)
> > in the spots that rely on the GNU sed behavior. Then we also remove the
> > sed checks from the check-block.sh script, so that "make check-block"
> > can now be run on systems without GNU sed, too.
...
> > diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
> > index 935217aa65..a3b1708adc 100644
> > --- a/tests/qemu-iotests/common.filter
> > +++ b/tests/qemu-iotests/common.filter
> > @@ -21,58 +21,58 @@
> >   
> >   _filter_date()
> >   {
> > -    $SED -re 's/[0-9]{4}-[0-9]{2}-[0-9]{2} 
> > [0-9]{2}:[0-9]{2}:[0-9]{2}/yyyy-mm-dd hh:mm:ss/'
> > +    gsed -re 's/[0-9]{4}-[0-9]{2}-[0-9]{2} 
> > [0-9]{2}:[0-9]{2}:[0-9]{2}/yyyy-mm-dd hh:mm:ss/'
>
> GNU sed recommends spelling it 'sed -E', not 'sed -r', when using
> extended regex.  Older POSIX did not support either spelling, but the
> next version will require -E, as many implementations have it now:
> https://www.austingroupbugs.net/view.php?id=528

Thanks for the pointer ... I originally checked "man 1p sed" on
my system and did not see -r or -E in there, so I thought that
this must be really something specific to GNU sed. But now that
you've mentioned this, I just double-checked the build environments
that we support with QEMU, and seems like -E should be supported
everywhere:

macOS 11:

$ sed --help
sed: illegal option -- -
usage: sed script [-Ealnru] [-i extension] [file ...]
        sed [-Ealnu] [-i extension] [-e script] ... [-f script_file] ... [file 
...]


NetBSD 9.2:

$ sed --help
sed: unknown option -- -
Usage:  sed [-aElnru] command [file ...]
        sed [-aElnru] [-e command] [-f command_file] [-I[extension]]
            [-i[extension]] [file ...]


FreeBSD 12.3:

$ sed --help
sed: illegal option -- -
usage: sed script [-Ealnru] [-i extension] [file ...]
        sed [-Ealnu] [-i extension] [-e script] ... [-f script_file] ... [file 
...]


OpenBSD 7.0:

$ sed --help
sed: unknown option -- -
usage: sed [-aEnru] [-i[extension]] command [file ...]
       sed [-aEnru] [-e command] [-f command_file] [-i[extension]] [file ...]


Illumos:

Has -E according to https://illumos.org/man/1/sed


Busybox:

Has -E according to https://www.commandlinux.com/man-page/man1/busybox.1.html


Haiku:

Seems to use GNU sed, so -E is available.


We likely never will run the iotests on Windows, so I did not check
those (but I assume MSYS and friends are using GNU sed, too).


So I think it should be safe to change these spots that are
using "-r" to "sed -E" (and not use gsed here).

> Other than the fact that this was easier to write with ERE, I'm not
> seeing any other GNU-only extensions in use here; but I'd recommend
> that as long as we're touching the line, we spell it 'gsed -Ee'
> instead of -re (here, and in several other places).
>
> >   _filter_qom_path()
> >   {
> > -    $SED -e '/Attached to:/s/\device[[0-9]\+\]/device[N]/g'
> > +    gsed -e '/Attached to:/s/\device[[0-9]\+\]/device[N]/g'
>
> Here, it is our use of \+ that is a GNU sed extension, although it is
> fairly easy (but verbose) to translate that one to portable sed
> (<PAT>\+ is the same as <PAT><PAT>*).  So gsed is correct.  On the
> other hand, the use of [[0-9]\+\] looks ugly - it probably does NOT
> match what we meant (we have a bracket expression '[...]' that matches
> the 11 characters [ and 0-9, then '\+' to match that bracket
> expression 1 or more times, then '\]' which in its context is
> identical to ']' to match a closing ], since only opening [ needs \
> escaping for literal treatment).  What we probably meant is:
>
> '/Attached to:/s/\device\[[0-9][0-9]*]/device[N]/g'
>
> at which point normal sed would do.

Ok ... but I'd prefer to clean such spots rather in a second step,
to make sure not to introduce bugs and to make the review easier.

> >   # Removes \r from messages
> >   _filter_win32()
> >   {
> > -    $SED -e 's/\r//g'
> > +    gsed -e 's/\r//g'
>
> Yep, \r is another GNU sed extension.
>
> >   }
> >   
> >   # sanitize qemu-io output
> >   _filter_qemu_io()
> >   {
> > -    _filter_win32 | $SED -e "s/[0-9]* ops\; [0-9/:. sec]* ([0-9/.inf]* 
> > [EPTGMKiBbytes]*\/sec and [0-9/.inf]* ops\/sec)/X ops\; XX:XX:XX.X (XXX YYY\/sec and XXX 
> > ops\/sec)/" \
> > +    _filter_win32 | gsed -e "s/[0-9]* ops\; [0-9/:. sec]* ([0-9/.inf]* 
> > [EPTGMKiBbytes]*\/sec and [0-9/.inf]* ops\/sec)/X ops\; XX:XX:XX.X (XXX YYY\/sec and XXX 
> > ops\/sec)/" \
> >           -e "s/: line [0-9][0-9]*:  *[0-9][0-9]*\( Aborted\| Killed\)/:\1/" \
> >           -e "s/qemu-io> //g"
>
> I'm not seeing anything specific to GNU sed in this (long) sed script;
> can we relax this one to plain 'sed'?  Use of s#some/text## might be
> easier than having to s/some\/text//, but that's not essential.

If I switch that to plain sed, I'm getting errors like this on NetBSD:

--- /home/qemu/qemu-test.is2SLq/src/tests/qemu-iotests/046.out
+++ 11296-046.out.bad
@@ -66,6 +66,7 @@
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote 65536/65536 bytes at offset 2031616
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=6442450944 
backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT

 == Some concurrent requests touching the same cluster ==

So I'll keep gsed here for now - we need it for _filter_qemu_io
anyway since it's calling _filter_win32 that currently needs
gsed, too.

> > @@ -142,7 +142,7 @@ _do_filter_img_create()
> >       # precedes ", fmt=") and the options part ($options, which starts
> >       # with "fmt=")
> >       # (And just echo everything before the first "^Formatting")
> > -    readarray formatting_line < <($SED -e 's/, fmt=/\n/')
> > +    readarray formatting_line < <(gsed -e 's/, fmt=/\n/')
>
> This one looks like it should work with plain 'sed'.

Using normal sed does not really work for me here. For example
with NetBSD, I get errors like this:

--- /home/qemu/qemu-test.cSYvEb/src/tests/qemu-iotests/027.out
+++ 13694-027.out.bad
@@ -1,5 +1,5 @@
 QA output created by 027
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+Formatting 'TEST_DIR/t.IMGFMT'nIMGFMT cluster_size=65536 extended_l2=off 
compression_type=zlib size=134217728 lazy_refcounts=off refcount_bits=16, fmt=

 == writing first cluster to populate metadata ==
 wrote 65536/65536 bytes at offset 65536

> > @@ -209,7 +209,7 @@ _filter_img_create()
> >   
> >   _filter_img_create_size()
> >   {
> > -    $SED -e "s# size=[0-9]\\+# size=SIZE#g"
> > +    sed -e "s# size=[0-9]\\+# size=SIZE#g"
>
> The use of "\\+" here either needs gsed, or respelling as [0-9][0-9]*.

I'll change it to gsed for now ... we can update the \+ spots in a
second patch later if we think that it helps to make the iotests run
on more systems.

> >   }
> >   
> >   _filter_img_info()
> > @@ -223,7 +223,7 @@ _filter_img_info()
> >   
> >       discard=0
> >       regex_json_spec_start='^ *"format-specific": \{'
> > -    $SED -e "s#$REMOTE_TEST_DIR#TEST_DIR#g" \
> > +    gsed -e "s#$REMOTE_TEST_DIR#TEST_DIR#g" \
> >           -e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \
> >           -e "s#$TEST_DIR#TEST_DIR#g" \
> >           -e "s#$SOCK_DIR#SOCK_DIR#g" \
>
> I didn't check context for whether this one needs to be gsed, or could
> be plain sed.

Complete statement looks like this:

 gsed -e "s#$REMOTE_TEST_DIR#TEST_DIR#g" \
   -e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \
   -e "s#$TEST_DIR#TEST_DIR#g" \
   -e "s#$SOCK_DIR#SOCK_DIR#g" \
   -e "s#$IMGFMT#IMGFMT#g" \
   -e 's#nbd://127.0.0.1:[0-9]\\+$#TEST_DIR/t.IMGFMT#g' \
   -e 's#nbd+unix:///\??socket=SOCK_DIR/nbd#TEST_DIR/t.IMGFMT#g'\
   -e 's#SOCK_DIR/fuse-#TEST_DIR/#g' \
   -e "/encrypted: yes/d" \
   -e "/cluster_size: [0-9]\\+/d" \
   -e "/table_size: [0-9]\\+/d" \
   -e "/compat: '[^']*'/d" \
   -e "/compat6: \\(on\\|off\\)/d" \
   -e "s/cid: [0-9]\+/cid: XXXXXXXXXX/" \
   -e "/static: \\(on\\|off\\)/d" \
   -e "/zeroed_grain: \\(on\\|off\\)/d" \
   -e "/subformat: '[^']*'/d" \
   -e "/adapter_type: '[^']*'/d" \
   -e "/hwversion: '[^']*'/d" \
   -e "/lazy_refcounts: \\(on\\|off\\)/d" \
   -e "/extended_l2=\\(on\\|off\\)/d" \
   -e "/block_size: [0-9]\\+/d" \
   -e "/block_state_zero: \\(on\\|off\\)/d" \
   -e "/log_size: [0-9]\\+/d" \
   -e "s/iters: [0-9]\\+/iters: 1024/" \
   -e 's/\(compression type: \)\(zlib\|zstd\)/\1COMPRESSION_TYPE/' \
   -e "s/uuid: [-a-f0-9]\\+/uuid: 00000000-0000-0000-0000-000000000000/" | \

There are some \\+ in here, so I think this needs gsed for now.

> > +++ b/tests/qemu-iotests/common.rc
> > @@ -17,17 +17,28 @@
> >   # along with this program.  If not, see <http://www.gnu.org/licenses/>.
> >   #
> >   
> > -SED=
> > -for sed in sed gsed; do
> > -    ($sed --version | 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
> > +# bail out, setting up .notrun file
> > +_notrun()
> > +{
> > +    echo "$*" >"$OUTPUT_DIR/$seq.notrun"
> > +    echo "$seq not run: $*"
> > +    status=0
> > +    exit
> > +}
> > +
> > +if ! command -v gsed >/dev/null 2>&1; then
> > +    if sed --version 2>&1 | grep -v 'not GNU sed' | grep 'GNU sed' > /dev/null;
> > +    then
> > +        gsed()
> > +        {
> > +            sed "$@"
> > +        }
> > +    else
> > +        gsed()
> > +        {
> > +            _notrun "GNU sed not available"
> > +        }
> > +    fi
> >   fi
>
> This one looks good.
>
> I found one or two issues that need to be fixed, and a couple of
> "might as well improve them while touching the line anyway", but
> overall I like where this is headed.

Thanks a lot of your review and suggestions, I'll respin a v2 with the 
updates...

 Thomas