Re: [PATCH v3] acpi/tests/bios-tables-test: make iasl tool handling simpler

[Michael S. Tsirkin]

On Wed, Jun 28, 2023 at 12:05:46PM +0530, Ani Sinha wrote:
> 
> 
> > On 26-Jun-2023, at 6:49 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > 
> > On Mon, Jun 26, 2023 at 06:33:14PM +0530, Ani Sinha wrote:
> >> 
> >> 
> >>> On 26-Jun-2023, at 6:28 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>> 
> >>> On Mon, May 22, 2023 at 04:00:39PM +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 in
> >>>> config-host.h header.This is then used at compile time by 
> >>>> bios-tables-test to
> >>>> set iasl path.
> >>>> 
> >>>> This has two disadvantages:
> >>>> - If iasl was not previously installed in the PATH, one has to install 
> >>>> iasl
> >>>>  and rebuild QEMU in order to regenerate the header and pick up the found
> >>>>  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 set a default iasl 
> >>>> path
> >>>> as "/usr/bin/iasl". bios-tables-test also checks for the environment 
> >>>> variable
> >>>> IASL_PATH that can be set by the developer. IASL_PATH passed from the
> >>>> environment overrides the default path. This way developers can point
> >>>> IASL_PATH environment variable to a possibly a non-standard custom build
> >>>> binary and quickly run bios-tables-test without rebuilding. If the 
> >>>> default
> >>>> path of iasl changes, one simply needs to update the default path and 
> >>>> rebuild
> >>>> just the test, not whole QEMU.
> >>>> 
> >>>> [1] https://mesonbuild.com/Reference-manual_functions.html#find_program
> >>>> 
> >>>> CC: alex.bennee@linaro.org
> >>>> CC: pbonzini@redhat.com
> >>>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> >>> 
> >>> I don't much like environment variables since they are
> >>> not discoverable.
> >> 
> >> I do have this:
> >> 
> >> +                " Set IASL_PATH environment variable to the path of iasl 
> >> binary\n"
> >> +                " if iasl is installed somewhere other than %s.\n",
> > 
> > You only see this if there's a diff.
> > 
> > And then people stick this in their scripts and are scratching their
> > heads trying to figure out why is a wrong iasl running.  Or someone
> > comes up with a different use for IASL_PATH and they conflict.
> 
> OK in that case I think its ok to simply remove the environment
> variable part. If people are going to be changing a header file,

Not people. configure script

> they
> might as well change the DEFAULT_IASL_PATH in the test itself where
> its easier to find. What additional complication meson provides is
> that it uses find_program() to find the IASL binary in a list of
> predefined locations. I do not think this additional tie up with meson
> is worth it for the niche iasl use case.  Simple is beautiful.

The just the below then? And we can let it be?

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>


diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index ed1c69cf01..d0e1655d2e 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -102,6 +102,7 @@ typedef struct {
 
 static char disk[] = "tests/acpi-test-disk-XXXXXX";
 static const char *data_dir = "tests/data/acpi";
+/* If you want your own path, change the below to iasl = "/home/usr/bin/iasl" 
*/
 #ifdef CONFIG_IASL
 static const char *iasl = CONFIG_IASL;
 #else