Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
818433002:
Add LoggingOpenFileFor{Read|Write} (Closed)
|
Created:
6 years ago by scottmg Modified:
6 years ago Reviewers:
Mark Mentovai CC:
crashpad-dev_chromium.org Base URL:
https://chromium.googlesource.com/crashpad/crashpad@scoped-handle-land Target Ref:
refs/heads/master Project:
crashpad Visibility:
Public. |
DescriptionAdd LoggingOpenFileFor{Read|Write}
I started (https://codereview.chromium.org/812403002/) emulating oflag values
on Windows in FileWriter, but it seemed awkward. On the assumption that we're
only likely to need "read a file" and "write a file" this seemed simpler, and
sufficient (but I don't know if that's necessarily true).
Users of open are not yet switched.
R=mark@chromium.org
BUG=crashpad:1
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/4034d30023dcc3c1151edcad32f1e840c0314c15
Patch Set 1 #Patch Set 2 : posix #
Total comments: 9
Patch Set 3 : mode #
Total comments: 19
Patch Set 4 : . #Patch Set 5 : posix impl #
Total comments: 7
Patch Set 6 : fixes #Patch Set 7 : win impl #
Total comments: 7
Patch Set 8 : . #
Messages
Total messages: 19 (5 generated)
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Short story: we probably don’t need anything as in-depth as what you started in https://codereview.chromium.org/812403002, at least not yet, but we probably need more than this. https://codereview.chromium.org/818433002/diff/80001/util/file/file_io.h File util/file/file_io.h (right): https://codereview.chromium.org/818433002/diff/80001/util/file/file_io.h#newc... util/file/file_io.h:18: #include <inttypes.h> 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. https://codereview.chromium.org/818433002/diff/80001/util/file/file_io.h#newc... util/file/file_io.h:29: } // namespace base https://codereview.chromium.org/818433002/diff/80001/util/file/file_io.h#newc... util/file/file_io.h:134: //! \return The newly opened FileHandle, or an invalid FileHandle on failure. 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 https://codereview.chromium.org/818433002/diff/80001/util/file/file_io.h#newc... util/file/file_io.h:138: FileHandle LoggingOpenFileForWrite(const base::FilePath& path, uint32_t mode); 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. https://codereview.chromium.org/818433002/diff/80001/util/file/file_io.h#newc... util/file/file_io.h:147: FileHandle LoggingOpenFileForRead(const base::FilePath& path); On the other hand, I see no problems with an interface like this to open a file for reading.
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
https://codereview.chromium.org/818433002/diff/80001/util/file/file_io.h File util/file/file_io.h (right): https://codereview.chromium.org/818433002/diff/80001/util/file/file_io.h#newc... util/file/file_io.h:18: #include <inttypes.h> On 2014/12/18 19:04:27, Mark Mentovai wrote: > 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. Done. (removed) https://codereview.chromium.org/818433002/diff/80001/util/file/file_io.h#newc... util/file/file_io.h:29: } On 2014/12/18 19:04:27, Mark Mentovai wrote: > // namespace base Done. https://codereview.chromium.org/818433002/diff/80001/util/file/file_io.h#newc... util/file/file_io.h:134: //! \return The newly opened FileHandle, or an invalid FileHandle on failure. On 2014/12/18 19:04:27, Mark Mentovai wrote: > 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 Done. https://codereview.chromium.org/818433002/diff/80001/util/file/file_io.h#newc... util/file/file_io.h:138: FileHandle LoggingOpenFileForWrite(const base::FilePath& path, uint32_t mode); On 2014/12/18 19:04:26, Mark Mentovai wrote: > 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. 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. > > So I guess we at least need something to decide between O_EXCL/CREATE_NEW and > no-O_EXCL/CREATE_ALWAYS. 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). > > 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. Maybe just `bool world_readable` then?
I like the approach. The big question here is that I don’t see the difference between kCreate and kTruncate. https://codereview.chromium.org/818433002/diff/140001/util/file/file_io.h File util/file/file_io.h (right): https://codereview.chromium.org/818433002/diff/140001/util/file/file_io.h#new... util/file/file_io.h:39: //! \brief Determines the mode that LoggingOpenFileForWrite uses. () on function names. https://codereview.chromium.org/818433002/diff/140001/util/file/file_io.h#new... util/file/file_io.h:41: //! \brief Opens the file if it exists and seeks to the end of the file, or Meh, I don’t know about having this actually do the seek. https://codereview.chromium.org/818433002/diff/140001/util/file/file_io.h#new... util/file/file_io.h:46: //! overwritten. Overwritten? What’s the difference between kCreate and kTruncate? I don’t think we can maintain that distinction on POSIX. https://codereview.chromium.org/818433002/diff/140001/util/file/file_io.h#new... util/file/file_io.h:55: }; Everything here has the potential to create a file. The three options I see are: - require that the file be created - allow the file to exist but discard existing contents (whether it’s via truncation or replacing it - allow the file to exist and retain existing contents https://codereview.chromium.org/818433002/diff/140001/util/file/file_io.h#new... util/file/file_io.h:150: //! \a write_mode determines the style (clobber, truncate, etc.) that is used Normally parameters are done with \param[in] style but the other functions in here don’t have that, so this is OK. https://codereview.chromium.org/818433002/diff/140001/util/file/file_io.h#new... util/file/file_io.h:152: //! be set to 0644, otherwise, 0600. On Windows, the file is always opened in Numeric literals also get the `backtick` treatment. https://codereview.chromium.org/818433002/diff/140001/util/file/file_io.h#new... util/file/file_io.h:152: //! be set to 0644, otherwise, 0600. On Windows, the file is always opened in “set to” is not totally correct, because the umask will still modify the bits.
I like the approach. The big question here is that I don’t see the difference between kCreate and kTruncate.
https://codereview.chromium.org/818433002/diff/140001/util/file/file_io.h File util/file/file_io.h (right): https://codereview.chromium.org/818433002/diff/140001/util/file/file_io.h#new... util/file/file_io.h:54: kTruncate, Oh, also, these names should be something like kFileWriteModeTruncate. Since they’re just in namespace crashpad and not in a class or anything, they should be a little more descriptive, because they’ll be used throughout the namespace just by their bare names. Or maybe this is a good application for this codebase’s very first “enum class”!
https://codereview.chromium.org/818433002/diff/140001/util/file/file_io.h File util/file/file_io.h (right): https://codereview.chromium.org/818433002/diff/140001/util/file/file_io.h#new... util/file/file_io.h:39: //! \brief Determines the mode that LoggingOpenFileForWrite uses. On 2014/12/18 20:24:59, Mark Mentovai wrote: > () on function names. Done. https://codereview.chromium.org/818433002/diff/140001/util/file/file_io.h#new... util/file/file_io.h:41: //! \brief Opens the file if it exists and seeks to the end of the file, or On 2014/12/18 20:24:59, Mark Mentovai wrote: > Meh, I don’t know about having this actually do the seek. OK, removed that part. (Next CL will probably be LoggingSeekFile, now that I think about it...) https://codereview.chromium.org/818433002/diff/140001/util/file/file_io.h#new... util/file/file_io.h:46: //! overwritten. On 2014/12/18 20:24:59, Mark Mentovai wrote: > Overwritten? What’s the difference between kCreate and kTruncate? I don’t think > we can maintain that distinction on POSIX. I meant for kTruncate to be open-but-must-already-exist. I was vaguely thinking of some sort of temp file being created elsewhere and having the name passed to here. But as we don't have a need, I'll remove it. https://codereview.chromium.org/818433002/diff/140001/util/file/file_io.h#new... util/file/file_io.h:54: kTruncate, On 2014/12/18 20:27:34, Mark Mentovai wrote: > Oh, also, these names should be something like kFileWriteModeTruncate. Since > they’re just in namespace crashpad and not in a class or anything, they should > be a little more descriptive, because they’ll be used throughout the namespace > just by their bare names. > > Or maybe this is a good application for this codebase’s very first “enum class”! I'll go with enum class then for the excitement and glory! https://codereview.chromium.org/818433002/diff/140001/util/file/file_io.h#new... util/file/file_io.h:55: }; On 2014/12/18 20:24:59, Mark Mentovai wrote: > Everything here has the potential to create a file. The three options I see are: > > - require that the file be created > - allow the file to exist but discard existing contents (whether it’s via > truncation or replacing it > - allow the file to exist and retain existing contents That sounds right after removing kTruncate. So the first is kCreateNew, the second is kCreate, and the third kAppend. Better name suggestions? https://codereview.chromium.org/818433002/diff/140001/util/file/file_io.h#new... util/file/file_io.h:150: //! \a write_mode determines the style (clobber, truncate, etc.) that is used On 2014/12/18 20:24:59, Mark Mentovai wrote: > Normally parameters are done with \param[in] style but the other functions in > here don’t have that, so this is OK. OK. LMK if you want to change them all and I can do it in a separate CL. (Unrelatedly, are the "!" necessary in the comment blocks? Vim (or my config of Vim?) doesn't deal well with them which make them tedious to edit.) https://codereview.chromium.org/818433002/diff/140001/util/file/file_io.h#new... util/file/file_io.h:152: //! be set to 0644, otherwise, 0600. On Windows, the file is always opened in On 2014/12/18 20:24:59, Mark Mentovai wrote: > “set to” is not totally correct, because the umask will still modify the bits. Attempted improvement. https://codereview.chromium.org/818433002/diff/140001/util/file/file_io.h#new... util/file/file_io.h:152: //! be set to 0644, otherwise, 0600. On Windows, the file is always opened in On 2014/12/18 20:24:59, Mark Mentovai wrote: > Numeric literals also get the `backtick` treatment. Done.
https://codereview.chromium.org/818433002/diff/140001/util/file/file_io.h File util/file/file_io.h (right): https://codereview.chromium.org/818433002/diff/140001/util/file/file_io.h#new... util/file/file_io.h:150: //! \a write_mode determines the style (clobber, truncate, etc.) that is used On 2014/12/18 21:18:54, scottmg wrote: > On 2014/12/18 20:24:59, Mark Mentovai wrote: > > Normally parameters are done with \param[in] style but the other functions in > > here don’t have that, so this is OK. > > OK. LMK if you want to change them all and I can do it in a separate CL. > (Unrelatedly, are the "!" necessary in the comment blocks? Vim (or my config of > Vim?) doesn't deal well with them which make them tedious to edit.) Answering my own parenthetical, http://www.stack.nl/~dimitri/doxygen/manual/docblocks.html yes, either //! or ///. Vim doesn't do seem to do well with either, oh well.
On 2014/12/18 21:36:45, scottmg wrote: > https://codereview.chromium.org/818433002/diff/140001/util/file/file_io.h > File util/file/file_io.h (right): > > https://codereview.chromium.org/818433002/diff/140001/util/file/file_io.h#new... > util/file/file_io.h:150: //! \a write_mode determines the style (clobber, > truncate, etc.) that is used > On 2014/12/18 21:18:54, scottmg wrote: > > On 2014/12/18 20:24:59, Mark Mentovai wrote: > > > Normally parameters are done with \param[in] style but the other functions > in > > > here don’t have that, so this is OK. > > > > OK. LMK if you want to change them all and I can do it in a separate CL. > > (Unrelatedly, are the "!" necessary in the comment blocks? Vim (or my config > of > > Vim?) doesn't deal well with them which make them tedious to edit.) > > Answering my own parenthetical, > http://www.stack.nl/~dimitri/doxygen/manual/docblocks.html yes, either //! or > ///. Vim doesn't do seem to do well with either, oh well. And, continuing my monologue, here's the silly magic to make it at least 'gq' work. au BufRead,BufNewFile,BufEnter *.c,*.cc,*.cpp,*.h set comments=sO:*\ -,mO:*\ \ ,exO:*/,s1:/*,mb:*,ex:*/,bO://!,O://
https://codereview.chromium.org/818433002/diff/140001/util/file/file_io.h File util/file/file_io.h (right): https://codereview.chromium.org/818433002/diff/140001/util/file/file_io.h#new... util/file/file_io.h:46: //! overwritten. scottmg wrote: > I meant for kTruncate to be open-but-must-already-exist. I was vaguely thinking > of some sort of temp file being created elsewhere and having the name passed to > here. But as we don't have a need, I'll remove it. Hopefully if we have a need for that, we’ll be able to pass the FileHandle instead of or alongside the path. https://codereview.chromium.org/818433002/diff/140001/util/file/file_io.h#new... util/file/file_io.h:150: //! \a write_mode determines the style (clobber, truncate, etc.) that is used scottmg wrote: > OK. LMK if you want to change them all and I can do it in a separate CL. No rush, but it’d be good to be consistent with the rest of the documentation. I think we wound up in this state because sometimes, when I have functions that just wrap others, there’s not much point in documenting the parameters individually, so I just leave it out. But some of these, at least, are no longer straight wrappers. > (Unrelatedly, are the "!" necessary in the comment blocks? Vim (or my config of > Vim?) doesn't deal well with them which make them tedious to edit.) There are a few different comment styles for Doxygen. See http://www.doxygen.org/manual/docblocks.html. Feel free to experiment with the other alternatives, maybe one of them works better with editors, and we can do a mass scripted rewrite. But in my experience, the other alternatives typically work as poorly as //!, and the advantage of //! over /// is that it’s more immediately visible as distinct from //. It’s possible to change many editors’ ideas of what a comment looks like, and sometimes it’s possible to tell an editor that //! is distinct from // and takes precedence. https://codereview.chromium.org/818433002/diff/180001/util/file/file_io.h File util/file/file_io.h (right): https://codereview.chromium.org/818433002/diff/180001/util/file/file_io.h#new... util/file/file_io.h:42: kAppend, kAppend still sounds like it seeks to the end… How about: kReuseOrCreate kTruncateOrCreate kCreateOrFail so that everything specifies a conceptual operation and its fallback. I also realize that the one thing all three of these have in common is that they can create, and I’m almost of the mind that we can name the enum FileCreateMode and then have kReuse, kTruncate, and k…? It falls apart because there’s no great name for the third one, and if you saw FileCreateMode::kReuse in code, you still might erroneously think that it means “don’t create, just reuse.” https://codereview.chromium.org/818433002/diff/180001/util/file/file_io.h#new... util/file/file_io.h:145: //! \a write_mode determines the style (clobber, truncate, etc.) that is used clobber == truncate, right? https://codereview.chromium.org/818433002/diff/180001/util/file/file_io.h#new... util/file/file_io.h:147: //! `mode` permissions bits for `open()`, otherwise `0600` will be used. On Yup, score! https://codereview.chromium.org/818433002/diff/180001/util/file/file_io_posix.cc File util/file/file_io_posix.cc (right): https://codereview.chromium.org/818433002/diff/180001/util/file/file_io_posix... util/file/file_io_posix.cc:82: int flags = O_WRONLY; This should be O_WRONLY | O_CREAT, because we always want to create the file even if it doesn’t exist. The cases below should adapt accordingly.
https://codereview.chromium.org/818433002/diff/180001/util/file/file_io.h File util/file/file_io.h (right): https://codereview.chromium.org/818433002/diff/180001/util/file/file_io.h#new... util/file/file_io.h:42: kAppend, On 2014/12/18 21:52:32, Mark Mentovai wrote: > kAppend still sounds like it seeks to the end… > > How about: > > kReuseOrCreate > kTruncateOrCreate > kCreateOrFail > > so that everything specifies a conceptual operation and its fallback. > > I also realize that the one thing all three of these have in common is that they > can create, and I’m almost of the mind that we can name the enum FileCreateMode > and then have kReuse, kTruncate, and k…? It falls apart because there’s no great > name for the third one, and if you saw FileCreateMode::kReuse in code, you still > might erroneously think that it means “don’t create, just reuse.” "Reuse" sounds a bit funny to me, but I can't think of a better one. kOpenOrCreate sort of works, but since it's for a function that is just doing opens, doesn't clear things up. Done as above. https://codereview.chromium.org/818433002/diff/180001/util/file/file_io.h#new... util/file/file_io.h:145: //! \a write_mode determines the style (clobber, truncate, etc.) that is used On 2014/12/18 21:52:32, Mark Mentovai wrote: > clobber == truncate, right? Done. https://codereview.chromium.org/818433002/diff/180001/util/file/file_io_posix.cc File util/file/file_io_posix.cc (right): https://codereview.chromium.org/818433002/diff/180001/util/file/file_io_posix... util/file/file_io_posix.cc:82: int flags = O_WRONLY; On 2014/12/18 21:52:32, Mark Mentovai wrote: > This should be O_WRONLY | O_CREAT, because we always want to create the file > even if it doesn’t exist. The cases below should adapt accordingly. Done.
LGTM! and I was hoping we’d get to COM today ;) https://codereview.chromium.org/818433002/diff/220001/util/file/file_io.h File util/file/file_io.h (right): https://codereview.chromium.org/818433002/diff/220001/util/file/file_io.h#new... util/file/file_io.h:33: //! \brief Platform-specific alias for a low level file handle. nit: low-level https://codereview.chromium.org/818433002/diff/220001/util/file/file_io.h#new... util/file/file_io.h:42: kReuseOrCreate, I agree about “reuse” but I couldn’t come up with anything better either. https://codereview.chromium.org/818433002/diff/220001/util/file/file_io.h#new... util/file/file_io.h:45: //! overwritten. Hanging indents are 4 spaces. Same on the functions below. We used to align like this but it got really bad with long parameter names. https://codereview.chromium.org/818433002/diff/220001/util/file/file_io.h#new... util/file/file_io.h:160: //! \brief Wraps `open()` or `CreateFile()`, opening an existing file for Nit: this file puts read ops before write ops, so swap the order of these two, and update the .cc files to match.
Thanks! (stepping out for a bit, will land when I'm back to watch it) https://codereview.chromium.org/818433002/diff/220001/util/file/file_io.h File util/file/file_io.h (right): https://codereview.chromium.org/818433002/diff/220001/util/file/file_io.h#new... util/file/file_io.h:33: //! \brief Platform-specific alias for a low level file handle. On 2014/12/18 22:20:59, Mark Mentovai wrote: > nit: low-level Done. https://codereview.chromium.org/818433002/diff/220001/util/file/file_io.h#new... util/file/file_io.h:45: //! overwritten. On 2014/12/18 22:20:58, Mark Mentovai wrote: > Hanging indents are 4 spaces. Same on the functions below. > > We used to align like this but it got really bad with long parameter names. Done. https://codereview.chromium.org/818433002/diff/220001/util/file/file_io.h#new... util/file/file_io.h:160: //! \brief Wraps `open()` or `CreateFile()`, opening an existing file for On 2014/12/18 22:20:59, Mark Mentovai wrote: > Nit: this file puts read ops before write ops, so swap the order of these two, > and update the .cc files to match. Done.
Message was sent while issue was closed.
Committed patchset #8 (id:240001) manually as 4034d30023dcc3c1151edcad32f1e840c0314c15 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
