Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1139)

Issue 1050233002: If sysconfig fails, fallback to sysinfo

Created:
5 years, 8 months ago by smcgruer2
Modified:
5 years, 8 months ago
Reviewers:
danakj
CC:
chromium-reviews, erikwright+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

If sysconfig fails, fallback to sysinfo The _SC_PHYS_PAGES and _SC_AVPHYS_PAGES options to sysconf are optional extensions, which may not be implemented in a Linux system (such as one running UCLIBC.) For these platforms, we can fallback to sysinfo for the required information. BUG=472701 R=danakj@chromium.org

Patch Set 1 #

Total comments: 12

Patch Set 2 : Address correctness & style comments from danakj #

Patch Set 3 : Rebase to HEAD #

Unified diffs Side-by-side diffs Delta from patch set Stats (+11 lines, -2 lines) Patch
M base/sys_info_linux.cc View 1 2 2 chunks +11 lines, -2 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
smcgruer2
5 years, 8 months ago (2015-04-01 18:46:16 UTC) #1
danakj
https://codereview.chromium.org/1050233002/diff/1/base/sys_info_linux.cc File base/sys_info_linux.cc (right): https://codereview.chromium.org/1050233002/diff/1/base/sys_info_linux.cc#newcode19 base/sys_info_linux.cc:19: long pages = sysconf(pages_name); can you DCHECK up here ...
5 years, 8 months ago (2015-04-02 19:05:48 UTC) #2
smcgruer2
5 years, 8 months ago (2015-04-07 21:25:43 UTC) #3
https://codereview.chromium.org/1050233002/diff/1/base/sys_info_linux.cc
File base/sys_info_linux.cc (right):

https://codereview.chromium.org/1050233002/diff/1/base/sys_info_linux.cc#newc...
base/sys_info_linux.cc:19: long pages = sysconf(pages_name);
On 2015/04/02 19:05:47, danakj wrote:
> can you DCHECK up here that pages_name is _SC_PHYS_PAGES or _SC_ACPHYS_PAGES?

Done.

https://codereview.chromium.org/1050233002/diff/1/base/sys_info_linux.cc#newc...
base/sys_info_linux.cc:23: if (sysinfo(&si)) {
On 2015/04/02 19:05:47, danakj wrote:
> on success 0 is returned. so this won't work right now. are there unit tests
for
> this code? can you run them on the target system and make sure they pass? if
not
> can you add some?

Whoops! Good eye, the original code was checking '== 0' but I accidentally
dropped it when making this cl. That issue is fixed by your DCHECK_EQ
suggestion, but a unittest is probably a good idea.

Is there a nice way to add a unittest given that this code is part of a
platform-specific implementation (so can't add it to a header file), and it
calls into system functions? I'm unfamiliar with Chromium's unittest setup.

https://codereview.chromium.org/1050233002/diff/1/base/sys_info_linux.cc#newc...
base/sys_info_linux.cc:25: return si.totalram * si.mem_unit;
On 2015/04/02 19:05:47, danakj wrote:
> can you cast the totalram to an int64 before multiplying?

Done.

https://codereview.chromium.org/1050233002/diff/1/base/sys_info_linux.cc#newc...
base/sys_info_linux.cc:26: } else if (pages_name == _SC_AVPHYS_PAGES) {
On 2015/04/02 19:05:47, danakj wrote:
> just else should be sufficient? (can leave ..AVPHYS.. in a comment)

Done.

https://codereview.chromium.org/1050233002/diff/1/base/sys_info_linux.cc#newc...
base/sys_info_linux.cc:27: return si.freeram * si.mem_unit;
On 2015/04/02 19:05:47, danakj wrote:
> dittos

Done.

https://codereview.chromium.org/1050233002/diff/1/base/sys_info_linux.cc#newc...
base/sys_info_linux.cc:30: NOTREACHED();
On 2015/04/02 19:05:47, danakj wrote:
> can you restructure this code so we don't have NOTREACHED(); return; anymore?
> 
> int ret = sysinfo(&si);
> DCHECK_EQ(ret, 0);
> ...
> 
> would work.

Done.

Powered by Google App Engine
This is Rietveld 408576698