OpenBSD patches for base and build, part 2

base/sys_info_openbsd.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/logging.h"

[Mark Mentovai, 2011/10/24 13:43:15]

This should come below the system #includes. The #include order has this file’s
associated .h coming first, then system #includes (C followed by C++), then
non-system #includes such as base/logging.h.

[Robert Nagy, 2011/10/24 16:26:56]

Done.

 #include "base/sys_info.h"
 
 #include <sys/param.h>
 #include <sys/sysctl.h>
-
-#include "base/logging.h"
+#include <sys/shm.h>

[Mark Mentovai, 2011/10/24 13:43:15]

Sort. shm < sysctl.

[Robert Nagy, 2011/10/24 16:26:56]

Done.

 
 namespace base {
 
 int SysInfo::NumberOfProcessors() {
   int mib[] = { CTL_HW, HW_NCPU };
   int ncpu;
   size_t size = sizeof(ncpu);
-  if (sysctl(mib, 2, &ncpu, &size, NULL, 0) == -1) {
+  if (sysctl(mib, arraysize(mib), &ncpu, &size, NULL, 0) == -1) {
     NOTREACHED();
     return 1;
   }
   return ncpu;
 }
 
 int64 SysInfo::AmountOfPhysicalMemory() {
   long pages = sysconf(_SC_PHYS_PAGES);
   long page_size = sysconf(_SC_PAGESIZE);
   if (pages == -1 || page_size == -1) {
     NOTREACHED();
     return 0;
   }
 
   return static_cast<int64>(pages) * page_size;
 }
 
+size_t SysInfo::MaxSharedMemorySize() {
+  int mib[] = { CTL_KERN, KERN_SHMINFO, KERN_SHMINFO_SHMMAX };
+  size_t limit;
+  size_t size = sizeof(limit);
+
+  if (sysctl(mib, arraysize(mib), &limit, &size, NULL, 0) < 0)
+    return 0;

[Mark Mentovai, 2011/10/24 13:43:15]

NOTREACHED() to throw up a flare in debug mode? (Similar to how
AmountOfPhysicalMemory does NOTREACHED().)

[Robert Nagy, 2011/10/24 16:26:56]

None of the other platform code do this. Any reason we should?

[Mark Mentovai, 2011/10/24 17:06:11]

Robert Nagy wrote:
NumberOfProcessors and AmountOfPhysicalMemory do. MaxSharedMemorySize was added
later, and apparently a bit more sloppily.

The FreeBSD code looks wrong because it just returns uninitialized stack garbage
if the sysctl fails. The sysctl isn’t expected to fail, but this is exactly the
sort of thing that you’d want a DCHECK (or NOTREACHED) to make an assertion
about in debug builds.

Your OpenBSD implementation is better in that it at least returns 0 instead of
garbage, but I bet that’d lead to subsequent problems in its own way, and it’d
be nice to get the NOTREACHED to throw up a clue before things fall apart in
harder-to-diagnose ways.

Similarly, the Linux implementation uses strtoul on a string that may not
contain anything, and so it also might return 0 in the unexpected event that the
kernel interface to get this data is not present or is malfunctioning. If I were
writing that code, I’d at least put a DCHECK(!string.empty()) on it. Better yet,
I’d use something like base::StringToInt64 from base/string_number_conversons.h
and DCHECK the return value to ensure that the conversion was “precise” as well
as DCHECKing that the result is in the appropriate range for a size_t.

+
+  return limit;
+}
+
 }  // namespace base