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

[Tom Rini]

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?".

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20251008/19dbe006/attachment.sig>