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.

More Reviews

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 #

Created: 5 years, 8 months ago

Download [raw] [tar.bz2]

		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)

Expand Messages | Collapse Messages

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.

Expand Messages | Collapse Messages