[PATCH v2 06/17] boot: Add a new test for global bootmeths

[Simon Glass]

Hi Tom,

On Wed, 8 Oct 2025 at 09:32, Tom Rini <trini at konsulko.com> 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 <trini at konsulko.com> 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 <sjg at chromium.org>
> > > [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