POSIX: Transfer network data using shared memory

POSIX: Transfer network data using shared memory

This patch adds the long planned support for sharing memory on POSIX
by transporting file descriptors. It largely builds on the shared
memory cleanup work by jrg.

We move FileDescriptor out of chrome/common/file_descriptor_posix.h
and into base/file_descriptor_posix.h. Since all that's left in the
chrome/common verion is the DescriptorSet, those files are renamed to
descriptor_set.[h|cc].

The SharedMemoryHandle on POSIX then becomes a typedef to a
FileDescriptor and thus can be serialised over IPC.

After that, it's mostly a case of cleaning up those snippets of code
which considered SharedMemoryHandles to be scaler values.

[agl, 2009-02-10 20:34:21]

jrg: base/shared_memory*
darin: everything else

[John Grabowski, 2009-02-10 21:20:47]

LGTM for shmem stuff

http://codereview.chromium.org/21208/diff/1021/1022
File base/file_descriptor_posix.h (right):

http://codereview.chromium.org/21208/diff/1021/1022#newcode10
Line 10: struct FileDescriptor {
A casual reader might not understand what this class is used for.
Perhaps add a brief description of it's use (sending fds across a pipe;
auto_close implies an ownership transition).

http://codereview.chromium.org/21208/diff/1021/1029
File chrome/common/descriptor_set_posix.cc (right):

http://codereview.chromium.org/21208/diff/1021/1029#newcode9
Line 9: DescriptorSet::DescriptorSet()
No unit test for this class?  (I realize there was none for
file_descriptor_posix.h).

http://codereview.chromium.org/21208/diff/1021/1029#newcode56
Line 56: for (std::vector<base::FileDescriptor>::const_iterator
DCHECK(next_descriptor_ == 0);

http://codereview.chromium.org/21208/diff/1021/1029#newcode68
Line 68: descriptors_.clear();
next_descriptor_ = 0;

http://codereview.chromium.org/21208/diff/1021/1030
File chrome/common/descriptor_set_posix.h (right):

http://codereview.chromium.org/21208/diff/1021/1030#newcode39
Line 39: void Add(int fd);
return false if full?

[agl, 2009-02-10 21:40:02]

http://codereview.chromium.org/21208/diff/1021/1022
File base/file_descriptor_posix.h (right):

http://codereview.chromium.org/21208/diff/1021/1022#newcode10
Line 10: struct FileDescriptor {
On 2009/02/10 21:20:47, John Grabowski wrote:
> A casual reader might not understand what this class is used for.
> Perhaps add a brief description of it's use (sending fds across a pipe;
> auto_close implies an ownership transition).

I actually took that comment /out/ because it's talking about IPC and such in
base, which I suspect Darin won't like. However, I can always put it back it if
he doesn't mind the layer violation.

http://codereview.chromium.org/21208/diff/1021/1029
File chrome/common/descriptor_set_posix.cc (right):

http://codereview.chromium.org/21208/diff/1021/1029#newcode9
Line 9: DescriptorSet::DescriptorSet()
On 2009/02/10 21:20:47, John Grabowski wrote:
> No unit test for this class?  (I realize there was none for
> file_descriptor_posix.h).

A unittest will be coming. I'm still discussing with Darin what the future of
this class is going to look like. Once I'm happy with it, I'll lash it down with
tests.

http://codereview.chromium.org/21208/diff/1021/1029#newcode56
Line 56: for (std::vector<base::FileDescriptor>::const_iterator
On 2009/02/10 21:20:47, John Grabowski wrote:
> DCHECK(next_descriptor_ == 0);

Done.

http://codereview.chromium.org/21208/diff/1021/1029#newcode68
Line 68: descriptors_.clear();
On 2009/02/10 21:20:47, John Grabowski wrote:
> next_descriptor_ = 0;

Done.

http://codereview.chromium.org/21208/diff/1021/1030
File chrome/common/descriptor_set_posix.h (right):

http://codereview.chromium.org/21208/diff/1021/1030#newcode39
Line 39: void Add(int fd);
On 2009/02/10 21:20:47, John Grabowski wrote:
> return false if full?

Sounds reasonable, done.

[darin (slow to review), 2009-02-11 08:14:34]

LG, just some nits:

http://codereview.chromium.org/21208/diff/1045/1046
File base/file_descriptor_posix.h (right):

http://codereview.chromium.org/21208/diff/1045/1046#newcode16
Line 16: // If true, close this descriptor after it has been sent.
"sent"?  this comment is a bit out-of-place down in base/.

reading just this class in isolation, i would find it confusing to know what it
means for auto_close to be true.

http://codereview.chromium.org/21208/diff/1045/1047
File base/shared_memory.h (right):

http://codereview.chromium.org/21208/diff/1045/1047#newcode34
Line 34: bool SharedMemoryHandleValid(const SharedMemoryHandle& h);
nit: SharedMemoryHandleIsValid <-- helps to insert a verb

or maybe a static method on SharedMemory...

SharedMemory::IsValidHandle

http://codereview.chromium.org/21208/diff/1045/1048
File base/shared_memory_posix.cc (right):

http://codereview.chromium.org/21208/diff/1045/1048#newcode288
Line 288: base::FileDescriptor descriptor;
a constructor might be nice:

return FileDescriptor(mapped_file_, false);

(note: we are in the base namespace already.)

http://codereview.chromium.org/21208/diff/1045/1054
File chrome/common/descriptor_set_posix.h (right):

http://codereview.chromium.org/21208/diff/1045/1054#newcode18
Line 18: class DescriptorSet {
nit: shouldn't this be FileDescriptorSet to be more consistently named?

[agl, 2009-02-11 18:54:33]

http://codereview.chromium.org/21208/diff/1045/1046
File base/file_descriptor_posix.h (right):

http://codereview.chromium.org/21208/diff/1045/1046#newcode16
Line 16: // If true, close this descriptor after it has been sent.
On 2009/02/11 08:14:34, darin wrote:
> reading just this class in isolation, i would find it confusing to know what
it
> means for auto_close to be true.

Cleaned up.

http://codereview.chromium.org/21208/diff/1045/1047
File base/shared_memory.h (right):

http://codereview.chromium.org/21208/diff/1045/1047#newcode34
Line 34: bool SharedMemoryHandleValid(const SharedMemoryHandle& h);
On 2009/02/11 08:14:34, darin wrote:
> nit: SharedMemoryHandleIsValid <-- helps to insert a verb
> 
> or maybe a static method on SharedMemory...
> 
> SharedMemory::IsValidHandle

Done.

http://codereview.chromium.org/21208/diff/1045/1048
File base/shared_memory_posix.cc (right):

http://codereview.chromium.org/21208/diff/1045/1048#newcode288
Line 288: base::FileDescriptor descriptor;
On 2009/02/11 08:14:34, darin wrote:
> a constructor might be nice:
> 
> return FileDescriptor(mapped_file_, false);
> 
> (note: we are in the base namespace already.)

Done.

http://codereview.chromium.org/21208/diff/1045/1054
File chrome/common/descriptor_set_posix.h (right):

http://codereview.chromium.org/21208/diff/1045/1054#newcode18
Line 18: class DescriptorSet {
On 2009/02/11 08:14:34, darin wrote:
> nit: shouldn't this be FileDescriptorSet to be more consistently named?

Leaving this for the cleanup patch to follow.