| Message ID | 20191022072135.11188-5-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable more iotests during "make check-block" | expand |
On 22.10.19 09:21, Thomas Huth wrote: > The next patch is going to add some python-based tests to the "auto" > group, and these tests require virtio-blk to work properly. Running > iotests without virtio-blk likely does not make too much sense anyway, > so instead of adding a check for the availability of virtio-blk to each > and every test (which does not sound very appealing), let's rather add > a check for this at the top level in the check-block.sh script instead > (so that it is possible to run "make check" without the "check-block" > part for qemu-system-tricore for example). > > Reviewed-by: Max Reitz <mreitz@redhat.com> > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > tests/check-block.sh | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/tests/check-block.sh b/tests/check-block.sh > index 679aedec50..e9e2978818 100755 > --- a/tests/check-block.sh > +++ b/tests/check-block.sh > @@ -26,10 +26,24 @@ if grep -q "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null ; then > exit 0 > fi > > -if [ -z "$(find . -name 'qemu-system-*' -print)" ]; then > +if [ -n "$QEMU_PROG" ]; then > + qemu_prog="$QEMU_PROG" > +else > + for binary in *-softmmu/qemu-system-* ; do Hm, I know I’ve already given my R-b, but looking at this again – what if the user builds qemu for multiple targets? Then this will just test any target, whereas the iotests might test something else, because the algorithm there is slightly different: First, check $QEMU_PROG (same as here). Second, check $build_iotests/qemu. I think we can do this here, because we know that $build_iotests is $PWD/tests/qemu-iotests (or invoking ./check below wouldn’t work). Third, and this is actually important, I think, is that we first look for the qemu that matches the host architecture (uname -m, with an exception for ppc64). I think we really should do that here, too. Fourth, look for any qemu, as is done here. So I think we could do without #2, but it probably doesn’t hurt to check that, too. I don’t think we should do without #3, though. Max
On 30/10/2019 12.21, Max Reitz wrote: > On 22.10.19 09:21, Thomas Huth wrote: >> The next patch is going to add some python-based tests to the "auto" >> group, and these tests require virtio-blk to work properly. Running >> iotests without virtio-blk likely does not make too much sense anyway, >> so instead of adding a check for the availability of virtio-blk to each >> and every test (which does not sound very appealing), let's rather add >> a check for this at the top level in the check-block.sh script instead >> (so that it is possible to run "make check" without the "check-block" >> part for qemu-system-tricore for example). >> >> Reviewed-by: Max Reitz <mreitz@redhat.com> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> tests/check-block.sh | 16 +++++++++++++++- >> 1 file changed, 15 insertions(+), 1 deletion(-) >> >> diff --git a/tests/check-block.sh b/tests/check-block.sh >> index 679aedec50..e9e2978818 100755 >> --- a/tests/check-block.sh >> +++ b/tests/check-block.sh >> @@ -26,10 +26,24 @@ if grep -q "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null ; then >> exit 0 >> fi >> >> -if [ -z "$(find . -name 'qemu-system-*' -print)" ]; then >> +if [ -n "$QEMU_PROG" ]; then >> + qemu_prog="$QEMU_PROG" >> +else >> + for binary in *-softmmu/qemu-system-* ; do > > Hm, I know I’ve already given my R-b, but looking at this again – what > if the user builds qemu for multiple targets? Then this will just test > any target, whereas the iotests might test something else, because the > algorithm there is slightly different: > > First, check $QEMU_PROG (same as here). > > Second, check $build_iotests/qemu. I think we can do this here, because > we know that $build_iotests is $PWD/tests/qemu-iotests (or invoking > ./check below wouldn’t work). > > Third, and this is actually important, I think, is that we first look > for the qemu that matches the host architecture (uname -m, with an > exception for ppc64). I think we really should do that here, too. > > Fourth, look for any qemu, as is done here. > > So I think we could do without #2, but it probably doesn’t hurt to check > that, too. I don’t think we should do without #3, though. Maybe we should simply move the check into tests/qemu-iotests/check to avoid duplication of that logic? We could then also only simply skip the python tests instead of skipping everything, in case the chosen QEMU binary does not support virtio-blk. Thomas
On 11.11.19 15:02, Thomas Huth wrote: > On 30/10/2019 12.21, Max Reitz wrote: >> On 22.10.19 09:21, Thomas Huth wrote: >>> The next patch is going to add some python-based tests to the "auto" >>> group, and these tests require virtio-blk to work properly. Running >>> iotests without virtio-blk likely does not make too much sense anyway, >>> so instead of adding a check for the availability of virtio-blk to each >>> and every test (which does not sound very appealing), let's rather add >>> a check for this at the top level in the check-block.sh script instead >>> (so that it is possible to run "make check" without the "check-block" >>> part for qemu-system-tricore for example). >>> >>> Reviewed-by: Max Reitz <mreitz@redhat.com> >>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> --- >>> tests/check-block.sh | 16 +++++++++++++++- >>> 1 file changed, 15 insertions(+), 1 deletion(-) >>> >>> diff --git a/tests/check-block.sh b/tests/check-block.sh >>> index 679aedec50..e9e2978818 100755 >>> --- a/tests/check-block.sh >>> +++ b/tests/check-block.sh >>> @@ -26,10 +26,24 @@ if grep -q "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null ; then >>> exit 0 >>> fi >>> >>> -if [ -z "$(find . -name 'qemu-system-*' -print)" ]; then >>> +if [ -n "$QEMU_PROG" ]; then >>> + qemu_prog="$QEMU_PROG" >>> +else >>> + for binary in *-softmmu/qemu-system-* ; do >> >> Hm, I know I’ve already given my R-b, but looking at this again – what >> if the user builds qemu for multiple targets? Then this will just test >> any target, whereas the iotests might test something else, because the >> algorithm there is slightly different: >> >> First, check $QEMU_PROG (same as here). >> >> Second, check $build_iotests/qemu. I think we can do this here, because >> we know that $build_iotests is $PWD/tests/qemu-iotests (or invoking >> ./check below wouldn’t work). >> >> Third, and this is actually important, I think, is that we first look >> for the qemu that matches the host architecture (uname -m, with an >> exception for ppc64). I think we really should do that here, too. >> >> Fourth, look for any qemu, as is done here. >> >> So I think we could do without #2, but it probably doesn’t hurt to check >> that, too. I don’t think we should do without #3, though. > > Maybe we should simply move the check into tests/qemu-iotests/check to > avoid duplication of that logic? > We could then also only simply skip the python tests instead of skipping > everything, in case the chosen QEMU binary does not support virtio-blk. You mean by adding a new flag e.g. -batch that’s supposed to generally skip cases when in doubt that they can be run on the current host? Sounds good to me. Max
diff --git a/tests/check-block.sh b/tests/check-block.sh index 679aedec50..e9e2978818 100755 --- a/tests/check-block.sh +++ b/tests/check-block.sh @@ -26,10 +26,24 @@ if grep -q "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null ; then exit 0 fi -if [ -z "$(find . -name 'qemu-system-*' -print)" ]; then +if [ -n "$QEMU_PROG" ]; then + qemu_prog="$QEMU_PROG" +else + for binary in *-softmmu/qemu-system-* ; do + if [ -x "$binary" ]; then + qemu_prog="$binary" + break + fi + done +fi +if [ -z "$qemu_prog" ]; then echo "No qemu-system binary available ==> Not running the qemu-iotests." exit 0 fi +if ! "$qemu_prog" -M none -device help | grep -q virtio-blk >/dev/null 2>&1 ; then + echo "$qemu_prog does not support virtio-blk ==> Not running the qemu-iotests." + exit 0 +fi if ! command -v bash >/dev/null 2>&1 ; then echo "bash not available ==> Not running the qemu-iotests."