dump_power_status aborts instead of exiting with 1 when run on broken systems |
|||||
Issue description
Chrome Version: 63.0.3212.0 / 9928.0.0
What steps will reproduce the problem?
(1) I have a mighty PVT SKU2 with dead battery.
(2) After unplugging power for ~2 hours and replugging it, running dump_power_status from the console reproducibly results in a crash. (After a few minutes the crash disappears, likely because some input values have changed.)
Output:
[0910/034659:WARNING:power_supply.cc(898)] Ignoring reading with bad or missing nominal (0) and instantaneous (0) voltages
open("/proc/self/status", O_RDONLY|O_LARGEFILE) = 11
[0910/034659:FATAL:dump_power_status.cc(31)] Check failed: power_supply.RefreshImmediately().
Attaching log of "strace -eopen dump_power_status".
,
Sep 14 2017
Ideas after discussion with tnagel: The CHECK() was appropriate while dump_power_status was just a testing tool. Since we're now using it in production, it makes sense to replace that CHECK either with an exit(N) where N is a status code that indicates "broken battery". Or alternatively, we could continue and print status, making sure that battery_present=0, or maybe even battery_present=broken?
,
Sep 14 2017
The initial report appears to be describing dump_power_status being used as a testing tool (i.e. run from the console). Is the fact that it aborts when run on a broken system rather than exiting with 1 causing problems in some production use? (I wasn't aware that there were any non-test uses of dump_power_status, although it looks like it's used by some firmware-updating scripts now.)
,
Sep 15 2017
I am guilty for using dump_power_status in the tpm firmware updater script, but only after I had asked you whether that's OK. I have an email thread to prove this :-p The abort we're seeing is not a problem right now as we behave correctly and assume there's no battery present so we don't need to check for sufficient charge. So this is merely a cosmetic issue as long as we don't have consumers of dump_power_status that want to distinguish "broken" from "no battery" situations. tnagel@ still has a point that aborting in a situation that will happen from time to time isn't ideal (unnecessary crash reports etc.)
,
Sep 15 2017
This is indicative of broken hardware and/or EC or kernel bugs, though. In theory, someone could use these crash reports to identify and try to fix these issues. I'm fine with changing dump_power_status to exit with 1 if you think that that'll never happen. Making it continue in the face of errors would be a bit more work (since this code is shared with powerd, and I don't want to change powerd's behavior).
,
Sep 15 2017
,
Oct 6 2017
Tend to agree w/ Dan's point that we want to surface these crash reports for faulty HW. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by mnissler@chromium.org
, Sep 14 2017