Re: [PATCH] acpi/tests/bios-tables-test: add an environment variable for iasl location

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, May 17, 2023 at 05:37:51PM +0530, Ani Sinha wrote:
> Currently the meson based QEMU build process locates the iasl binary from the
> current PATH and other locations [1] and uses that to set CONFIG_IASL which is
> then used by the test.
> 
> This has two disadvantages:
>  - If iasl was not previously installed in the PATH, one has to install iasl
>    and rebuild QEMU in order to pick up the iasl location. One cannot simply
>    use the existing bios-tables-test binary because CONFIG_IASL is only set
>    during the QEMU build time by meson and then bios-tables-test has to be
>    rebuilt with CONFIG_IASL set in order to use iasl.
>  - Sometimes, the stock iasl that comes with distributions is simply not good
>    enough because it does not support the latest ACPI changes - newly
>    introduced tables or new table attributes etc. In order to test ACPI code
>    in QEMU, one has to clone the latest acpica upstream repository and
>    rebuild iasl in order to get support for it. In those cases, one may want
>    the test to use the iasl binary from a non-standard location.
> 
> In order to overcome the above two disadvantages, we introduce a new
> environment variable IASL_PATH that can be set by the tester pointing to an
> possibly non-standard iasl binary location. Bios-tables-test then uses this
> environment variable to set its iasl location, possibly also overriding the
> location that was pointed to by CONFIG_IASL that was set by meson. This way
> developers can not only use this new environment variable to set iasl
> location to quickly run bios-tables-test but also can point the test to a
> custom iasl if required.
> 
> [1] https://mesonbuild.com/Reference-manual_functions.html#find_program
> 
> Signed-off-by: Ani Sinha <anisinha@redhat.com>

I don't much like it that the default is now a bit harder
to run. Case of playing with iasl is really esotetic.
I propose a simpler idea.
- add config-iasl.h with only CONFIG_IASL set to path
- include from bios test

Now if you change path only bios test is rebuilt.

Hmm?


> ---
>  tests/qtest/bios-tables-test.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> sample runs:
> 
> $ QTEST_QEMU_BINARY=./qemu-system-x86_64 V=2 ./tests/qtest/bios-tables-test
> ...
> acpi-test: Warning! APIC binary file mismatch. Actual [aml:/tmp/aml-DLHA51], Expected [aml:tests/data/acpi/pc/APIC].
> See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
> Using iasl: /usr/bin/iasl
> acpi-test: Warning! APIC mismatch. Actual [asl:/tmp/asl-L9GA51.dsl, aml:/tmp/aml-DLHA51], Expected [asl:/tmp/asl-10EA51.dsl, aml:tests/data/acpi/pc/APIC].
> 
> $ QTEST_QEMU_BINARY=./qemu-system-x86_64 V=2 IASL_PATH=/home/anisinha/workspace/acpica/generate/unix/bin/iasl ./tests/qtest/bios-tables-test
> ...
> acpi-test: Warning! APIC binary file mismatch. Actual [aml:/tmp/aml-5CQ341], Expected [aml:tests/data/acpi/pc/APIC].
> See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
> User has provided an iasl path, using that: /home/anisinha/workspace/acpica/generate/unix/bin/iasl
> acpi-test: Warning! APIC mismatch. Actual [asl:/tmp/asl-2GQ341.dsl, aml:/tmp/aml-5CQ341], Expected [asl:/tmp/asl-IBR341.dsl, aml:tests/data/acpi/pc/APIC].
> 
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index 7fd88b0e9c..37e8e484cb 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -440,7 +440,12 @@ static void test_acpi_asl(test_data *data)
>      AcpiSdtTable *sdt, *exp_sdt;
>      test_data exp_data = {};
>      gboolean exp_err, err, all_tables_match = true;
> +    const char *user_iasl_path = getenv("IASL_PATH");
>  
> +    /* if user has provided a path to iasl, use that */
> +    if (user_iasl_path) {
> +        iasl = user_iasl_path;
> +    }
>      exp_data.tables = load_expected_aml(data);
>      dump_aml_files(data, false);
>      for (i = 0; i < data->tables->len; ++i) {
> @@ -473,6 +478,15 @@ static void test_acpi_asl(test_data *data)
>              continue;
>          }
>  
> +        if (verbosity_level >= 2) {
> +            if (user_iasl_path) {
> +                fprintf(stderr, "User has provided an iasl path," \
> +                        "using that: %s\n", user_iasl_path);
> +            } else {
> +                fprintf(stderr, "Using iasl: %s\n", iasl);
> +            }
> +        }
> +
>          err = load_asl(data->tables, sdt);
>          asl = normalize_asl(sdt->asl);
>  
> -- 
> 2.39.1