On Fri, Jun 27, 2025 at 12:06:02PM -0700, Jesse Taube wrote:
On Fri, Jun 27, 2025 at 6:02 AM Andrew Jones andrew.jones@linux.dev wrote:
On Wed, Jun 18, 2025 at 08:44:52AM -0700, Jesse Taube wrote:
...
report(shmem->data.tdata1 == tdata1, "tdata1 expected: 0x%016lx", tdata1);report_info("tdata1 found: 0x%016lx", shmem->data.tdata1);report(shmem->data.tdata2 == ((unsigned long)&test), "tdata2 expected: 0x%016lx",(unsigned long)&test);report_info("tdata2 found: 0x%016lx", shmem->data.tdata2);report(shmem->data.tstate == tstatus_expected, "tstate expected: 0x%016lx", tstatus_expected);report_info("tstate found: 0x%016lx", shmem->data.tstate);These don't need to be split into report/report_info pairs because the report is checking for a specific value. We only split when report is checking for some nonzero value and we also want to output what that specific value was for informational purposes.
I'm a bit confused. Should it only report_info when the test fails?
Usually, and that's what we do in sbiret_report().
I haven't been very good at describing my idea about this nor really implementing it consistently throughout the framework. The idea is that when platform specific or SBI implementation specific values are involved that we should avoid putting them in report() strings in order to ensure we can capture output from a successful run, where INFO lines have been filtered out, and then subsequent runs can have their filtered output directly diffed with that originally captured output, even if we run on another platform / SBI implementation. I don't think we can achieve that right now, but I try to keep it in mind when reviewing to avoid making things worse. Besides the sbiret_report() approach we can also always output values with report_info if that information may be useful, i.e.
report(check(value), "no values output here"); report_info("value is %d");
or, with an expected value output
expected = getenv("SOME_VAR"); if (!report(value == expected, "value matches expectation (%d)", expected)) report_info("value is %d");
The second approach won't allow diffing output captures arbitrarily, but should still allow diffing for targets with the same configurations, described by the environment variables, allowing the report lines to contain more information.
Looking at your use above again, then, assuming the expected value is something we want to always output, like in the environment variable example, I guess there's nothing wrong with it. We could just avoid the info lines on success with the if (!report(... guarding them.
...
+void check_dbtr(void) +{
static struct sbi_dbtr_shmem_entry shmem[RV_MAX_TRIGGERS] = {};unsigned long num_trigs;enum McontrolType trig_type;struct sbiret ret;report_prefix_push("dbtr");if (!sbi_probe(SBI_EXT_DBTR)) {report_skip("extension not available");report_prefix_pop();return;Could rename the 'error' label to something else and goto it from here too.
Is `dtbr_exit_test` ok?
Sure, or even just exit_test, since this is a dbtr-only function.
Thanks, drew