kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH kvm-unit-tests 0/2] runtime: Allow additional VMM parameters to be provided
@ 2020-02-13 14:32 Andrew Jones
  2020-02-13 14:32 ` [PATCH kvm-unit-tests 1/2] arch-run: Allow $QEMU to include parameters Andrew Jones
  2020-02-13 14:33 ` [PATCH kvm-unit-tests 2/2] runtime: Introduce VMM_PARAMS Andrew Jones
  0 siblings, 2 replies; 14+ messages in thread
From: Andrew Jones @ 2020-02-13 14:32 UTC (permalink / raw)
  To: kvm
  Cc: peter.maydell, alex.bennee, pbonzini, lvivier, thuth, david,
	frankja, eric.auger

Occasionally users want to temporarily add additional VMM (QEMU)
parameters to all tests (run_tests.sh) or to tests executed with
$ARCH/run. With $ARCH/run it's already easy to do, just append
them to the command line, but with run_tests.sh there wasn't any
way to do it. This series provides two ways. It allows the
parameters to be appended to the $QEMU environment variable,
which is reasonable thing to expect to work, but doesn't
solve the problem of providing parameters that override what
is specified in the unittests.cfg file. So VMM_PARAMS is also
introduced, which is another environment variable just for
the additional parameters, and that variable takes care to
show up on the command line in the appropriate places, depending
on how the tests are being invoked.

Thanks,
drew


Andrew Jones (2):
  arch-run: Allow $QEMU to include parameters
  runtime: Introduce VMM_PARAMS

 README.md             |  5 +++++
 arm/run               |  4 +++-
 powerpc/run           |  4 +++-
 s390x/run             |  1 +
 scripts/arch-run.bash | 14 +++++++++++++-
 scripts/runtime.bash  |  4 +++-
 x86/run               |  4 +++-
 7 files changed, 31 insertions(+), 5 deletions(-)

-- 
2.21.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH kvm-unit-tests 1/2] arch-run: Allow $QEMU to include parameters
  2020-02-13 14:32 [PATCH kvm-unit-tests 0/2] runtime: Allow additional VMM parameters to be provided Andrew Jones
@ 2020-02-13 14:32 ` Andrew Jones
  2020-02-14 10:31   ` Paolo Bonzini
  2020-02-13 14:33 ` [PATCH kvm-unit-tests 2/2] runtime: Introduce VMM_PARAMS Andrew Jones
  1 sibling, 1 reply; 14+ messages in thread
From: Andrew Jones @ 2020-02-13 14:32 UTC (permalink / raw)
  To: kvm
  Cc: peter.maydell, alex.bennee, pbonzini, lvivier, thuth, david,
	frankja, eric.auger

Now it's possible to run all tests using run_tests.sh with a
QEMU specified by the $QEMU environment variable that also
includes additional parameters. E.g. QEMU="/path/to/qemu -icount 8"

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Alex Bennée <alex.bennee@linaro.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Laurent Vivier <lvivier@redhat.com>
Cc: Thomas Huth <thuth@redhat.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Janosch Frank <frankja@linux.ibm.com>
Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 scripts/arch-run.bash | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index d3ca19d49952..ebe4d3cb2a09 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -172,8 +172,20 @@ search_qemu_binary ()
 	local save_path=$PATH
 	local qemucmd qemu
 
+	if [ -n "$QEMU" ]; then
+		set -- $QEMU
+		if ! "$1" --help 2>/dev/null | grep -q 'QEMU'; then
+			echo "\$QEMU environment variable not set to a QEMU binary." >&2
+			return 2
+		fi
+		qemu=$(command -v "$1")
+		shift
+		echo "$qemu $@"
+		return
+	fi
+
 	export PATH=$PATH:/usr/libexec
-	for qemucmd in ${QEMU:-qemu-system-$ARCH_NAME qemu-kvm}; do
+	for qemucmd in qemu-system-$ARCH_NAME qemu-kvm; do
 		if $qemucmd --help 2>/dev/null | grep -q 'QEMU'; then
 			qemu="$qemucmd"
 			break
-- 
2.21.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH kvm-unit-tests 2/2] runtime: Introduce VMM_PARAMS
  2020-02-13 14:32 [PATCH kvm-unit-tests 0/2] runtime: Allow additional VMM parameters to be provided Andrew Jones
  2020-02-13 14:32 ` [PATCH kvm-unit-tests 1/2] arch-run: Allow $QEMU to include parameters Andrew Jones
