Report all system temperatures, not just CPU

Report all system temperatures, not just CPU

Intel devices put CPU temperatures in a devices/ sub directory of hwmon*. ARM
devices and non-CPU sensors on Intel don't use a devices directory and also may
not contain labels. Use the name file for a label if label doesn't exist.

BUG=530499
TEST=configure managed device to auto-start kiosk. ARM devices should show temps. Intel devices should show non-CPU temps.

Committed: https://crrev.com/7d1b94768d646c9ae2318a85447f544044b38a4b
Cr-Commit-Position: refs/heads/master@{#420629}

[Jay Lee, 2016-07-01 14:38:02]

Description was changed from

==========
Report all system temperatures, not just CPU

Intel devices put CPU temperatures in a devices/ sub directory of hwmon*. ARM
devices and non-CPU sensors on Intel don't use a devices directory and also may
not contain labels. Use the name file for a label if label doesn't exist.

BUG=530499
TEST=configure managed device to auto-start kiosk. ARM devices should show
temps. Intel devices should show non-CPU temps.
==========

to

==========
Report all system temperatures, not just CPU

Intel devices put CPU temperatures in a devices/ sub directory of hwmon*. ARM
devices and non-CPU sensors on Intel don't use a devices directory and also may
not contain labels. Use the name file for a label if label doesn't exist.

BUG=530499
TEST=configure managed device to auto-start kiosk. ARM devices should show
temps. Intel devices should show non-CPU temps.
==========

[Jay Lee, 2016-07-01 14:38:03]

jayhlee@google.com changed reviewers:
+ bartfab@chromium.org, pbond@chromium.org

[Polina Bondarenko, 2016-07-01 15:22:26]

https://codereview.chromium.org/2115723003/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/device_status_collector.cc (right):

https://codereview.chromium.org/2115723003/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/device_status_collector.cc:170: hwmon_path =
hwmon_enumerator.Next()) {
s/devices/device

https://codereview.chromium.org/2115723003/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/device_status_collector.cc:204: // CPU temp in
millidegree Celsius to Celsius
Are added temperatures still in Millicelsius e.g.for link?

[Jay Lee, 2016-07-01 17:04:04]

On 2016/07/01 15:22:26, Polina Bondarenko wrote:

> s/devices/device

fixed.

> Are added temperatures still in Millicelsius e.g.for link?

Seems like it for Veyron. The daisy I looked at had similar numbers:

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3....

[Jay Lee, 2016-07-12 08:13:04]

The CQ bit was checked by jayhlee@google.com

[commit-bot: I haz the power, 2016-07-12 08:13:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-12 08:13:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-12 08:13:41]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Jay Lee, 2016-07-12 08:14:15]

The CQ bit was checked by jayhlee@google.com

[commit-bot: I haz the power, 2016-07-12 08:15:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-12 08:15:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-12 08:15:09]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Polina Bondarenko, 2016-07-12 08:42:25]

On 2016/07/12 08:15:09, commit-bot: I haz the power wrote:
> No L-G-T-M from a valid reviewer yet. 
> CQ run can only be started by full committers or once the patch has
> received an L-G-T-M from a full committer.
> Even if an L-G-T-M may have been provided, it was from a non-committer,
> _not_ a full super star committer.
> See http://www.chromium.org/getting-involved/become-a-committer
> Note that this has nothing to do with OWNERS files.

lgtm

[Jay Lee, 2016-07-12 08:46:21]

The CQ bit was checked by jayhlee@google.com

[commit-bot: I haz the power, 2016-07-12 08:46:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-12 08:54:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-12 08:54:16]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Jay Lee, 2016-07-13 14:52:29]

The CQ bit was checked by jayhlee@google.com

[commit-bot: I haz the power, 2016-07-13 14:52:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-13 15:00:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-13 15:00:34]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Jay Lee, 2016-07-13 15:57:53]

The CQ bit was checked by jayhlee@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-13 15:58:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-13 16:37:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-13 16:37:34]

Dry run: This issue passed the CQ dry run.

[Jay Lee, 2016-07-13 17:02:12]

The CQ bit was checked by jayhlee@google.com

[Jay Lee, 2016-07-13 17:02:13]

The patchset sent to the CQ was uploaded after l-g-t-m from pbond@chromium.org
Link to the patchset: https://codereview.chromium.org/2115723003/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2016-07-13 17:02:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-13 17:12:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-13 17:12:27]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Jay Lee, 2016-07-13 17:36:45]

On 2016/07/13 17:12:27, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Looks like I need another lgtm even though latest patchset was just a rebase to
account for another CL on the file.

pbond@ can you take a look? Thanks!

[Polina Bondarenko, 2016-07-14 07:21:33]

On 2016/07/13 17:36:45, Jay Lee wrote:
> On 2016/07/13 17:12:27, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> 
> Looks like I need another lgtm even though latest patchset was just a rebase
to
> account for another CL on the file.
> 
> pbond@ can you take a look? Thanks!

I'm not an owner of this file. bartfab@ could you take a look?

[Jay Lee, 2016-08-23 15:33:39]

The CQ bit was checked by jayhlee@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 15:33:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 16:26:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-23 16:26:43]

Dry run: This issue passed the CQ dry run.

[Jay Lee, 2016-09-06 13:48:06]

jayhlee@google.com changed reviewers:
+ atwilson@chromium.org, cschuet@chromium.org, pastarmovj@chromium.org,
tnagel@chromium.org
- bartfab@chromium.org, pbond@chromium.org

