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

[Stephen Warren]

On 09/20/2017 11:43 AM, Heinrich Schuchardt wrote:
> 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?

Because it's a very simple change, that the user can simply copy/paste 
without any impact to the complexity of how they follow these steps, 
that helps avoid other problems, such as:

- Placing build results in the source tree which might confuse them 
later. (Hopefully .gitignore ignores them all though...)

- Interacting badly with the user's own decision to use O= when building 
themselves in other cases, if they've done that. Not using O= for the 
build will cause the build system to refuse to build with O= later until 
the user has deleted all the build results from the tree.

Of course, if the user has already built in-tree, I suppose that using 
O= in these examples won't work for them either:-(

I will point out that if you run "test/py/test.py --build ..." then 
test.py will automatically build U-Boot for you, and it will use an O= 
option based on the board name, and that value will be ./build-qemu-x86 
in this case, which is part of the reason I suggested this change.

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

Because the example needs to work correctly. Without this step, it 
won't, except by accident, due to the race condition I mentioned.

Note that this is the kind of reason why I created the example 
repository for these scripts; users can just clone that repo and run 
anything that's there without the need for a README that describes how 
to create these files. (Almost, for qemu targets at least)

Perhaps an alternative would be to host some examples directly in the 
source tree. For example, we could create ${src}/test/py/hooks/ and just 
check in all the files there. That way, users wouldn't have to create 
them for simple qemu cases, and we wouldn't have to write a README to 
tell users how to create them.

>> 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();

Yes, and those all operate correctly if my suggestions are implemented. 
cons.restart_uboot() closes the stdin/stdout to the u-boot-console 
process, which causes qemu to exit, and then cons.restart_uboot() 
invokes a new instance of u-boot-console which creates a new qemu 
process. This restart process also happens if any test fails, so that 
subsequent tests start with a clean state, avoiding issues caused by 
previous failures.

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

I know you joked, but I do want to make sure you're aware that 
travis-ci.org (which runs U-Boot testing) runs the same OS I use, and 
that OS is supported until 2019, so we shouldn't do anything that 
prevents people using it. Once it's out of support, it's quite 
reasonable for U-Boot not to support it any more.

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

No, we need to find out why it fails and fix that. If either echo or 
socat are returning a non-zero exit code, there's a bug in the example 
scripts that we must fix.

I suspect it's related to the race condition I mentioned re: when qemu 
creates the monitor socket (and whether it's a plain file as the example 
script creates, or it's re-created as a socket some time after qemu 
starts). The suggestions I gave in my email will solve that, and thus 
you won't need the true command.

>>> +    env__net_dhcp_server = True
>>> +    env__efi_loader_helloworld_file = {
>>> +      "fn": "helloworld.efi",
>>> +      "size": 4298,
>>> +      "crc32": "55d96ef8",
>>> +    }
...
>>> +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.

file_full should be roughly just file_name. I copy/pasted that and 
forgot to update those lines. Actually, file_name is the name relative 
to the TFTP directory (the name that U-Boot will request over TFTP 
during the test) and file_full is a path-name that's valid on the host 
when the Python code is being parsed. The two can be the same if the 
current working directory is the TFTP server directory when the code is 
executed, but it's probably not.

> Are the necessary packages imported when the python script is included?
> I doubt it. There is no string binascii in U-Boot.

binascii is part of the standard Python library. It's available anywhere 
that Python is available. Put the following at the top of the script:

import os
import binascii

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

The code can work out where the TFTP directory is by taking the path of 
the Python code, and computing the TFTP directory name relative to that. 
Just a few lines of code. Possible code:

file_name = 'xxx'
py_dir = os.path.dirname(__file__)
hook_dir = os.path.dirname(door_py_dir)
tftp_dir = os.path.join(hook_dir, 'tftp)
full_name = os.path.join(tftp_dir, file_name)

> I would prefer to keep the example simple.

There are different kinds of simple.

Keeping the code simple is a good general idea.

However, if you keep the code too simple, then the user will have to do 
all kinds of manual operations, such as manually substituting the 
correct file size and CRC32 values any time that file changes, instead 
of blinding copy/pasting the file content and forgetting about it. 
People will forget that, and then have to debug why the test is broken. 
I believe that making the act of running the tests as simple as possible 
is of higher priority than making the code as small/simple as possible. 
After all, people just blindly following the instructions don't 
absolutely have to read and understand every single line of code that 
they're pasting into the files.