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.

Reply via email to