| Message ID | 20200812092705.17774-4-mhartmay@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: Add Protected VM support | expand |
On Wed, Aug 12, 2020 at 11:27:04AM +0200, Marc Hartmayer wrote: > This allows us, for example, to auto generate a new test case based on > an existing test case. > > Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> > --- > run_tests.sh | 2 +- > scripts/common.bash | 13 +++++++++++++ > scripts/mkstandalone.sh | 2 +- > 3 files changed, 15 insertions(+), 2 deletions(-) > > diff --git a/run_tests.sh b/run_tests.sh > index 24aba9cc3a98..23658392c488 100755 > --- a/run_tests.sh > +++ b/run_tests.sh > @@ -160,7 +160,7 @@ trap "wait; exit 130" SIGINT > # preserve stdout so that process_test_output output can write TAP to it > exec 3>&1 > test "$tap_output" == "yes" && exec > /dev/null > - for_each_unittest $config run_task > + for_each_unittest $config run_task arch_cmd Let's just require that arch cmd hook be specified by the "$arch_cmd" variable. Then we don't need to pass it to for_each_unittest. > ) | postprocess_suite_output > > # wait until all tasks finish > diff --git a/scripts/common.bash b/scripts/common.bash > index f9c15fd304bd..62931a40b79a 100644 > --- a/scripts/common.bash > +++ b/scripts/common.bash > @@ -1,8 +1,15 @@ > +function arch_cmd() > +{ > + # Dummy function, can be overwritten by architecture dependent > + # code > + return > +} This dummy function appears unused and can be dropped. > > function for_each_unittest() > { > local unittests="$1" > local cmd="$2" > + local arch_cmd="${3-}" > local testname > local smp > local kernel > @@ -19,6 +26,9 @@ function for_each_unittest() > if [[ "$line" =~ ^\[(.*)\]$ ]]; then > if [ -n "${testname}" ]; then > "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" > + if [ "${arch_cmd}" ]; then > + "${arch_cmd}" "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" > + fi Rather than assuming we should run both $cmd ... and $arch_cmd $cmd ..., let's just run $arch_cmd $cmd ..., when it exists. If $arch_cmd wants to run $cmd ... first, then it can do so itself. > fi > testname=${BASH_REMATCH[1]} > smp=1 > @@ -49,6 +59,9 @@ function for_each_unittest() > done > if [ -n "${testname}" ]; then > "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" > + if [ "${arch_cmd}" ]; then > + "${arch_cmd}" "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" > + fi > fi > exec {fd}<&- > } > diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh > index cefdec30cb33..3b18c0cf090b 100755 > --- a/scripts/mkstandalone.sh > +++ b/scripts/mkstandalone.sh > @@ -128,4 +128,4 @@ fi > > mkdir -p tests > > -for_each_unittest $cfg mkstandalone > +for_each_unittest $cfg mkstandalone arch_cmd > -- > 2.25.4 > In summary, I think this patch should just be diff --git a/scripts/common.bash b/scripts/common.bash index 9a6ebbd7f287..b409b0529ea6 100644 --- a/scripts/common.bash +++ b/scripts/common.bash @@ -17,7 +17,7 @@ function for_each_unittest() while read -r -u $fd line; do if [[ "$line" =~ ^\[(.*)\]$ ]]; then - "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" + "$arch_cmd" "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" testname=${BASH_REMATCH[1]} smp=1 kernel="" @@ -45,6 +45,6 @@ function for_each_unittest() timeout=${BASH_REMATCH[1]} fi done - "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" + "$arch_cmd" "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" exec {fd}<&- } Thanks, drew
On Thu, Aug 13, 2020 at 10:30 AM +0200, Andrew Jones <drjones@redhat.com> wrote: > On Wed, Aug 12, 2020 at 11:27:04AM +0200, Marc Hartmayer wrote: >> This allows us, for example, to auto generate a new test case based on >> an existing test case. >> >> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> >> --- >> run_tests.sh | 2 +- >> scripts/common.bash | 13 +++++++++++++ >> scripts/mkstandalone.sh | 2 +- >> 3 files changed, 15 insertions(+), 2 deletions(-) >> >> diff --git a/run_tests.sh b/run_tests.sh >> index 24aba9cc3a98..23658392c488 100755 >> --- a/run_tests.sh >> +++ b/run_tests.sh >> @@ -160,7 +160,7 @@ trap "wait; exit 130" SIGINT >> # preserve stdout so that process_test_output output can write TAP to it >> exec 3>&1 >> test "$tap_output" == "yes" && exec > /dev/null >> - for_each_unittest $config run_task >> + for_each_unittest $config run_task arch_cmd > > Let's just require that arch cmd hook be specified by the "$arch_cmd" > variable. Then we don't need to pass it to for_each_unittest. Where is it then specified? > >> ) | postprocess_suite_output >> >> # wait until all tasks finish >> diff --git a/scripts/common.bash b/scripts/common.bash >> index f9c15fd304bd..62931a40b79a 100644 >> --- a/scripts/common.bash >> +++ b/scripts/common.bash >> @@ -1,8 +1,15 @@ >> +function arch_cmd() >> +{ >> + # Dummy function, can be overwritten by architecture dependent >> + # code >> + return >> +} > > This dummy function appears unused and can be dropped. So what is then used if the function is not defined by the architecture specific code? > >> >> function for_each_unittest() >> { >> local unittests="$1" >> local cmd="$2" >> + local arch_cmd="${3-}" >> local testname >> local smp >> local kernel >> @@ -19,6 +26,9 @@ function for_each_unittest() >> if [[ "$line" =~ ^\[(.*)\]$ ]]; then >> if [ -n "${testname}" ]; then >> "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" >> + if [ "${arch_cmd}" ]; then >> + "${arch_cmd}" "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" >> + fi > > Rather than assuming we should run both $cmd ... and $arch_cmd $cmd ..., > let's just run $arch_cmd $cmd ..., when it exists. If $arch_cmd wants to > run $cmd ... first, then it can do so itself. Yep, makes sense. > >> fi >> testname=${BASH_REMATCH[1]} >> smp=1 >> @@ -49,6 +59,9 @@ function for_each_unittest() >> done >> if [ -n "${testname}" ]; then >> "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" >> + if [ "${arch_cmd}" ]; then >> + "${arch_cmd}" "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" >> + fi >> fi >> exec {fd}<&- >> } >> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh >> index cefdec30cb33..3b18c0cf090b 100755 >> --- a/scripts/mkstandalone.sh >> +++ b/scripts/mkstandalone.sh >> @@ -128,4 +128,4 @@ fi >> >> mkdir -p tests >> >> -for_each_unittest $cfg mkstandalone >> +for_each_unittest $cfg mkstandalone arch_cmd >> -- >> 2.25.4 >> > > In summary, I think this patch should just be > > diff --git a/scripts/common.bash b/scripts/common.bash > index 9a6ebbd7f287..b409b0529ea6 100644 > --- a/scripts/common.bash > +++ b/scripts/common.bash > @@ -17,7 +17,7 @@ function for_each_unittest() > > while read -r -u $fd line; do > if [[ "$line" =~ ^\[(.*)\]$ ]]; then > - "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" > + "$arch_cmd" "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" If we remove the $arch_cmd variable we can directly use: arch_cmd "$cmd" … > testname=${BASH_REMATCH[1]} > smp=1 > kernel="" > @@ -45,6 +45,6 @@ function for_each_unittest() > timeout=${BASH_REMATCH[1]} > fi > done > - "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" > + "$arch_cmd" "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" > exec {fd}<&- > } > > > Thanks, > drew >
On Fri, Aug 14, 2020 at 03:06:36PM +0200, Marc Hartmayer wrote: > On Thu, Aug 13, 2020 at 10:30 AM +0200, Andrew Jones <drjones@redhat.com> wrote: > > On Wed, Aug 12, 2020 at 11:27:04AM +0200, Marc Hartmayer wrote: > >> This allows us, for example, to auto generate a new test case based on > >> an existing test case. > >> > >> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> > >> --- > >> run_tests.sh | 2 +- > >> scripts/common.bash | 13 +++++++++++++ > >> scripts/mkstandalone.sh | 2 +- > >> 3 files changed, 15 insertions(+), 2 deletions(-) > >> > >> diff --git a/run_tests.sh b/run_tests.sh > >> index 24aba9cc3a98..23658392c488 100755 > >> --- a/run_tests.sh > >> +++ b/run_tests.sh > >> @@ -160,7 +160,7 @@ trap "wait; exit 130" SIGINT > >> # preserve stdout so that process_test_output output can write TAP to it > >> exec 3>&1 > >> test "$tap_output" == "yes" && exec > /dev/null > >> - for_each_unittest $config run_task > >> + for_each_unittest $config run_task arch_cmd > > > > Let's just require that arch cmd hook be specified by the "$arch_cmd" > > variable. Then we don't need to pass it to for_each_unittest. > > Where is it then specified? Just using it that way in the source is enough. We should probably call it $ARCH_CMD to indicate that it's a special variable. Also, we could return it from a $(arch_cmd) function, which is how $(migration_cmd) and $(timeout_cmd) work. > > > > >> ) | postprocess_suite_output > >> > >> # wait until all tasks finish > >> diff --git a/scripts/common.bash b/scripts/common.bash > >> index f9c15fd304bd..62931a40b79a 100644 > >> --- a/scripts/common.bash > >> +++ b/scripts/common.bash > >> @@ -1,8 +1,15 @@ > >> +function arch_cmd() > >> +{ > >> + # Dummy function, can be overwritten by architecture dependent > >> + # code > >> + return > >> +} > > > > This dummy function appears unused and can be dropped. > > So what is then used if the function is not defined by the architecture > specific code? Nothing, which works fine $ arch_cmd= $ $arch_cmd echo foo # just do 'echo foo' However, with what I wrote above, we now need a common arch_cmd function. Something like arch_cmd() { [ "$ARCH_CMD" ] && return "$ARCH_CMD" } Which would allow us to write $(arch_cmd) $cmd ... That does the same thing as above, but it now follows the pattern of migration_cmd and timeout_cmd. Thanks, drew
On Fri, Aug 14, 2020 at 03:29 PM +0200, Andrew Jones <drjones@redhat.com> wrote: > On Fri, Aug 14, 2020 at 03:06:36PM +0200, Marc Hartmayer wrote: >> On Thu, Aug 13, 2020 at 10:30 AM +0200, Andrew Jones <drjones@redhat.com> wrote: >> > On Wed, Aug 12, 2020 at 11:27:04AM +0200, Marc Hartmayer wrote: >> >> This allows us, for example, to auto generate a new test case based on >> >> an existing test case. >> >> >> >> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> >> >> --- >> >> run_tests.sh | 2 +- >> >> scripts/common.bash | 13 +++++++++++++ >> >> scripts/mkstandalone.sh | 2 +- >> >> 3 files changed, 15 insertions(+), 2 deletions(-) >> >> >> >> diff --git a/run_tests.sh b/run_tests.sh >> >> index 24aba9cc3a98..23658392c488 100755 >> >> --- a/run_tests.sh >> >> +++ b/run_tests.sh >> >> @@ -160,7 +160,7 @@ trap "wait; exit 130" SIGINT >> >> # preserve stdout so that process_test_output output can write TAP to it >> >> exec 3>&1 >> >> test "$tap_output" == "yes" && exec > /dev/null >> >> - for_each_unittest $config run_task >> >> + for_each_unittest $config run_task arch_cmd >> > >> > Let's just require that arch cmd hook be specified by the "$arch_cmd" >> > variable. Then we don't need to pass it to for_each_unittest. >> >> Where is it then specified? > > Just using it that way in the source is enough. We should probably call > it $ARCH_CMD to indicate that it's a special variable. Also, we could > return it from a $(arch_cmd) function, which is how $(migration_cmd) and > $(timeout_cmd) work. My first approach was different… First we source the (common) functions that could be overridden by architecture dependent code, and then source the architecture dependent code. But I’m not sure which approach is cleaner - if you prefer your proposed solution with the global variables I can change it. Thanks for the feedback! […snip]
On Tue, Aug 18, 2020 at 11:03:27AM +0200, Marc Hartmayer wrote: > On Fri, Aug 14, 2020 at 03:29 PM +0200, Andrew Jones <drjones@redhat.com> wrote: > > On Fri, Aug 14, 2020 at 03:06:36PM +0200, Marc Hartmayer wrote: > >> On Thu, Aug 13, 2020 at 10:30 AM +0200, Andrew Jones <drjones@redhat.com> wrote: > >> > On Wed, Aug 12, 2020 at 11:27:04AM +0200, Marc Hartmayer wrote: > >> >> This allows us, for example, to auto generate a new test case based on > >> >> an existing test case. > >> >> > >> >> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> > >> >> --- > >> >> run_tests.sh | 2 +- > >> >> scripts/common.bash | 13 +++++++++++++ > >> >> scripts/mkstandalone.sh | 2 +- > >> >> 3 files changed, 15 insertions(+), 2 deletions(-) > >> >> > >> >> diff --git a/run_tests.sh b/run_tests.sh > >> >> index 24aba9cc3a98..23658392c488 100755 > >> >> --- a/run_tests.sh > >> >> +++ b/run_tests.sh > >> >> @@ -160,7 +160,7 @@ trap "wait; exit 130" SIGINT > >> >> # preserve stdout so that process_test_output output can write TAP to it > >> >> exec 3>&1 > >> >> test "$tap_output" == "yes" && exec > /dev/null > >> >> - for_each_unittest $config run_task > >> >> + for_each_unittest $config run_task arch_cmd > >> > > >> > Let's just require that arch cmd hook be specified by the "$arch_cmd" > >> > variable. Then we don't need to pass it to for_each_unittest. > >> > >> Where is it then specified? > > > > Just using it that way in the source is enough. We should probably call > > it $ARCH_CMD to indicate that it's a special variable. Also, we could > > return it from a $(arch_cmd) function, which is how $(migration_cmd) and > > $(timeout_cmd) work. > > My first approach was different… > > First we source the (common) functions that could be overridden by > architecture dependent code, and then source the architecture dependent > code. But I’m not sure which approach is cleaner - if you prefer your > proposed solution with the global variables I can change it. I prefer my proposed solution. It's not necessary to provide and source an arch-neutral function that will never do anything. And, it will never do anything, because the function is supposed to be arch-specific. If an arch doesn't implement the function, then we don't need to call anything at all. Thanks, drew > > Thanks for the feedback! > > […snip] > > -- > Kind regards / Beste Grüße > Marc Hartmayer > > IBM Deutschland Research & Development GmbH > Vorsitzender des Aufsichtsrats: Gregor Pillen > Geschäftsführung: Dirk Wittkopp > Sitz der Gesellschaft: Böblingen > Registergericht: Amtsgericht Stuttgart, HRB 243294 >
diff --git a/run_tests.sh b/run_tests.sh index 24aba9cc3a98..23658392c488 100755 --- a/run_tests.sh +++ b/run_tests.sh @@ -160,7 +160,7 @@ trap "wait; exit 130" SIGINT # preserve stdout so that process_test_output output can write TAP to it exec 3>&1 test "$tap_output" == "yes" && exec > /dev/null - for_each_unittest $config run_task + for_each_unittest $config run_task arch_cmd ) | postprocess_suite_output # wait until all tasks finish diff --git a/scripts/common.bash b/scripts/common.bash index f9c15fd304bd..62931a40b79a 100644 --- a/scripts/common.bash +++ b/scripts/common.bash @@ -1,8 +1,15 @@ +function arch_cmd() +{ + # Dummy function, can be overwritten by architecture dependent + # code + return +} function for_each_unittest() { local unittests="$1" local cmd="$2" + local arch_cmd="${3-}" local testname local smp local kernel @@ -19,6 +26,9 @@ function for_each_unittest() if [[ "$line" =~ ^\[(.*)\]$ ]]; then if [ -n "${testname}" ]; then "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" + if [ "${arch_cmd}" ]; then + "${arch_cmd}" "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" + fi fi testname=${BASH_REMATCH[1]} smp=1 @@ -49,6 +59,9 @@ function for_each_unittest() done if [ -n "${testname}" ]; then "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" + if [ "${arch_cmd}" ]; then + "${arch_cmd}" "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" + fi fi exec {fd}<&- } diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh index cefdec30cb33..3b18c0cf090b 100755 --- a/scripts/mkstandalone.sh +++ b/scripts/mkstandalone.sh @@ -128,4 +128,4 @@ fi mkdir -p tests -for_each_unittest $cfg mkstandalone +for_each_unittest $cfg mkstandalone arch_cmd
This allows us, for example, to auto generate a new test case based on an existing test case. Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- run_tests.sh | 2 +- scripts/common.bash | 13 +++++++++++++ scripts/mkstandalone.sh | 2 +- 3 files changed, 15 insertions(+), 2 deletions(-)