Add LoggingOpenFileFor{Read|Write}

util/file/file_io.h

 // Copyright 2014 The Crashpad Authors. All rights reserved.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
 // You may obtain a copy of the License at
 //
 //     http://www.apache.org/licenses/LICENSE-2.0
 //
 // Unless required by applicable law or agreed to in writing, software
 // distributed under the License is distributed on an "AS IS" BASIS,
 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
 #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"
 
 #if defined(OS_WIN)
 #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)
 using FileHandle = int;
 #elif defined(OS_WIN)
 using FileHandle = HANDLE;
 #endif
 
 //! \brief Reads from a file, retrying when interrupted on POSIX or following a
 //!        short read.
@@ skipping 78 matching lines @@
 //! \brief Wraps ReadFile(), ensuring that it indicates end-of-file.
 //!
 //! Attempts to read a single byte from \a file, expecting no data to be read.
 //! If the underlying ReadFile() fails, or if a byte actually is read, this
 //! function causes execution to terminate without returning.
 //!
 //! \sa CheckedReadFile
 //! \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.
 //!
 //! \return On success, `true` is returned. On failure, an error is logged and
 //!     `false` is returned.
 bool LoggingCloseFile(FileHandle file);
 
 //! \brief Wraps `close()` or `CloseHandle()`, ensuring that it succeeds.
 //!
 //! If the underlying function fails, this function causes execution to
 //! terminate without returning.
 void CheckedCloseFile(FileHandle file);
 
 }  // namespace crashpad
 
 #endif  // CRASHPAD_UTIL_FILE_FILE_IO_H_