@ 2020-02-13 14:33 ` Andrew Jones
  2020-02-14 10:14   ` David Hildenbrand
  2020-02-14 10:27   ` Paolo Bonzini
  1 sibling, 2 replies; 14+ messages in thread
From: Andrew Jones @ 2020-02-13 14:33 UTC (permalink / raw)
  To: kvm
  Cc: peter.maydell, alex.bennee, pbonzini, lvivier, thuth, david,
	frankja, eric.auger

Users may need to temporarily provide additional VMM parameters.
The VMM_PARAMS environment variable provides a way to do that.
We take care to make sure when executing ./run_tests.sh that
the VMM_PARAMS parameters come last, allowing unittests.cfg
parameters to be overridden. However, when running a command
line like `$ARCH/run $TEST $PARAMS` we want $PARAMS to override
$VMM_PARAMS, so we ensure that too.

Additional QEMU parameters can still be provided by appending
them to the $QEMU environment variable when it provides the
path to QEMU, but as those parameters will then be the first
in the command line they cannot override anything, and may
themselves be overridden.

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Alex Bennée <alex.bennee@linaro.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Laurent Vivier <lvivier@redhat.com>
Cc: Thomas Huth <thuth@redhat.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Janosch Frank <frankja@linux.ibm.com>
Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 README.md            | 5 +++++
 arm/run              | 4 +++-
 powerpc/run          | 4 +++-
 s390x/run            | 1 +
 scripts/runtime.bash | 4 +++-
 x86/run              | 4 +++-
 6 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/README.md b/README.md
index 396fbf809a4e..834d6202ac97 100644
--- a/README.md
+++ b/README.md
@@ -47,6 +47,11 @@ environment variable:
 
     QEMU=/tmp/qemu/x86_64-softmmu/qemu-system-x86_64 ./x86-run ./x86/msr.flat
 
+If QEMU additional parameters are needed for all tests, then they may be
+provided in the VMM_PARAMS environment variable:
+
+    VMM_PARAMS="-additional parameters -go here" ./run_tests.sh
+
 To select an accelerator, for example "kvm" or "tcg", specify the
 ACCEL=name environment variable:
 
diff --git a/arm/run b/arm/run
index 277db9bb4a02..a8a93591b825 100755
--- a/arm/run
+++ b/arm/run
@@ -60,7 +60,9 @@ fi
 
 M+=",accel=$ACCEL"
 command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev"
-command+=" -display none -serial stdio -kernel"
+command+=" -display none -serial stdio"
+command+=" $VMM_PARAMS"
+command+=" -kernel"
 command="$(timeout_cmd) $command"
 
 run_qemu $command "$@"
diff --git a/powerpc/run b/powerpc/run
index 597ab96ed8a8..b07aa18f26bf 100755
--- a/powerpc/run
+++ b/powerpc/run
@@ -23,7 +23,9 @@ fi
 M='-machine pseries'
 M+=",accel=$ACCEL"
 command="$qemu -nodefaults $M -bios $FIRMWARE"
-command+=" -display none -serial stdio -kernel"
+command+=" -display none -serial stdio"
+command+=" $VMM_PARAMS"
+command+=" -kernel"
 command="$(migration_cmd) $(timeout_cmd) $command"
 
 # powerpc tests currently exit with rtas-poweroff, which exits with 0.
diff --git a/s390x/run b/s390x/run
index 0980504448ce..9bfb95397064 100755
--- a/s390x/run
+++ b/s390x/run
@@ -19,6 +19,7 @@ M='-machine s390-ccw-virtio'
 M+=",accel=$ACCEL"
 command="$qemu -nodefaults -nographic $M"
 command+=" -chardev stdio,id=con0 -device sclpconsole,chardev=con0"
+command+=" $VMM_PARAMS"
 command+=" -kernel"
 command="$(timeout_cmd) $command"
 
diff --git a/scripts/runtime.bash b/scripts/runtime.bash
index eb6089091e23..5ea3772d9b2b 100644
--- a/scripts/runtime.bash
+++ b/scripts/runtime.bash
@@ -30,7 +30,9 @@ premature_failure()
 get_cmdline()
 {
     local kernel=$1
-    echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts"
+
+    # Move VMM_PARAMS to the end to override parameters provided in unittests.cfg:extra_params
+    echo "VMM_PARAMS= TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts $VMM_PARAMS"
 }
 
 skip_nodefault()
diff --git a/x86/run b/x86/run
index 1ac91f1d880f..3770c16ad4e6 100755
--- a/x86/run
+++ b/x86/run
@@ -38,7 +38,9 @@ else
 fi
 
 command="${qemu} -nodefaults $pc_testdev -vnc none -serial stdio $pci_testdev"
-command+=" -machine accel=$ACCEL -kernel"
+command+=" -machine accel=$ACCEL"
+command+=" $VMM_PARAMS"
+command+=" -kernel"
 command="$(timeout_cmd) $command"
 
 run_qemu ${command} "$@"
-- 
2.21.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH kvm-unit-tests 2/2] runtime: Introduce VMM_PARAMS
  2020-02-13 14:33 ` [PATCH kvm-unit-tests 2/2] runtime: Introduce VMM_PARAMS Andrew Jones
