OpenBSD patches for base and build, part 2

OpenBSD patches for base and build, part 2

base/base.gyp: 
  - exclude test/test_file_util_linux.cc on OpenBSD because of
    missing fdatasync()

base/base.gypi:
  - include linux_util.cc on OpenBSD too
  - exclude some linux specific files from the OpenBSD build
    because the openbsd implementation is different

base/file_util_posix.cc:
  - Use file_util::GetTempDir directly to get the shm dir
    on !linux

base/sys_info_freebsd.cc:
   - return 0 and do a NOTREACHED() in case sysctlbyname() fails

base/sys_info_openbsd.cc:
  - cleanup includes
  - use arraysize() in the sysctl() call
  - implement SysInfo::MaxSharedMemorySize()

build/common.gypi:
  - -Wno-deprecated is C++ only flag so move it to cflags_cc
  - Do not exclude linux specific files, dirs on OpenBSD since
    90% of it is sharable

base/sys_info_linux.cc:
  - DCHECK if the read string is empty
  - Use base::StringToInt64() to convert the string
    to an int64
  - DCHECK if the returned int64 is okay for a size_t
  - call NOTREACHED() and return 0 if fails

BUG=
TEST=runs on linux?


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=106984

[Robert Nagy, 2011-10-24 12:30:20]

Please review the following patchset.

Just some additional information about why I am using the linux files and dirs
on OpenBSD:
After investigating which method would be the best we picked this one with Mark,
since there are about a hundred files needed to be included on OpenBSD which
have linux in their name and it is easier to exclude the ones that are not
needed because there aren't many, and since we can't just rename these files to
*_posix*, this is the ideal solution for now.

Thank you

[Mark Mentovai, 2011-10-24 13:43:14]

http://codereview.chromium.org/8382001/diff/1/base/base.gypi
File base/base.gypi (right):

http://codereview.chromium.org/8382001/diff/1/base/base.gypi#newcode509
base/base.gypi:509: ['include', '^base_paths_linux\\.cc$'],
You can get rid of these includes now.

http://codereview.chromium.org/8382001/diff/1/base/file_util_posix.cc
File base/file_util_posix.cc (right):

http://codereview.chromium.org/8382001/diff/1/base/file_util_posix.cc#newcode918
base/file_util_posix.cc:918: *path = FilePath("/tmp");
return GetTempDir(path) instead?

http://codereview.chromium.org/8382001/diff/1/base/sys_info_openbsd.cc
File base/sys_info_openbsd.cc (right):

http://codereview.chromium.org/8382001/diff/1/base/sys_info_openbsd.cc#newcode5
base/sys_info_openbsd.cc:5: #include "base/logging.h"
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.

http://codereview.chromium.org/8382001/diff/1/base/sys_info_openbsd.cc#newcode10
base/sys_info_openbsd.cc:10: #include <sys/shm.h>
Sort. shm < sysctl.

http://codereview.chromium.org/8382001/diff/1/base/sys_info_openbsd.cc#newcode42
base/sys_info_openbsd.cc:42: return 0;
NOTREACHED() to throw up a flare in debug mode? (Similar to how
AmountOfPhysicalMemory does NOTREACHED().)

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

I've also added a patch for base/linux_util.cc since we were not returning
anything in the GetLinuxDistro() function, just hitting NOTIMPLEMENTED();

http://codereview.chromium.org/8382001/diff/1/base/base.gypi
File base/base.gypi (right):

http://codereview.chromium.org/8382001/diff/1/base/base.gypi#newcode509
base/base.gypi:509: ['include', '^base_paths_linux\\.cc$'],
On 2011/10/24 13:43:15, Mark Mentovai wrote:
> You can get rid of these includes now.

Done.

http://codereview.chromium.org/8382001/diff/1/base/file_util_posix.cc
File base/file_util_posix.cc (right):

http://codereview.chromium.org/8382001/diff/1/base/file_util_posix.cc#newcode918
base/file_util_posix.cc:918: *path = FilePath("/tmp");
On 2011/10/24 13:43:15, Mark Mentovai wrote:
> return GetTempDir(path) instead?

