[PATCH] bios-tables-test: do not ignore allowed diff list

[PATCH] bios-tables-test: do not ignore allowed diff list

[Michael S. Tsirkin]

we had such a beautiful structure for updating
expected files, designed to keep bisect working.
It turns out that we ignored the result of
the allow list checks unless all tables matched
anyway.

Sigh.

Let's at least make it work going forward.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 tests/qtest/bios-tables-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index e6096e7f73..a72f6ca326 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -458,7 +458,7 @@ static void test_acpi_asl(test_data *data)
                 "for instructions on how to update expected files.\n",
                 exp_sdt->aml, sdt->aml_file, exp_sdt->aml_file);
 
-        all_tables_match = all_tables_match &&
+        all_tables_match = all_tables_match ||
             test_acpi_find_diff_allowed(exp_sdt);
 
         /*
-- 
MST

Re: [PATCH] bios-tables-test: do not ignore allowed diff list

[Ani Sinha]

On Thu, Oct 27, 2022 at 8:41 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> we had such a beautiful structure for updating
> expected files, designed to keep bisect working.
> It turns out that we ignored the result of
> the allow list checks unless all tables matched
> anyway.


Doh! Seems the bug is present from the beginning?

>
> Sigh.
>
> Let's at least make it work going forward.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Fixes: df7cafdeb68b6572fa81 ("bios-tables-test: list all tables that differ")

other than that,
Reviewed-by: Ani Sinha <ani@anisinha.ca>




> ---
>  tests/qtest/bios-tables-test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index e6096e7f73..a72f6ca326 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -458,7 +458,7 @@ static void test_acpi_asl(test_data *data)
>                  "for instructions on how to update expected files.\n",
>                  exp_sdt->aml, sdt->aml_file, exp_sdt->aml_file);
>
> -        all_tables_match = all_tables_match &&
> +        all_tables_match = all_tables_match ||
>              test_acpi_find_diff_allowed(exp_sdt);
>
>          /*
> --
> MST
>

Re: [PATCH] bios-tables-test: do not ignore allowed diff list

[Igor Mammedov]

On Thu, 27 Oct 2022 11:11:48 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> we had such a beautiful structure for updating
> expected files, designed to keep bisect working.
> It turns out that we ignored the result of
> the allow list checks unless all tables matched
> anyway.
> 
> Sigh.

strange,
it seems to be working fine (I mean white-listing) here

> 
> Let's at least make it work going forward.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  tests/qtest/bios-tables-test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index e6096e7f73..a72f6ca326 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -458,7 +458,7 @@ static void test_acpi_asl(test_data *data)
>                  "for instructions on how to update expected files.\n",
>                  exp_sdt->aml, sdt->aml_file, exp_sdt->aml_file);
>  
> -        all_tables_match = all_tables_match &&
> +        all_tables_match = all_tables_match ||
>              test_acpi_find_diff_allowed(exp_sdt);
>  
>          /*

Re: [PATCH] bios-tables-test: do not ignore allowed diff list

[Michael S. Tsirkin]

On Mon, Oct 31, 2022 at 11:49:42AM +0100, Igor Mammedov wrote:
> On Thu, 27 Oct 2022 11:11:48 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > we had such a beautiful structure for updating
> > expected files, designed to keep bisect working.
> > It turns out that we ignored the result of
> > the allow list checks unless all tables matched
> > anyway.
> > 
> > Sigh.
> 
> strange,
> it seems to be working fine (I mean white-listing) here

it's pretty clear no? if we only check test_acpi_find_diff_allowed
when all tables match anyway, it won't help test pass.

> > 
> > Let's at least make it work going forward.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  tests/qtest/bios-tables-test.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> > index e6096e7f73..a72f6ca326 100644
> > --- a/tests/qtest/bios-tables-test.c
> > +++ b/tests/qtest/bios-tables-test.c
> > @@ -458,7 +458,7 @@ static void test_acpi_asl(test_data *data)
> >                  "for instructions on how to update expected files.\n",
> >                  exp_sdt->aml, sdt->aml_file, exp_sdt->aml_file);
> >  
> > -        all_tables_match = all_tables_match &&
> > +        all_tables_match = all_tables_match ||
> >              test_acpi_find_diff_allowed(exp_sdt);
> >  
> >          /*

Re: [PATCH] bios-tables-test: do not ignore allowed diff list

[Igor Mammedov]

On Mon, 31 Oct 2022 06:52:11 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Oct 31, 2022 at 11:49:42AM +0100, Igor Mammedov wrote:
> > On Thu, 27 Oct 2022 11:11:48 -0400
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > we had such a beautiful structure for updating
> > > expected files, designed to keep bisect working.
> > > It turns out that we ignored the result of
> > > the allow list checks unless all tables matched
> > > anyway.
> > > 
> > > Sigh.  
> > 
> > strange,
> > it seems to be working fine (I mean white-listing) here  
> 
> it's pretty clear no? if we only check test_acpi_find_diff_allowed
> when all tables match anyway, it won't help test pass.

currently all_tables_match is accumulated value that starts with 'true'
and with the meaning 'do not explode unless at least a table was not
explicitly whitelisted'
[...]
> > >  
> > > -        all_tables_match = all_tables_match &&
  '&&' here serves as a trigger that lets flip always initial 'all_tables_match = true'

> > > +        all_tables_match = all_tables_match ||
  once it changes to '||' the all_tables_match will never flip to false
and trigger
  g_assert(all_tables_match);
at the end, when there is unexpected (non-whitelisted) table mismatch.

Am I missing something?

> > >              test_acpi_find_diff_allowed(exp_sdt);
> > >  
> > >          /*  
> 

Re: [PATCH] bios-tables-test: do not ignore allowed diff list

[Michael S. Tsirkin]

On Mon, Oct 31, 2022 at 01:31:04PM +0100, Igor Mammedov wrote:
> On Mon, 31 Oct 2022 06:52:11 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Oct 31, 2022 at 11:49:42AM +0100, Igor Mammedov wrote:
> > > On Thu, 27 Oct 2022 11:11:48 -0400
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >   
> > > > we had such a beautiful structure for updating
> > > > expected files, designed to keep bisect working.
> > > > It turns out that we ignored the result of
> > > > the allow list checks unless all tables matched
> > > > anyway.
> > > > 
> > > > Sigh.  
> > > 
> > > strange,
> > > it seems to be working fine (I mean white-listing) here  
> > 
> > it's pretty clear no? if we only check test_acpi_find_diff_allowed
> > when all tables match anyway, it won't help test pass.
> 
> currently all_tables_match is accumulated value that starts with 'true'

Problem is, it can be false because of another unrelated table.







> and with the meaning 'do not explode unless at least a table was not
> explicitly whitelisted'



> [...]
> > > >  
> > > > -        all_tables_match = all_tables_match &&
>   '&&' here serves as a trigger that lets flip always initial 'all_tables_match = true'
> 
> > > > +        all_tables_match = all_tables_match ||
>   once it changes to '||' the all_tables_match will never flip to false
> and trigger
>   g_assert(all_tables_match);
> at the end, when there is unexpected (non-whitelisted) table mismatch.
> 
> Am I missing something?


I agree this patch is bad.

I did not record the issue I saw and don't really want to go
debugging. Will drop for now.

> > > >              test_acpi_find_diff_allowed(exp_sdt);
> > > >  
> > > >          /*  
> > 

Re: [PATCH] bios-tables-test: do not ignore allowed diff list

[Ani Sinha]

[-- Attachment #1: Type: text/plain, Size: 1620 bytes --]

On Mon, Oct 31, 2022 at 18:01 Igor Mammedov <imammedo@redhat.com> wrote:

> On Mon, 31 Oct 2022 06:52:11 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Mon, Oct 31, 2022 at 11:49:42AM +0100, Igor Mammedov wrote:
> > > On Thu, 27 Oct 2022 11:11:48 -0400
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >
> > > > we had such a beautiful structure for updating
> > > > expected files, designed to keep bisect working.
> > > > It turns out that we ignored the result of
> > > > the allow list checks unless all tables matched
> > > > anyway.
> > > >
> > > > Sigh.
> > >
> > > strange,
> > > it seems to be working fine (I mean white-listing) here
> >
> > it's pretty clear no? if we only check test_acpi_find_diff_allowed
> > when all tables match anyway, it won't help test pass.
>
> currently all_tables_match is accumulated value that starts with 'true'
> and with the meaning 'do not explode unless at least a table was not
> explicitly whitelisted'
> [...]
> > > >
> > > > -        all_tables_match = all_tables_match &&
>   '&&' here serves as a trigger that lets flip always initial
> 'all_tables_match = true'
>
> > > > +        all_tables_match = all_tables_match ||
>   once it changes to '||' the all_tables_match will never flip to false
> and trigger
>   g_assert(all_tables_match);
> at the end, when there is unexpected (non-whitelisted) table mismatch.
>
> Am I missing something?


Ah you are right. My bad I didn’t see this either.


>
> > > >              test_acpi_find_diff_allowed(exp_sdt);
> > > >
> > > >          /*
> >
>
>

[-- Attachment #2: Type: text/html, Size: 2696 bytes --]