Chromium side patch for DB support on Linux.

Chromium side patch for DB support on Linux.

BUG=none
TEST=none


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

[dumi, 2009-08-21 17:55:21]

Adam: How can i send a "good" fd to the renderer process in
database_dispatcher_host.cc?

OpenFileFlagsAreConsistent() was added based on the asserts found at the
beginning of unixOpen() in third_party/sqlite/src/os_unix.cc.

[dumi, 2009-08-21 18:07:43]

See issue 173205 for WebKit side changes.

Michael, Dimitri: I refactored a bit the code in the original unix VFS
(os_unix.cc) to fit our model better, and I moved some of the code to the
browser process (assertions in unixOpen() for example), but I think the logic
should be the same. Please double-check this.

[agl, 2009-08-21 18:12:07]

http://codereview.chromium.org/174232/diff/8/12
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/174232/diff/8/12#newcode57
Line 57: int fileType = flags & 0x00007F00;
these should all be const bool

http://codereview.chromium.org/174232/diff/8/12#newcode65
Line 65: if ((isReadOnly && isReadWrite) || (!isReadOnly && !isReadWrite)) {
Isn't this "if (!(isReadOnly ^ isReadWrite))"?

http://codereview.chromium.org/174232/diff/8/12#newcode162
Line 162: target_handle = file_handle;
In render_messages_internal.h, you need to change the argument type to a
typedef. The typedef should be #ifdefed to a HANDLE on Windows and a
base::FileDescriptor on POSIX. See base/shared_memory.h, the SharedMemoryHandle
is exactly that. (So you could even typedef to one of those, it's just a
question of making it clear that you're not actually passing shared memory
around.)

Then here you would do target_handle = FileHandle(file_handle, true /*
auto-close */). The true argument means that the handle will be closed in the
browser after being sent over the IPC channel. Actually, do you close the
Windows HANDLE in the code above after duplicating it? If you want to keep the
handle open on the browser side, you can just pass false as the second argument
to FileDescriptor's constructor.

http://codereview.chromium.org/174232/diff/8/12#newcode206
Line 206: #elif defined(OS_LINUX)
OS_POSIX? I don't see anything which wouldn't be ok on the Mac

http://codereview.chromium.org/174232/diff/8/12#newcode207
Line 207: uint32 attributes = 0L;
remove 'L'

http://codereview.chromium.org/174232/diff/8/12#newcode218
Line 218: uint32 attributes = -1L;
might as well remove the L here too, although it's not your code.

[michaeln, 2009-08-21 18:40:14]

http://codereview.chromium.org/174232/diff/8/11
File base/platform_file_posix.cc (right):

http://codereview.chromium.org/174232/diff/8/11#newcode67
Line 67: unlink(WideToUTF8(name).c_str());
Does this effectively delete the file prior to close? I'm not a 'nix person. If
so, I think this may break the semantics. The file created here should be
visible to other would be file openers until its closed/deleted (i think). Does
the unlink here accomplish that?

http://codereview.chromium.org/174232/diff/8/12
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/174232/diff/8/12#newcode62
Line 62: int isReadWrite = flags & SQLITE_OPEN_READWRITE;
nit: naming foo_bar, not camelCase

http://codereview.chromium.org/174232/diff/8/12#newcode87
Line 87: (fileType == SQLITE_OPEN_MASTER_JOURNAL)) &&
Oh... i didn't know we got the file type at this vfs layer... that's good to
know! We may be able to utilize that info to our advantage in some way? Out of
scope for this CL, but just sayin...

http://codereview.chromium.org/174232/diff/8/9
File third_party/sqlite/src/os_unix.c (right):

