[PATCH 02/14] bood

[Simon Glass]

Hi Heinrich,

On Sat, 10 Sept 2022 at 00:53, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 9/9/22 20:20, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Fri, 9 Sept 2022 at 09:33, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> >>
> >>
> >>
> >> Am 9. September 2022 17:17:49 MESZ schrieb Simon Glass <sjg at chromium.org>:
> >>> At present when bootm fails, it says:
> >>>
> >>>     subcommand not supported
> >>>
> >>> and then prints help for the bootm command. This is not very useful, since
> >>> generally the error is related to something else, such as fixups failing.
> >>> It is quite confusing to see this in a test run.
> >>>
> >>> Change the error and show the error code.
> >>>
> >>> We could update the OS functions to return -ENOSYS when they do not
> >>> support the bootm subcommand. But this involves some thought since this is
> >>> arch-specific code and proper errno error codes are not always returned.
> >>> Also, with the code as is, all required subcommands are of course
> >>> supported - a problem would only come if someone added a new one or
> >>> removed support for one from an existing OS. Therefore it seems better to
> >>> leave that sort of effort for when our bootm tests are improved.
> >>>
> >>> Signed-off-by: Simon Glass <sjg at chromium.org>
> >>> ---
> >>>
> >>> boot/bootm.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/boot/bootm.c b/boot/bootm.c
> >>> index f6713807fda..ed6b489c4b3 100644
> >>> --- a/boot/bootm.c
> >>> +++ b/boot/bootm.c
> >>> @@ -790,7 +790,7 @@ int do_bootm_states(struct cmd_tbl *cmdtp, int flag, int argc,
> >>>
> >>>        /* Check for unsupported subcommand. */
> >>>        if (ret) {
> >>> -              puts("subcommand not supported\n");
> >>> +              printf("subcommand failed (err=%d)\n", ret);
> >>
> >> Return codes are only interpretable by developers. We have a function to convert errno to a string.
> >>
> >> For the average user it would be helpful to know which  (sub-)command failed especially if this boot command is executed in an automated way.
> >
> > I don't disagree, but:
> > 1. The error strings add to code size, about 5KB or so
>
> This is controlled by CONFIG_ERRNO_STR.
>
> > 2. For devs the error number is much easier to use
> > 3. For bug reports the error number is better too IMO
> > 4. As per the commit message, we don't have a consistent way for
> > subcommands to report errors
> >
> > So I think this patch is an improvement, in that it actually says what
> > is happening (rather than mostly saying something that is untrue) and
> > does not increase code size much.
> >
> > I wonder if we should have a way to show an error number + string in printf()?
> >
> > printf("subcommand failed (%pE)\n", ret);
>
> %p is meant for pointers only. Using it for an integer will lead to a
> build error:
>
>      format ‘%p’ expects argument of type ‘void *’,
>      but argument 2 has type ‘int’

Yes and that is the other piece of the puzzle - we cannot add random
chars between % and p since the C compiler complains.
>
> For signed int we have the choice of:
> '%d', '%i', '%o', or '%x'.
>
> I suggest to use %iE.
>
> %dE already occurs in existing code:
>
> include/ansi.h:15
> :#define ANSI_CURSOR_NEXTLINE          "\e[%dE"
>
> >
> > I don't fully understand how we allow things after %p without
> > ambiguity...do you know?
>
> We rely on developers only wanting to print a pointer not using any
> character with special meaning after %p. If you actually wanted to print
> the letter 'D' directly after a pointer you would have to put it into a
> separate string:
>
>      printf("%p""D", p);
>
> In our existing code %i is succeeded by the following characters:
> ' ', '!', '"', ')', ',', '.', ':', '@', '\', ']', '0', 'a', 'g', 'n', 'o'.
>
> So using 'E' is safe.
>
> For %d succeeding characters are:
> ' ', '!', '"', '%', ''', '(', ')', '*', '+', ',', '-', '.', '/', ':',
> ';', '<', '=', '>', '?', '@', '[', '\', ']', '_', '}', '0', '1', '2',
> '3', '4', '5', '7', 'a', 'A', 'b', 'B', 'C', 'd', 'D', 'e', 'E', 'f',
> 'F', 'G', 'H', 'i', 'k', 'K', 'm', 'M', 'n', 'o', 'p', 'r', 'R', 's',
> 'T', 'u', 'W', 'x'.
>
> For %o:
> ' ', '"', ')', 'b', 'p', 'r'.
>
> For %x:
> '!', '"', '#', '%', ''', '(', ')', ',', '-', '.', '/', ':', ';', '>',
> '[', '\', ']', '_', '}', '1', 'h', 'n'

I was assuming that %dE (the obvious choice since errors start with a
capital E) would be confusing / annoying, but I think you are right. I
will take a look.

Regards,
Simon