Find device-dependent disk stats file, and skip disk stats if not available.

Find device-dependent disk stats file, and skip disk stats if not available.

Change-Id: I03afb85e3357dd4c2cf5effd98b194c71d77c71d

BUG=12171
TEST=unit tested

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=595a968

[Luigi Semenzato, 2011-02-18 01:02:56]

This makes the daemon work on ARM and makes it behave better for future systems
that use disks with different names.

I have a corresponding change for the ebuild DEPEND that I will send out shortly
and push first.

Thanks!

[petkov, 2011-02-18 04:52:00]

http://codereview.chromium.org/6541007/diff/2001/metrics_daemon.cc
File metrics_daemon.cc (right):

http://codereview.chromium.org/6541007/diff/2001/metrics_daemon.cc#newcode150
metrics_daemon.cc:150: diskstats_path_("") {}
no need to initialize std::string to "" -- it will happen automatically

http://codereview.chromium.org/6541007/diff/2001/metrics_daemon.cc#newcode218
metrics_daemon.cc:218: std::string diskstats_path) {
s/std::string/string/

http://codereview.chromium.org/6541007/diff/2001/metrics_daemon_main.cc
File metrics_daemon_main.cc (right):

http://codereview.chromium.org/6541007/diff/2001/metrics_daemon_main.cc#newco...
metrics_daemon_main.cc:23: if (strcmp(dev_path, "/dev/sda") == 0) {
this is probably ok as is... just a suggestion though:

why not check for and strip "/dev/" prefix and then check for the existence of
"/sys/class/block/" + suffix + "/stat"? it will be a tiny bit more generic and
somewhat more robust.

[Luigi Semenzato, 2011-02-25 23:45:07]

Thanks.  I followed the suggestions, PTAL

On 2011/02/18 04:52:00, petkov wrote:
> http://codereview.chromium.org/6541007/diff/2001/metrics_daemon.cc
> File metrics_daemon.cc (right):
> 
> http://codereview.chromium.org/6541007/diff/2001/metrics_daemon.cc#newcode150
> metrics_daemon.cc:150: diskstats_path_("") {}
> no need to initialize std::string to "" -- it will happen automatically
> 
> http://codereview.chromium.org/6541007/diff/2001/metrics_daemon.cc#newcode218
> metrics_daemon.cc:218: std::string diskstats_path) {
> s/std::string/string/
> 
> http://codereview.chromium.org/6541007/diff/2001/metrics_daemon_main.cc
> File metrics_daemon_main.cc (right):
> 
>
http://codereview.chromium.org/6541007/diff/2001/metrics_daemon_main.cc#newco...
> metrics_daemon_main.cc:23: if (strcmp(dev_path, "/dev/sda") == 0) {
> this is probably ok as is... just a suggestion though:
> 
> why not check for and strip "/dev/" prefix and then check for the existence of
> "/sys/class/block/" + suffix + "/stat"? it will be a tiny bit more generic and
> somewhat more robust.

[petkov, 2011-02-26 00:26:46]

A few more readability nits, LGTM otherwise.

http://codereview.chromium.org/6541007/diff/6001/metrics_daemon.cc
File metrics_daemon.cc (right):

http://codereview.chromium.org/6541007/diff/6001/metrics_daemon.cc#newcode150
metrics_daemon.cc:150: diskstats_path_() {}
just remove this one -- implicit construction is OK

http://codereview.chromium.org/6541007/diff/6001/metrics_daemon_main.cc
File metrics_daemon_main.cc (right):

http://codereview.chromium.org/6541007/diff/6001/metrics_daemon_main.cc#newco...
metrics_daemon_main.cc:18: int ret;
per style, variables should be declared close to their use. also,
"initialization should be used instead of declaration and assignment"

http://codereview.chromium.org/6541007/diff/6001/metrics_daemon_main.cc#newco...
metrics_daemon_main.cc:24: ret = rootdev(dev_path_cstr, sizeof(dev_path_cstr),
true, true);
int ret = ...

http://codereview.chromium.org/6541007/diff/6001/metrics_daemon_main.cc#newco...
metrics_daemon_main.cc:31: if (dev_path.compare(0, dev_prefix.length(),
dev_prefix) != 0) {
if (base::StartsWithASCII(dev_path, dev_prefix, false))

http://codereview.chromium.org/6541007/diff/6001/metrics_daemon_main.cc#newco...
metrics_daemon_main.cc:38: return stat_path.append(dev_name.append("/stat"));
This is simpler and doesn't need a comment or stat_path variables:

return "/sys/class/block/" + dev_name + "/stat";