Done.

http://codereview.chromium.org/8382001/diff/1/base/sys_info_openbsd.cc
File base/sys_info_openbsd.cc (right):

http://codereview.chromium.org/8382001/diff/1/base/sys_info_openbsd.cc#newcode5
base/sys_info_openbsd.cc:5: #include "base/logging.h"
On 2011/10/24 13:43:15, Mark Mentovai wrote:
> 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.

Done.

http://codereview.chromium.org/8382001/diff/1/base/sys_info_openbsd.cc#newcode10
base/sys_info_openbsd.cc:10: #include <sys/shm.h>
On 2011/10/24 13:43:15, Mark Mentovai wrote:
> Sort. shm < sysctl.

Done.

http://codereview.chromium.org/8382001/diff/1/base/sys_info_openbsd.cc#newcode42
base/sys_info_openbsd.cc:42: return 0;
On 2011/10/24 13:43:15, Mark Mentovai wrote:
> NOTREACHED() to throw up a flare in debug mode? (Similar to how
> AmountOfPhysicalMemory does NOTREACHED().)

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

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

http://codereview.chromium.org/8382001/diff/1/base/sys_info_openbsd.cc
File base/sys_info_openbsd.cc (right):

http://codereview.chromium.org/8382001/diff/1/base/sys_info_openbsd.cc#newcode42
base/sys_info_openbsd.cc:42: return 0;
Robert Nagy wrote:
> On 2011/10/24 13:43:15, Mark Mentovai wrote:
> > NOTREACHED() to throw up a flare in debug mode? (Similar to how
> > AmountOfPhysicalMemory does NOTREACHED().)
> 
> None of the other platform code do this. Any reason we should?

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.

http://codereview.chromium.org/8382001/diff/2003/base/base.gypi
File base/base.gypi (right):

http://codereview.chromium.org/8382001/diff/2003/base/base.gypi#newcode504
base/base.gypi:504: ['exclude', '^files\\/file_path_watcher_linux\\.cc$'],
Don’t \\ the /.

http://codereview.chromium.org/8382001/diff/2003/base/file_util_posix.cc
File base/file_util_posix.cc (right):

http://codereview.chromium.org/8382001/diff/2003/base/file_util_posix.cc#newc...
base/file_util_posix.cc:919: // Set it to /tmp if all else fails.
I would have just made this do |return GetTempDir(path);|, because GetTempDir is
right in all cases, has its own fallback, and knows how to return a value.

[Mark Mentovai, 2011-10-24 17:11:25]

