Hi Tom, On Wed, 8 Oct 2025 at 09:32, Tom Rini <[email protected]> wrote: > > On Wed, Oct 08, 2025 at 06:54:21AM -0600, Simon Glass wrote: > > Hi Tom, > > > > On Tue, 7 Oct 2025 at 11:58, Tom Rini <[email protected]> wrote: > > > > > > On Wed, Oct 01, 2025 at 03:26:31PM -0600, Simon Glass wrote: > > > > > > > These have different behaviour from normal bootmeths and we are about to > > > > enhance it. So add a test and also an extra check in bootflow_iter() > > > > > > > > Signed-off-by: Simon Glass <[email protected]> > > > [snip] > > > > @@ -388,6 +390,49 @@ static int bootflow_iter(struct unit_test_state > > > > *uts) > > > > BOOTSTD_TEST(bootflow_iter, UTF_DM | UTF_SCAN_FDT | UTF_CONSOLE); > > > > > > > > #if defined(CONFIG_SANDBOX) && defined(CONFIG_BOOTMETH_GLOBAL) > > > > + > > > > +/* Check iterating through available bootflows to test global > > > > bootmeths */ > > > > +static int bootflow_iter_glob(struct unit_test_state *uts) > > > > +{ > > > > + struct bootflow_iter iter; > > > > + struct bootflow bflow; > > > > + > > > > + bootstd_clear_glob(); > > > > + > > > > + /* we should get the global bootmeth initially */ > > > > + ut_asserteq(-EINVAL, > > > > + bootflow_scan_first(NULL, NULL, &iter, BOOTFLOWIF_ALL > > > > | > > > > + BOOTFLOWIF_SHOW, &bflow)); > > > > + bootflow_show(0, &bflow, true); > > > > > > Here is the one call of bootflow_show outside of where show_bootflow is > > > called today, and bootflow_show is called by the end of this series. > > > > > > > + ut_asserteq(3, iter.num_methods); > > > > + ut_assert(iter.doing_global); > > > > + ut_asserteq(2, iter.first_glob_method); > > > > + > > > > + ut_asserteq(2, iter.cur_method); > > > > + ut_asserteq(0, iter.part); > > > > + ut_asserteq(0, iter.max_part); > > > > + ut_asserteq_str("firmware0", iter.method->name); > > > > + ut_asserteq(0, bflow.err); > > > > + bootflow_free(&bflow); > > > > + > > > > + /* next we should get the first non-global bootmeth */ > > > > + ut_asserteq(-EPROTONOSUPPORT, bootflow_scan_next(&iter, &bflow)); > > > > + > > > > + /* at this point the global bootmeths are stranded above > > > > num_methods */ > > > > + ut_asserteq(2, iter.num_methods); > > > > + ut_asserteq(2, iter.first_glob_method); > > > > + ut_assert(!iter.doing_global); > > > > + > > > > + ut_asserteq(0, iter.cur_method); > > > > + ut_asserteq(0, iter.part); > > > > + ut_asserteq(0, iter.max_part); > > > > + ut_asserteq_str("extlinux", iter.method->name); > > > > + ut_asserteq(0, bflow.err); > > > > > > Nothing in the rest of this test is checking the output of strings, and > > > nor should it since the test is checking the struct itself to be as > > > expected. Am I missing something? If not, I don't think we should bother > > > with the previous patch. > > > > You're missing that when the test fails you need to try to figure out > > why...it is much easier to do that if it prints out info on the > > bootflow. In my case it was printing a completely different one. > > I want to come back to this for a moment, because (along with a few > other parts of this series) it shows the hard question of: > > - After debugging a problem, how much logging / debug statements should > remain? > > More comments are always good. There's no size cost, there's no run time > cost and it helps everyone who comes along later understand the code > better. > > Given that CONFIG_LOG isn't usually enabled, on the one hand log_debug > should be fine as that should be compile-time optimized out. On the > other hand, is that information that we should have available already? > Do we need a debug statement about returning -EFOO vs the caller(s) > printing "got %d as ret" ? > > And then it's also a question of "will anyone use this again? Is the > churn to git blame for adding '{' here worth it?" > > I often lean towards the last question being answered with "No" and so > just making sure comments are correct. That also means not keeping in > things like show_bootflow being a public function now even if I had > exposed it as a quick "what does this number mean again?".
The best initial debugging aid I have with bootstd etc. is to enable CONFIG_LOG_ERROR_RETURN since it shows a trace of where things happened. It doesn't have a code cost unless it is enabled. For myself, I do like adding things like show_bootflow() in tests. It is so much easier to understand what is going wrong when you can pass -v and see the test output. Regards, Simon
