Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1178)

Issue 2830263002: [sql] Remove mmap before Raze(). (Closed)

Created:
12 months ago by Scott Hess - ex-Googler
Modified:
12 months ago
Reviewers:
pwnall
CC:
chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[sql] Remove mmap before Raze(). On Windows, file truncation silently fails when the file is memory mapped. Modify Raze() to disable memory mapping before truncating. BUG=none R=pwnall@chromium.org Review-Url: https://codereview.chromium.org/2830263002 Cr-Commit-Position: refs/heads/master@{#466555} Committed: https://chromium.googlesource.com/chromium/src/+/92a6fb2bc85e4563090de085d19c39be8bc3b795

Patch Set 1 #

Total comments: 4

Patch Set 2 : ifdef OS_WIN #

Patch Set 3 : speeeling #

Unified diffs Side-by-side diffs Delta from patch set Stats (+45 lines, -0 lines) Patch
M sql/connection.cc View 1 2 1 chunk +9 lines, -0 lines 0 comments Download
M sql/connection_unittest.cc View 1 chunk +36 lines, -0 lines 0 comments Download

Messages

Total messages: 22 (13 generated)
pwnall
https://codereview.chromium.org/2830263002/diff/1/sql/connection.cc File sql/connection.cc (right): https://codereview.chromium.org/2830263002/diff/1/sql/connection.cc#newcode1118 sql/connection.cc:1118: // prevent the problem. Is there any reference for ...
12 months ago (2017-04-21 11:00:14 UTC) #5
Scott Hess - ex-Googler
ifdef OS_WIN
12 months ago (2017-04-21 14:16:05 UTC) #6
Scott Hess - ex-Googler
speeeling
12 months ago (2017-04-21 14:18:41 UTC) #9
Scott Hess - ex-Googler
https://codereview.chromium.org/2830263002/diff/1/sql/connection.cc File sql/connection.cc (right): https://codereview.chromium.org/2830263002/diff/1/sql/connection.cc#newcode1118 sql/connection.cc:1118: // prevent the problem. On 2017/04/21 11:00:14, pwnall wrote: ...
12 months ago (2017-04-21 14:18:47 UTC) #11
pwnall
LGTM w/ the comment below. Also, thank you very much for doing this! https://codereview.chromium.org/2830263002/diff/1/sql/connection.cc File ...
12 months ago (2017-04-21 19:00:18 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2830263002/40001
12 months ago (2017-04-23 03:34:23 UTC) #17
Scott Hess - ex-Googler
https://codereview.chromium.org/2830263002/diff/1/sql/connection.cc File sql/connection.cc (right): https://codereview.chromium.org/2830263002/diff/1/sql/connection.cc#newcode1118 sql/connection.cc:1118: // prevent the problem. On 2017/04/21 19:00:18, pwnall wrote: ...
12 months ago (2017-04-23 03:36:09 UTC) #18
commit-bot: I haz the power
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/92a6fb2bc85e4563090de085d19c39be8bc3b795
12 months ago (2017-04-23 04:34:55 UTC) #21
pwnall
12 months ago (2017-04-23 09:12:42 UTC) #22
Message was sent while issue was closed.
On 2017/04/23 03:36:09, Scott Hess wrote:
> OK, I think the workaround should be Windows-only, but I'll leave the test
> cross-platform.

Excellent! This is what I meant -- the test should definitely be cross-platform,
so we learn if the SQLite that we use diverges from our expectations in the
future.

Thank you very much for compensating for my lack of clarity!

Powered by Google App Engine
This is Rietveld 408576698