@ 2020-02-14 10:14   ` David Hildenbrand
  2020-02-14 10:30     ` Andrew Jones
  2020-02-14 10:27   ` Paolo Bonzini
  1 sibling, 1 reply; 14+ messages in thread
From: David Hildenbrand @ 2020-02-14 10:14 UTC (permalink / raw)
  To: Andrew Jones, kvm
  Cc: peter.maydell, alex.bennee, pbonzini, lvivier, thuth, frankja,
	eric.auger

On 13.02.20 15:33, Andrew Jones wrote:
> Users may need to temporarily provide additional VMM parameters.
> The VMM_PARAMS environment variable provides a way to do that.
> We take care to make sure when executing ./run_tests.sh that
> the VMM_PARAMS parameters come last, allowing unittests.cfg
> parameters to be overridden. However, when running a command
> line like `$ARCH/run $TEST $PARAMS` we want $PARAMS to override
> $VMM_PARAMS, so we ensure that too.

While reading this, I was wondering why not simply "$QEMU_PARAMS"?


-- 
Thanks,

David / dhildenb


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH kvm-unit-tests 2/2] runtime: Introduce VMM_PARAMS
  2020-02-13 14:33 ` [PATCH kvm-unit-tests 2/2] runtime: Introduce VMM_PARAMS Andrew Jones
  2020-02-14 10:14   ` David Hildenbrand
@ 2020-02-14 10:27   ` Paolo Bonzini
  2020-02-14 10:38     ` Andrew Jones
  1 sibling, 1 reply; 14+ messages in thread
From: Paolo Bonzini @ 2020-02-14 10:27 UTC (permalink / raw)
  To: Andrew Jones, kvm
  Cc: peter.maydell, alex.bennee, lvivier, thuth, david, frankja, eric.auger

On 13/02/20 15:33, Andrew Jones wrote:
> Users may need to temporarily provide additional VMM parameters.
> The VMM_PARAMS environment variable provides a way to do that.
> We take care to make sure when executing ./run_tests.sh that
> the VMM_PARAMS parameters come last, allowing unittests.cfg
> parameters to be overridden. However, when running a command
> line like `$ARCH/run $TEST $PARAMS` we want $PARAMS to override
> $VMM_PARAMS, so we ensure that too.
> 
> Additional QEMU parameters can still be provided by appending
> them to the $QEMU environment variable when it provides the
> path to QEMU, but as those parameters will then be the first
> in the command line they cannot override anything, and may
> themselves be overridden.

What about looking for "--" and passing to QEMU all parameters after it?

Paolo


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH kvm-unit-tests 2/2] runtime: Introduce VMM_PARAMS
  2020-02-14 10:14   ` David Hildenbrand
