crashpad: Introduce a read-only mode for crashpad databases and settings [crashpad/crashpad : main]
Joshua Pawlicki (Gerrit)
Joshua Pawlicki voted and added 3 comments![]( "Open in Gerrit")
Votes added by Joshua Pawlicki
| Commit-Queue | +1 |
3 comments
Mark, PTAL.
On one hand, this wound up being not that bad.
On the other hand, as I progressed, I realized that really the databases can barely do anything at all in read-only mode; _generic is probably the worst, since it will do things like cleaning the database during a read operation if the report data is corrupt. Basically the only thing that is left is getting a (read-only) settings object, at which point maybe it's more sensible to just create a GetReadOnlySettings method on the existing objects (or as a static method, to dodge the directory creation issue mentioned in a comment below).
//! not yet exist, it will not be created. Note that for databases
//! implemented as directory structures, existence refers to the existence
//! of both the outermost directory and the inner directory structure.This was easier to implement and might be what the users of this function really want, but it is an API change, so flagging that. If we don't want to take that change, then each of the Initialize functions could choose a pair of ensure/create directory functions (one for the outer, one for the inner), or maybe you have a better idea.
CrashReportDatabaseMac::CrashReportDatabaseMac(const base::FilePath& path,TODO: add test coverage of all the database operations in a read-only mode, after we re-sync on whether this is the right direction in the first place.
Related details
- Mark Mentovai
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 7 comments![]( "Open in Gerrit")
Mark, PTAL.
On one hand, this wound up being not that bad.
On the other hand, as I progressed, I realized that really the databases can barely do anything at all in read-only mode; _generic is probably the worst, since it will do things like cleaning the database during a read operation if the report data is corrupt. Basically the only thing that is left is getting a (read-only) settings object, at which point maybe it's more sensible to just create a GetReadOnlySettings method on the existing objects (or as a static method, to dodge the directory creation issue mentioned in a comment below).
Mark, PTAL.
On one hand, this wound up being not that bad.
On the other hand, as I progressed, I realized that really the databases can barely do anything at all in read-only mode; _generic is probably the worst, since it will do things like cleaning the database during a read operation if the report data is corrupt. Basically the only thing that is left is getting a (read-only) settings object, at which point maybe it's more sensible to just create a GetReadOnlySettings method on the existing objects (or as a static method, to dodge the directory creation issue mentioned in a comment below).
I feel this concern too. I had the same thought as I was reading through it. Something like
`static CrashReportDatabase::GetReadOnlySettingsForDatabase(const FilePath &);`
?
The “databases can barely do anything at all in read-only mode; _generic is probably the worst” is what tipped this in my mind.
//! not yet exist, it will not be created. Note that for databases
//! implemented as directory structures, existence refers to the existence
//! of both the outermost directory and the inner directory structure.This was easier to implement and might be what the users of this function really want, but it is an API change, so flagging that. If we don't want to take that change, then each of the Initialize functions could choose a pair of ensure/create directory functions (one for the outer, one for the inner), or maybe you have a better idea.
This was easier to implement and might be what the users of this function really want, but it is an API change, so flagging that. If we don't want to take that change, then each of the Initialize functions could choose a pair of ensure/create directory functions (one for the outer, one for the inner), or maybe you have a better idea.
If we’re going to do the `static GetReadOnlySettingsForDatabase` thing, I think we leave this alone _in this change_. You may be right and we may still want to make the change, but at that point it’d be fully decoupled from this and we’d be best off considering it in isolation.
const bool read_only_;`bool` goes toward the end for better struct packing. Here, it would go after `settings_init_`. Applies to all CrashReportDatabase implementations.
return false;Gotta `LOG(ERROR)` this and all others to adhere to the documented behavior.
return MakeScopedLockedFileHandle(options, FileLocking::kShared, file_path());In the context of what we’re trying to fix, the user that runs this can still `flock(…, LOCK_SH)` any file that it has access to?
ScopedLockedFileHandle handle;I would move the `read_only` bar and associated `LOG(ERROR)` here. This will cover the two setters that we have now, and also be much more likely to catch future setters.
Related details
- Joshua Pawlicki
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
Related details
- Joshua Pawlicki
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |