[U-Boot] [RFC PATCH v3 0/2] env: Make environment loading log more clear

[U-Boot] [RFC PATCH v3 0/2] env: Make environment loading log more clear

[Sam Protsenko]

This is another attempt to make boot log better, without hackery this
time. Basically here we just remove unwanted error messages, relying on
the message from most deep API to be printed (like mmc subsystem). This
approach *might* have its shortcomings, though:
 1. With no "Failed" message, at some point we *can* end up with no
    error messages printed at all
 2. Removing some collateral error messages *may* lead to loss of useful
    debug info in other use-cases (env_load() is not only user of those
    APIs).

That being said, at the moment this looks like most clean solution to
cluttered log problem, as any other solution either will be hackish or
will require some big architectural changes. If one of mentioned
shortcomings occur, we can fix it ad hoc.

With this patch set applied we will see something like this:

    Loading Environment from FAT... MMC: no card present
    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


Sam Protsenko (2):
  env: Don't print "Failed" error message
  disk: part: Remove redundant error message

 disk/part.c | 1 -
 env/env.c   | 4 +---
 2 files changed, 1 insertion(+), 4 deletions(-)

-- 
2.18.0

[U-Boot] [RFC PATCH v3 1/2] env: Don't print "Failed" error message

[Sam Protsenko]

This only clutters the log with unnecessary details, as we already have
all needed warnings by the time. Example:

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

Remove this "Failed" message to keep log short and clear.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 env/env.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/env/env.c b/env/env.c
index 5c0842ac07..3ab4ec4237 100644
--- a/env/env.c
+++ b/env/env.c
@@ -196,9 +196,7 @@ int env_load(void)
 
 		printf("Loading Environment from %s... ", drv->name);
 		ret = drv->load();
-		if (ret)
-			printf("Failed (%d)\n", ret);
-		else
+		if (!ret)
 			printf("OK\n");
 
 		if (!ret)
-- 
2.18.0

[U-Boot] [RFC PATCH v3 2/2] disk: part: Remove redundant error message

[Sam Protsenko]

Underlying API should already print some meaningful error message, so
this one is just brings more noise. E.g. we can see log like this:

    MMC: no card present
    ** Bad device mmc 0 **

