Introduce MinidumpWritable, its dependencies, and their tests

Introduce MinidumpWritable, its dependencies, and their tests.

MinidumpWritable is the base class for all minidump-writing operations.

R=rsesek@chromium.org

Committed: https://chromium.googlesource.com/crashpad/crashpad/+/995012534e34

[Mark Mentovai, 2014-07-31 19:02:50]

Here’s another 2500 lines or so, but at least half are in tests, and a good
chunk of the rest are comments. Sorry.

[Robert Sesek, 2014-08-01 14:38:04]

Yes, this would have been easier (and faster to turn around) had util been split
out into its own CL.

https://codereview.chromium.org/432003005/diff/1/minidump/minidump_writable.cc
File minidump/minidump_writable.cc (right):

https://codereview.chromium.org/432003005/diff/1/minidump/minidump_writable.c...
minidump/minidump_writable.cc:161: // Populate the RVA fileds in other objects
that have registered to point to
nit: fields

https://codereview.chromium.org/432003005/diff/1/minidump/minidump_writable.c...
minidump/minidump_writable.cc:185: registered_location_descriptors_) {
nit: indent 4 more, or are we just letting clang-format rule here?

https://codereview.chromium.org/432003005/diff/1/minidump/minidump_writable.c...
minidump/minidump_writable.cc:193: // registered to have populated may not yet
be populated, because this
This sentence is hard to parse.

https://codereview.chromium.org/432003005/diff/1/minidump/minidump_writable.c...
minidump/minidump_writable.cc:215: auto unaligned_child_offset = local_offset +
size;
This use of auto seems out of place IMO.

https://codereview.chromium.org/432003005/diff/1/minidump/minidump_writable.h
File minidump/minidump_writable.h (right):

https://codereview.chromium.org/432003005/diff/1/minidump/minidump_writable.h...
minidump/minidump_writable.h:63: // This is public instead of protected because
objects of derived classes need
Deliberately not //! ?

https://codereview.chromium.org/432003005/diff/1/minidump/minidump_writable.h...
minidump/minidump_writable.h:136: ~MinidumpWritable();
virtual?

https://codereview.chromium.org/432003005/diff/1/util/file/file_writer.h
File util/file/file_writer.h (right):

https://codereview.chromium.org/432003005/diff/1/util/file/file_writer.h#newc...
util/file/file_writer.h:47: class FileWriterInterface {
The assumption here is that checking errno is silly and logging is better, with
just a boolean success?

https://codereview.chromium.org/432003005/diff/1/util/file/file_writer.h#newc...
util/file/file_writer.h:85: //! \brief Wraps `open()`.
Probably need to mention that you can't call this again until calling Close().

https://codereview.chromium.org/432003005/diff/1/util/file/file_writer.h#newc...
util/file/file_writer.h:89: bool Open(const std::string& path, int oflag, mode_t
mode);
Any thought on using base::FilePath?

https://codereview.chromium.org/432003005/diff/1/util/file/file_writer.h#newc...
util/file/file_writer.h:94: virtual bool Write(const void* data, size_t size)
override;
Style is typically:

// FileWriterInterface:

And then no blank lines between the overridden functions.

https://codereview.chromium.org/432003005/diff/1/util/file/string_file_writer.cc
File util/file/string_file_writer.cc (right):

https://codereview.chromium.org/432003005/diff/1/util/file/string_file_writer...
util/file/string_file_writer.cc:38: size_t offset = offset_.ValueOrDie();
Mark this const so it isn't accidentally used as a destination for arithmetic
outside its safety harness.

https://codereview.chromium.org/432003005/diff/1/util/file/string_file_writer...
util/file/string_file_writer.cc:117: base::CheckedNumeric<off_t>
new_offset_checked(base_offset);
Could use AssignIfInRange() for lines 112-117.

https://codereview.chromium.org/432003005/diff/1/util/file/string_file_writer...
util/file/string_file_writer.cc:125: if
(!base::IsValueInRangeForNumericType<size_t>(new_offset)) {
Same.

https://codereview.chromium.org/432003005/diff/1/util/file/string_file_writer.h
File util/file/string_file_writer.h (right):

https://codereview.chromium.org/432003005/diff/1/util/file/string_file_writer...
util/file/string_file_writer.h:49: 
C.f. comment about overridden methods in other file. At the least, why blank
lines in that file but in this one, only here?

[Mark Mentovai, 2014-08-01 16:09:35]

OK, we’ll do a smaller chunk next time. Sorry.

https://codereview.chromium.org/432003005/diff/1/minidump/minidump_writable.cc
File minidump/minidump_writable.cc (right):

https://codereview.chromium.org/432003005/diff/1/minidump/minidump_writable.c...
minidump/minidump_writable.cc:185: registered_location_descriptors_) {
rsesek wrote:
> nit: indent 4 more, or are we just letting clang-format rule here?

I’ll fix cases like this where clang-format clearly did a less-good thing.

https://codereview.chromium.org/432003005/diff/1/minidump/minidump_writable.c...
minidump/minidump_writable.cc:215: auto unaligned_child_offset = local_offset +
size;
rsesek wrote:
> This use of auto seems out of place IMO.

It’s unfortunately necessary because we’re dealing with distinct types, size_t
and off_t, and we don’t know here which one has a greater range. I’ll add a
comment.

https://codereview.chromium.org/432003005/diff/1/util/file/file_writer.h
File util/file/file_writer.h (right):

https://codereview.chromium.org/432003005/diff/1/util/file/file_writer.h#newc...
util/file/file_writer.h:47: class FileWriterInterface {
rsesek wrote:
> The assumption here is that checking errno is silly and logging is better,
with
> just a boolean success?

Yeah, basically this does the normal HANDLE_EINTR and continues after partial
writes, so from the caller’s perspective, either the operation succeeded or it
failed. The “why” of a failure isn’t normally interesting at that level.

If you do want the byte count and errno for some reason, but you also want
HANDLE_EINTR and partial write handling, use WriteFD from fd_io.{cc,h}.

https://codereview.chromium.org/432003005/diff/1/util/file/file_writer.h#newc...
util/file/file_writer.h:89: bool Open(const std::string& path, int oflag, mode_t
mode);
rsesek wrote:
> Any thought on using base::FilePath?

When I didn’t care about Windows support, I just wanted to use std::string
everywhere. Now that it looks like we might need Windows support in some of
this, I guess we should be using FilePath.

https://codereview.chromium.org/432003005/diff/1/util/file/file_writer.h#newc...
util/file/file_writer.h:94: virtual bool Write(const void* data, size_t size)
override;
rsesek wrote:
> Style is typically:
> 
> // FileWriterInterface:
> 
> And then no blank lines between the overridden functions.

I added some extra documentation here per your previous suggestion, so the blank
lines have to stay.

https://codereview.chromium.org/432003005/diff/1/util/file/string_file_writer.cc
File util/file/string_file_writer.cc (right):

https://codereview.chromium.org/432003005/diff/1/util/file/string_file_writer...
util/file/string_file_writer.cc:38: size_t offset = offset_.ValueOrDie();
rsesek wrote:
> Mark this const so it isn't accidentally used as a destination for arithmetic
> outside its safety harness.

Yeah, I’d actually like to use more and more const for safety in all of this
code.

https://codereview.chromium.org/432003005/diff/1/util/file/string_file_writer...
util/file/string_file_writer.cc:117: base::CheckedNumeric<off_t>
new_offset_checked(base_offset);
rsesek wrote:
> Could use AssignIfInRange() for lines 112-117.

It actually works out being an extra line that way, because you can’t assign
right into a CheckedNumeric, but I made the change to leverage util anyway.

[Robert Sesek, 2014-08-01 16:47:05]

LGTM

[Mark Mentovai, 2014-08-01 16:48:32]

Committed patchset #2 manually as 995012534e34 (presubmit successful).