Use LockFile/UnlockFile for Settings to port to Windows

Use LockFile/UnlockFile for Settings to port to Windows

Adds LoggingOpenFileForReadAndWrite, LoggingTruncateFile, and UUID::GenerateFromSystem
in support.

R=mark@chromium.org, rsesek@chromium.org
BUG=crashpad:13
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/8272a4cefe5cbcf25f3f24601c104b97b9ada9d0

[scottmg, 2015-03-27 00:10:22]

scottmg@chromium.org changed reviewers:
+ mark@chromium.org, rsesek@chromium.org

[scottmg, 2015-04-01 00:49:14]

This CL has been dragging on forever 5 minutes at a time so I wanted to get your
feedback on Settings::Initialize. The tests pass this way, but that doesn't
necessarily mean too much for this sort of code.

https://codereview.chromium.org/999953002/diff/180001/client/settings.cc
File client/settings.cc (left):

https://codereview.chromium.org/999953002/diff/180001/client/settings.cc#oldc...
client/settings.cc:64: ScopedFileHandle handle(HANDLE_EINTR(
I think the/a case that has changed here:

- X enters, attempts open, succeeds because the file didn't exist.
- Y enters, attempts open, fails because the file does exist.
- Y continues down to the "normal" case, opens for RW.
- Now they're racing for the lock.
- If X locks, all is well as it will initialize, release, and Y continues
- if Y locks, then ReadSettings will fail, causing RecoverSettings to
InitializeSettings (which is what we want), except after unlock, it'll happen
again on X which is a bit weird.

Which makes me think that simply deleting the "initialize" path would be
equivalent.

But, I'm not sure if I understand the subtlety of atomicity of O_EXLOCK or the
various conditions, so please correct me if I'm way off here.

[Robert Sesek, 2015-04-01 15:08:26]

https://codereview.chromium.org/999953002/diff/180001/client/settings.cc
File client/settings.cc (left):

https://codereview.chromium.org/999953002/diff/180001/client/settings.cc#oldc...
client/settings.cc:64: ScopedFileHandle handle(HANDLE_EINTR(
On 2015/04/01 00:49:14, scottmg wrote:
> I think the/a case that has changed here:
> 
> - X enters, attempts open, succeeds because the file didn't exist.
> - Y enters, attempts open, fails because the file does exist.
> - Y continues down to the "normal" case, opens for RW.
> - Now they're racing for the lock.
> - If X locks, all is well as it will initialize, release, and Y continues
> - if Y locks, then ReadSettings will fail, causing RecoverSettings to
> InitializeSettings (which is what we want), except after unlock, it'll happen
> again on X which is a bit weird.

I think RecoverSettings won't call InitializeSettings again, since it takes the
O_EXLOCK and then tries to read them again, to prevent the double-recovery case.

> Which makes me think that simply deleting the "initialize" path would be
> equivalent.

Yes, it could be. Non-existent file is technically just another error/recovery
condition, so I'd be OK with removing the entire Initialize step I think. The
only issue is that then when creating the database for the first time, there
will be some inaccurate (or misleading) log messages saying that we're trying to
"recover" the settings file.

> But, I'm not sure if I understand the subtlety of atomicity of O_EXLOCK or the
> various conditions, so please correct me if I'm way off here.

Yes, this is the race condition I mentioned on the bug.

https://codereview.chromium.org/999953002/diff/200001/client/settings.cc
File client/settings.cc (right):

https://codereview.chromium.org/999953002/diff/200001/client/settings.cc#newc...
client/settings.cc:162: void Settings::ScopedLockedFileHandle::reset() {
nit: Implementation order should match header order.

https://codereview.chromium.org/999953002/diff/200001/client/settings.h
File client/settings.h (right):

https://codereview.chromium.org/999953002/diff/200001/client/settings.h#newco...
client/settings.h:99: class ScopedLockedFileHandle {
Can you use ScopedGeneric for this? It should handle the rvalue stuff, at least.

https://codereview.chromium.org/999953002/diff/200001/client/settings_test.cc
File client/settings_test.cc (right):

https://codereview.chromium.org/999953002/diff/200001/client/settings_test.cc...
client/settings_test.cc:156: EXPECT_EQ(0,
_wunlink(settings_path().value().c_str()));
Log the message, like for POSIX?

https://codereview.chromium.org/999953002/diff/200001/util/misc/uuid.cc
File util/misc/uuid.cc (right):

https://codereview.chromium.org/999953002/diff/200001/util/misc/uuid.cc#newco...
util/misc/uuid.cc:142: #endif  // OS_MACOSX
#else
#error "This function needs to be ported"

?

[scottmg, 2015-04-01 19:32:11]

Thanks.

I changed RecoverSettings() to simply say "Initializing..." as it's not easily
detectable that the file was newly created.

https://codereview.chromium.org/999953002/diff/200001/client/settings.cc
File client/settings.cc (right):

https://codereview.chromium.org/999953002/diff/200001/client/settings.cc#newc...
client/settings.cc:162: void Settings::ScopedLockedFileHandle::reset() {
On 2015/04/01 15:08:26, Robert Sesek wrote:
> nit: Implementation order should match header order.

Removed.

https://codereview.chromium.org/999953002/diff/200001/client/settings.h
File client/settings.h (right):

https://codereview.chromium.org/999953002/diff/200001/client/settings.h#newco...
client/settings.h:99: class ScopedLockedFileHandle {
On 2015/04/01 15:08:26, Robert Sesek wrote:
> Can you use ScopedGeneric for this? It should handle the rvalue stuff, at
least.

Done.

https://codereview.chromium.org/999953002/diff/200001/client/settings_test.cc
File client/settings_test.cc (right):

https://codereview.chromium.org/999953002/diff/200001/client/settings_test.cc...
client/settings_test.cc:156: EXPECT_EQ(0,
_wunlink(settings_path().value().c_str()));
On 2015/04/01 15:08:26, Robert Sesek wrote:
> Log the message, like for POSIX?

Done.

https://codereview.chromium.org/999953002/diff/200001/util/misc/uuid.cc
File util/misc/uuid.cc (right):

https://codereview.chromium.org/999953002/diff/200001/util/misc/uuid.cc#newco...
util/misc/uuid.cc:142: #endif  // OS_MACOSX
On 2015/04/01 15:08:26, Robert Sesek wrote:
> #else
> #error "This function needs to be ported"
> 
> ?

I figured it just wouldn't compile, but sure.

[Mark Mentovai, 2015-04-01 21:15:19]

Not a complete review, just something that I spotted.

https://codereview.chromium.org/999953002/diff/220001/util/misc/uuid.h
File util/misc/uuid.h (right):

https://codereview.chromium.org/999953002/diff/220001/util/misc/uuid.h#newcode88
util/misc/uuid.h:88: static bool GenerateFromSystem(UUID* into);
Thank you for doing this! It’s been on my list catch-up cleanups to do.

I don’t see why this should be a static method. We should have a non-static
InitializeWithNew() or something that does this.

We may also want a constructor mode that does InitializeWithNew() instead of
giving a zero UUID.

[scottmg, 2015-04-01 21:51:37]

https://codereview.chromium.org/999953002/diff/220001/util/misc/uuid.h
File util/misc/uuid.h (right):

https://codereview.chromium.org/999953002/diff/220001/util/misc/uuid.h#newcode88
util/misc/uuid.h:88: static bool GenerateFromSystem(UUID* into);
On 2015/04/01 21:15:19, Mark Mentovai wrote:
> Thank you for doing this! It’s been on my list catch-up cleanups to do.
> 
> I don’t see why this should be a static method. We should have a non-static
> InitializeWithNew() or something that does this.
> 
> We may also want a constructor mode that does InitializeWithNew() instead of
> giving a zero UUID.

Done. I didn't add the constructor mode as UuidCreate has a failure mode on
Windows, so the caller should deal with that.

[Mark Mentovai, 2015-04-17 21:32:44]

This does look correct. I’m very sorry that this fell to the bottom of my inbox.

I’ll LG this unless Robert also wants to take another loko.

https://codereview.chromium.org/999953002/diff/260001/client/settings_test.cc
File client/settings_test.cc (right):

https://codereview.chromium.org/999953002/diff/260001/client/settings_test.cc...
client/settings_test.cc:157: << ErrorMessage("_wunlink");
ErrorMessage() is for GetLastError(). Does _wunlink() guarantee that
GetLastError() will be updated if it fails?

https://codereview.chromium.org/999953002/diff/260001/util/misc/uuid.h
File util/misc/uuid.h (right):

https://codereview.chromium.org/999953002/diff/260001/util/misc/uuid.h#newcode41
util/misc/uuid.h:41: //! \brief Initializes the %UUID to zero.
The constructor form would be nice because it avoids having to zero it out
followed by populating it, but it’s not a must-have.

[Robert Sesek, 2015-04-17 21:33:36]

Oops I had unsent comments!

https://codereview.chromium.org/999953002/diff/260001/client/settings.cc
File client/settings.cc (right):

https://codereview.chromium.org/999953002/diff/260001/client/settings.cc#newc...
client/settings.cc:37: CheckedCloseFile(handle);
Shouldn't this be protected with the condition too?

https://codereview.chromium.org/999953002/diff/260001/client/settings.h
File client/settings.h (right):

https://codereview.chromium.org/999953002/diff/260001/client/settings.h#newco...
client/settings.h:24: #include "base/move.h"
Needed?

[scottmg, 2015-04-20 18:20:41]

Thanks

https://codereview.chromium.org/999953002/diff/260001/client/settings.cc
File client/settings.cc (right):

https://codereview.chromium.org/999953002/diff/260001/client/settings.cc#newc...
client/settings.cc:37: CheckedCloseFile(handle);
On 2015/04/17 21:33:36, Robert Sesek wrote:
> Shouldn't this be protected with the condition too?

Done.

https://codereview.chromium.org/999953002/diff/260001/client/settings.h
File client/settings.h (right):

https://codereview.chromium.org/999953002/diff/260001/client/settings.h#newco...
client/settings.h:24: #include "base/move.h"
On 2015/04/17 21:33:36, Robert Sesek wrote:
> Needed?

Done.

https://codereview.chromium.org/999953002/diff/260001/client/settings_test.cc
File client/settings_test.cc (right):

https://codereview.chromium.org/999953002/diff/260001/client/settings_test.cc...
client/settings_test.cc:157: << ErrorMessage("_wunlink");
On 2015/04/17 21:32:44, Mark Mentovai wrote:
> ErrorMessage() is for GetLastError(). Does _wunlink() guarantee that
> GetLastError() will be updated if it fails?

Oops, no it's errno of course. Fixed.

https://codereview.chromium.org/999953002/diff/260001/util/misc/uuid.h
File util/misc/uuid.h (right):

https://codereview.chromium.org/999953002/diff/260001/util/misc/uuid.h#newcode41
util/misc/uuid.h:41: //! \brief Initializes the %UUID to zero.
On 2015/04/17 21:32:44, Mark Mentovai wrote:
> The constructor form would be nice because it avoids having to zero it out
> followed by populating it, but it’s not a must-have.

OK, added. I made it CHECKing for Windows' potential failure as having it
initialize to zeros seems like a bad failure case.

[Mark Mentovai, 2015-04-20 20:45:08]

https://codereview.chromium.org/999953002/diff/280001/client/settings.cc
File client/settings.cc (right):

https://codereview.chromium.org/999953002/diff/280001/client/settings.cc#newc...
client/settings.cc:249: LOG(INFO) << "Initializing settings file " <<
file_path().value().c_str();
Now we’ll get this spam every time a new Settings file is created?

https://codereview.chromium.org/999953002/diff/280001/util/misc/uuid.h
File util/misc/uuid.h (right):

https://codereview.chromium.org/999953002/diff/280001/util/misc/uuid.h#newcode32
util/misc/uuid.h:32: enum class InitializeWithNewTag {
Move into struct UUID?

[scottmg, 2015-04-20 21:11:28]

https://codereview.chromium.org/999953002/diff/280001/client/settings.cc
File client/settings.cc (right):

https://codereview.chromium.org/999953002/diff/280001/client/settings.cc#newc...
client/settings.cc:249: LOG(INFO) << "Initializing settings file " <<
file_path().value().c_str();
On 2015/04/20 20:45:08, Mark Mentovai wrote:
> Now we’ll get this spam every time a new Settings file is created?

Removed. I don't think there's a good way to only log in the true recovery case.

https://codereview.chromium.org/999953002/diff/280001/util/misc/uuid.h
File util/misc/uuid.h (right):

https://codereview.chromium.org/999953002/diff/280001/util/misc/uuid.h#newcode32
util/misc/uuid.h:32: enum class InitializeWithNewTag {
On 2015/04/20 20:45:08, Mark Mentovai wrote:
> Move into struct UUID?

Done.

[Robert Sesek, 2015-04-20 21:17:48]

LGTM

[Mark Mentovai, 2015-04-20 21:17:53]

https://codereview.chromium.org/999953002/diff/280001/client/settings.cc
File client/settings.cc (right):

https://codereview.chromium.org/999953002/diff/280001/client/settings.cc#newc...
client/settings.cc:249: LOG(INFO) << "Initializing settings file " <<
file_path().value().c_str();
scottmg wrote:
> On 2015/04/20 20:45:08, Mark Mentovai wrote:
> > Now we’ll get this spam every time a new Settings file is created?
> 
> Removed. I don't think there's a good way to only log in the true recovery
case.

It’d take some extra parameter-passing that would probably be confusing. I don’t
insist on this. Robert might have an opinion.

[Mark Mentovai, 2015-04-20 21:18:26]

If there are no more changes, LGTM from me too. Thanks!

[Robert Sesek, 2015-04-20 21:18:32]

https://codereview.chromium.org/999953002/diff/280001/client/settings.cc
File client/settings.cc (right):

https://codereview.chromium.org/999953002/diff/280001/client/settings.cc#newc...
client/settings.cc:249: LOG(INFO) << "Initializing settings file " <<
file_path().value().c_str();
On 2015/04/20 21:17:53, Mark Mentovai wrote:
> scottmg wrote:
> > On 2015/04/20 20:45:08, Mark Mentovai wrote:
> > > Now we’ll get this spam every time a new Settings file is created?
> > 
> > Removed. I don't think there's a good way to only log in the true recovery
> case.
> 
> It’d take some extra parameter-passing that would probably be confusing. I
don’t
> insist on this. Robert might have an opinion.

No, we talked about this earlier in the review. I think it's fine to just not
log.

[scottmg, 2015-04-20 21:21:54]

Committed patchset #16 (id:300001) manually as
8272a4cefe5cbcf25f3f24601c104b97b9ada9d0 (presubmit successful).