@ 2020-02-14 10:30     ` Andrew Jones
  2020-02-14 10:33       ` David Hildenbrand
  0 siblings, 1 reply; 14+ messages in thread
From: Andrew Jones @ 2020-02-14 10:30 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: kvm, peter.maydell, alex.bennee, pbonzini, lvivier, thuth,
	frankja, eric.auger

On Fri, Feb 14, 2020 at 11:14:49AM +0100, David Hildenbrand wrote:
> On 13.02.20 15:33, Andrew Jones wrote:
> > Users may need to temporarily provide additional VMM parameters.
> > The VMM_PARAMS environment variable provides a way to do that.
> > We take care to make sure when executing ./run_tests.sh that
> > the VMM_PARAMS parameters come last, allowing unittests.cfg
> > parameters to be overridden. However, when running a command
> > line like `$ARCH/run $TEST $PARAMS` we want $PARAMS to override
> > $VMM_PARAMS, so we ensure that too.
> 
> While reading this, I was wondering why not simply "$QEMU_PARAMS"?

I'd like to slowly move away from assuming QEMU is the VMM. We
already have support for kvmtool (to some degree) and I'd like
to see that support increase. Also, maybe we'll eventually add
support for a rust-vmm reference VMM to drive kvm-unit-tests.
IOW, rather than add yet another QEMU named variable I'd like
to start a trend of using VMM named variables. Actually, we
could add VMM named alternatives for the QEMU named ones we have
now and start using them in the scripts. We'd just need to remap
the old names for backward compatibility early in the run.

Thanks,
drew


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH kvm-unit-tests 1/2] arch-run: Allow $QEMU to include parameters
  2020-02-13 14:32 ` [PATCH kvm-unit-tests 1/2] arch-run: Allow $QEMU to include parameters Andrew Jones
@ 2020-02-14 10:31   ` Paolo Bonzini
  2020-02-14 10:51     ` Andrew Jones
  0 siblings, 1 reply; 14+ messages in thread
From: Paolo Bonzini @ 2020-02-14 10:31 UTC (permalink / raw)
  To: Andrew Jones, kvm
  Cc: peter.maydell, alex.bennee, lvivier, thuth, david, frankja, eric.auger

On 13/02/20 15:32, Andrew Jones wrote:
> +	if [ -n "$QEMU" ]; then
> +		set -- $QEMU
> +		if ! "$1" --help 2>/dev/null | grep -q 'QEMU'; then
> +			echo "\$QEMU environment variable not set to a QEMU binary." >&2
> +			return 2
> +		fi
> +		qemu=$(command -v "$1")
> +		shift
> +		echo "$qemu $@"

I think $* is more appropriate here.  Something like "foo $@ bar" has a
weird effect:

	$ set -x
	$ set a b c

	$ echo "foo $@ bar"
	+ echo 'foo a' b 'c bar'
	foo a b c bar

	$ echo "foo $* bar"
	+ echo 'foo a b c bar'
	foo a b c bar

Otherwise, this is a good idea.

Thanks,

Paolo

> +		return
> +	fi
> +


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH kvm-unit-tests 2/2] runtime: Introduce VMM_PARAMS
  2020-02-14 10:30     ` Andrew Jones
@ 2020-02-14 10:33       ` David Hildenbrand
  2020-02-14 10:44         ` Andrew Jones
  0 siblings, 1 reply; 14+ messages in thread
From: David Hildenbrand @ 2020-02-14 10:33 UTC (permalink / raw)
  To: Andrew Jones
  Cc: kvm, peter.maydell, alex.bennee, pbonzini, lvivier, thuth,
	frankja, eric.auger

