[RFC,2/2] env: Add prefix to error messages when loading env

Message ID 20180717220912.11358-2-semen.protsenko@linaro.org
State New
Headers show
Series
  • [RFC,1/2] env: Drop error messages when loading environment
Related show

Commit Message

Sam Protsenko July 17, 2018, 10:09 p.m.
This is just a draft to discuss ideas related to "Make U-Boot log great
again" thread.

With this patch we will see something like:

    Loading Environment from FAT...
       --> MMC: no card present
       --> ** Bad device mmc 0 **
       --> Failed (-5)
    Loading Environment from MMC...
       --> OK

instead of:

    Loading Environment from FAT... MMC: no card present
    ** Bad device mmc 0 **
    Failed (-5)
    Loading Environment from MMC... OK

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 common/console.c                  | 8 ++++++++
 env/env.c                         | 4 +++-
 include/asm-generic/global_data.h | 1 +
 3 files changed, 12 insertions(+), 1 deletion(-)

Comments

Wolfgang Denk July 18, 2018, 6:23 a.m. | #1
Dear Sam,

In message <20180717220912.11358-2-semen.protsenko@linaro.org> you wrote:
> This is just a draft to discuss ideas related to "Make U-Boot log great
> again" thread.
> 
> With this patch we will see something like:
> 
>     Loading Environment from FAT...
>        --> MMC: no card present
>        --> ** Bad device mmc 0 **
>        --> Failed (-5)

This may be ok in the error case, but...

>     Loading Environment from MMC...
>        --> OK

it is definitely really ugly in the normal, non-error case.

NAK.

