Re: [PATCH] selftests/liveupdate: add end to end test infrastructure and scripts

[Jordan Richards <jordanrichards@google.com>]

Sending v2 addressing this feedback soon:

>> +	err = kexec_file_load(kernel_fd, initrd_fd, strlen(cmdline) + 1,
>> +			      cmdline, 0);
>> +
>> +	close(initrd_fd);
>> +	close(kernel_fd);
>> +
>> +	return err ? : 0;
>
> Just return err?

Good point, fixed.

>> diff --git a/tools/testing/selftests/liveupdate/luo_test.sh b/tools/testing/selftests/liveupdate/luo_test.sh
>> new file mode 100755
>> index 000000000000..90ecb16e87bb
>> --- /dev/null
>> +++ b/tools/testing/selftests/liveupdate/luo_test.sh
>
>...
>
>> +function detect_cross_compile() {
>> +	local target=$1
>> +	local host=$(uname -m)
>> +
>
> This function works fine if you run luo_test.sh directly or have cross
> compilers named the way it expects in $PATH.
>
> But if I run
> CROSS_COMPILE=~/cross/gcc-13.2.0-nolibc/aarch64-linux/bin/aarch64-linux- ./run.sh
> on x86, x86 tests fail
>
>> +	if [ -n "$CROSS_COMPILE" ]; then
>> +		return
>> +	fi
>> +
>> +	[[ "$host" == "arm64" ]] && host="aarch64"
>> +	[[ "$target" == "arm64" ]] && target="aarch64"
>> +
>> +	if [[ "$host" == "$target" ]]; then
>> +		CROSS_COMPILE=""
>> +		return
>> +	fi

The host==target test needed to be first, fixed.

>> +	local fragments=()
>> +
>> +	if [[ -f "$common_conf" ]]; then
>> +		fragments+=("$common_conf")
>> +	fi
>> +
>> +	if [[ -f "$arch_conf" ]]; then
>> +		fragments+=("$arch_conf")
>> +	fi
>
> I think the common and arch config fragments are required and we can just
> assign fragments directly and run merge_config.sh.

Done.

I wasn't sure if arch_conf would be necessary for all architectures, but it
is kind of needless complexity.

> +	if [[ "$qemu_cmd" == *"$host_machine"* ]]; then
> +		if [ -w /dev/kvm ]; then
> +			accel="-accel kvm"
> +		fi
> +	fi
>
> Do we care that much about qemu warnings about invalid accelerator to have
> this logic here?
>
> -accel kvm -accel hvf -accel tcg
>
> seems to cover all bases.

The warning doesn't break the tests, so it should be fine.

>> +#include <sys.h>
>
> This breaks running normal make:
>
> luo_test_utils.c:16:10: fatal error: sys.h: No such file or directory
>   16 | #include <sys.h>
>      |          ^~~~~~~
>
> NOLIBC specific includes and calls should be guarded with #ifdef NOLIBC

Done.

> diff --git a/tools/testing/selftests/liveupdate/run.sh b/tools/testing/selftests/liveupdate/run.sh
> new file mode 100755
> index 000000000000..3f6b29a26648
> --- /dev/null
> +++ b/tools/testing/selftests/liveupdate/run.sh
> @@ -0,0 +1,68 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
>> +OUTPUT_DIR="results_$(date +%Y%m%d_%H%M%S)"
>
> I don't think that putting the results in the current directory rather than
> in SCRIPT_DIR or in an explicitly named directory is a good idea.

Changed to "$SCRIPT_DIR/results_*".

>> +TEST_NAMES=()
>> +while IFS= read -r file; do
>> +    TEST_NAMES+=("$(basename "$file" .c)")
>> +done < <(find "$SCRIPT_DIR" -maxdepth 1 -name "luo_*.c" ! -name "luo_test_utils.c")
>
> I don't like name based detection of tests. Listing them explicitly seems a
viable option.

Agreed, changed to a fixed array.