| Message ID | 20220909151801.336551-3-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | vbe: Support device tree fixups for OS requests | expand |
Am 9. September 2022 17:17:49 MESZ schrieb Simon Glass <sjg@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@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. Best regards Heinrich > return ret; > } >
Hi Heinrich, On Fri, 9 Sept 2022 at 09:33, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > Am 9. September 2022 17:17:49 MESZ schrieb Simon Glass <sjg@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@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 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); I don't fully understand how we allow things after %p without ambiguity...do you know? Regards, Simon
On 9/9/22 20:20, Simon Glass wrote: > Hi Heinrich, > > On Fri, 9 Sept 2022 at 09:33, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> >> >> >> Am 9. September 2022 17:17:49 MESZ schrieb Simon Glass <sjg@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@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
Hi Heinrich, On Sat, 10 Sept 2022 at 00:53, Heinrich Schuchardt <xypron.glpk@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@gmx.de> wrote: > >> > >> > >> > >> Am 9. September 2022 17:17:49 MESZ schrieb Simon Glass <sjg@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@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
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 ret; }
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@chromium.org> --- boot/bootm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)