[Jay Lee, 2016-09-06 13:48:55]

+ Others in OWNERS

Is anyone available to review this? Much appreciated!

Jay

[Andrew T Wilson (Slow), 2016-09-06 18:10:45]

https://codereview.chromium.org/2115723003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_status_collector.cc (right):

https://codereview.chromium.org/2115723003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector.cc:195:
base::ReadFileToString(base::FilePath(label_path), &label);
I'm concerned that if ReadFileToString() returns an error, behavior is undefined
(since |label| can be modified here). Is ReadFileToString() guaranteed to leave
|label| empty if there's an error reading the file?

[Jay Lee, 2016-09-14 15:24:54]

https://codereview.chromium.org/2115723003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_status_collector.cc (right):

https://codereview.chromium.org/2115723003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector.cc:195:
base::ReadFileToString(base::FilePath(label_path), &label);
On 2016/09/06 18:10:45, Andrew T Wilson (Slow) wrote:
> I'm concerned that if ReadFileToString() returns an error, behavior is >
undefined (since |label| can be modified here). Is ReadFileToString()
> guaranteed to leave |label| empty if there's an error reading the 
> file?

As far as I can see at:

https://chromium.googlesource.com/chromium/src/+/master/base/files/file_util....

ReadFileToString() and ReadFileToStringWithMaxSize() would only return empty,
partial file content or full file content.

I'm happy to rewrite this block if you think it's necessary but I'm uncertain
what it would need to look like.

[Jay Lee, 2016-09-22 18:51:27]

friendly bump. Yes, as far as I can see label would be left empty.

> On 2016/09/06 18:10:45, Andrew T Wilson (Slow) wrote:
> > I'm concerned that if ReadFileToString() returns an error, behavior is >
> undefined (since |label| can be modified here). Is ReadFileToString()
> > guaranteed to leave |label| empty if there's an error reading the 
> > file?
> 
> As far as I can see at:
> 
>
https://chromium.googlesource.com/chromium/src/+/master/base/files/file_util....
> 
> ReadFileToString() and ReadFileToStringWithMaxSize() would only return empty,
> partial file content or full file content.
> 
> I'm happy to rewrite this block if you think it's necessary but I'm uncertain
> what it would need to look like.

[pastarmovj, 2016-09-23 09:28:11]

lgtm with one request for documentation fix.

https://codereview.chromium.org/2115723003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_status_collector.cc (right):

https://codereview.chromium.org/2115723003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector.cc:159: // Reads the CPU
temperature info from
Please update this comment to reflect the new logic.

[Jay Lee, 2016-09-23 15:26:15]

The CQ bit was checked by jayhlee@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 15:26:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 15:33:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-23 15:33:38]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Jay Lee, 2016-09-23 15:37:52]

The CQ bit was checked by jayhlee@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 15:38:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 15:48:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-23 15:48:30]

Dry run: This issue passed the CQ dry run.

[Jay Lee, 2016-09-23 15:51:08]

The CQ bit was checked by jayhlee@google.com

[Jay Lee, 2016-09-23 15:51:08]

The patchset sent to the CQ was uploaded after l-g-t-m from pbond@chromium.org,
pastarmovj@chromium.org
Link to the patchset: https://codereview.chromium.org/2115723003/#ps60001
(title: "comment update to reflect new logic")

[commit-bot: I haz the power, 2016-09-23 15:51:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 16:36:21]

Description was changed from

==========
Report all system temperatures, not just CPU

Intel devices put CPU temperatures in a devices/ sub directory of hwmon*. ARM
devices and non-CPU sensors on Intel don't use a devices directory and also may
not contain labels. Use the name file for a label if label doesn't exist.

BUG=530499
TEST=configure managed device to auto-start kiosk. ARM devices should show
temps. Intel devices should show non-CPU temps.
==========

to

==========
Report all system temperatures, not just CPU

Intel devices put CPU temperatures in a devices/ sub directory of hwmon*. ARM
devices and non-CPU sensors on Intel don't use a devices directory and also may
not contain labels. Use the name file for a label if label doesn't exist.

BUG=530499
TEST=configure managed device to auto-start kiosk. ARM devices should show
temps. Intel devices should show non-CPU temps.
==========

[commit-bot: I haz the power, 2016-09-23 16:36:22]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-23 16:37:48]

Description was changed from

==========
Report all system temperatures, not just CPU

Intel devices put CPU temperatures in a devices/ sub directory of hwmon*. ARM
devices and non-CPU sensors on Intel don't use a devices directory and also may
not contain labels. Use the name file for a label if label doesn't exist.

BUG=530499
TEST=configure managed device to auto-start kiosk. ARM devices should show
temps. Intel devices should show non-CPU temps.
==========

to

==========
Report all system temperatures, not just CPU

Intel devices put CPU temperatures in a devices/ sub directory of hwmon*. ARM
devices and non-CPU sensors on Intel don't use a devices directory and also may
not contain labels. Use the name file for a label if label doesn't exist.

BUG=530499
TEST=configure managed device to auto-start kiosk. ARM devices should show
temps. Intel devices should show non-CPU temps.

Committed: https://crrev.com/7d1b94768d646c9ae2318a85447f544044b38a4b
Cr-Commit-Position: refs/heads/master@{#420629}
==========

[commit-bot: I haz the power, 2016-09-23 16:37:50]

Patchset 4 (id:??) landed as
https://crrev.com/7d1b94768d646c9ae2318a85447f544044b38a4b
Cr-Commit-Position: refs/heads/master@{#420629}