On 14.02.20 11:30, Andrew Jones wrote:
> On Fri, Feb 14, 2020 at 11:14:49AM +0100, David Hildenbrand wrote:
>> On 13.02.20 15:33, Andrew Jones wrote:
>>> Users may need to temporarily provide additional VMM parameters.
>>> The VMM_PARAMS environment variable provides a way to do that.
>>> We take care to make sure when executing ./run_tests.sh that
>>> the VMM_PARAMS parameters come last, allowing unittests.cfg
>>> parameters to be overridden. However, when running a command
>>> line like `$ARCH/run $TEST $PARAMS` we want $PARAMS to override
>>> $VMM_PARAMS, so we ensure that too.
>>
>> While reading this, I was wondering why not simply "$QEMU_PARAMS"?
> 
> I'd like to slowly move away from assuming QEMU is the VMM. We
> already have support for kvmtool (to some degree) and I'd like
> to see that support increase. Also, maybe we'll eventually add
> support for a rust-vmm reference VMM to drive kvm-unit-tests.
> IOW, rather than add yet another QEMU named variable I'd like
> to start a trend of using VMM named variables. Actually, we
> could add VMM named alternatives for the QEMU named ones we have
> now and start using them in the scripts. We'd just need to remap
> the old names for backward compatibility early in the run.

And we do expect other VMMs to eat the same set of parameters? If it's
QEMU specific, I think we should make this clear.

But no strong opinion.

-- 
Thanks,

David / dhildenb


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH kvm-unit-tests 2/2] runtime: Introduce VMM_PARAMS
  2020-02-14 10:27   ` Paolo Bonzini
@ 2020-02-14 10:38     ` Andrew Jones
  2020-02-14 10:50       ` Paolo Bonzini
  0 siblings, 1 reply; 14+ messages in thread
From: Andrew Jones @ 2020-02-14 10:38 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: kvm, peter.maydell, alex.bennee, lvivier, thuth, david, frankja,
	eric.auger

On Fri, Feb 14, 2020 at 11:27:17AM +0100, Paolo Bonzini wrote:
> On 13/02/20 15:33, Andrew Jones wrote:
> > Users may need to temporarily provide additional VMM parameters.
> > The VMM_PARAMS environment variable provides a way to do that.
> > We take care to make sure when executing ./run_tests.sh that
> > the VMM_PARAMS parameters come last, allowing unittests.cfg
> > parameters to be overridden. However, when running a command
> > line like `$ARCH/run $TEST $PARAMS` we want $PARAMS to override
> > $VMM_PARAMS, so we ensure that too.
> > 
> > Additional QEMU parameters can still be provided by appending
> > them to the $QEMU environment variable when it provides the
> > path to QEMU, but as those parameters will then be the first
> > in the command line they cannot override anything, and may
> > themselves be overridden.
> 
> What about looking for "--" and passing to QEMU all parameters after it?
>

That was the way we originally planned on doing it when Alex Bennee posted
his patch. However since d4d34e648482 ("run_tests: fix command line
options handling") the "--" has become the divider between run_tests.sh
parameter inputs and individually specified tests. We'd have to change
that behavior, potentially breaking command lines, to go back to the
"--" approach.

Also, the nice thing about using an environment variable is that it works
with standalone tests too.

 VMM_PARAMS="-foo bar" tests/mytest

will run the test with "-foo bar" appended to the command line. We could
modify mkstandalone.sh to get that feature too (allowing the additional
parameters to be given after tests/mytest), but with VMM_PARAMS we don't
have to.

Thanks,
drew


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH kvm-unit-tests 2/2] runtime: Introduce VMM_PARAMS
  2020-02-14 10:33       ` David Hildenbrand
