Implement missing SysInfo methods on OpenBSD

Implement missing SysInfo methods on OpenBSD

Adds AmountOfAvailablePhysicalMemory (based on Linux's implementation)
and CPUModelName() (based on OS X's implementation).

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193295

[mdempsky_google, 2013-04-09 20:05:41]

Hi Mark, I'd like you to do a code review.

This adds two missing functions needed to build Chromium on OpenBSD.  It
shouldn't affect any official Chrome builds.

Thanks!

[Mark Mentovai, 2013-04-09 20:37:08]

https://codereview.chromium.org/13945012/diff/5002/base/sys_info_openbsd.cc
File base/sys_info_openbsd.cc (right):

https://codereview.chromium.org/13945012/diff/5002/base/sys_info_openbsd.cc#n...
base/sys_info_openbsd.cc:39: long available_pages = sysconf(_SC_AVPHYS_PAGES);
Other than the constant used on this line, this is the same as
AmountOfPhysicalMemory. Perhaps you should have a helper function, in an
anonymous namespace, that implements this entire function body with the constant
parameterized.

I would make the same change to the _linux.cc, and perhaps a similar change to
the _win.cc.

https://codereview.chromium.org/13945012/diff/5002/base/sys_info_openbsd.cc#n...
base/sys_info_openbsd.cc:61: int mib[] = { CTL_HW, HW_MODEL };
Can this be const? Same on line 49.

https://codereview.chromium.org/13945012/diff/5002/base/sys_info_openbsd.cc#n...
base/sys_info_openbsd.cc:65: return name;
Matching the function above, test for the failure case and handle it inside the
conditional, and put the success case outside and after.

[Paweł Hajdan Jr., 2013-04-09 20:42:53]

I defer to Mark. Thanks for the patch!

https://codereview.chromium.org/13945012/diff/5002/base/sys_info_openbsd.cc
File base/sys_info_openbsd.cc (right):

https://codereview.chromium.org/13945012/diff/5002/base/sys_info_openbsd.cc#n...
base/sys_info_openbsd.cc:39: long available_pages = sysconf(_SC_AVPHYS_PAGES);
On 2013/04/09 20:37:08, Mark Mentovai wrote:
> Other than the constant used on this line, this is the same as
> AmountOfPhysicalMemory. Perhaps you should have a helper function, in an
> anonymous namespace, that implements this entire function body with the
constant
> parameterized.
> 
> I would make the same change to the _linux.cc, and perhaps a similar change to
> the _win.cc.

+1

[mdempsky_google, 2013-04-09 21:21:54]

Thanks, please take another look! :)

https://codereview.chromium.org/13945012/diff/5002/base/sys_info_openbsd.cc
File base/sys_info_openbsd.cc (right):

https://codereview.chromium.org/13945012/diff/5002/base/sys_info_openbsd.cc#n...
base/sys_info_openbsd.cc:39: long available_pages = sysconf(_SC_AVPHYS_PAGES);
Done for sys_info_openbsd.cc.  Let me know if you like that, and I'll make the
same changes to the other sys_info_*.cc implementations.

https://codereview.chromium.org/13945012/diff/5002/base/sys_info_openbsd.cc#n...
base/sys_info_openbsd.cc:61: int mib[] = { CTL_HW, HW_MODEL };
On 2013/04/09 20:37:08, Mark Mentovai wrote:
> Can this be const? Same on line 49.

Sort of.  The sysctl() interface on OpenBSD (currently) requires an "int *", but
I just checked with other OpenBSD developers and they agree we can change it to
"const int *" because it's a read-only value.

I changed the mib arrays to "static const" and added an appropriate
const_cast<int*>.  By time OpenBSD 5.4 releases (Nov 2013), we should be able to
remove the const_casts though.

https://codereview.chromium.org/13945012/diff/5002/base/sys_info_openbsd.cc#n...
base/sys_info_openbsd.cc:65: return name;
On 2013/04/09 20:37:08, Mark Mentovai wrote:
> Matching the function above, test for the failure case and handle it inside
the
> conditional, and put the success case outside and after.

Done.

[Mark Mentovai, 2013-04-09 21:25:28]

https://codereview.chromium.org/13945012/diff/3002/base/sys_info_openbsd.cc
File base/sys_info_openbsd.cc (right):

https://codereview.chromium.org/13945012/diff/3002/base/sys_info_openbsd.cc#n...
base/sys_info_openbsd.cc:15: int64 AmountOfMemory(int pages_name) {
This is good. You can do Linux this way too, and Windows a similar way.

https://codereview.chromium.org/13945012/diff/3002/base/sys_info_openbsd.cc#n...
base/sys_info_openbsd.cc:34: if (sysctl(const_cast<int*>(mib), arraysize(mib),
&ncpu, &size, NULL, 0)
No no, don’t const_cast it, that’s even worse. Just go back to using int. Broken
system headers are a reasonable reason to have to do that. Same on lines 54/57
and 67/70.

[mdempsky_google, 2013-04-09 21:45:09]

https://codereview.chromium.org/13945012/diff/3002/base/sys_info_openbsd.cc
File base/sys_info_openbsd.cc (right):

https://codereview.chromium.org/13945012/diff/3002/base/sys_info_openbsd.cc#n...
base/sys_info_openbsd.cc:15: int64 AmountOfMemory(int pages_name) {
On 2013/04/09 21:25:28, Mark Mentovai wrote:
> This is good. You can do Linux this way too, and Windows a similar way.

Done.  (Not sure if the way I did the Windows code is idiomatic Chromium code
though.)

https://codereview.chromium.org/13945012/diff/3002/base/sys_info_openbsd.cc#n...
base/sys_info_openbsd.cc:34: if (sysctl(const_cast<int*>(mib), arraysize(mib),
&ncpu, &size, NULL, 0)
On 2013/04/09 21:25:28, Mark Mentovai wrote:
> No no, don’t const_cast it, that’s even worse. Just go back to using int.
Broken
> system headers are a reasonable reason to have to do that. Same on lines 54/57
> and 67/70.

Okay, reverted.

https://codereview.chromium.org/13945012/diff/7003/base/sys_info_linux.cc
File base/sys_info_linux.cc (right):

https://codereview.chromium.org/13945012/diff/7003/base/sys_info_linux.cc#new...
base/sys_info_linux.cc:17: long page_size = sysconf(_SC_PAGESIZE);
I intentionally changed this from _SC_PAGE_SIZE to _SC_PAGESIZE.  _SC_PAGESIZE
is the standardized name used by POSIX, and _SC_PAGE_SIZE is merely an alias for
it in glibc.  (Curiously, they're distinct #defines in bionic, but bionic's
sysconf() returns the same value for both of them.)

[Mark Mentovai, 2013-04-09 22:14:21]

https://codereview.chromium.org/13945012/diff/7003/base/sys_info_openbsd.cc
File base/sys_info_openbsd.cc (right):

https://codereview.chromium.org/13945012/diff/7003/base/sys_info_openbsd.cc#n...
base/sys_info_openbsd.cc:56: if (sysctl(mib, arraysize(mib), &limit, &size,
NULL, 0) == -1) {
I wouldn’t normally pick on this, but I can’t see a reason for this change, so…

For instances where you’re looking at syscall return values, where -1 is the
failure return value and don’t expect any other negative numbers, the |< 0| form
that was here before generally produces slightly better (and smaller) object
code. Same in the new code on line 68.

https://codereview.chromium.org/13945012/diff/7003/base/sys_info_win.cc
File base/sys_info_win.cc (right):

https://codereview.chromium.org/13945012/diff/7003/base/sys_info_win.cc#newco...
base/sys_info_win.cc:18: int64 AmountOfMemory(DWORDLONG MEMORYSTATUSEX::*
memory_field) {
It’s unusual in Chrome code, but I can’t find a reason to outlaw it, and it’s
probably better code than any of the alternatives.

[Mark Mentovai, 2013-04-09 22:14:44]

LGTM with the minor |== -1| / |< 0| thing fixed.

[mdempsky_google, 2013-04-09 23:06:32]

https://codereview.chromium.org/13945012/diff/7003/base/sys_info_openbsd.cc
File base/sys_info_openbsd.cc (right):

https://codereview.chromium.org/13945012/diff/7003/base/sys_info_openbsd.cc#n...
base/sys_info_openbsd.cc:56: if (sysctl(mib, arraysize(mib), &limit, &size,
NULL, 0) == -1) {
On 2013/04/09 22:14:21, Mark Mentovai wrote:
> For instances where you’re looking at syscall return values, where -1 is the
> failure return value and don’t expect any other negative numbers, the |< 0|
form
> that was here before generally produces slightly better (and smaller) object
> code. Same in the new code on line 68.

No problem.  Your project, your rules. :)

https://codereview.chromium.org/13945012/diff/7003/base/sys_info_win.cc
File base/sys_info_win.cc (right):

https://codereview.chromium.org/13945012/diff/7003/base/sys_info_win.cc#newco...
base/sys_info_win.cc:18: int64 AmountOfMemory(DWORDLONG MEMORYSTATUSEX::*
memory_field) {
On 2013/04/09 22:14:21, Mark Mentovai wrote:
> It’s unusual in Chrome code, but I can’t find a reason to outlaw it, and it’s
> probably better code than any of the alternatives.

Exactly how I felt about it then. ;)

[commit-bot: I haz the power, 2013-04-09 23:06:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mdempsky@google.com/13945012/13001

[commit-bot: I haz the power, 2013-04-10 02:22:17]

Change committed as 193295