simplify posix MappedFile::Init and add useful logs

simplify posix MappedFile::Init and add useful logs

This ancient CL suggested a simplication in mapped_file_posix.cc:
https://codereview.chromium.org/160288

After hitting this DCHECK on an embedded device where using a debugger
is difficult, I decided to make the suggested change and add some useful
logging while I was at it.

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

[Mostyn Bramley-Moore, 2014-07-07 22:19:04]

@Evan: spotted an old suggestion of yours...

@willchan: PTAL for OWNER approval.

[willchan no longer on Chromium, 2014-07-07 23:13:50]

Ricardo, I'm sending this your way. FWIW, seems fine to me.

[rvargas (doing something else), 2014-07-07 23:30:32]

lgtm after nit

https://codereview.chromium.org/372113003/diff/1/net/disk_cache/blockfile/map...
File net/disk_cache/blockfile/mapped_file_posix.cc (right):

https://codereview.chromium.org/372113003/diff/1/net/disk_cache/blockfile/map...
net/disk_cache/blockfile/mapped_file_posix.cc:8: #include <string.h>
(and this should not be needed)

https://codereview.chromium.org/372113003/diff/1/net/disk_cache/blockfile/map...
net/disk_cache/blockfile/mapped_file_posix.cc:31: << ": " << strerror(errno);
nit: use DPLOG_IF instead

[Mostyn Bramley-Moore, 2014-07-08 08:07:20]

https://codereview.chromium.org/372113003/diff/1/net/disk_cache/blockfile/map...
File net/disk_cache/blockfile/mapped_file_posix.cc (right):

https://codereview.chromium.org/372113003/diff/1/net/disk_cache/blockfile/map...
net/disk_cache/blockfile/mapped_file_posix.cc:8: #include <string.h>
On 2014/07/07 23:30:32, rvargas wrote:
> (and this should not be needed)

Done.

https://codereview.chromium.org/372113003/diff/1/net/disk_cache/blockfile/map...
net/disk_cache/blockfile/mapped_file_posix.cc:31: << ": " << strerror(errno);
On 2014/07/07 23:30:32, rvargas wrote:
> nit: use DPLOG_IF instead

I hadn't noticed the P logging macros before- nice.

Done.

[Mostyn Bramley-Moore, 2014-07-08 08:13:08]

The CQ bit was checked by mostynb@opera.com

[commit-bot: I haz the power, 2014-07-08 08:13:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mostynb@opera.com/372113003/20001

[commit-bot: I haz the power, 2014-07-08 12:23:04]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-08 14:33:27]

Change committed as 281747

[Evan Martin, 2014-07-13 22:21:17]

Dean and I no longer work on Chrome, and it looks like Will is on vacation.
 Maybe try thestig@ as a reviewer.


On Mon, Jul 7, 2014 at 3:19 PM, <mostynb@opera.com> wrote:

> Reviewers: Evan Martin, willchan (traveling 5.23-6.27), Dean McNamee,
>
> Message:
> @Evan: spotted an old suggestion of yours...
>
> @willchan: PTAL for OWNER approval.
>
> Description:
> simplify posix MappedFile::Init and add useful logs
>
> This ancient CL suggested a simplication in mapped_file_posix.cc:
> https://codereview.chromium.org/160288
>
> After hitting this DCHECK on an embedded device where using a debugger
> is difficult, I decided to make the suggested change and add some useful
> logging while I was at it.
>
> BUG=391937
>
> Please review this at https://codereview.chromium.org/372113003/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files (+4, -2 lines):
>   M net/disk_cache/blockfile/mapped_file_posix.cc
>
>
> Index: net/disk_cache/blockfile/mapped_file_posix.cc
> diff --git a/net/disk_cache/blockfile/mapped_file_posix.cc
> b/net/disk_cache/blockfile/mapped_file_posix.cc
> index 966fc72686e7b6e2f33bcc4ac4e824027f81806e..
> 159bb95de820d7c2e2e1d6cfdfb2fc4ce2ff6600 100644
> --- a/net/disk_cache/blockfile/mapped_file_posix.cc
> +++ b/net/disk_cache/blockfile/mapped_file_posix.cc
> @@ -5,6 +5,7 @@
>  #include "net/disk_cache/blockfile/mapped_file.h"
>
>  #include <errno.h>
> +#include <string.h>
>  #include <sys/mman.h>
>
>  #include "base/files/file_path.h"
> @@ -26,8 +27,9 @@ void* MappedFile::Init(const base::FilePath& name,
> size_t size) {
>                   platform_file(), 0);
>    init_ = true;
>    view_size_ = size;
> -  DCHECK(reinterpret_cast<intptr_t>(buffer_) != -1);
> -  if (reinterpret_cast<intptr_t>(buffer_) == -1)
> +  DLOG_IF(FATAL, buffer_ == MAP_FAILED) << "Failed to mmap " <<
> name.value()
> +                                        << ": " << strerror(errno);
> +  if (buffer_ == MAP_FAILED)
>      buffer_ = 0;
>
>    // Make sure we detect hardware failures reading the headers.
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mostyn Bramley-Moore, 2014-07-13 22:49:43]

On 2014/07/13 22:21:17, Evan Martin wrote:
> Dean and I no longer work on Chrome, and it looks like Will is on vacation.
>  Maybe try thestig@ as a reviewer.

This landed a few days back, but thanks anyway.

-Mostyn.