[U-Boot] [PATCH v3 1/1] test/py: provide example scripts for integrating qemu

[Stephen Warren]

On 09/19/2017 02:04 PM, Heinrich Schuchardt wrote:
> The necessary parameters for running Python tests on qemu are
> tedious to find.
> 
> The patch adds a complete example for running the Python tests for
> qemu-x86_defconfig on an X86 system.

> diff --git a/test/py/README.md b/test/py/README.md

> +#### Running tests for qemu-x86\_defconfig on an x86 system
> +
> +We build u-boot.rom with
> +
> +    export BUILD_ROM=y
> +    make mrproper
> +    make qemu-x86_defconfig
> +    make

Nit: I'd rather write that as:

BUILD_ROM=y make

That way, we don't have to set BUILD_ROM permanently in the current 
shell's environment, so it won't affect anything else that's done later 
in the shell.

To avoid polluting the source tree with build results, why not build 
with O=./build-qemu-x86, and use that as the --build-dir argument to 
test/py?

> +We create directories `$HOME/ubtest/bin` and `$HOME/ubtest/py` for the script
> +files below and `../tftp` for the tftp server.

Why not $HOME/ubtest/tftp? The meaning of "../tftp" can change depending 
on current directory.

> +File `u-boot-test-console` chmod 755
> +
> +    #!/bin/sh
> +    touch /tmp/u-boot-monitor-socket

This creates a regular file not a socket. I believe you can remove this 
command.

Ah, there's a race condition here. By the time u-boot-test-reset is run 
the first time, qemu typically hasn't run far enough to create 
/tmp/u-boot-monitor-socket, and so u-boot-test-reset fails to open it. 
If I fix that by making u-boot-test-reset wait until the control socket 
actually does exist, then test/py fails because it sees two signon 
messages from U-Boot and thinks it crashed; one from when qemu started 
and booted U-Boot, and one because the first invocation of 
u-boot-test-reset resets the system which causes another signon to 
appear. Perhaps this is why Tom Rini's test/py+qemu integration spawns a 
new instance of qemu every time u-boot-test-console is run, and make 
u-boot-test-reset a no-op.

To solve this, I was going to say:

a) Add -S to the qemu command. This causes qemu to perform all 
initialization (e.g. creating/opening the monitor socket) but doesn't 
actually allow the target to run. This prevents the undesired immediate 
boot of the system.

b) Add the following to the top of u-boot-test-reset so that it waits 
for the monitor socket to exist before attempting to use it:

for i in $(seq 50); do
   if [ -f /tmp/u-boot-monitor-socket ]; then
     break
   fi
   sleep 0.1
done

c) Make u-boot-test-reset send U-Boot command "c" to start U-Boot 
executing instead of or in additionk to "system_reset" to cause a reset.

However, none of that is necessary. u-boot-test-console is re-executed 
every time test/py wants to connect to the target, which happens (a) 
when test/py starts the first test, and (b) any time a test fails, and 
test/py wants to clear all state and reconnect to the target (e.g. the 
console command might have failed rather than the target, so the console 
command is restarted too). As such, there's no need to implement 
u-boot-test-reset at all with qemu; just make it a complete no-op. That 
way, you also don't need qemu's -monitor at all.

> +    exec qemu-system-x86_64 -bios u-boot.rom -nographic -netdev \
> +      user,id=eth0,tftp=../tftp,net=192.168.76.0/24,dhcpstart=192.168.76.9 \
> +      -device e1000,netdev=eth0 -machine pc-i440fx-2.8 \

Is machine version 2.8 strictly necessary? The qemu packaged in Ubuntu 
14.04 doesn't support that version, which restricts the usefulness of 
the example. Can version 2.0 work for example? It seems to work fine for 
me. My qemu supports the following i440fx:

