On Mon, Dec 21, 2015 at 06:04:20PM +0100, Radim Krčmář wrote:
> 2015-12-17 14:10-0600, Andrew Jones:
> > Signed-off-by: Andrew Jones <drjo...@redhat.com>
> > ---
> > diff --git a/arm/run b/arm/run
> > @@ -75,10 +75,14 @@ chr_testdev+=' -device virtconsole,chardev=ctd -chardev 
> > testdev,id=ctd'
> >  M+=",accel=$ACCEL"
> >  command="$qemu $M -cpu $processor $chr_testdev"
> >  command+=" -display none -serial stdio -kernel"
> > -echo $command "$@"
> > +
> > +if [ "$TIMEOUT" ]; then
> > +   timeout_cmd="timeout --foreground $TIMEOUT"
> 
> (command="timeout --foreground $TIMEOUT $command" # to keep the clutter
>  down.)
> 
> > +fi
> 
> (We paste it three times, so I'd rather see this abstracted in a
>  "scripts/" library.)

Sounds good

> 
> > diff --git a/run_tests.sh b/run_tests.sh
> > @@ -21,6 +21,7 @@ function run()
> > +    local timeout="${9:-$TIMEOUT}"
> > diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
> > @@ -97,8 +98,12 @@ if [ "\$QEMU" ]; then
> > +if [ "$timeout" ]; then
> > +   timeout_cmd='timeout --foreground $timeout'
> 
> Both would be nicer if they took the TIMEOUT variable as an override.

Everything already takes TIMEOUT as an override, i.e.

TIMEOUT=3 ./run_tests.sh

and

TIMEOUT=3 arm/run arm/test.flat

will both already set a timeout for any test that didn't have a timeout
set in unittests.cfg, or wasn't run with run()/unittests.cfg. Or, did
you mean that you'd prefer TIMEOUT to override the timeout in
unittests.cfg? That does make some sense, in the case the one in the
config is longer than desired, however it also makes sense the way I
have it now when the one in the config is shorter than TIMEOUT (the
fallback default). I think I like it this way better.

> We already don't do that for accel and the patch seems ok in other
> regards,

Hmm, for accel I see a need for a patch allowing us to do

ACCEL=?? ./run_tests.sh

as I already have for TIMEOUT. Also, for both I should add a
mkstandalone patch allowing

TIMEOUT=? ACCEL=? make standalone

Thanks,
drew

> 
> Reviewed-by: Radim Krčmář <rkrc...@redhat.com>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to