Obviously, second error message is unwanted. Let's remove it to make log
more short and clear.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 disk/part.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/disk/part.c b/disk/part.c
index 9266a09ec3..0762a0750f 100644
--- a/disk/part.c
+++ b/disk/part.c
@@ -400,7 +400,6 @@ int blk_get_device_by_str(const char *ifname, const char *dev_hwpart_str,
 
 	*dev_desc = get_dev_hwpart(ifname, dev, hwpart);
 	if (!(*dev_desc) || ((*dev_desc)->type == DEV_TYPE_UNKNOWN)) {
-		printf("** Bad device %s %s **\n", ifname, dev_hwpart_str);
 		dev = -ENOENT;
 		goto cleanup;
 	}
-- 
2.18.0

[U-Boot] [RFC PATCH v3 2/2] disk: part: Remove redundant error message

[Tom Rini]

On Fri, Jul 20, 2018 at 01:28:43AM +0300, Sam Protsenko wrote:

> Underlying API should already print some meaningful error message, so
> this one is just brings more noise. E.g. we can see log like this:
> 
>     MMC: no card present
>     ** Bad device mmc 0 **
> 
> Obviously, second error message is unwanted. Let's remove it to make log
> more short and clear.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  disk/part.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/disk/part.c b/disk/part.c
> index 9266a09ec3..0762a0750f 100644
> --- a/disk/part.c
> +++ b/disk/part.c
> @@ -400,7 +400,6 @@ int blk_get_device_by_str(const char *ifname, const char *dev_hwpart_str,
>  
>  	*dev_desc = get_dev_hwpart(ifname, dev, hwpart);
>  	if (!(*dev_desc) || ((*dev_desc)->type == DEV_TYPE_UNKNOWN)) {
> -		printf("** Bad device %s %s **\n", ifname, dev_hwpart_str);
>  		dev = -ENOENT;
>  		goto cleanup;
>  	}

We should move this to debug() I think.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180719/6c3c92e5/attachment.sig>

[U-Boot] [RFC PATCH v3 2/2] disk: part: Remove redundant error message

[Sam Protsenko]

On Fri, Jul 20, 2018 at 1:53 AM, Tom Rini <trini@konsulko.com> wrote:
> On Fri, Jul 20, 2018 at 01:28:43AM +0300, Sam Protsenko wrote:
>
>> Underlying API should already print some meaningful error message, so
>> this one is just brings more noise. E.g. we can see log like this:
>>
>>     MMC: no card present
>>     ** Bad device mmc 0 **
>>
>> Obviously, second error message is unwanted. Let's remove it to make log
>> more short and clear.
>>
>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
>> ---
>>  disk/part.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/disk/part.c b/disk/part.c
>> index 9266a09ec3..0762a0750f 100644
>> --- a/disk/part.c
>> +++ b/disk/part.c
>> @@ -400,7 +400,6 @@ int blk_get_device_by_str(const char *ifname, const char *dev_hwpart_str,
>>
>>       *dev_desc = get_dev_hwpart(ifname, dev, hwpart);
>>       if (!(*dev_desc) || ((*dev_desc)->type == DEV_TYPE_UNKNOWN)) {
>> -             printf("** Bad device %s %s **\n", ifname, dev_hwpart_str);
>>               dev = -ENOENT;
>>               goto cleanup;
>>       }
>
> We should move this to debug() I think.
>

Yes, better to use debug() than remove it completely, thanks. So, if
there is no further objections, I'm gonna set real patch series soon.

> --
> Tom

[U-Boot] [RFC PATCH v3 0/2] env: Make environment loading log more clear

[Wolfgang Denk]

Dear Sam,

In message <20180719222843.28316-1-semen.protsenko@linaro.org> you wrote:
>
>  1. With no "Failed" message, at some point we *can* end up with no
>     error messages printed at all

That would mean that we did not check the whole call tree as needed.
It is not that complicated, or is it?

>  2. Removing some collateral error messages *may* lead to loss of useful
>     debug info in other use-cases (env_load() is not only user of those
>     APIs).

I dislike the "can" and "may" parts here.  If this is done
thoroughly, we should know exactly that no such damage gets done.

As I mentioend before: if we get here, somewhere an error must have
occurred.  And in the error handling an error message (one) must be
printed.

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 at denx.de
Software entities are more complex for their size  than  perhaps  any
other human construct because no two parts are alike. If they are, we
make  the  two  similar parts into a subroutine -- open or closed. In
this respect, software  systems  differ  profoundly  from  computers,
buildings, or automobiles, where repeated elements abound.
                                                   - Fred Brooks, Jr.

[U-Boot] [RFC PATCH v3 0/2] env: Make environment loading log more clear

[Sam Protsenko]

On Fri, Jul 20, 2018 at 4:27 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Sam,
>
> In message <20180719222843.28316-1-semen.protsenko@linaro.org> you wrote:
>>
>>  1. With no "Failed" message, at some point we *can* end up with no
>>     error messages printed at all
>
> That would mean that we did not check the whole call tree as needed.
> It is not that complicated, or is it?
>

I've checked the error chain for FAT/MMC env load: [1]. As for the
rest of possible sources, frankly I didn't check, as I don't have such
boards at my disposal anyway, and I may not be aware of all sources we
have. That's why I've only corrected the path for loading from FAT,
which I can test and can argue it's working right. I will add the
comment in env_load() saying that error message must be printed from
underlying subsystem, and it should be exactly one error. If we catch
any related issues further, we can check it on the sport. I hope
you're ok with this approach, as fixing all possible error cases for
all possible sources in at once seems to be too complex task. And some
further messages can appear further, if someone would manage to get
such a patch merged. So let's start with clear requirement in
env_load().

[1] https://pastebin.com/q6kgpprb

>>  2. Removing some collateral error messages *may* lead to loss of useful
>>     debug info in other use-cases (env_load() is not only user of those
>>     APIs).
>
> I dislike the "can" and "may" parts here.  If this is done
> thoroughly, we should know exactly that no such damage gets done.
>

Agree. Also, I like Tom's suggestion to move those to debug() instead
of removing.

> As I mentioend before: if we get here, somewhere an error must have
> occurred.  And in the error handling an error message (one) must be
> printed.
>
> 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 at denx.de
> Software entities are more complex for their size  than  perhaps  any
> other human construct because no two parts are alike. If they are, we
> make  the  two  similar parts into a subroutine -- open or closed. In
> this respect, software  systems  differ  profoundly  from  computers,
> buildings, or automobiles, where repeated elements abound.
>                                                    - Fred Brooks, Jr.