@ 2020-02-14 10:44         ` Andrew Jones
  0 siblings, 0 replies; 14+ messages in thread
From: Andrew Jones @ 2020-02-14 10:44 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: kvm, peter.maydell, alex.bennee, pbonzini, lvivier, thuth,
	frankja, eric.auger

On Fri, Feb 14, 2020 at 11:33:55AM +0100, David Hildenbrand wrote:
> On 14.02.20 11:30, Andrew Jones wrote:
> > On Fri, Feb 14, 2020 at 11:14:49AM +0100, David Hildenbrand wrote:
> >> On 13.02.20 15:33, Andrew Jones wrote:
> >>> Users may need to temporarily provide additional VMM parameters.
> >>> The VMM_PARAMS environment variable provides a way to do that.
> >>> We take care to make sure when executing ./run_tests.sh that
> >>> the VMM_PARAMS parameters come last, allowing unittests.cfg
> >>> parameters to be overridden. However, when running a command
> >>> line like `$ARCH/run $TEST $PARAMS` we want $PARAMS to override
> >>> $VMM_PARAMS, so we ensure that too.
> >>
> >> While reading this, I was wondering why not simply "$QEMU_PARAMS"?
> > 
> > I'd like to slowly move away from assuming QEMU is the VMM. We
> > already have support for kvmtool (to some degree) and I'd like
> > to see that support increase. Also, maybe we'll eventually add
> > support for a rust-vmm reference VMM to drive kvm-unit-tests.
> > IOW, rather than add yet another QEMU named variable I'd like
> > to start a trend of using VMM named variables. Actually, we
> > could add VMM named alternatives for the QEMU named ones we have
> > now and start using them in the scripts. We'd just need to remap
> > the old names for backward compatibility early in the run.
> 
> And we do expect other VMMs to eat the same set of parameters? If it's
> QEMU specific, I think we should make this clear.

It won't matter. The parameters are VMM specific. Whether or not where
they show up on the command line has the same semantics as QEMU could
be an issue, but what the parameters are is up to the user, and they
should know which vmm they're currently using.

kvmtool support isn't complete because the run scripts haven't yet
provided a way for non-QEMU vmms to use the unittests.cfg files.
Andre had some patches once, though. So maybe someday.

Thanks,
drew


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH kvm-unit-tests 2/2] runtime: Introduce VMM_PARAMS
  2020-02-14 10:38     ` Andrew Jones
@ 2020-02-14 10:50       ` Paolo Bonzini
  2020-02-14 11:50         ` Andrew Jones
  0 siblings, 1 reply; 14+ messages in thread
From: Paolo Bonzini @ 2020-02-14 10:50 UTC (permalink / raw)
  To: Andrew Jones
  Cc: kvm, peter.maydell, alex.bennee, lvivier, thuth, david, frankja,
	eric.auger

