[PATCH 02/14] bood

[Heinrich Schuchardt]

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’

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'

Best regards

Heinrich