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

Wed Sep 20 17:43:52 UTC 2017

On 09/20/2017 06:41 PM, Stephen Warren wrote:
> 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?

Building in the source tree is the default for U-Boot.
Why should we make the example more complicated?

> 
>> +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.

I can give it a try.

> 
>> +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

Why make the example more complicated?

> 
> 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.

Some test scripts explicitly reset U-Boot:

test/py/tests/test_fit.py:362: cons.restart_uboot()
test/py/tests/test_fit.py:387: cons.restart_uboot()
test/py/tests/test_fit.py:399: cons.restart_uboot()
test/py/tests/test_fit.py:411: cons.restart_uboot()
test/py/tests/test_fit.py:428: cons.restart_uboot()
test/py/tests/test_vboot.py:70: cons.restart_uboot()
test/py/tests/test_vboot.py:198: cons.restart_uboot()
test/py/tests/test_efi_selftest.py:25: u_boot_console.restart_uboot();

> 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:

It is time to upgrade your Linux distribution ;)
I can change that to pc-i440fx-2.0.

> 
> 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.
> 

I will remove u-boot-test-quit.

>> +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.

Without true all tests fail on my machine (Debian Stretch) which uses
dash to implement /bin/sh. So we should keep it. It does no harm.

> 
>> +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.

644

> 
>> +    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:],
> }

What does file_full refer to? I cannot find this string in U-Boot.
Are the necessary packages imported when the python script is included?
I doubt it. There is no string binascii in U-Boot.

We cannot hard code the full path to helloworld.efi here because the
value of $HOME_PATH is not known.

I would prefer to keep the example simple.

> 
>> +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"

I can change it.

> 
>> +    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"
>

Together with your suggestion below this is fine.

>> +    ./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
> 

That makes sense.

Thank you for reviewing.

Best regards

Heinrich