On 14/02/20 11:38, Andrew Jones wrote:
> That was the way we originally planned on doing it when Alex Bennee posted
> his patch. However since d4d34e648482 ("run_tests: fix command line
> options handling") the "--" has become the divider between run_tests.sh
> parameter inputs and individually specified tests.

Hmm, more precisely that is how getopt separates options from other 
parameters.

Since we don't expect test names starting with a dash, we could do 
something like (untested):

diff --git a/run_tests.sh b/run_tests.sh
index 01e36dc..8b71cf2 100755
--- a/run_tests.sh
+++ b/run_tests.sh
@@ -35,7 +35,20 @@ RUNTIME_arch_run="./$TEST_DIR/run"
 source scripts/runtime.bash
 
 only_tests=""
-args=`getopt -u -o ag:htj:v -l all,group:,help,tap13,parallel:,verbose -- $*`
+args=""
+vmm_args=""
+while [ $# -gt 0 ]; do
+   if test "$1" = --; then
+       shift
+       vmm_args=$*
+       break
+   else
+       args="args $1"
+       shift
+   fi
+done
+
+args=`getopt -u -o ag:htj:v -l all,group:,help,tap13,parallel:,verbose -- $args`
 [ $? -ne 0 ] && exit 2;
 set -- $args;
 while [ $# -gt 0 ]; do

> will run the test with "-foo bar" appended to the command line. We could
> modify mkstandalone.sh to get that feature too (allowing the additional
> parameters to be given after tests/mytest), but with VMM_PARAMS we don't
> have to.

Yes, having consistency between standalone and run_tests.sh is a good thing.

Paolo


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH kvm-unit-tests 1/2] arch-run: Allow $QEMU to include parameters
  2020-02-14 10:31   ` Paolo Bonzini
@ 2020-02-14 10:51     ` Andrew Jones
  0 siblings, 0 replies; 14+ messages in thread
From: Andrew Jones @ 2020-02-14 10:51 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: kvm, peter.maydell, alex.bennee, lvivier, thuth, david, frankja,
	eric.auger

On Fri, Feb 14, 2020 at 11:31:04AM +0100, Paolo Bonzini wrote:
> On 13/02/20 15:32, Andrew Jones wrote:
> > +	if [ -n "$QEMU" ]; then
> > +		set -- $QEMU
> > +		if ! "$1" --help 2>/dev/null | grep -q 'QEMU'; then
> > +			echo "\$QEMU environment variable not set to a QEMU binary." >&2
> > +			return 2
> > +		fi
> > +		qemu=$(command -v "$1")
> > +		shift
> > +		echo "$qemu $@"
> 
> I think $* is more appropriate here.  Something like "foo $@ bar" has a
> weird effect:

Will fix for v2.

> 
> 	$ set -x
> 	$ set a b c
> 
> 	$ echo "foo $@ bar"
> 	+ echo 'foo a' b 'c bar'
> 	foo a b c bar
> 
> 	$ echo "foo $* bar"
> 	+ echo 'foo a b c bar'
> 	foo a b c bar
> 
> Otherwise, this is a good idea.

Thanks,
drew

> 
> Thanks,
> 
> Paolo
> 
> > +		return
> > +	fi
> > +
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH kvm-unit-tests 2/2] runtime: Introduce VMM_PARAMS
  2020-02-14 10:50       ` Paolo Bonzini
@ 2020-02-14 11:50         ` Andrew Jones
  2020-02-14 13:10           ` Paolo Bonzini
  0 siblings, 1 reply; 14+ messages in thread
From: Andrew Jones @ 2020-02-14 11:50 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: kvm, peter.maydell, alex.bennee, lvivier, thuth, david, frankja,
	eric.auger

On Fri, Feb 14, 2020 at 11:50:08AM +0100, Paolo Bonzini wrote:
> On 14/02/20 11:38, Andrew Jones wrote:
> > That was the way we originally planned on doing it when Alex Bennee posted
> > his patch. However since d4d34e648482 ("run_tests: fix command line
> > options handling") the "--" has become the divider between run_tests.sh
> > parameter inputs and individually specified tests.
> 
> Hmm, more precisely that is how getopt separates options from other 
> parameters.
> 
> Since we don't expect test names starting with a dash, we could do 
> something like (untested):
> 
> diff --git a/run_tests.sh b/run_tests.sh
> index 01e36dc..8b71cf2 100755
> --- a/run_tests.sh
> +++ b/run_tests.sh
> @@ -35,7 +35,20 @@ RUNTIME_arch_run="./$TEST_DIR/run"
>  source scripts/runtime.bash
>  
>  only_tests=""
> -args=`getopt -u -o ag:htj:v -l all,group:,help,tap13,parallel:,verbose -- $*`
> +args=""
> +vmm_args=""
> +while [ $# -gt 0 ]; do
> +   if test "$1" = --; then
> +       shift
> +       vmm_args=$*
> +       break
> +   else
> +       args="args $1"
> +       shift
> +   fi
> +done
> +
> +args=`getopt -u -o ag:htj:v -l all,group:,help,tap13,parallel:,verbose -- $args`
>  [ $? -ne 0 ] && exit 2;
>  set -- $args;
>  while [ $# -gt 0 ]; do

Unfortunately it regresses the current command line. Here's what I tested

Before
------

$ ./run_tests.sh -j 2 -v pmu psci
VMM_PARAMS= TESTNAME=pmu TIMEOUT=90s ACCEL= ./arm/run arm/pmu.flat -smp 1
VMM_PARAMS= TESTNAME=psci TIMEOUT=90s ACCEL= ./arm/run arm/psci.flat -smp $MAX_SMP
PASS pmu (3 tests)
PASS psci (4 tests)

$ ./run_tests.sh pmu psci -j 2 -v
VMM_PARAMS= TESTNAME=pmu TIMEOUT=90s ACCEL= ./arm/run arm/pmu.flat -smp 1
VMM_PARAMS= TESTNAME=psci TIMEOUT=90s ACCEL= ./arm/run arm/psci.flat -smp $MAX_SMP
PASS pmu (3 tests)
PASS psci (4 tests)

$ ./run_tests.sh -j 2 -v -- pmu psci
VMM_PARAMS= TESTNAME=pmu TIMEOUT=90s ACCEL= ./arm/run arm/pmu.flat -smp 1
VMM_PARAMS= TESTNAME=psci TIMEOUT=90s ACCEL= ./arm/run arm/psci.flat -smp $MAX_SMP
PASS pmu (3 tests)
PASS psci (4 tests)

After
-----

$ ./run_tests.sh -j 2 -v pmu psci
PASS psci (4 tests)

$ ./run_tests.sh pmu psci -j 2 -v
$

(no output)

$ ./run_tests.sh -j 2 -v -- pmu psci
$

(no output)

> 
> > will run the test with "-foo bar" appended to the command line. We could
> > modify mkstandalone.sh to get that feature too (allowing the additional
> > parameters to be given after tests/mytest), but with VMM_PARAMS we don't
> > have to.
> 
> Yes, having consistency between standalone and run_tests.sh is a good thing.
>

So should we:

 1) try to get "--" working, but also keep the environment variable as
    an alternative which works with standalone?
 2) drop the environment variable, get "--" working and modify
    mkstandalone.sh?
 3) drop the environment variable, get "--" working, but forget about
    standalone?
 4) just keep the VMM_PARAMS approach and forget about "--"?

Thanks,
drew


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH kvm-unit-tests 2/2] runtime: Introduce VMM_PARAMS
  2020-02-14 11:50         ` Andrew Jones
@ 2020-02-14 13:10           ` Paolo Bonzini
  0 siblings, 0 replies; 14+ messages in thread
From: Paolo Bonzini @ 2020-02-14 13:10 UTC (permalink / raw)
  To: Andrew Jones
  Cc: kvm, peter.maydell, alex.bennee, lvivier, thuth, david, frankja,
	eric.auger

On 14/02/20 12:50, Andrew Jones wrote:
> After
> -----
> 
> $ ./run_tests.sh -j 2 -v pmu psci
> PASS psci (4 tests)
> 
> $ ./run_tests.sh pmu psci -j 2 -v
> $
> 
> (no output)

Bugs. :)

> $ ./run_tests.sh -j 2 -v -- pmu psci
> $
> 
> (no output)

This is intended, it shouldn't be a big deal because we don't have tests
starting with a dash.

Paolo


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2020-02-14 13:10 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-13 14:32 [PATCH kvm-unit-tests 0/2] runtime: Allow additional VMM parameters to be provided Andrew Jones
2020-02-13 14:32 ` [PATCH kvm-unit-tests 1/2] arch-run: Allow $QEMU to include parameters Andrew Jones
2020-02-14 10:31   ` Paolo Bonzini
2020-02-14 10:51     ` Andrew Jones
2020-02-13 14:33 ` [PATCH kvm-unit-tests 2/2] runtime: Introduce VMM_PARAMS Andrew Jones
2020-02-14 10:14   ` David Hildenbrand
2020-02-14 10:30     ` Andrew Jones
2020-02-14 10:33       ` David Hildenbrand
2020-02-14 10:44         ` Andrew Jones
2020-02-14 10:27   ` Paolo Bonzini
2020-02-14 10:38     ` Andrew Jones
2020-02-14 10:50       ` Paolo Bonzini
2020-02-14 11:50         ` Andrew Jones
2020-02-14 13:10           ` Paolo Bonzini

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).