| Message ID | 20221027151135.496368-1-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | bios-tables-test: do not ignore allowed diff list | expand |
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 >
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); > > /*
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); > > > > /*
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); > > > > > > /* >
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); > > > > > > > > /* > >
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); > > > > > > > > /* > > > >
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); /*
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(-)