http://codereview.chromium.org/8382001/diff/2003/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/8382001/diff/2003/build/common.gypi#newcode1251
build/common.gypi:1251: ['OS!="linux" and OS!="openbsd"', {
Although we’ve discussed it, this will appear freaky to people. It deserves its
own comment right here.

[Robert Nagy, 2011-10-24 18:00:51]

On 2011/10/24 17:06:11, Mark Mentovai wrote:
> http://codereview.chromium.org/8382001/diff/1/base/sys_info_openbsd.cc
> File base/sys_info_openbsd.cc (right):
> 
>
http://codereview.chromium.org/8382001/diff/1/base/sys_info_openbsd.cc#newcode42
> base/sys_info_openbsd.cc:42: return 0;
> Robert Nagy wrote:
> > On 2011/10/24 13:43:15, Mark Mentovai wrote:
> > > NOTREACHED() to throw up a flare in debug mode? (Similar to how
> > > AmountOfPhysicalMemory does NOTREACHED().)
> > 
> > None of the other platform code do this. Any reason we should?
> 
> 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.
> 
> http://codereview.chromium.org/8382001/diff/2003/base/base.gypi
> File base/base.gypi (right):
> 
> http://codereview.chromium.org/8382001/diff/2003/base/base.gypi#newcode504
> base/base.gypi:504: ['exclude', '^files\\/file_path_watcher_linux\\.cc$'],
> Don’t \\ the /.
> 
> http://codereview.chromium.org/8382001/diff/2003/base/file_util_posix.cc
> File base/file_util_posix.cc (right):
> 
>
http://codereview.chromium.org/8382001/diff/2003/base/file_util_posix.cc#newc...
> base/file_util_posix.cc:919: // Set it to /tmp if all else fails.
> I would have just made this do |return GetTempDir(path);|, because GetTempDir
is
> right in all cases, has its own fallback, and knows how to return a value.

I've fixed both the Open and FreeBSD code to call NOTREACHED() and also the
freebsd code is returning 0 on failure. It is highly unlikely that the sysctl
subsystem fails because if that happens, you are **cked on boot already. And
returning 0 is also okay since I can set the limit to 0 and in that case, 0 is
going to be returned.

For the linux case, since I do not have any linux machines to test on, I would
like you to have a look at my modifications based on your comments. I've
basically done what you have suggested and that should be enough to cover all of
the failures, and return 0 in the worst case which should be also okay on Linux.

[Robert Nagy, 2011-10-24 18:00:57]

http://codereview.chromium.org/8382001/diff/2003/base/base.gypi
File base/base.gypi (right):

http://codereview.chromium.org/8382001/diff/2003/base/base.gypi#newcode504
base/base.gypi:504: ['exclude', '^files\\/file_path_watcher_linux\\.cc$'],
On 2011/10/24 17:06:12, Mark Mentovai wrote:
> Don’t \\ the /.

Done.

http://codereview.chromium.org/8382001/diff/2003/base/file_util_posix.cc
File base/file_util_posix.cc (right):

http://codereview.chromium.org/8382001/diff/2003/base/file_util_posix.cc#newc...
base/file_util_posix.cc:919: // Set it to /tmp if all else fails.
On 2011/10/24 17:06:12, Mark Mentovai wrote:
> I would have just made this do |return GetTempDir(path);|, because GetTempDir
is
> right in all cases, has its own fallback, and knows how to return a value.

Done.

http://codereview.chromium.org/8382001/diff/2003/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/8382001/diff/2003/build/common.gypi#newcode1251
build/common.gypi:1251: ['OS!="linux" and OS!="openbsd"', {
On 2011/10/24 17:11:25, Mark Mentovai wrote:
> Although we’ve discussed it, this will appear freaky to people. It deserves
its
> own comment right here.

Done.

[Robert Nagy, 2011-10-24 18:03:52]

updated issue description

[Mark Mentovai, 2011-10-24 18:09:58]

http://codereview.chromium.org/8382001/diff/4004/base/file_util_posix.cc
File base/file_util_posix.cc (right):

http://codereview.chromium.org/8382001/diff/4004/base/file_util_posix.cc#newc...
base/file_util_posix.cc:918: file_util::GetTempDir(path);
It would be a good idea to structure this as

#if defined(OS_LINUX)
  *path = FilePath("/dev/shm");
  return true;
#else
  return GetTempDir(path);
#endif

in case someone adds a false-returning path to GetTempDir.

http://codereview.chromium.org/8382001/diff/4004/base/sys_info_linux.cc
File base/sys_info_linux.cc (right):

http://codereview.chromium.org/8382001/diff/4004/base/sys_info_linux.cc#newco...
base/sys_info_linux.cc:33: DCHECK((size_t) limit <= sizeof(limit));
Use static_cast<size_t>(limit). Reference:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Castin...

http://codereview.chromium.org/8382001/diff/4004/base/sys_info_linux.cc#newco...
base/sys_info_linux.cc:33: DCHECK((size_t) limit <= sizeof(limit));
Also DCHECK(limit >= 0).

[Robert Nagy, 2011-10-24 18:20:17]

http://codereview.chromium.org/8382001/diff/4004/base/file_util_posix.cc
File base/file_util_posix.cc (right):

http://codereview.chromium.org/8382001/diff/4004/base/file_util_posix.cc#newc...
base/file_util_posix.cc:918: file_util::GetTempDir(path);
On 2011/10/24 18:09:58, Mark Mentovai wrote:
> It would be a good idea to structure this as
> 
> #if defined(OS_LINUX)
>   *path = FilePath("/dev/shm");
>   return true;
> #else
>   return GetTempDir(path);
> #endif
> 
> in case someone adds a false-returning path to GetTempDir.

Done.

http://codereview.chromium.org/8382001/diff/4004/base/sys_info_linux.cc
File base/sys_info_linux.cc (right):

http://codereview.chromium.org/8382001/diff/4004/base/sys_info_linux.cc#newco...
base/sys_info_linux.cc:33: DCHECK((size_t) limit <= sizeof(limit));
On 2011/10/24 18:09:58, Mark Mentovai wrote:
> Use static_cast<size_t>(limit). Reference:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Castin...

Done.

http://codereview.chromium.org/8382001/diff/4004/base/sys_info_linux.cc#newco...
base/sys_info_linux.cc:33: DCHECK((size_t) limit <= sizeof(limit));
On 2011/10/24 18:09:58, Mark Mentovai wrote:
> Also DCHECK(limit >= 0).

Done.

[Mark Mentovai, 2011-10-24 18:51:55]

http://codereview.chromium.org/8382001/diff/1009/base/sys_info_linux.cc
File base/sys_info_linux.cc (right):

http://codereview.chromium.org/8382001/diff/1009/base/sys_info_linux.cc#newco...
base/sys_info_linux.cc:34: DCHECK(static_cast<size_t>(limit) <= sizeof(limit));
You didn’t really mean sizeof, did you?

http://codereview.chromium.org/8382001/diff/1009/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/8382001/diff/1009/build/common.gypi#newcode1251
build/common.gypi:1251: # Do not exclude the linux files on OpenBSD since most
of them can be
“most of them are shared at this point” because “can be and has to be” doesn’t
make sense.

http://codereview.chromium.org/8382001/diff/1009/build/common.gypi#newcode1253
build/common.gypi:1253: # In case a file is not needed, it is going to be
excluded laster on.
Spelling: laster->later

[Robert Nagy, 2011-10-24 19:30:20]

http://codereview.chromium.org/8382001/diff/1009/base/sys_info_linux.cc
File base/sys_info_linux.cc (right):

http://codereview.chromium.org/8382001/diff/1009/base/sys_info_linux.cc#newco...
base/sys_info_linux.cc:34: DCHECK(static_cast<size_t>(limit) <= sizeof(limit));
On 2011/10/24 18:51:55, Mark Mentovai wrote:
> You didn’t really mean sizeof, did you?

Done.

http://codereview.chromium.org/8382001/diff/1009/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/8382001/diff/1009/build/common.gypi#newcode1251
build/common.gypi:1251: # Do not exclude the linux files on OpenBSD since most
of them can be
On 2011/10/24 18:51:55, Mark Mentovai wrote:
> “most of them are shared at this point” because “can be and has to be” doesn’t
> make sense.

Done.

http://codereview.chromium.org/8382001/diff/1009/build/common.gypi#newcode1253
build/common.gypi:1253: # In case a file is not needed, it is going to be
excluded laster on.
On 2011/10/24 18:51:55, Mark Mentovai wrote:
> Spelling: laster->later

Done.

[Mark Mentovai, 2011-10-24 19:33:41]

LGTM and CQ.

[commit-bot: I haz the power, 2011-10-24 19:33:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robert.nagy@gmail.com/8382001/10003

[Mark Mentovai, 2011-10-24 19:44:11]

LGTM and CQ again.

[commit-bot: I haz the power, 2011-10-24 19:44:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robert.nagy@gmail.com/8382001/2014

[Mark Mentovai, 2011-10-24 20:39:55]

LGTM and CQ v3.0

[commit-bot: I haz the power, 2011-10-24 20:40:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robert.nagy@gmail.com/8382001/9

[commit-bot: I haz the power, 2011-10-24 22:05:29]

Change committed as 106984