>  void puts(const char *s)
>  {
> +	if (gd->pr_prefix) {
> +		const char *prefix = gd->pr_prefix;
> +
> +		gd->pr_prefix = NULL;
> +		puts(prefix);
> +		gd->pr_prefix = prefix;
> +	}
> +

Besides - global data is a precious resource on may systems,
sometimes limited to very few kB of memory.  It should be only used
to the extend where it really cannot be avoided, but not for any
such "beautifying" stuff.

Best regards,

Wolfgang Denk
Tom Rini July 18, 2018, 12:53 p.m. | #2
On Wed, Jul 18, 2018 at 01:09:12AM +0300, Sam Protsenko wrote:

> This is just a draft to discuss ideas related to "Make U-Boot log great

> again" thread.

> 

> With this patch we will see something like:

> 

>     Loading Environment from FAT...

>        --> MMC: no card present

>        --> ** Bad device mmc 0 **

>        --> Failed (-5)

>     Loading Environment from MMC...

>        --> OK

> 

> instead of:

> 

>     Loading Environment from FAT... MMC: no card present

>     ** Bad device mmc 0 **

>     Failed (-5)

>     Loading Environment from MMC... OK


So, I think maybe (and given Wolfgang's comments) we should think about
how the output might want to look, and how to get there without GD
changes.  Perhaps:
Attempting to load Environment from FAT (do we have more easily
available info at this point?):
MMC: no card present
** Bad device mmc 0 **
Failed (-5)
Loading Environment from MMC...
Attempting to load Environment from MMC:
Succeeded

-- 
Tom
Sam Protsenko July 18, 2018, 1:04 p.m. | #3
On Wed, Jul 18, 2018 at 3:53 PM, Tom Rini <trini@konsulko.com> wrote:
> On Wed, Jul 18, 2018 at 01:09:12AM +0300, Sam Protsenko wrote:
>
>> This is just a draft to discuss ideas related to "Make U-Boot log great
>> again" thread.
>>
>> With this patch we will see something like:
>>
>>     Loading Environment from FAT...
>>        --> MMC: no card present
>>        --> ** Bad device mmc 0 **
>>        --> Failed (-5)
>>     Loading Environment from MMC...
>>        --> OK
>>
>> instead of:
>>
>>     Loading Environment from FAT... MMC: no card present
>>     ** Bad device mmc 0 **
>>     Failed (-5)
>>     Loading Environment from MMC... OK
>
> So, I think maybe (and given Wolfgang's comments) we should think about
> how the output might want to look, and how to get there without GD
> changes.  Perhaps:
> Attempting to load Environment from FAT (do we have more easily
> available info at this point?):

Which exactly info do you mean?

> MMC: no card present
> ** Bad device mmc 0 **
> Failed (-5)
> Loading Environment from MMC...
> Attempting to load Environment from MMC:
> Succeeded
>

What do you think if we add some prefix to first message, like:

    ---> Attempting to load Environment from FAT:
    MMC: no card present
    ** Bad device mmc 0 **
    Failed (-5)
    Loading Environment from MMC...
    ---> Attempting to load Environment from MMC:
    Succeeded

just to emphasize that possible errors are belong to prefixed line?
Does it seem better or more ugly to you?

Overall, I agree that this is probably the only sane thing we can do
right now, without meddling too much with drivers code.

> --
> Tom
Wolfgang Denk July 18, 2018, 2:09 p.m. | #4
Dear Tom,

In message <20180718125351.GE4609@bill-the-cat> you wrote:
> 
> >     Loading Environment from FAT...
> >        --> MMC: no card present
> >        --> ** Bad device mmc 0 **
> >        --> Failed (-5)
> >     Loading Environment from MMC...
> >        --> OK
> > 
> > instead of:
> > 
> >     Loading Environment from FAT... MMC: no card present
> >     ** Bad device mmc 0 **
> >     Failed (-5)
> >     Loading Environment from MMC... OK
>
> So, I think maybe (and given Wolfgang's comments) we should think about
> how the output might want to look, and how to get there without GD
> changes.  Perhaps:
> Attempting to load Environment from FAT (do we have more easily
> available info at this point?):
> MMC: no card present
> ** Bad device mmc 0 **
> Failed (-5)
> Loading Environment from MMC...
> Attempting to load Environment from MMC:
> Succeeded

Just my 0.02€:

In the non-error case, the output should be a single (ideally short)
line.

Rationale:  to many lines of ourput clutter your screen and make you
miss context faster; to many/long lines take time to print so they
make booting slower.

In the error case, the user should be able to understand what the
problem was and decide if it was critical or can be ignored (like
here when intentionally booting without SDCard).



Best regards,

Wolfgang Denk
Tom Rini July 18, 2018, 2:28 p.m. | #5
On Wed, Jul 18, 2018 at 04:04:49PM +0300, Sam Protsenko wrote:
> On Wed, Jul 18, 2018 at 3:53 PM, Tom Rini <trini@konsulko.com> wrote:

> > On Wed, Jul 18, 2018 at 01:09:12AM +0300, Sam Protsenko wrote:

> >

> >> This is just a draft to discuss ideas related to "Make U-Boot log great

> >> again" thread.

> >>

> >> With this patch we will see something like:

> >>

> >>     Loading Environment from FAT...

> >>        --> MMC: no card present

> >>        --> ** Bad device mmc 0 **

> >>        --> Failed (-5)

> >>     Loading Environment from MMC...

> >>        --> OK

> >>

> >> instead of:

> >>

> >>     Loading Environment from FAT... MMC: no card present

> >>     ** Bad device mmc 0 **

> >>     Failed (-5)

> >>     Loading Environment from MMC... OK

> >

> > So, I think maybe (and given Wolfgang's comments) we should think about

> > how the output might want to look, and how to get there without GD

> > changes.  Perhaps:

> > Attempting to load Environment from FAT (do we have more easily

> > available info at this point?):

> 

> Which exactly info do you mean?


Do we easily know things like what device / partition we're trying?  Or
only "env type is $X" ?

> > MMC: no card present

> > ** Bad device mmc 0 **

> > Failed (-5)

> > Loading Environment from MMC...

> > Attempting to load Environment from MMC:

> > Succeeded

> >

> 

> What do you think if we add some prefix to first message, like:

> 

>     ---> Attempting to load Environment from FAT:

>     MMC: no card present

>     ** Bad device mmc 0 **

>     Failed (-5)

>     Loading Environment from MMC...

>     ---> Attempting to load Environment from MMC:

>     Succeeded

> 

> just to emphasize that possible errors are belong to prefixed line?

> Does it seem better or more ugly to you?


I don't know.  I'm not a fan, but I don't always have the best taste.

-- 
Tom
Tom Rini July 19, 2018, 12:52 p.m. | #6
On Wed, Jul 18, 2018 at 04:09:33PM +0200, Wolfgang Denk wrote:
> Dear Tom,

> 

> In message <20180718125351.GE4609@bill-the-cat> you wrote:

> > 

> > >     Loading Environment from FAT...

> > >        --> MMC: no card present

> > >        --> ** Bad device mmc 0 **

> > >        --> Failed (-5)

> > >     Loading Environment from MMC...

> > >        --> OK

> > > 

> > > instead of:

> > > 

> > >     Loading Environment from FAT... MMC: no card present

> > >     ** Bad device mmc 0 **

> > >     Failed (-5)

> > >     Loading Environment from MMC... OK

> >

> > So, I think maybe (and given Wolfgang's comments) we should think about

> > how the output might want to look, and how to get there without GD

> > changes.  Perhaps:

> > Attempting to load Environment from FAT (do we have more easily

> > available info at this point?):

> > MMC: no card present

> > ** Bad device mmc 0 **

> > Failed (-5)

> > Loading Environment from MMC...

> > Attempting to load Environment from MMC:

> > Succeeded

> 

> Just my 0.02€:

> 

> In the non-error case, the output should be a single (ideally short)

> line.

> 

> Rationale:  to many lines of ourput clutter your screen and make you

> miss context faster; to many/long lines take time to print so they

> make booting slower.

> 

> In the error case, the user should be able to understand what the

> problem was and decide if it was critical or can be ignored (like

> here when intentionally booting without SDCard).


I understand, but I don't know if we can get there still.  The problem
is we don't know if we've succeeded until we've done the relevant
probing and that in turn is what's breaking the single line, and got us
to where we are now.

-- 
Tom
Sam Protsenko July 19, 2018, 1:12 p.m. | #7
On Thu, Jul 19, 2018 at 3:52 PM, Tom Rini <trini@konsulko.com> wrote:
> On Wed, Jul 18, 2018 at 04:09:33PM +0200, Wolfgang Denk wrote:
>> Dear Tom,
>>
>> In message <20180718125351.GE4609@bill-the-cat> you wrote:
>> >
>> > >     Loading Environment from FAT...
>> > >        --> MMC: no card present
>> > >        --> ** Bad device mmc 0 **
>> > >        --> Failed (-5)
>> > >     Loading Environment from MMC...
>> > >        --> OK
>> > >
>> > > instead of:
>> > >
>> > >     Loading Environment from FAT... MMC: no card present
>> > >     ** Bad device mmc 0 **
>> > >     Failed (-5)
>> > >     Loading Environment from MMC... OK
>> >
>> > So, I think maybe (and given Wolfgang's comments) we should think about
>> > how the output might want to look, and how to get there without GD
>> > changes.  Perhaps:
>> > Attempting to load Environment from FAT (do we have more easily
>> > available info at this point?):
>> > MMC: no card present
>> > ** Bad device mmc 0 **
>> > Failed (-5)
>> > Loading Environment from MMC...
>> > Attempting to load Environment from MMC:
>> > Succeeded
>>
>> Just my 0.02€:
>>
>> In the non-error case, the output should be a single (ideally short)
>> line.
>>
>> Rationale:  to many lines of ourput clutter your screen and make you
>> miss context faster; to many/long lines take time to print so they
>> make booting slower.
>>
>> In the error case, the user should be able to understand what the
>> problem was and decide if it was critical or can be ignored (like
>> here when intentionally booting without SDCard).
>
> I understand, but I don't know if we can get there still.  The problem
> is we don't know if we've succeeded until we've done the relevant
> probing and that in turn is what's breaking the single line, and got us
> to where we are now.
>

Actually we can, please see my new RFC patch [1]. It's a bit hacky,
but the only other way to do so is to rework drivers (MMC, etc).

Also, I figured how to do prefixing (to display MMC errors as nested
w.r.t. "Loading environment), without adding new field to gd. We can
just add some new GD_LG_ and print prefix when it's installed. I'm
gonna send new RFC soon. Please let me know what you think about [1].

[1] https://lists.denx.de/pipermail/u-boot/2018-July/335223.html

> --
> Tom
Wolfgang Denk July 19, 2018, 7:49 p.m. | #8
Dear Tom,

In message <20180719125230.GJ4609@bill-the-cat> you wrote:
> 
> > > >     Loading Environment from FAT... MMC: no card present
> > > >     ** Bad device mmc 0 **
> > > >     Failed (-5)
> > > >     Loading Environment from MMC... OK
...
> > In the non-error case, the output should be a single (ideally short)
> > line.
> > 
> > Rationale:  to many lines of ourput clutter your screen and make you
> > miss context faster; to many/long lines take time to print so they
> > make booting slower.
> > 
> > In the error case, the user should be able to understand what the
> > problem was and decide if it was critical or can be ignored (like
> > here when intentionally booting without SDCard).
>
> I understand, but I don't know if we can get there still.  The problem
> is we don't know if we've succeeded until we've done the relevant
> probing and that in turn is what's breaking the single line, and got us
> to where we are now.

Well, IMO the approach should be the other way round - think about
where we print errror messages, and when.

In the example above the "MMC: no card present" makes most sense.

When we come to printing the "** Bad device mmc 0 **" we should be
in an error path, where all possible causes have already printed an
appropriate message, so this line can be removed.

Ditto for the "Failed (-5)" which is pretty useless anyway - if
error handling was consequent, this message should never be needed,
as all error paths ending there would have printed proper messages
long before.

So instead of adding prefixes or fancy postformatting we should
clean up error handling and use a consistent style to report the
errors where they are found and the causes for the errors are known.

Thanks.

Best regards,

Wolfgang Denk
Wolfgang Denk July 19, 2018, 7:52 p.m. | #9
Dear Sam,

In message <CAKaJLVuxoS0mz9pDDYqmqomsSfd83kSed=LZ261=wfUUC-Q-Xw@mail.gmail.com> you wrote:
>
> Also, I figured how to do prefixing (to display MMC errors as nested
> w.r.t. "Loading environment), without adding new field to gd. We can
> just add some new GD_LG_ and print prefix when it's installed. I'm
> gonna send new RFC soon. Please let me know what you think about [1].

This is IMO a totally wrong approach.  We don't want to add more
code (and more output) jsut to paper over inconsistent error
reporting.  This problem should be fixed at the root cause, i. e.
we should report errors where they are detected, and only once.
If all callers can rely that, in the error path, their callees which
return error codes, have already printed appropriate messages, there
is no need to print even more lines.

What we need is not fancy code, but consistent (and consequent)
error handling.

Best regards,

Wolfgang Denk
Sam Protsenko July 19, 2018, 8:16 p.m. | #10
On Thu, Jul 19, 2018 at 10:52 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Sam,
>
> In message <CAKaJLVuxoS0mz9pDDYqmqomsSfd83kSed=LZ261=wfUUC-Q-Xw@mail.gmail.com> you wrote:
>>
>> Also, I figured how to do prefixing (to display MMC errors as nested
>> w.r.t. "Loading environment), without adding new field to gd. We can
>> just add some new GD_LG_ and print prefix when it's installed. I'm
>> gonna send new RFC soon. Please let me know what you think about [1].
>
> This is IMO a totally wrong approach.  We don't want to add more
> code (and more output) jsut to paper over inconsistent error
> reporting.  This problem should be fixed at the root cause, i. e.
> we should report errors where they are detected, and only once.
> If all callers can rely that, in the error path, their callees which
> return error codes, have already printed appropriate messages, there
> is no need to print even more lines.
>
> What we need is not fancy code, but consistent (and consequent)
> error handling.
>

As a matter of fact, that was first thing that came into my mind when
I started looking into this (and actually I mentioned that way in my
first RFC, I guess). I will try to investigate it further and come
back with more meaningful patch.

My concern w.r.t. this approach is next: if we suppress consequent
warning messages, there might be some other use-case (other than
loading environment), where there is another caller of that API, and
we would just lose all error messages at all. I'll try to check if we
have such cases as well.

Overall: agree with your review, thanks.

> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> "One day," said a dull voice from down below, "I'm going to  be  back
> in  form again and you're going to be very sorry you said that. For a
> very long time. I might even go so far as to make even more Time just
> for you to be sorry in."              - Terry Pratchett, _Small Gods_

Patch

diff --git a/common/console.c b/common/console.c
index 2ba33dc574..1bbafa33a1 100644
--- a/common/console.c
+++ b/common/console.c
@@ -533,6 +533,14 @@  void putc(const char c)
 
 void puts(const char *s)
 {
+	if (gd->pr_prefix) {
+		const char *prefix = gd->pr_prefix;
+
+		gd->pr_prefix = NULL;
+		puts(prefix);
+		gd->pr_prefix = prefix;
+	}
+
 #ifdef CONFIG_DEBUG_UART
 	if (!gd || !(gd->flags & GD_FLG_SERIAL_READY)) {
 		while (*s) {
diff --git a/env/env.c b/env/env.c
index 5c0842ac07..56a105ea55 100644
--- a/env/env.c
+++ b/env/env.c
@@ -194,12 +194,14 @@  int env_load(void)
 		if (!env_has_inited(drv->location))
 			continue;
 
-		printf("Loading Environment from %s... ", drv->name);
+		printf("Loading Environment from %s...\n", drv->name);
+		gd->pr_prefix = "   --> ";
 		ret = drv->load();
 		if (ret)
 			printf("Failed (%d)\n", ret);
 		else
 			printf("OK\n");
+		gd->pr_prefix = NULL;
 
 		if (!ret)
 			return 0;
diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
index 0fd4900392..32b80db96b 100644
--- a/include/asm-generic/global_data.h
+++ b/include/asm-generic/global_data.h
@@ -44,6 +44,7 @@  typedef struct global_data {
 	unsigned long board_type;
 #endif
 	unsigned long have_console;	/* serial_init() was called */
+	const char *pr_prefix;		/* print prefix before message */
 #if CONFIG_IS_ENABLED(PRE_CONSOLE_BUFFER)
 	unsigned long precon_buf_idx;	/* Pre-Console buffer index */
 #endif