http://codereview.chromium.org/174232/diff/8/9#newcode2098
Line 2098: int fillInUnixFile(
Is there a README file that documents our changes to this library?

[dumi, 2009-08-21 20:26:14]

http://codereview.chromium.org/174232/diff/8/11
File base/platform_file_posix.cc (right):

http://codereview.chromium.org/174232/diff/8/11#newcode67
Line 67: unlink(WideToUTF8(name).c_str());
On 2009/08/21 18:40:14, michaeln wrote:
> Does this effectively delete the file prior to close? I'm not a 'nix person.
If
> so, I think this may break the semantics. The file created here should be
> visible to other would be file openers until its closed/deleted (i think).
Does
> the unlink here accomplish that?

From 'man 2 unlink':

unlink() deletes a name from the filesystem.  If that name was the last
link to a file and no processes have the file open the file is  deleted
and the space it was using is made available for reuse.

If  the  name  was the last link to a file but any processes still have
the file open the file will remain in existence  until  the  last  file
descriptor referring to it is closed.

So we should be OK here (assuming sending a FileDescriptor over an IPC doesn't
close the existing fd and then reopens it). This is what unixOpen() in the
default VFS does too.

http://codereview.chromium.org/174232/diff/8/12
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/174232/diff/8/12#newcode57
Line 57: int fileType = flags & 0x00007F00;
On 2009/08/21 18:12:08, agl wrote:
> these should all be const bool

done.

http://codereview.chromium.org/174232/diff/8/12#newcode62
Line 62: int isReadWrite = flags & SQLITE_OPEN_READWRITE;
On 2009/08/21 18:40:14, michaeln wrote:
> nit: naming foo_bar, not camelCase

done.

http://codereview.chromium.org/174232/diff/8/12#newcode65
Line 65: if ((isReadOnly && isReadWrite) || (!isReadOnly && !isReadWrite)) {
On 2009/08/21 18:12:08, agl wrote:
> Isn't this "if (!(isReadOnly ^ isReadWrite))"?

technically, we want a logical XOR operator here, not a binary one. i changed it
to what you suggested anyway, since i guess in practice it's the same.

http://codereview.chromium.org/174232/diff/8/12#newcode162
Line 162: target_handle = file_handle;
On 2009/08/21 18:12:08, agl wrote:
> In render_messages_internal.h, you need to change the argument type to a
> typedef. The typedef should be #ifdefed to a HANDLE on Windows and a
> base::FileDescriptor on POSIX. See base/shared_memory.h, the
SharedMemoryHandle
> is exactly that. (So you could even typedef to one of those, it's just a
> question of making it clear that you're not actually passing shared memory
> around.)
> 
> Then here you would do target_handle = FileHandle(file_handle, true /*
> auto-close */). The true argument means that the handle will be closed in the
> browser after being sent over the IPC channel. Actually, do you close the
> Windows HANDLE in the code above after duplicating it? If you want to keep the
> handle open on the browser side, you can just pass false as the second
argument
> to FileDescriptor's constructor.

done. on windows, DuplicateHandle() closes the handle for us, because of the
DUPLICATE_CLOSE_SOURCE flag.

i still wonder if we should just add a base/file_handle.h file (or at least a
common/db_file_handle.h file) that looks something like this to avoid all these
#ifdefs everywhere:

#if defined(OS_WIN)
typedef HANDLE FileHandle;
#elif defined(OS_POSIX)
typedef FileDescriptor FileHandle;
#endif

base::PlatformFile GetRawFileHandle(const FileHandle& file_handle) {
#if defined(OS_WIN)
  return file_handle;
#elif defined(OS_POSIX)
  return file_handle.id;
#endif
}

thoughts?

http://codereview.chromium.org/174232/diff/8/12#newcode206
Line 206: #elif defined(OS_LINUX)
On 2009/08/21 18:12:08, agl wrote:
> OS_POSIX? I don't see anything which wouldn't be ok on the Mac

changed here and in DatabaseOpenFile (does the name file_descriptor_posix.h
imply that the same fd-passing technique works on macs too?)

http://codereview.chromium.org/174232/diff/8/12#newcode207
Line 207: uint32 attributes = 0L;
On 2009/08/21 18:12:08, agl wrote:
> remove 'L'

done.

http://codereview.chromium.org/174232/diff/8/12#newcode218
Line 218: uint32 attributes = -1L;
On 2009/08/21 18:12:08, agl wrote:
> might as well remove the L here too, although it's not your code.

done. this is actually my code too. :)

http://codereview.chromium.org/174232/diff/8/9
File third_party/sqlite/src/os_unix.c (right):

http://codereview.chromium.org/174232/diff/8/9#newcode2098
Line 2098: int fillInUnixFile(
On 2009/08/21 18:40:14, michaeln wrote:
> Is there a README file that documents our changes to this library?

yes, forgot to make this change. done.

[michaeln, 2009-08-22 19:53:47]

http://codereview.chromium.org/174232/diff/1017/16
File base/platform_file_posix.cc (right):

http://codereview.chromium.org/174232/diff/1017/16#newcode67
Line 67: unlink(WideToUTF8(name).c_str());
probably should only do this if the file was successfully opened

But a more significant concern... 

I think as implemented here, PLATFORM_FILE_DELETE_ON_CLOSE has different
behavior then the windows impl. In windows the file is still visible in the file
system until it is closed and actually deleted. Here the file is invisible to
others immediately. I'm not sure we should introduce this variant of the
behavior here.

Assuming the unlink calls is the right thing todo for sqlite in its use case,
the unlink function call could be made up in the DatabaseDispatcherHost class
(which has platform #ifdefs in that area already). And we wouldn't have to
reconcile what 'delete on close' means universally.

fyi: Here's what MSDN says about its delete on close behavior...

"The file is to be deleted immediately after all of its handles are closed,
which includes the specified handle and any other open or duplicated handles.
If there are existing open handles to a file, the call fails unless they were
all opened with the FILE_SHARE_DELETE share mode.
Subsequent open requests for the file fail, unless the FILE_SHARE_DELETE share
mode is specified."

http://codereview.chromium.org/174232/diff/1017/17
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/174232/diff/1017/17#newcode143
Line 143: base::PLATFORM_FILE_EXCLUSIVE_WRITE;
Were these not needed for windows, but sqlite4linux needs them?

http://codereview.chromium.org/174232/diff/1017/17#newcode215
Line 215: #elif defined(OS_LINUX)
OS_POSIX?

[dumi, 2009-08-24 18:22:54]

http://codereview.chromium.org/174232/diff/1017/16
File base/platform_file_posix.cc (right):

http://codereview.chromium.org/174232/diff/1017/16#newcode67
Line 67: unlink(WideToUTF8(name).c_str());
On 2009/08/22 19:53:47, michaeln wrote:
> probably should only do this if the file was successfully opened

Done, will show up in the next version of the patch.

> But a more significant concern... 
> 
> I think as implemented here, PLATFORM_FILE_DELETE_ON_CLOSE has different
> behavior then the windows impl. In windows the file is still visible in the
file
> system until it is closed and actually deleted. Here the file is invisible to
> others immediately. I'm not sure we should introduce this variant of the
> behavior here.
> 
> Assuming the unlink calls is the right thing todo for sqlite in its use case,
> the unlink function call could be made up in the DatabaseDispatcherHost class
> (which has platform #ifdefs in that area already). And we wouldn't have to
> reconcile what 'delete on close' means universally.
> 
> fyi: Here's what MSDN says about its delete on close behavior...
> 
> "The file is to be deleted immediately after all of its handles are closed,
> which includes the specified handle and any other open or duplicated handles.
> If there are existing open handles to a file, the call fails unless they were
> all opened with the FILE_SHARE_DELETE share mode.
> Subsequent open requests for the file fail, unless the FILE_SHARE_DELETE share
> mode is specified."

We could say that PLATFORM_FILE_DELETE_ON_CLOSE means that the data in the file
becomes unavailable as soon as all handles to the file are closed. The fact that
Windows decides to keep the file itself visible to others too while the data is
there is just an implementation detail that nobody should rely on. In fact, on
Windows the VFS also uses FILE_ATTRIBUTE_TEMPORARY | FILE_ATTRIBUTE_HIDDEN
whenever it uses FILE_FLAG_DELETE_ON_CLOSE, which seems to me like an attempt to
emulate as close as possible the Linux behavior.

In any case, this is what SQLite's default VFSs do on Windows and Linux.

http://codereview.chromium.org/174232/diff/1017/17
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/174232/diff/1017/17#newcode143
Line 143: base::PLATFORM_FILE_EXCLUSIVE_WRITE;
On 2009/08/22 19:53:47, michaeln wrote:
> Were these not needed for windows, but sqlite4linux needs them?

I think this flag is not used in the Windows VFS because CreateFile() gets
exclusive access to files by default. So on Windows we only needed to specify
when NOT to get exclusive access (main DB file). On Linux, open() gives us
shared access to files by default. So we need to specify when we want exclusive
access.

http://codereview.chromium.org/174232/diff/1017/17#newcode215
Line 215: #elif defined(OS_LINUX)
On 2009/08/22 19:53:47, michaeln wrote:
> OS_POSIX?

done.

[michaeln, 2009-08-25 19:52:11]

I think the subtle platform specific 'delete on close' behavior of the platform
file abstraction is ok.

http://codereview.chromium.org/174232/diff/1057/1061
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/174232/diff/1057/1061#newcode179
Line 179: int fileType = params.desired_flags & 0x00007F00;
file_type

http://codereview.chromium.org/174232/diff/1057/1061#newcode180
Line 180: bool creatingNewFile = (params.desired_flags & SQLITE_OPEN_CREATE);
ditto camel_case 'isms

http://codereview.chromium.org/174232/diff/1057/1061#newcode183
Line 183: target_dir_handle = base::CreatePlatformFile(
nit: a code comment nearby stating that we also return a file handle to the
containing directory and why may be nice

http://codereview.chromium.org/174232/diff/1057/1061#newcode225
Line 225: if (file_util::PathExists(params.file_name) &&
Would it make sense to test !PathExists seperately to make it more clear what
the return value is in that case, and to avoid sync'ing the db dir in that case?

http://codereview.chromium.org/174232/diff/1057/1061#newcode401
Line 401: DeleteFileParams params = { db_dir, db_file_name, sync_dir };
Could just invoke GetDBDir() when initializing the 'params' struct directly to
get rid of the db_dir local var.

http://codereview.chromium.org/174232/diff/1057/1072
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/174232/diff/1057/1072#newcode398
Line 398: base::PlatformFile dir_handle;      // DB directory handle
Is dir_handle ever used on windows? if its always set to
kInvalidPlatformFileValue, do we need it in the struct?

http://codereview.chromium.org/174232/diff/1057/1072#newcode1879
Line 1879: ReadParam(m, iter, &p->dir_handle);
nit: maybe align the second call to ReadParam with the first

http://codereview.chromium.org/174232/diff/1057/1064
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/174232/diff/1057/1064#newcode667
Line 667: /* DB file + dir handles */
you don't need to comment, the param type is sufficient documentation... also
the closing ')' is hanging out below, should be brought up

http://codereview.chromium.org/174232/diff/1057/1065
File chrome/renderer/renderer_webkitclient_impl.cc (right):

http://codereview.chromium.org/174232/diff/1057/1065#newcode240
Line 240: base::PlatformFile* dir_handle) {
nit: indent the method arguments one more tab stop

http://codereview.chromium.org/174232/diff/1057/1065#newcode249
Line 249: base::FileDescriptor(base::kInvalidPlatformFileValue, true) };
i've seen this block to init a struct with empty values a few times now... i
wonder if we should have a ctor to do this

http://codereview.chromium.org/174232/diff/1057/1073
File chrome/renderer/renderer_webkitclient_impl.h (right):

http://codereview.chromium.org/174232/diff/1057/1073#newcode49
Line 49: virtual int databaseDeleteFile(const WebKit::WebString& file_name, bool
sync_dir);
line length

http://codereview.chromium.org/174232/diff/1057/1069
File webkit/api/public/WebKitClient.h (right):

http://codereview.chromium.org/174232/diff/1057/1069#newcode126
Line 126: virtual FileType databaseOpenFile(const WebString& fileName, int
desiredFlags, FileType* dirHandle) = 0;
is dirHandle optional, can the caller provide NULL if not interested in that
value... doc comment?

http://codereview.chromium.org/174232/diff/1057/1067
File webkit/glue/webkitclient_impl.cc (right):

http://codereview.chromium.org/174232/diff/1057/1067#newcode244
Line 244: *dirHandle = base::kInvalidPlatformFileValue;
ah ha... yes... the caller can provide NULL!

[dumi, 2009-08-26 19:35:03]

http://codereview.chromium.org/174232/diff/1057/1061
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/174232/diff/1057/1061#newcode179
Line 179: int fileType = params.desired_flags & 0x00007F00;
On 2009/08/25 19:52:12, michaeln wrote:
> file_type

done.

http://codereview.chromium.org/174232/diff/1057/1061#newcode180
Line 180: bool creatingNewFile = (params.desired_flags & SQLITE_OPEN_CREATE);
On 2009/08/25 19:52:12, michaeln wrote:
> ditto camel_case 'isms

done.

http://codereview.chromium.org/174232/diff/1057/1061#newcode183
Line 183: target_dir_handle = base::CreatePlatformFile(
On 2009/08/25 19:52:12, michaeln wrote:
> nit: a code comment nearby stating that we also return a file handle to the
> containing directory and why may be nice

done.

http://codereview.chromium.org/174232/diff/1057/1061#newcode225
Line 225: if (file_util::PathExists(params.file_name) &&
On 2009/08/25 19:52:12, michaeln wrote:
> Would it make sense to test !PathExists seperately to make it more clear what
> the return value is in that case, and to avoid sync'ing the db dir in that
case?

done. refactored this code into multiple if-blocks.

http://codereview.chromium.org/174232/diff/1057/1061#newcode401
Line 401: DeleteFileParams params = { db_dir, db_file_name, sync_dir };
On 2009/08/25 19:52:12, michaeln wrote:
> Could just invoke GetDBDir() when initializing the 'params' struct directly to
> get rid of the db_dir local var.

done. made the same change in OnDatabaseOpenFile().

http://codereview.chromium.org/174232/diff/1057/1072
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/174232/diff/1057/1072#newcode398
Line 398: base::PlatformFile dir_handle;      // DB directory handle
On 2009/08/25 19:52:12, michaeln wrote:
> Is dir_handle ever used on windows? if its always set to
> kInvalidPlatformFileValue, do we need it in the struct?

good point. removed and made all related changes.

http://codereview.chromium.org/174232/diff/1057/1072#newcode1879
Line 1879: ReadParam(m, iter, &p->dir_handle);
On 2009/08/25 19:52:12, michaeln wrote:
> nit: maybe align the second call to ReadParam with the first

aligned and inside #if defined(OS_POSIX) now.

http://codereview.chromium.org/174232/diff/1057/1064
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/174232/diff/1057/1064#newcode667
Line 667: /* DB file + dir handles */
On 2009/08/25 19:52:12, michaeln wrote:
> you don't need to comment, the param type is sufficient documentation... also
> the closing ')' is hanging out below, should be brought up

fixed.

http://codereview.chromium.org/174232/diff/1057/1065
File chrome/renderer/renderer_webkitclient_impl.cc (right):

http://codereview.chromium.org/174232/diff/1057/1065#newcode249
Line 249: base::FileDescriptor(base::kInvalidPlatformFileValue, true) };
On 2009/08/25 19:52:12, michaeln wrote:
> i've seen this block to init a struct with empty values a few times now... i
> wonder if we should have a ctor to do this

it's initialized with empty values only once in database_dispatcher_host.cc. i
have a slight preference for not having a constructor because no other IPC
params in render_messages.h have one, but i don't have a strong opinion here, so
please let me know if you want me to add one.

http://codereview.chromium.org/174232/diff/1057/1073
File chrome/renderer/renderer_webkitclient_impl.h (right):

http://codereview.chromium.org/174232/diff/1057/1073#newcode49
Line 49: virtual int databaseDeleteFile(const WebKit::WebString& file_name, bool
sync_dir);
On 2009/08/25 19:52:12, michaeln wrote:
> line length

fixed.

http://codereview.chromium.org/174232/diff/1057/1069
File webkit/api/public/WebKitClient.h (right):

http://codereview.chromium.org/174232/diff/1057/1069#newcode126
Line 126: virtual FileType databaseOpenFile(const WebString& fileName, int
desiredFlags, FileType* dirHandle) = 0;
On 2009/08/25 19:52:12, michaeln wrote:
> is dirHandle optional, can the caller provide NULL if not interested in that
> value... doc comment?

update the comment.

[michaeln, 2009-08-26 20:33:43]

LGTM