On 10.03.2017 16:04, Andrew Jones wrote:
> On Thu, Mar 09, 2017 at 06:27:06PM +0100, Thomas Huth wrote:
>> To be able to do simple migration tests with kvm-unit-tests, too,
>> add a helper script that does all the necessary work: Start two
>> instances of QEMU (source and destination) with QMP sockets for
>> sending commands to them, then trigger the migration from one
>> instance to the other and finally signal the end of the migration
>> to the guest by injecting an NMI.
>> This helper script is now used automatically for powerpc tests
>> if the test is put into the "migration" group in the unittests.cfg
>> file.
>>
>> Signed-off-by: Thomas Huth <thuth@xxxxxxxxxx>
>> ---
>> powerpc/run | 5 ++++
>> scripts/qemu-migration-helper.sh | 51 ++++++++++++++++++++++++++++++++++++++++
>> scripts/runtime.bash | 3 +++
>> 3 files changed, 59 insertions(+)
>> create mode 100755 scripts/qemu-migration-helper.sh
>>
>> diff --git a/powerpc/run b/powerpc/run
>> index 6269abb..126fed5 100755
>> --- a/powerpc/run
>> +++ b/powerpc/run
>> @@ -41,6 +41,11 @@ if ! $qemu -machine '?' 2>&1 | grep 'pseries' > /dev/null; then
>> exit 2
>> fi
>>
>> +if [ "$MIGRATION" = "1" ]; then
>
> I'd prefer we use "yes" like our other bash booleans.
Sure. I'll change it.
>> + export QEMU_BIN="$qemu"
>> + qemu=`dirname $0`/../scripts/qemu-migration-helper.sh
>
> Can't we just use a relative pathname, i.e. no need for dirname?
I thought that dirname trick would be nice to be able to call the "run"
script from another $PWD, too, but apparently the other scripts like
scripts/arch-run.bash are also called there directly. So I'll change to
that style here, too.
>> +fi
>> +
>> M='-machine pseries'
>> M+=",accel=$ACCEL"
>> command="$qemu -nodefaults $M -bios $FIRMWARE"
>> diff --git a/scripts/qemu-migration-helper.sh b/scripts/qemu-migration-helper.sh
>> new file mode 100755
>> index 0000000..0cb7e4a
>> --- /dev/null
>> +++ b/scripts/qemu-migration-helper.sh
>> @@ -0,0 +1,51 @@
>> +#!/bin/sh
>
> Might as well use bash, as we require it for all the other scripts.
OK.
>> +
>
> nit: prefer no blank line between the interpreter and comment block
>
>> +# This script runs two instances of QEMU and then migrates the guest from one
>> +# instance to the other. The end of the migration is signalled to the guest by
>> +# injecting an NMI.
>> +
>> +if [ "x$QEMU_BIN" = "x" ]; then
>
> [ -z "$QEMU" ] ? (I don't think we need the ugly x-stuff with bash)
>
> We use $QEMU everywhere else, any reason to avoid it here (using
> QEMU_BIN)?
I did not want to interfere with the user supplied $QEMU in the "run"
script. But thinking about this again, I likely don't need an env
variable here at all and could also simply pass it as the first
parameter to the script. I'll try to do that instead...
>> + echo "QEMU_BIN must be set to the QEMU binary"
>> + exit 1
>> +fi
>> +
>> +if ! command -v nc >/dev/null 2>&1; then
>
> 'command' is nice (I wasn't aware of it when I used 'which' elsewhere)
Yes, I've been told that this is more portable (i.e. POSIX compliant)
than using "which".
>> + echo "$0 needs nc (netcat)"
>> + exit 1
>> +fi
>> +
>> +qmp_cmd()
>> +{
>> + echo '{ "execute": "qmp_capabilities" }{ "execute":' "$2" '}' | nc -U $1
>> +}
>> +
>> +tempname=`mktemp`
>> +qmp1=${tempname}.qmp1
>> +qmp2=${tempname}.qmp2
>> +qmpout1=${tempname}.qmpout1
>> +qmpout2=${tempname}.qmpout2
>> +stdout1=${tempname}.stdout1
>> +stdout2=${tempname}.stdout2
>> +migsock=${tempname}.mig
>
> I think all the above should invoke mktemp themselves, otherwise this
> script is a bit unsafe. If the suffixes are useful for debug purposes
> then you can use mktemp --suffix.
OK, I'll rework this section.
>> +
>> +$QEMU_BIN $* -chardev socket,id=mon1,path=${qmp1},server,nowait \
>> + -mon chardev=mon1,mode=control > ${stdout1} &
>> +
>> +$QEMU_BIN $* -chardev socket,id=mon2,path=${qmp2},server,nowait \
>> + -mon chardev=mon2,mode=control -incoming unix:${migsock} > ${stdout2} &
>> +
>> +sleep 1
>> +
>> +qmp_cmd ${qmp1} '"migrate", "arguments": { "uri": "unix:'${migsock}'" }' > ${qmpout1}
>> +while qmp_cmd ${qmp1} '"query-migrate"' | grep -q '"active"' ; do
>> + sleep 1
>
> nit: should use tab like above
I guess I stared at runtime.bash for too long 8-) ... that one is only
using spaces for indentation.
>> +done
>> +qmp_cmd ${qmp1} '"quit"'> ${qmpout1} 2>/dev/null
>> +
>> +qmp_cmd ${qmp2} '"inject-nmi"'> ${qmpout2}
>> +
>> +wait
>> +
>> +cat ${stdout1} ${stdout2}
>> +
>> +rm -f ${tempname} ${qmpout1} ${qmpout2} ${stdout1} ${stdout2} ${migsock}
>
> How about doing this in an EXIT trap handler to make sure the files get
> removed?
Good idea, I'll add that.
Thanks for the review!
Thomas
