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.