| Message ID | 20180719222843.28316-1-semen.protsenko@linaro.org |
|---|---|
| Headers | show |
| Series | env: Make environment loading log more clear | expand |
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
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@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.
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(-)