Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1189203003:
Add UMA for consistency between TPM and install attributes. (Closed)
|
Created:
5 years, 6 months ago by Thiemo Nagel Modified:
5 years, 5 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org, stevenjb+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master3 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd UMA for consistency between TPM and install attributes.
BUG=503941
TBR=stevenjb@chromium.org (for fake_cryptohome_client.cc)
Committed: https://crrev.com/9ad56f55bb18f48ecced91751c0d9648c203451a
Cr-Commit-Position: refs/heads/master@{#336348}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Improved version. #
Total comments: 30
Patch Set 3 : Address comments. #Patch Set 4 : Simplified after comments by Mattias. #
Total comments: 26
Patch Set 5 : Applied more comments, prevent concurrent TPM locking through multiple LockDevice() calls. #
Total comments: 6
Patch Set 6 : Address more comments. #
Total comments: 10
Patch Set 7 : Address nit. #Patch Set 8 : Cosmetics and compilation fix. #Patch Set 9 : Rebase. #Patch Set 10 : More compilation fixes. #Patch Set 11 : Fix consistency check and device lock interplay. #Patch Set 12 : Fix more tests. #
Messages
Total messages: 52 (20 generated)
tnagel@chromium.org changed reviewers: + mnissler@chromium.org
Hi Mattias, my I kindly ask you to take a look at my first draft for enrollment consistency UMA? Barring style, formatting, comments, etc. does the approach look remotely reasonable to you? Note that the histogram built from 2bit x 1bit x 1bit x 1bit combinatorics has some impossible combinations, but I'm not sure whether it would make sense to give up the combinatorial layout in favour of enumerating only the possible combinations instead. (After all we're looking for impossible combinations. It might be error prone to reason about which combinations are just plain impossible and which are really, truly, absolutely impossible.) What do you think? Thiemo
https://codereview.chromium.org/1189203003/diff/1/chrome/browser/chromeos/pol... File chrome/browser/chromeos/policy/enterprise_install_attributes.cc (left): https://codereview.chromium.org/1189203003/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:71: device_locked_ = true; You're changing semantics here. If the file is present, we should assume the device is locked, even if we fail to make sense of the file contents. https://codereview.chromium.org/1189203003/diff/1/chrome/browser/chromeos/pol... File chrome/browser/chromeos/policy/enterprise_install_attributes.cc (right): https://codereview.chromium.org/1189203003/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:336: void EnterpriseInstallAttributes::CheckConsistency() { The code you have here purely tests whether the crypthome DBus interface supplies consistent values. I don't think that TpmIsOwned is of any relevance though - it's merely an artifact of how install attributes are implemented in cryptohome. I think what you really want to test is whether device ownership agrees with install attributes. Status of the former can be obtained via DeviceSettingsService. https://codereview.chromium.org/1189203003/diff/1/chrome/browser/chromeos/pol... File chrome/browser/chromeos/policy/enterprise_install_attributes.h (right): https://codereview.chromium.org/1189203003/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/enterprise_install_attributes.h:63: // during the boot process to work around slow cryptohome startup, which takes nit: Your edit makes this sentence ambiguous. Now it sounds like the "early" aspect of cache file creation works around slow cryptohome startup, whereas the original meaning was that using a cache file in the first place is only necessary to address the crypthome slowness.
Thank you for your comments, Mattias! The new upload is tested and polished. Could you please take another look? https://codereview.chromium.org/1189203003/diff/1/chrome/browser/chromeos/pol... File chrome/browser/chromeos/policy/enterprise_install_attributes.cc (left): https://codereview.chromium.org/1189203003/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:71: device_locked_ = true; On 2015/06/19 07:32:45, Mattias Nissler wrote: > You're changing semantics here. If the file is present, we should assume the > device is locked, even if we fail to make sense of the file contents. That was intentional, but I forgot the intention behind it. I guess I'll change it back. (I don't think it matters a lot.) https://codereview.chromium.org/1189203003/diff/1/chrome/browser/chromeos/pol... File chrome/browser/chromeos/policy/enterprise_install_attributes.cc (right): https://codereview.chromium.org/1189203003/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:336: void EnterpriseInstallAttributes::CheckConsistency() { On 2015/06/19 07:32:45, Mattias Nissler wrote: > The code you have here purely tests whether the crypthome DBus interface > supplies consistent values. I don't think that TpmIsOwned is of any relevance > though - it's merely an artifact of how install attributes are implemented in > cryptohome. As far as I can see, TpmIsOwned() calls through to the TPM. In my tests, I can observe the combination of install attributes missing but TPM reporting as locked. https://codereview.chromium.org/1189203003/diff/1/chrome/browser/chromeos/pol... File chrome/browser/chromeos/policy/enterprise_install_attributes.h (right): https://codereview.chromium.org/1189203003/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/enterprise_install_attributes.h:63: // during the boot process to work around slow cryptohome startup, which takes On 2015/06/19 07:32:45, Mattias Nissler wrote: > nit: Your edit makes this sentence ambiguous. Now it sounds like the "early" > aspect of cache file creation works around slow cryptohome startup, whereas the > original meaning was that using a cache file in the first place is only > necessary to address the crypthome slowness. Done.
https://codereview.chromium.org/1189203003/diff/1/chrome/browser/chromeos/pol... File chrome/browser/chromeos/policy/enterprise_install_attributes.cc (right): https://codereview.chromium.org/1189203003/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:336: void EnterpriseInstallAttributes::CheckConsistency() { On 2015/06/19 17:47:03, Thiemo Nagel wrote: > On 2015/06/19 07:32:45, Mattias Nissler wrote: > > The code you have here purely tests whether the crypthome DBus interface > > supplies consistent values. I don't think that TpmIsOwned is of any relevance > > though - it's merely an artifact of how install attributes are implemented in > > cryptohome. > > As far as I can see, TpmIsOwned() calls through to the TPM. In my tests, I can > observe the combination of install attributes missing but TPM reporting as > locked. I'm sure you can actually observe this situation. The question however is why this is relevant for enrollment? IIRC, the goal was to detect whether install attributes say we're enterprise while device ownership shows a consumer user? Maybe I'm missing something - just convince me that the condition you're detecting here is actually interesting to us.
On 2015/06/22 07:24:50, Mattias Nissler wrote: > https://codereview.chromium.org/1189203003/diff/1/chrome/browser/chromeos/pol... > File chrome/browser/chromeos/policy/enterprise_install_attributes.cc (right): > > https://codereview.chromium.org/1189203003/diff/1/chrome/browser/chromeos/pol... > chrome/browser/chromeos/policy/enterprise_install_attributes.cc:336: void > EnterpriseInstallAttributes::CheckConsistency() { > On 2015/06/19 17:47:03, Thiemo Nagel wrote: > > On 2015/06/19 07:32:45, Mattias Nissler wrote: > > > The code you have here purely tests whether the crypthome DBus interface > > > supplies consistent values. I don't think that TpmIsOwned is of any > relevance > > > though - it's merely an artifact of how install attributes are implemented > in > > > cryptohome. > > > > As far as I can see, TpmIsOwned() calls through to the TPM. In my tests, I > can > > observe the combination of install attributes missing but TPM reporting as > > locked. > > I'm sure you can actually observe this situation. The question however is why > this is relevant for enrollment? IIRC, the goal was to detect whether install > attributes say we're enterprise while device ownership shows a consumer user? > Maybe I'm missing something - just convince me that the condition you're > detecting here is actually interesting to us. For posterity: Offline discussions clarified that this CL is meant to detect the situation of stateful data loss, thus comparing TPM ownership state with install attributes presence on disk is a useful signal.
https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/enterprise_install_attributes.cc (right): https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:29: int kDbusRetryIntervalInSeconds = 2; 2 seems a bit too aggressive. How about 10? https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:65: dbus_tries_remaining_(30), 30 tries seems a bit excessive. How about 10? https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:78: lock_state_ = STATE_NOT_LOCKED; Note that the cache file will also be missing if lockbox-cache runs into an error. https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:298: lock_state_ = STATE_NOT_LOCKED; AFAICS, this should not update |lock_state_| - ReadImmutableAttributes might already have set STATE_LOCKED. Even though you might get a registration user mismatch here, that doesn't mean the device returns to STATE_NOT_LOCKED. https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:303: lock_state_ = STATE_LOCKED; Ditto - ReadImmutableAttributes should decide. https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:352: dbus_tries_remaining_--; How about binding this as a function parameter that you decrement before the retry? That way, you don't have to keep it around in class state. https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:361: base::HistogramBase::Sample state = lock_state_; It seems like the code is written in a way s.t. STATE_UNKNOWN and STATE_LOCKING are not going to show up at all. Except for the remote chance that TriggerConsistencyCheck executes in parallel to the lock operation. Given that, does it really make sense to introduce these two enum constants in the first place? It looks like the bit you want here is cache-file-present-or-not. You probably don't even want to handle the case of enrollment here, it's good enough to just report things for steady state, no? https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:362: state |= 0x4 * (registration_mode_ != DEVICE_MODE_ENTERPRISE); Why invert this bit? https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:363: if (call_status == chromeos::DBUS_METHOD_CALL_SUCCESS) { nit: no curlies for consistency with the rest of the file. https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/enterprise_install_attributes.h (right): https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.h:29: STATE_UNKNOWN = 0, // Not initialized, yet. suggestion: remove comma https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.h:110: void TriggerConsistencyCheck(); Does it make sense to trigger this externally? Might be simpler to just call this internally after ReadImmutableAttributes() and ReadCacheFile().
Thank you! Could you please take another look? https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/enterprise_install_attributes.cc (right): https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:29: int kDbusRetryIntervalInSeconds = 2; On 2015/06/23 11:34:09, Mattias Nissler wrote: > 2 seems a bit too aggressive. How about 10? I'd like to avoid querying the TPM while enrollment runs. 10 seconds seem to be sufficient to click through OOBE and start enrollment. Can we compromise on 5 seconds? https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:65: dbus_tries_remaining_(30), On 2015/06/23 11:34:08, Mattias Nissler wrote: > 30 tries seems a bit excessive. How about 10? Let's keep the discussion to kDbusRetryIntervalInSeconds and kDbusRetryDurationInSeconds. https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:78: lock_state_ = STATE_NOT_LOCKED; On 2015/06/23 11:34:09, Mattias Nissler wrote: > Note that the cache file will also be missing if lockbox-cache runs into an > error. Yes, this could happen. Setting STATE_NOT_LOCKED however seems to be best in line with the present behaviour of device_locked_ = false. https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:298: lock_state_ = STATE_NOT_LOCKED; On 2015/06/23 11:34:08, Mattias Nissler wrote: > AFAICS, this should not update |lock_state_| - ReadImmutableAttributes might > already have set STATE_LOCKED. Even though you might get a registration user > mismatch here, that doesn't mean the device returns to STATE_NOT_LOCKED. Thanks, I agree, this doesn't seem right. https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:303: lock_state_ = STATE_LOCKED; On 2015/06/23 11:34:09, Mattias Nissler wrote: > Ditto - ReadImmutableAttributes should decide. Ditto. https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:352: dbus_tries_remaining_--; On 2015/06/23 11:34:09, Mattias Nissler wrote: > How about binding this as a function parameter that you decrement before the > retry? That way, you don't have to keep it around in class state. I guess that would make sense only if TriggerConsistencyCheck() is made private. (But if we make it private, I'm happy to go that way.) https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:361: base::HistogramBase::Sample state = lock_state_; On 2015/06/23 11:34:09, Mattias Nissler wrote: > It seems like the code is written in a way s.t. STATE_UNKNOWN and STATE_LOCKING > are not going to show up at all. Except for the remote chance that > TriggerConsistencyCheck executes in parallel to the lock operation. > > Given that, does it really make sense to introduce these two enum constants in > the first place? > > It looks like the bit you want here is cache-file-present-or-not. You probably > don't even want to handle the case of enrollment here, it's good enough to just > report things for steady state, no? I guess I need STATE_PENDING to catch the case in which the consistency check is executed while enrollment takes place. Since I was already adding another state, I felt it made sense to differentiate between NOT_LOCKED and not initialized. Maybe the real problem here is the clumsy semantics of having to call ReadCacheFile() after the constructor has run. Probably it would be best to avoid STATE_UNKNOWN by having the constructor call ReadCacheFile() and to inject the cache file name as a constructor parameter. Wdyt? https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:362: state |= 0x4 * (registration_mode_ != DEVICE_MODE_ENTERPRISE); On 2015/06/23 11:34:09, Mattias Nissler wrote: > Why invert this bit? That seeped through from an earlier implementation. Fixed. https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:363: if (call_status == chromeos::DBUS_METHOD_CALL_SUCCESS) { On 2015/06/23 11:34:09, Mattias Nissler wrote: > nit: no curlies for consistency with the rest of the file. Done. https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/enterprise_install_attributes.h (right): https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.h:29: STATE_UNKNOWN = 0, // Not initialized, yet. On 2015/06/23 11:34:09, Mattias Nissler wrote: > suggestion: remove comma Done. (I was told in school that a comma was required, but apparently my teacher lied.) https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.h:110: void TriggerConsistencyCheck(); On 2015/06/23 11:34:09, Mattias Nissler wrote: > Does it make sense to trigger this externally? Might be simpler to just call > this internally after ReadImmutableAttributes() and ReadCacheFile(). I only want to call it after ReadCacheFile() because I'd like to have one UMA entry per boot. I'd really like to move ReadCacheFile() into the constructor and then trigger the consistency check from there as well (if the file name is not empty).
replying to replies... https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/enterprise_install_attributes.cc (right): https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:29: int kDbusRetryIntervalInSeconds = 2; On 2015/06/23 13:17:55, Thiemo Nagel wrote: > On 2015/06/23 11:34:09, Mattias Nissler wrote: > > 2 seems a bit too aggressive. How about 10? > > I'd like to avoid querying the TPM while enrollment runs. 10 seconds seem to be > sufficient to click through OOBE and start enrollment. Can we compromise on 5 > seconds? There are devices with notoriously slow TPMs where we take ages to initialize. I think in these cases, you're racing against the retry loop in EnrollmentHandlerChromeOS::HandleLockDeviceResult here as well. If you care about this, then you should properly synchronize the lock operation and the consistency check. You could just make sure that the consistency check finishes first before you start locking. https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:78: lock_state_ = STATE_NOT_LOCKED; On 2015/06/23 13:17:55, Thiemo Nagel wrote: > On 2015/06/23 11:34:09, Mattias Nissler wrote: > > Note that the cache file will also be missing if lockbox-cache runs into an > > error. > > Yes, this could happen. Setting STATE_NOT_LOCKED however seems to be best in > line with the present behaviour of device_locked_ = false. I guess this is language nitpicking, but the original intention for the |device_locked_| flag was that a value of true indicates a locked device, everything else doesn't state anything. Your STATE_NOT_LOCKED seems to convey more than that. https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:352: dbus_tries_remaining_--; On 2015/06/23 13:17:55, Thiemo Nagel wrote: > On 2015/06/23 11:34:09, Mattias Nissler wrote: > > How about binding this as a function parameter that you decrement before the > > retry? That way, you don't have to keep it around in class state. > > I guess that would make sense only if TriggerConsistencyCheck() is made private. > (But if we make it private, I'm happy to go that way.) Yup, this would require a private helper. That's orthogonal to whether there's a publicly-callable entry point though. https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:361: base::HistogramBase::Sample state = lock_state_; On 2015/06/23 13:17:55, Thiemo Nagel wrote: > On 2015/06/23 11:34:09, Mattias Nissler wrote: > > It seems like the code is written in a way s.t. STATE_UNKNOWN and > STATE_LOCKING > > are not going to show up at all. Except for the remote chance that > > TriggerConsistencyCheck executes in parallel to the lock operation. > > > > Given that, does it really make sense to introduce these two enum constants in > > the first place? > > > > It looks like the bit you want here is cache-file-present-or-not. You probably > > don't even want to handle the case of enrollment here, it's good enough to > just > > report things for steady state, no? > > I guess I need STATE_PENDING to catch the case in which the consistency check is > executed while enrollment takes place. Since I was already adding another > state, I felt it made sense to differentiate between NOT_LOCKED and not > initialized. Maybe the real problem here is the clumsy semantics of having to > call ReadCacheFile() after the constructor has run. Probably it would be best > to avoid STATE_UNKNOWN by having the constructor call ReadCacheFile() and to > inject the cache file name as a constructor parameter. Wdyt? Constructors shouldn't perform heavy-weight initialization per the style guide: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Doing_Work_in... Isn't it good enough to start the consistency check in ReadCacheFile()? That way, you don't need the UNKNOWN state. And if you properly synchronize consistency checking and locking, then you don't need PENDING either.
Patchset #4 (id:60001) has been deleted
Thank you. Could you please take another look? https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/enterprise_install_attributes.cc (right): https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:29: int kDbusRetryIntervalInSeconds = 2; On 2015/06/23 13:43:07, Mattias Nissler wrote: > On 2015/06/23 13:17:55, Thiemo Nagel wrote: > > On 2015/06/23 11:34:09, Mattias Nissler wrote: > > > 2 seems a bit too aggressive. How about 10? > > > > I'd like to avoid querying the TPM while enrollment runs. 10 seconds seem to > be > > sufficient to click through OOBE and start enrollment. Can we compromise on 5 > > seconds? > > There are devices with notoriously slow TPMs where we take ages to initialize. I > think in these cases, you're racing against the retry loop in > EnrollmentHandlerChromeOS::HandleLockDeviceResult here as well. If you care > about this, then you should properly synchronize the lock operation and the > consistency check. You could just make sure that the consistency check finishes > first before you start locking. Done. https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:78: lock_state_ = STATE_NOT_LOCKED; On 2015/06/23 13:43:07, Mattias Nissler wrote: > On 2015/06/23 13:17:55, Thiemo Nagel wrote: > > On 2015/06/23 11:34:09, Mattias Nissler wrote: > > > Note that the cache file will also be missing if lockbox-cache runs into an > > > error. > > > > Yes, this could happen. Setting STATE_NOT_LOCKED however seems to be best in > > line with the present behaviour of device_locked_ = false. > > I guess this is language nitpicking, but the original intention for the > |device_locked_| flag was that a value of true indicates a locked device, > everything else doesn't state anything. Your STATE_NOT_LOCKED seems to convey > more than that. Alright. After offline discussion, I went back to the binary state and documented it properly. https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:352: dbus_tries_remaining_--; On 2015/06/23 13:43:07, Mattias Nissler wrote: > On 2015/06/23 13:17:55, Thiemo Nagel wrote: > > On 2015/06/23 11:34:09, Mattias Nissler wrote: > > > How about binding this as a function parameter that you decrement before the > > > retry? That way, you don't have to keep it around in class state. > > > > I guess that would make sense only if TriggerConsistencyCheck() is made > private. > > (But if we make it private, I'm happy to go that way.) > > Yup, this would require a private helper. That's orthogonal to whether there's a > publicly-callable entry point though. Done. https://codereview.chromium.org/1189203003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:361: base::HistogramBase::Sample state = lock_state_; On 2015/06/23 13:43:07, Mattias Nissler wrote: > On 2015/06/23 13:17:55, Thiemo Nagel wrote: > > On 2015/06/23 11:34:09, Mattias Nissler wrote: > > > It seems like the code is written in a way s.t. STATE_UNKNOWN and > > STATE_LOCKING > > > are not going to show up at all. Except for the remote chance that > > > TriggerConsistencyCheck executes in parallel to the lock operation. > > > > > > Given that, does it really make sense to introduce these two enum constants > in > > > the first place? > > > > > > It looks like the bit you want here is cache-file-present-or-not. You > probably > > > don't even want to handle the case of enrollment here, it's good enough to > > just > > > report things for steady state, no? > > > > I guess I need STATE_PENDING to catch the case in which the consistency check > is > > executed while enrollment takes place. Since I was already adding another > > state, I felt it made sense to differentiate between NOT_LOCKED and not > > initialized. Maybe the real problem here is the clumsy semantics of having to > > call ReadCacheFile() after the constructor has run. Probably it would be best > > to avoid STATE_UNKNOWN by having the constructor call ReadCacheFile() and to > > inject the cache file name as a constructor parameter. Wdyt? > > Constructors shouldn't perform heavy-weight initialization per the style guide: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Doing_Work_in... Alright. I'm also renaming ReadCacheFile() to Init() to make clear that without it being called, the object is not properly initialized. > Isn't it good enough to start the consistency check in ReadCacheFile()? That > way, you don't need the UNKNOWN state. And if you properly synchronize > consistency checking and locking, then you don't need PENDING either. Done that.
Looks much better already! https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/enterprise_install_attributes.cc (right): https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:82: kDbusRetryDurationInSeconds / kDbusRetryIntervalInSeconds); Why not just a number-of-retries constant? https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:209: post_check_action_ = base::Bind(&EnterpriseInstallAttributes::LockDevice, So in theory this opens up an error condition where |callback| never gets invoked because somebody calls LockDevice twice in a row (the second call will clobber the first |post_check_action_|). I don't think the code is prepared to handle multiple concurrent LockDevice calls anyways. However, instead of compounding the problem, we might just as well fix this? We could just reject further LockDevice calls when one is still ongoing. For background, I think this whole issue with multiple concurrently LockDevices calls slipped in the review when people started converting the stuff in this class to make async DBus calls. https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:379: state |= 0x8; Think more about this bitset - does it make sense to encode the error condition as a bit? |result| is missing in that case, which means that the data point is pretty much useless anyways. I think your motivation is to catch cases where the consistency check fails to determine TPM ownership status. Wouldn't that sufficiently be covered by a separate UMA stat that captures the relative error frequency? That way, you would avoid to double the space of possible values for the state histogram. https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/enterprise_install_attributes.h (right): https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.h:102: bool device_locked_; nit: blank line before comment. https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.h:104: bool consistency_check_running_; nit: blank line before comment. https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.h:106: base::Closure post_check_action_; nit: blank line here. The members below conceptually belong together as they reflect the current state, but the post_check_action_ closure is separate. https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.h:174: void OnTpmIsOwned(int dbus_tries_remaining, The name of this function is misleading. Should OnTpmOwnershipCheckCompleted or something.
> Looks much better already! Very happy to hear that! :) Could you please take another look? Thank you, Thiemo https://codereview.chromium.org/1189203003/diff/1/chrome/browser/chromeos/pol... File chrome/browser/chromeos/policy/enterprise_install_attributes.cc (right): https://codereview.chromium.org/1189203003/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:336: void EnterpriseInstallAttributes::CheckConsistency() { On 2015/06/22 07:24:50, Mattias Nissler wrote: > On 2015/06/19 17:47:03, Thiemo Nagel wrote: > > On 2015/06/19 07:32:45, Mattias Nissler wrote: > > > The code you have here purely tests whether the crypthome DBus interface > > > supplies consistent values. I don't think that TpmIsOwned is of any > relevance > > > though - it's merely an artifact of how install attributes are implemented > in > > > cryptohome. > > > > As far as I can see, TpmIsOwned() calls through to the TPM. In my tests, I > can > > observe the combination of install attributes missing but TPM reporting as > > locked. > > I'm sure you can actually observe this situation. The question however is why > this is relevant for enrollment? IIRC, the goal was to detect whether install > attributes say we're enterprise while device ownership shows a consumer user? > Maybe I'm missing something - just convince me that the condition you're > detecting here is actually interesting to us. Quoting Mattias: "For posterity: Offline discussions clarified that this CL is meant to detect the situation of stateful data loss, thus comparing TPM ownership state with install attributes presence on disk is a useful signal." https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/enterprise_install_attributes.cc (right): https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:82: kDbusRetryDurationInSeconds / kDbusRetryIntervalInSeconds); On 2015/06/24 08:34:39, Mattias Nissler wrote: > Why not just a number-of-retries constant? I think it is easier to reason about retries in terms of retry interval and retry duration. If I'd specify retry interval and retry count, readers of the code would automatically multiply the two in their head, causing unnecessary load. https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:209: post_check_action_ = base::Bind(&EnterpriseInstallAttributes::LockDevice, On 2015/06/24 08:34:39, Mattias Nissler wrote: > So in theory this opens up an error condition where |callback| never gets > invoked because somebody calls LockDevice twice in a row (the second call will > clobber the first |post_check_action_|). > > I don't think the code is prepared to handle multiple concurrent LockDevice > calls anyways. However, instead of compounding the problem, we might just as > well fix this? We could just reject further LockDevice calls when one is still > ongoing. Sounds good. Done. https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:379: state |= 0x8; On 2015/06/24 08:34:39, Mattias Nissler wrote: > Think more about this bitset - does it make sense to encode the error condition > as a bit? |result| is missing in that case, which means that the data point is > pretty much useless anyways. > > I think your motivation is to catch cases where the consistency check fails to > determine TPM ownership status. Wouldn't that sufficiently be covered by a > separate UMA stat that captures the relative error frequency? That way, you > would avoid to double the space of possible values for the state histogram. If the TPM check fails, there's something amiss, and I'd like to obtain as much information as possible for that case. It might be interesting to know whether the failure is related to the install attributes state. For that reason, I'm compounding all results in a single enum. https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/enterprise_install_attributes.h (right): https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.h:102: bool device_locked_; On 2015/06/24 08:34:39, Mattias Nissler wrote: > nit: blank line before comment. Done. https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.h:104: bool consistency_check_running_; On 2015/06/24 08:34:39, Mattias Nissler wrote: > nit: blank line before comment. Done. https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.h:106: base::Closure post_check_action_; On 2015/06/24 08:34:39, Mattias Nissler wrote: > nit: blank line here. The members below conceptually belong together as they > reflect the current state, but the post_check_action_ closure is separate. Done. https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.h:174: void OnTpmIsOwned(int dbus_tries_remaining, On 2015/06/24 08:34:39, Mattias Nissler wrote: > The name of this function is misleading. Should OnTpmOwnershipCheckCompleted or > something. You're right. Done.
https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/enterprise_install_attributes.cc (right): https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:82: kDbusRetryDurationInSeconds / kDbusRetryIntervalInSeconds); On 2015/06/24 10:43:36, Thiemo Nagel wrote: > On 2015/06/24 08:34:39, Mattias Nissler wrote: > > Why not just a number-of-retries constant? > > I think it is easier to reason about retries in terms of retry interval and > retry duration. If I'd specify retry interval and retry count, readers of the > code would automatically multiply the two in their head, causing unnecessary > load. I found myself doing the exact inverse of what you described, i.e. doing the division in my brain to find out the total number of attempts :) So maybe this is just depends on who is reading. Given that multiplication is the simpler operation and the fact number-of-attempts constants are more common in the code base in my experience, I think we should still go with that. https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:379: state |= 0x8; On 2015/06/24 10:43:36, Thiemo Nagel wrote: > On 2015/06/24 08:34:39, Mattias Nissler wrote: > > Think more about this bitset - does it make sense to encode the error > condition > > as a bit? |result| is missing in that case, which means that the data point is > > pretty much useless anyways. > > > > I think your motivation is to catch cases where the consistency check fails to > > determine TPM ownership status. Wouldn't that sufficiently be covered by a > > separate UMA stat that captures the relative error frequency? That way, you > > would avoid to double the space of possible values for the state histogram. > > If the TPM check fails, there's something amiss, and I'd like to obtain as much > information as possible for that case. It might be interesting to know whether > the failure is related to the install attributes state. For that reason, I'm > compounding all results in a single enum. I don't think the install attributes state bit gives you much relevant information to debug this situtation though. The bit is generated by mount-encrypted and lockbox-cache during startup, however you'd need to start digging on the cryptohomed side to find out why these calls fail consistently, and cryptohomed contains completely separate code for handling install attributes. Given that, I don't think it's worth to complicate the UMA histogram here - I think the benefit of keeping the histogram at 8 easy-to understand values for casual inspectors and future maintainers is more beneficial than the hypothetical benefit that install attributes state bit in case of error provides. Regardless of that, the enterprise mode bit and the tpm-is-owned bit are totally irrelevant in case of error, so even if you want to keep track of install attributes state in presence of an error, a better way would be to create a separate histogram that just records the pair install attributes state and error-occurred bits. https://codereview.chromium.org/1189203003/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1189203003/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:52439: + should not happen: install attributes locked but TPM clear If I'm not mistaken, this actually does happen in the wild. TPM ownership can take quite a while to be established, and we may even reboot before we succeed in taking ownership. This state should be transient though as TPM ownership should get established eventually if the device keeps running for a long-enough amount of time. https://codereview.chromium.org/1189203003/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:52445: + inconsistent: install attributes locked but TPM clear ditto https://codereview.chromium.org/1189203003/diff/90001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/enterprise_install_attributes.cc (right): https://codereview.chromium.org/1189203003/diff/90001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:169: DCHECK_EQ(registration_running_, false); Let's make this a CHECK_EQ so we fail fast if this gets triggered by races in callers and whatnot that may slip through QA. https://codereview.chromium.org/1189203003/diff/90001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/enterprise_install_attributes.h (right): https://codereview.chromium.org/1189203003/diff/90001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.h:71: // called concurrently. nit: "Concurrently" isn't very specific, let's say "Must not be called while a previous LockDevice invocation is still pending". https://codereview.chromium.org/1189203003/diff/90001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.h:112: bool registration_running_; nit: Blank line here to separate bookkeeping information from attributes state.
Patchset #6 (id:110001) has been deleted
Again, thank you for your comments, and could you please take another look? https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/enterprise_install_attributes.cc (right): https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:82: kDbusRetryDurationInSeconds / kDbusRetryIntervalInSeconds); On 2015/06/24 11:55:03, Mattias Nissler wrote: > On 2015/06/24 10:43:36, Thiemo Nagel wrote: > > On 2015/06/24 08:34:39, Mattias Nissler wrote: > > > Why not just a number-of-retries constant? > > > > I think it is easier to reason about retries in terms of retry interval and > > retry duration. If I'd specify retry interval and retry count, readers of the > > code would automatically multiply the two in their head, causing unnecessary > > load. > > I found myself doing the exact inverse of what you described, i.e. doing the > division in my brain to find out the total number of attempts :) > > So maybe this is just depends on who is reading. Given that multiplication is > the simpler operation and the fact number-of-attempts constants are more common > in the code base in my experience, I think we should still go with that. Done. https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:379: state |= 0x8; On 2015/06/24 11:55:03, Mattias Nissler wrote: > On 2015/06/24 10:43:36, Thiemo Nagel wrote: > > On 2015/06/24 08:34:39, Mattias Nissler wrote: > > > Think more about this bitset - does it make sense to encode the error > > condition > > > as a bit? |result| is missing in that case, which means that the data point > is > > > pretty much useless anyways. > > > > > > I think your motivation is to catch cases where the consistency check fails > to > > > determine TPM ownership status. Wouldn't that sufficiently be covered by a > > > separate UMA stat that captures the relative error frequency? That way, you > > > would avoid to double the space of possible values for the state histogram. > > > > If the TPM check fails, there's something amiss, and I'd like to obtain as > much > > information as possible for that case. It might be interesting to know > whether > > the failure is related to the install attributes state. For that reason, I'm > > compounding all results in a single enum. > > I don't think the install attributes state bit gives you much relevant > information to debug this situtation though. The bit is generated by > mount-encrypted and lockbox-cache during startup, however you'd need to start > digging on the cryptohomed side to find out why these calls fail consistently, > and cryptohomed contains completely separate code for handling install > attributes. Given that, I don't think it's worth to complicate the UMA histogram > here - I think the benefit of keeping the histogram at 8 easy-to understand > values for casual inspectors and future maintainers is more beneficial than the > hypothetical benefit that install attributes state bit in case of error > provides. > > Regardless of that, the enterprise mode bit and the tpm-is-owned bit are totally > irrelevant in case of error, so even if you want to keep track of install > attributes state in presence of an error, a better way would be to create a > separate histogram that just records the pair install attributes state and > error-occurred bits. Alright. I've condensed the 4 error cases into a single one, but I'd prefer to keep it in the same histogram to prevent it from being overlooked and also to avoid the overhead of creating, maintaining and looking at yet another histogram. https://codereview.chromium.org/1189203003/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1189203003/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:52439: + should not happen: install attributes locked but TPM clear On 2015/06/24 11:55:03, Mattias Nissler wrote: > If I'm not mistaken, this actually does happen in the wild. TPM ownership can > take quite a while to be established, and we may even reboot before we succeed > in taking ownership. This state should be transient though as TPM ownership > should get established eventually if the device keeps running for a long-enough > amount of time. Interesting. Thanks! https://codereview.chromium.org/1189203003/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:52445: + inconsistent: install attributes locked but TPM clear On 2015/06/24 11:55:03, Mattias Nissler wrote: > ditto Done. https://codereview.chromium.org/1189203003/diff/90001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/enterprise_install_attributes.cc (right): https://codereview.chromium.org/1189203003/diff/90001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:169: DCHECK_EQ(registration_running_, false); On 2015/06/24 11:55:03, Mattias Nissler wrote: > Let's make this a CHECK_EQ so we fail fast if this gets triggered by races in > callers and whatnot that may slip through QA. Done. https://codereview.chromium.org/1189203003/diff/90001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/enterprise_install_attributes.h (right): https://codereview.chromium.org/1189203003/diff/90001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.h:71: // called concurrently. On 2015/06/24 11:55:03, Mattias Nissler wrote: > nit: "Concurrently" isn't very specific, let's say "Must not be called while a > previous LockDevice invocation is still pending". Done. https://codereview.chromium.org/1189203003/diff/90001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.h:112: bool registration_running_; On 2015/06/24 11:55:03, Mattias Nissler wrote: > nit: Blank line here to separate bookkeeping information from attributes state. Done.
This now LGTM, but please consider my optional nits inline. Happy to take another look if you decide to make further changes. https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/enterprise_install_attributes.cc (right): https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:379: state |= 0x8; On 2015/06/24 12:49:52, Thiemo Nagel wrote: > On 2015/06/24 11:55:03, Mattias Nissler wrote: > > On 2015/06/24 10:43:36, Thiemo Nagel wrote: > > > On 2015/06/24 08:34:39, Mattias Nissler wrote: > > > > Think more about this bitset - does it make sense to encode the error > > > condition > > > > as a bit? |result| is missing in that case, which means that the data > point > > is > > > > pretty much useless anyways. > > > > > > > > I think your motivation is to catch cases where the consistency check > fails > > to > > > > determine TPM ownership status. Wouldn't that sufficiently be covered by a > > > > separate UMA stat that captures the relative error frequency? That way, > you > > > > would avoid to double the space of possible values for the state > histogram. > > > > > > If the TPM check fails, there's something amiss, and I'd like to obtain as > > much > > > information as possible for that case. It might be interesting to know > > whether > > > the failure is related to the install attributes state. For that reason, > I'm > > > compounding all results in a single enum. > > > > I don't think the install attributes state bit gives you much relevant > > information to debug this situtation though. The bit is generated by > > mount-encrypted and lockbox-cache during startup, however you'd need to start > > digging on the cryptohomed side to find out why these calls fail consistently, > > and cryptohomed contains completely separate code for handling install > > attributes. Given that, I don't think it's worth to complicate the UMA > histogram > > here - I think the benefit of keeping the histogram at 8 easy-to understand > > values for casual inspectors and future maintainers is more beneficial than > the > > hypothetical benefit that install attributes state bit in case of error > > provides. > > > > Regardless of that, the enterprise mode bit and the tpm-is-owned bit are > totally > > irrelevant in case of error, so even if you want to keep track of install > > attributes state in presence of an error, a better way would be to create a > > separate histogram that just records the pair install attributes state and > > error-occurred bits. > > Alright. I've condensed the 4 error cases into a single one, but I'd prefer to > keep it in the same histogram to prevent it from being overlooked and also to > avoid the overhead of creating, maintaining and looking at yet another > histogram. I don't see how a separate would introduce significant additional maintenance overhead, but the point about the error condition being present in the original histogram being beneficial makes sense to me. https://codereview.chromium.org/1189203003/diff/130001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/enterprise_install_attributes.cc (right): https://codereview.chromium.org/1189203003/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:206: registration_running_ = true; nit: It's kinda unfortunate (and fragile) to have all these duplicated lines that reset registration_running_ and then invoke the callback. One way to avoid this might be to wrap the callback in another function like so: callback = base::Bind(&EnterpriseInstallAttributes::ReportLockDeviceResult, weak_ptr_factory_.GetWeakPtr(), callback); And then: void EnterpriseInstallAttributes::ReportLockDeviceResult( const LockResultCallback& callback, LockResult result) { registration_running_ = false; callback.Run(result); } https://codereview.chromium.org/1189203003/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:397: post_check_action_.Run(); nit: add a post_check_action_.reset() here https://codereview.chromium.org/1189203003/diff/130001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/enterprise_install_attributes.h (right): https://codereview.chromium.org/1189203003/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/policy/enterprise_install_attributes.h:112: bool registration_running_; nit: "registration" is somewhat odd naming here - or is there precedent of calling the process of locking install attributes "registration" anywhere else in the code?
tnagel@chromium.org changed reviewers: + isherman@chromium.org
Hi Ilya, while I sort out remaining nits with Mattias, may I kindly ask you to take a look at the changes to histograms.xml? Thank you and best regards, Thiemo https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/enterprise_install_attributes.cc (right): https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:379: state |= 0x8; > I don't see how a separate would introduce significant additional maintenance > overhead, Basically, it pollutes the namespace. Scrolling through Enterprise.* metrics takes longer without adding any value. Looking at two histograms takes longer than looking at one histogram (two dremel queries instead of one). https://codereview.chromium.org/1189203003/diff/130001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/enterprise_install_attributes.cc (right): https://codereview.chromium.org/1189203003/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:397: post_check_action_.Run(); On 2015/06/24 13:26:06, Mattias Nissler wrote: > nit: add a post_check_action_.reset() here The owner check _should_ only run once, but yes, seems like good practice. https://codereview.chromium.org/1189203003/diff/130001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/enterprise_install_attributes.h (right): https://codereview.chromium.org/1189203003/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/policy/enterprise_install_attributes.h:112: bool registration_running_; On 2015/06/24 13:26:06, Mattias Nissler wrote: > nit: "registration" is somewhat odd naming here - or is there precedent of > calling the process of locking install attributes "registration" anywhere else > in the code? The registration_* members? But I'd be happy to change that, how about lock_device_running_?
https://codereview.chromium.org/1189203003/diff/130001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/enterprise_install_attributes.cc (right): https://codereview.chromium.org/1189203003/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:206: registration_running_ = true; On 2015/06/24 13:26:06, Mattias Nissler wrote: > nit: It's kinda unfortunate (and fragile) to have all these duplicated lines > that reset registration_running_ and then invoke the callback. One way to avoid > this might be to wrap the callback in another function like so: > > callback = base::Bind(&EnterpriseInstallAttributes::ReportLockDeviceResult, > weak_ptr_factory_.GetWeakPtr(), > callback); > > And then: > > void EnterpriseInstallAttributes::ReportLockDeviceResult( > const LockResultCallback& callback, > LockResult result) { > registration_running_ = false; > callback.Run(result); > } I guess that one could also do void EnterpriseInstallAttributes::RunRegistrationEndCallback( const LockResultCallback& callback, LockResult result) { registration_running_ = false; callback.Run(result); } and then invoke it as RunRegistrationEndCallback(callback, result); however for both approaches, I'm not sure whether the benefit justifies the additional complexity.
https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/enterprise_install_attributes.cc (right): https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:379: state |= 0x8; On 2015/06/24 14:05:39, Thiemo Nagel wrote: > > I don't see how a separate would introduce significant additional maintenance > > overhead, > > Basically, it pollutes the namespace. Scrolling through Enterprise.* metrics > takes longer without adding any value. Looking at two histograms takes longer > than looking at one histogram (two dremel queries instead of one). I agree with these, although I would personally consider these points of negligible relevance ;) My point was meant to be about code maintenance. Sorry for not being entirely clear. https://codereview.chromium.org/1189203003/diff/130001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/enterprise_install_attributes.cc (right): https://codereview.chromium.org/1189203003/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:206: registration_running_ = true; On 2015/06/24 15:27:40, Thiemo Nagel wrote: > On 2015/06/24 13:26:06, Mattias Nissler wrote: > > nit: It's kinda unfortunate (and fragile) to have all these duplicated lines > > that reset registration_running_ and then invoke the callback. One way to > avoid > > this might be to wrap the callback in another function like so: > > > > callback = base::Bind(&EnterpriseInstallAttributes::ReportLockDeviceResult, > > weak_ptr_factory_.GetWeakPtr(), > > callback); > > > > And then: > > > > void EnterpriseInstallAttributes::ReportLockDeviceResult( > > const LockResultCallback& callback, > > LockResult result) { > > registration_running_ = false; > > callback.Run(result); > > } > > I guess that one could also do > > void EnterpriseInstallAttributes::RunRegistrationEndCallback( > const LockResultCallback& callback, > LockResult result) { > registration_running_ = false; > callback.Run(result); > } > > and then invoke it as > > RunRegistrationEndCallback(callback, result); Yes, that's also a valid approach. > > however for both approaches, I'm not sure whether the benefit justifies the > additional complexity. It'd probably reduce my personal mental load when reading this when I see that all the finish-the-operation stuff is handled by a common function. But I'm sure people will have different opinions, which is why I left the decision to you. https://codereview.chromium.org/1189203003/diff/130001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/enterprise_install_attributes.h (right): https://codereview.chromium.org/1189203003/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/policy/enterprise_install_attributes.h:112: bool registration_running_; On 2015/06/24 14:05:40, Thiemo Nagel wrote: > On 2015/06/24 13:26:06, Mattias Nissler wrote: > > nit: "registration" is somewhat odd naming here - or is there precedent of > > calling the process of locking install attributes "registration" anywhere else > > in the code? > > The registration_* members? But I'd be happy to change that, how about > lock_device_running_? The registration_* members are called such because they hold information pertinent to enrollment aka registration. Enrollment/Registration is much more than just locking install attributes though. The code here is only concerned with install attributes locking, which doesn't necessarily imply that there's an enrollment/registration flow ongoing. In fact, the code here also runs to lock install attributes for consumer kiosk devices, in which there is no such thing as registration/enrollment. Although I have to admit that the |registration_mode_| member is also named misleadingly as it's also relevant for the consumer kiosk case. That's to clarify the language I'm using and I think that language is used in the majority of the code dealing with these things. Anyhow, that's just for background, and I don't feel the name is strongly misleading, which is why I left the decision with you.
histograms lgtm
Patchset #8 (id:170001) has been deleted
The CQ bit was checked by tnagel@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from mnissler@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1189203003/#ps190001 (title: "Cosmetics and compilation fix.")
https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/enterprise_install_attributes.cc (right): https://codereview.chromium.org/1189203003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:379: state |= 0x8; On 2015/06/24 18:37:10, Mattias Nissler wrote: > On 2015/06/24 14:05:39, Thiemo Nagel wrote: > > > I don't see how a separate would introduce significant additional > maintenance > > > overhead, > > > > Basically, it pollutes the namespace. Scrolling through Enterprise.* metrics > > takes longer without adding any value. Looking at two histograms takes longer > > than looking at one histogram (two dremel queries instead of one). > > I agree with these, although I would personally consider these points of > negligible relevance ;) My point was meant to be about code maintenance. Sorry > for not being entirely clear. I wasn't entirely clear, either. I think my point applies more to a casual reader than to someone with a deep understanding of this part of the codebase. https://codereview.chromium.org/1189203003/diff/130001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/enterprise_install_attributes.cc (right): https://codereview.chromium.org/1189203003/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/policy/enterprise_install_attributes.cc:206: registration_running_ = true; On 2015/06/24 18:37:10, Mattias Nissler wrote: > On 2015/06/24 15:27:40, Thiemo Nagel wrote: > > On 2015/06/24 13:26:06, Mattias Nissler wrote: > > > nit: It's kinda unfortunate (and fragile) to have all these duplicated lines > > > that reset registration_running_ and then invoke the callback. One way to > > avoid > > > this might be to wrap the callback in another function like so: > > > > > > callback = base::Bind(&EnterpriseInstallAttributes::ReportLockDeviceResult, > > > weak_ptr_factory_.GetWeakPtr(), > > > callback); > > > > > > And then: > > > > > > void EnterpriseInstallAttributes::ReportLockDeviceResult( > > > const LockResultCallback& callback, > > > LockResult result) { > > > registration_running_ = false; > > > callback.Run(result); > > > } > > > > I guess that one could also do > > > > void EnterpriseInstallAttributes::RunRegistrationEndCallback( > > const LockResultCallback& callback, > > LockResult result) { > > registration_running_ = false; > > callback.Run(result); > > } > > > > and then invoke it as > > > > RunRegistrationEndCallback(callback, result); > > Yes, that's also a valid approach. > > > > > however for both approaches, I'm not sure whether the benefit justifies the > > additional complexity. > > It'd probably reduce my personal mental load when reading this when I see that > all the finish-the-operation stuff is handled by a common function. But I'm sure > people will have different opinions, which is why I left the decision to you. Alight. I'd prefer to keep it as it is, then. https://codereview.chromium.org/1189203003/diff/130001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/enterprise_install_attributes.h (right): https://codereview.chromium.org/1189203003/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/policy/enterprise_install_attributes.h:112: bool registration_running_; On 2015/06/24 18:37:10, Mattias Nissler wrote: > On 2015/06/24 14:05:40, Thiemo Nagel wrote: > > On 2015/06/24 13:26:06, Mattias Nissler wrote: > > > nit: "registration" is somewhat odd naming here - or is there precedent of > > > calling the process of locking install attributes "registration" anywhere > else > > > in the code? > > > > The registration_* members? But I'd be happy to change that, how about > > lock_device_running_? > > The registration_* members are called such because they hold information > pertinent to enrollment aka registration. Enrollment/Registration is much more > than just locking install attributes though. The code here is only concerned > with install attributes locking, which doesn't necessarily imply that there's an > enrollment/registration flow ongoing. > > In fact, the code here also runs to lock install attributes for consumer kiosk > devices, in which there is no such thing as registration/enrollment. Although I > have to admit that the |registration_mode_| member is also named misleadingly as > it's also relevant for the consumer kiosk case. > > That's to clarify the language I'm using and I think that language is used in > the majority of the code dealing with these things. > > Anyhow, that's just for background, and I don't feel the name is strongly > misleading, which is why I left the decision with you. Thanks. I've changed the name to device_lock_running_.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1189203003/190001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
The CQ bit was checked by tnagel@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, mnissler@chromium.org Link to the patchset: https://codereview.chromium.org/1189203003/#ps230001 (title: "More compilation fixes.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1189203003/230001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tnagel@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, mnissler@chromium.org Link to the patchset: https://codereview.chromium.org/1189203003/#ps250001 (title: "Fix consistency check and device lock interplay.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1189203003/250001
The CQ bit was checked by tnagel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, mnissler@chromium.org Link to the patchset: https://codereview.chromium.org/1189203003/#ps190002 (title: "Fix more tests.")
The CQ bit was unchecked by tnagel@chromium.org
The CQ bit was checked by tnagel@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, mnissler@chromium.org Link to the patchset: https://codereview.chromium.org/1189203003/#ps190002 (title: "Fix more tests.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1189203003/190002
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Hi Mattias, getting the tests right required some more changes. Do you want to take another look? (I think the patch set that you have l-g-t-m'ed was ca. #6 or #7.) Cheers, Thiemo
I had already done so, still LGTM.
On 2015/06/25 15:07:16, Mattias Nissler wrote: > I had already done so, still LGTM. Thanks a lot! I'll do some more manual testing and then (hopefully) land tomorrow.
The CQ bit was checked by tnagel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1189203003/190002
Message was sent while issue was closed.
Committed patchset #12 (id:190002)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/9ad56f55bb18f48ecced91751c0d9648c203451a Cr-Commit-Position: refs/heads/master@{#336348} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
