Add LoggingOpenFileFor{Read|Write}

util/file/file_io.h

Index: util/file/file_io.h
diff --git a/util/file/file_io.h b/util/file/file_io.h
index 927bc3cb13319ff5843a613cff77bbfd20588648..3b1adef2b816435ac50a50bb462e950ec51fe135 100644
--- a/util/file/file_io.h
+++ b/util/file/file_io.h
@@ -15,6 +15,7 @@
 #ifndef CRASHPAD_UTIL_FILE_FILE_IO_H_
 #define CRASHPAD_UTIL_FILE_FILE_IO_H_
 
+#include <inttypes.h>

[Mark Mentovai, 2014/12/18 19:04:27]

We use <stdint.h> to get intxx_t in Crashpad.

But the mode argument on POSIX Is actually mode_t, which could theoretically not
be compatible with uint32_t.

[scottmg, 2014/12/18 19:59:20]

Done. (removed)

 #include <sys/types.h>
 
 #include "build/build_config.h"
@@ -23,6 +24,10 @@
 #include <windows.h>
 #endif
 
+namespace base {
+class FilePath;
+}

[Mark Mentovai, 2014/12/18 19:04:27]

// namespace base

[scottmg, 2014/12/18 19:59:20]

Done.

+
 namespace crashpad {
 
 #if defined(OS_POSIX)
@@ -121,6 +126,26 @@ void CheckedWriteFile(FileHandle file, const void* buffer, size_t size);
 //! \sa ReadFile
 void CheckedReadFileAtEOF(FileHandle file);
 
+//! \brief Wraps `open()` or `CreateFile()`, creating a file for output. Logs
+//!        an error if the operation fails.
+//!
+//! On a POSIX system, \a mode will be used as the permissions bits.
+//!
+//! \return The newly opened FileHandle, or an invalid FileHandle on failure.

[Mark Mentovai, 2014/12/18 19:04:27]

Aha! We should have a \brief on FileHandle.

None of the OS_* macros are defined to Doxygen, so we’d need

#if defined(OS_POSIX) || DOXYGEN
//! \brief blah blah blah
using FileHandle = …
#elif defined(OS_WIN)
using FileHandle = …
#endif

[scottmg, 2014/12/18 19:59:20]

Done.

+//!
+//! \sa ScopedFD
+//! \sa ScopedFileHANDLE
+FileHandle LoggingOpenFileForWrite(const base::FilePath& path, uint32_t mode);

[Mark Mentovai, 2014/12/18 19:04:26]

I agree that we probably don’t need to worry about O_RDWR. I don’t really see a
use case for it in Crashpad right now. Maybe something in the future might want
to modify an existing dump? I can’t really envision how we’d want to use this,
so I think it’s OK to ignore O_RDWR for now if that makes this too frustrating
to design.

But even if we’re focusing only on O_WRONLY, I don’t think we can ignore all of
the other bits in oflag. We’ll have situations where we want O_EXCL, and others
where we won’t. (Normally you want to avoid clobbering something unexpectedly,
but for the tools, if the user asks to write to a file that already exists, then
the clobber is expected.) O_CREAT and O_TRUNC are normally right for things like
dumps, but are wrong if we want to do something like keep a running log. But I’m
not sure if we’ll want to do that, so like O_RDWR, I’m OK assuming O_CREAT and
O_TRUNC are always desired until the point that we have a caller that wants them
off for some good reason.

So I guess we at least need something to decide between O_EXCL/CREATE_NEW and
no-O_EXCL/CREATE_ALWAYS.

I also think that we can simplify “mode” to decide between 0600 and 0644. I
don’t think we need to care about setting the executable bits, at least for now.
We would want to write dumps with mode 0600 because they might contain sensitive
data and we shouldn’t trust it to the user’s umask, but other files should be OK
to write with mode 0644 and let the user’s umask control which of those bits
should be expressed. Having a more generic way to express the mode gets us over
the “uint32_t isn’t really right” problem without needing to go to the trouble
of defining a typedef that wouldn’t even be used on Windows.

[scottmg, 2014/12/18 19:59:20]

I was thinking a separate LoggingOpenFileForAppend if we needed it. But I'm not
familiar with all the uses and combinations of the O_ flags.
I couldn't think of a case where we wanted the "don't clobber unexpectedly",
maybe just because I'm used to fopen and that's just sort of what you get.

But then we need a OpenForWriteCreate, and OpenForWriteCreateNew, and maybe
Truncate, and then it starts to get a bit much, so maybe we do need a mode.

Added a new ps as proposal for discussion (not implemented yet, just the
header).
Maybe just `bool world_readable` then?

+
+//! \brief Wraps `open()` or `CreateFile()`, opening an existing file for
+//!        reading. Logs an error if the operation fails.
+//!
+//! \return The newly opened FileHandle, or an invalid FileHandle on failure.
+//!
+//! \sa ScopedFD
+//! \sa ScopedFileHANDLE
+FileHandle LoggingOpenFileForRead(const base::FilePath& path);

[Mark Mentovai, 2014/12/18 19:04:26]

On the other hand, I see no problems with an interface like this to open a file
for reading.

+
 //! \brief Wraps `close()` or `CloseHandle()`, logging an error if the operation
 //!        fails.
 //!