qemu-system-x86_64 -machine help | grep i440fx
pc-i440fx-2.0        Standard PC (i440FX + PIIX, 1996)
pc-i440fx-1.4        Standard PC (i440FX + PIIX, 1996)
pc-i440fx-1.5        Standard PC (i440FX + PIIX, 1996)
pc                   Ubuntu 14.04 PC (i440FX + PIIX, 1996) (alias of 
pc-i440fx-trusty)
pc-i440fx-trusty     Ubuntu 14.04 PC (i440FX + PIIX, 1996) (default)
pc-i440fx-1.7        Standard PC (i440FX + PIIX, 1996)
pc-i440fx-1.5-saucy  Standard PC (i440FX + PIIX, 1996) (alias of 
pc-i440fx-1.5-qemu-kvm)
pc-i440fx-1.5-qemu-kvm Standard PC (i440FX + PIIX, 1996)
pc-i440fx-1.6        Standard PC (i440FX + PIIX, 1996)

> +File `u-boot-test-quit` chmod 755
> +
> +    #!/bin/sh
> +    echo quit | socat - UNIX-CONNECT:/tmp/u-boot-monitor-socket
> +
> +This script is called when all tests are completed. The `quit` command is
> +passed to the qemu control console to terminate the qemu session.

test/py doesn't run u-boot-test-quit. Do you have a local change that 
makes it do that? My knee-jerk reaction is that it's OK to enhance 
test/py to run such a script, but let's not document it in the example 
until it does. That said, I'd assume qemu should exit as soon as test/py 
exits since stdin will go away, so this shouldn't actually be needed in 
this case.

> +File `u-boot-test-reset` chmod 755
> +
> +    #!/bin/sh
> +    echo system_reset | socat - UNIX-CONNECT:/tmp/u-boot-monitor-socket
> +    true

As mentioned previously, the true doesn't appear to be required, and 
hides legitimate errors.

> +In the `$HOME/ubtest/py` directory we create file `u_boot_boardenv_qemu_x86.py`

Since the file mode is mentioned for all the other files, let's mention 
it here too.

> +    env__net_dhcp_server = True
> +    env__efi_loader_helloworld_file = {
> +      "fn": "helloworld.efi",
> +      "size": 4298,
> +      "crc32": "55d96ef8",
> +    }
> +
> +The parameter `env__net_dhcp_server` enables the network tests. The parameter
> +`env__efi_loader_helloworld_file` is needed to make the file `helloworld.efi`
> +available which is loaded from the tftp server in `test_efi_loader.py`.
> +
> +The fields size and crc32 have to be updated to match the actual values. The
> +`crc32` command can be used to determine the latter.

Why not have Python determine those values automatically? Here's a 
snippet from my uboot-test-hooks repo that does that. Since people will 
simply be copy-and-pasting the example, it shouldn't matter that it's 
non-trivial:

{
   "fn": file_name,
   "size": os.path.getsize(file_full),
   "crc32": hex(binascii.crc32(open(file_full, 'rb').read()) & \
     0xffffffff)[2:],
}

> +We now can run the Python tests with
> +
> +    export PATH=$HOME/ubtest/bin:/usr/bin:/bin

Let's not hard-code /usr/bin:/bin; some people may rely on binaries 
elsewhere. Also I know that the recently added gpt tests rely on sgdisk 
which is in sbin on my system at least. Also quote to avoid issues with 
spaces in path names. How about:

PATH="$HOME/ubtest/bin:$PATH"

> +    export PYTHONPATH=$HOME/ubtest/py

People might have PYTHONPATH set already (e.g. if using a virtualenv or 
having installed some packages in their home directory). How about:

PYTHONPATH="$HOME/ubtest/py:$PYTHONPATH"

> +    ./test/py/test.py --bd=qemu-x86 --build-dir=.

As above, I'd suggest setting environment variables just for the one 
command rather than permanently changing them:

PATH="$HOME/ubtest/bin:$PATH" \
   PYTHONPATH="$HOME/ubtest/py:$PYTHONPATH" \
   ./test/py/test.py --bd=qemu-x86 --build-dir=./build-qemu-x86