Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2218703006:
Clean up handling of invalid HWID when running ChromeOS in VMs (Closed)
|
Created:
4 years, 4 months ago by norvez Modified:
4 years, 4 months ago CC:
achuith+watch_chromium.org, chromium-reviews, davemoore+watch_chromium.org, dzhioev+watch_chromium.org, Hung-Te, oshima+watch_chromium.org, Vincent Palatin Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGracefully handle cases where Chrome OS is running in a VM
Read from crossystem whether Chrome OS is running inside a VM or not. Do
not report an error if Chrome OS is in a VM, even if the HWID is
invalid, it is expected.
Add IsRunningOnVm() method similar to IsRunningOnChromeOS().
Cleanup:
- more consistent naming
- add unit tests to hwid_checker
BUG=chromium:632303
BUG=chromium:585514
TEST=Ran by hand on cyan board and cyan image in QEMU
Committed: https://crrev.com/b923b70a31b4fb3eb286f6a5e4482b844afa4a40
Cr-Commit-Position: refs/heads/master@{#413097}
Patch Set 1 #
Total comments: 5
Patch Set 2 : read is_in_vm from crossystem #Patch Set 3 : crossystem command changed from is_in_vm to inside_vm #
Total comments: 5
Patch Set 4 : address review comments #Patch Set 5 : add comment #
Total comments: 16
Patch Set 6 : review comments, unit tests #Patch Set 7 : kSystemVendorKey no longer exists #
Total comments: 7
Patch Set 8 : review comments #
Total comments: 4
Patch Set 9 : Move IsRunningOnVm() check and update unit tests accordingly #
Total comments: 4
Patch Set 10 : fix after review comments (no functional changes) #
Dependent Patchsets: Messages
Total messages: 52 (17 generated)
norvez@chromium.org changed reviewers: + achuith@chromium.org, derat@chromium.org, xdai@chromium.org
This CL makes it easier for callers to detect if we're running in a VM or not. Now that the logic has been moved to statistics_provider, callers simply need to check if the value of the "hardware_class" field is "VM". It should also help with crbug.com/585514, https://codereview.chromium.org/2218003005 should fix the issue.
https://codereview.chromium.org/2218703006/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/hwid_checker.cc (right): https://codereview.chromium.org/2218703006/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/hwid_checker.cc:130: if (chromeos::system::kHardwareClassValueVM != hwid) { nit: switch the order so the constant comes second, i.e. "hwid != ...VM" https://codereview.chromium.org/2218703006/diff/1/chromeos/system/statistics_... File chromeos/system/statistics_provider.cc (right): https://codereview.chromium.org/2218703006/diff/1/chromeos/system/statistics_... chromeos/system/statistics_provider.cc:469: // that ChromeOS is running inside a VM and set the HWID to "VM" nit: s/ChromeOS/Chrome OS/; also add trailing period https://codereview.chromium.org/2218703006/diff/1/chromeos/system/statistics_... chromeos/system/statistics_provider.cc:472: LOG(WARNING) << "Detected non-Chrome firmware with malformed HWID," nit: maybe move this down and combine it with the other log statement so we just log one line? https://codereview.chromium.org/2218703006/diff/1/chromeos/system/statistics_... chromeos/system/statistics_provider.cc:481: machine_info_[kSystemVendorKey] = kSystemVendorValueUnknown; if i'm not misreading this, machine_info_[kSystemVendorKey] only gets set in the unknown-hardware-class, non-chrome-firmware case now, while before it was always set. is this change intentional?
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
not lgtm https://codereview.chromium.org/2218703006/diff/1/chromeos/system/statistics_... File chromeos/system/statistics_provider.cc (right): https://codereview.chromium.org/2218703006/diff/1/chromeos/system/statistics_... chromeos/system/statistics_provider.cc:469: // that ChromeOS is running inside a VM and set the HWID to "VM" You should add a real test to detect that you're in a VM - not just assume that missing HWID implies a VM. This assumption is not right. I don't think this CL is ok. Some ideas here: http://www.dmo.ca/blog/detecting-virtualization-on-linux/
On 2016/08/05 at 21:23:32, achuith wrote: > not lgtm > > https://codereview.chromium.org/2218703006/diff/1/chromeos/system/statistics_... > File chromeos/system/statistics_provider.cc (right): > > https://codereview.chromium.org/2218703006/diff/1/chromeos/system/statistics_... > chromeos/system/statistics_provider.cc:469: // that ChromeOS is running inside a VM and set the HWID to "VM" > You should add a real test to detect that you're in a VM - not just assume that missing HWID implies a VM. This assumption is not right. > > I don't think this CL is ok. > > Some ideas here: > http://www.dmo.ca/blog/detecting-virtualization-on-linux/ I think dmidecode only works on x86. Parsing its output will also probably fail pretty often every time we encounter a new VM vendor -that's basically what was happening with xdai@'s original patch on GCE instances. I'm not sure that approach would be more reliable than the HWID/FWID combination (see https://codereview.chromium.org/2187473006/ and https://chromium-review.googlesource.com/#/c/363330/ for a bit more context). I'm happy to change the VM detection logic but I'm not sure dmidecode or strings in /var/log/messages are more reliable. But I'll defer to your judgement :) What do you think? Adding hungte@ and vpalatin@ to the discussion for their thoughts, let's make sure everyone is happy with the final decision!
On 2016/08/08 08:48:47, norvez wrote: > On 2016/08/05 at 21:23:32, achuith wrote: > > not lgtm > > > > > https://codereview.chromium.org/2218703006/diff/1/chromeos/system/statistics_... > > File chromeos/system/statistics_provider.cc (right): > > > > > https://codereview.chromium.org/2218703006/diff/1/chromeos/system/statistics_... > > chromeos/system/statistics_provider.cc:469: // that ChromeOS is running inside > a VM and set the HWID to "VM" > > You should add a real test to detect that you're in a VM - not just assume > that missing HWID implies a VM. This assumption is not right. > > > > I don't think this CL is ok. > > > > Some ideas here: > > http://www.dmo.ca/blog/detecting-virtualization-on-linux/ > > I think dmidecode only works on x86. Parsing its output will also probably fail > pretty often every time we encounter a new VM vendor -that's basically what was > happening with xdai@'s original patch on GCE instances. I'm not sure that > approach would be more reliable than the HWID/FWID combination (see > https://codereview.chromium.org/2187473006/ and > https://chromium-review.googlesource.com/#/c/363330/ for a bit more context). Ah, ok, I wasn't aware of these CLs and that you've looked into xdai's efforts and the GCE failures. I'll rescind my not-lgtm. Is it possible to have a stronger signal from cros that we are on a VM, and move this guesswork there instead of in statistics_provider in chrome? It seems like we only care about getting this right on GCE and QEMU. While a new VM vendor could be introduced in theory, I don't know that this is worth worrying about. > I'm happy to change the VM detection logic but I'm not sure dmidecode or strings > in /var/log/messages are more reliable. But I'll defer to your judgement :) What > do you think? > > Adding hungte@ and vpalatin@ to the discussion for their thoughts, let's make > sure everyone is happy with the final decision!
On 2016/08/08 17:59:43, achuithb wrote: > On 2016/08/08 08:48:47, norvez wrote: > > On 2016/08/05 at 21:23:32, achuith wrote: > > > not lgtm > > > > > > > > > https://codereview.chromium.org/2218703006/diff/1/chromeos/system/statistics_... > > > File chromeos/system/statistics_provider.cc (right): > > > > > > > > > https://codereview.chromium.org/2218703006/diff/1/chromeos/system/statistics_... > > > chromeos/system/statistics_provider.cc:469: // that ChromeOS is running > inside > > a VM and set the HWID to "VM" > > > You should add a real test to detect that you're in a VM - not just assume > > that missing HWID implies a VM. This assumption is not right. > > > > > > I don't think this CL is ok. > > > > > > Some ideas here: > > > http://www.dmo.ca/blog/detecting-virtualization-on-linux/ > > > > I think dmidecode only works on x86. Parsing its output will also probably > fail > > pretty often every time we encounter a new VM vendor -that's basically what > was > > happening with xdai@'s original patch on GCE instances. I'm not sure that > > approach would be more reliable than the HWID/FWID combination (see > > https://codereview.chromium.org/2187473006/ and > > https://chromium-review.googlesource.com/#/c/363330/ for a bit more context). > > Ah, ok, I wasn't aware of these CLs and that you've looked into xdai's efforts > and the GCE failures. I'll rescind my not-lgtm. > > Is it possible to have a stronger signal from cros that we are on a VM, and move > this guesswork there instead of in statistics_provider in chrome? It seems like > we only care about getting this right on GCE and QEMU. While a new VM vendor > could be introduced in theory, I don't know that this is worth worrying about. hungte@ suggested a new 'is_in_vm' value in crossystem, I could move the logic there. Leave HWID as "(error)", FWID as "nonchrome" and set is_in_vm to 1 in that particular case. Callers could then just check this field when HWID is invalid and not worry about the underlying VM detection guesswork. How does that sound? Anybody opposed? > > I'm happy to change the VM detection logic but I'm not sure dmidecode or > strings > > in /var/log/messages are more reliable. But I'll defer to your judgement :) > What > > do you think? > > > > Adding hungte@ and vpalatin@ to the discussion for their thoughts, let's make > > sure everyone is happy with the final decision!
On 2016/08/08 18:27:35, norvez wrote: > On 2016/08/08 17:59:43, achuithb wrote: > > On 2016/08/08 08:48:47, norvez wrote: > > > On 2016/08/05 at 21:23:32, achuith wrote: > > > > not lgtm > > > > > > > > > > > > > > https://codereview.chromium.org/2218703006/diff/1/chromeos/system/statistics_... > > > > File chromeos/system/statistics_provider.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2218703006/diff/1/chromeos/system/statistics_... > > > > chromeos/system/statistics_provider.cc:469: // that ChromeOS is running > > inside > > > a VM and set the HWID to "VM" > > > > You should add a real test to detect that you're in a VM - not just assume > > > that missing HWID implies a VM. This assumption is not right. > > > > > > > > I don't think this CL is ok. > > > > > > > > Some ideas here: > > > > http://www.dmo.ca/blog/detecting-virtualization-on-linux/ > > > > > > I think dmidecode only works on x86. Parsing its output will also probably > > fail > > > pretty often every time we encounter a new VM vendor -that's basically what > > was > > > happening with xdai@'s original patch on GCE instances. I'm not sure that > > > approach would be more reliable than the HWID/FWID combination (see > > > https://codereview.chromium.org/2187473006/ and > > > https://chromium-review.googlesource.com/#/c/363330/ for a bit more > context). > > > > Ah, ok, I wasn't aware of these CLs and that you've looked into xdai's efforts > > and the GCE failures. I'll rescind my not-lgtm. > > > > Is it possible to have a stronger signal from cros that we are on a VM, and > move > > this guesswork there instead of in statistics_provider in chrome? It seems > like > > we only care about getting this right on GCE and QEMU. While a new VM vendor > > could be introduced in theory, I don't know that this is worth worrying about. > > hungte@ suggested a new 'is_in_vm' value in crossystem, I could move the logic > there. Leave HWID as "(error)", FWID as "nonchrome" and set is_in_vm to 1 in > that particular case. Callers could then just check this field when HWID is > invalid and not worry about the underlying VM detection guesswork. How does that > sound? Anybody opposed? +1 from me
moving this to crossystem sounds good to me too.
+1
New patchset using the functionality provided by crossystem (see https://chromium-review.googlesource.com/#/c/367240/). The downside is that we now require crossystem to be installed to detect a VM. That may not be the case for all systems using that codebase so I'm not sure it will actually help with crbug.com/585514
On 2016/08/09 16:58:16, norvez wrote: > New patchset using the functionality provided by crossystem (see > https://chromium-review.googlesource.com/#/c/367240/). > > The downside is that we now require crossystem to be installed to detect a VM. > That may not be the case for all systems using that codebase so I'm not sure it > will actually help with crbug.com/585514 Note that isRunningOnChromeOSVM) looks a bit out of place and would probably make more sense in base::SysInfo, but because of the dependency on crossystem I'm not sure we should pollute base.
The name of the crossystem command has changed, it's now inside_vm (https://chromium-review.googlesource.com/#/c/367240)
https://codereview.chromium.org/2218703006/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/hwid_checker.cc (right): https://codereview.chromium.org/2218703006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/hwid_checker.cc:133: if (!chromeos::system::isRunningOnChromeOSVM()) { s/is/Is/ (see style guide) https://codereview.chromium.org/2218703006/diff/40001/chromeos/system/statist... File chromeos/system/statistics_provider.cc (right): https://codereview.chromium.org/2218703006/diff/40001/chromeos/system/statist... chromeos/system/statistics_provider.cc:194: if (StatisticsProvider::GetInstance()->GetMachineStatistic(kIsCrosVMKey, nit: return GetMachineStatistic(kIsCrosVmKey, &is_vm) && is_vm == kIsCrosVmValueTrue; https://codereview.chromium.org/2218703006/diff/40001/chromeos/system/statist... chromeos/system/statistics_provider.cc:495: } nit: add a blank line after this to set the two sections apart https://codereview.chromium.org/2218703006/diff/40001/chromeos/system/statist... File chromeos/system/statistics_provider.h (right): https://codereview.chromium.org/2218703006/diff/40001/chromeos/system/statist... chromeos/system/statistics_provider.h:51: CHROMEOS_EXPORT extern const char kIsCrosVMValueFalse[]; nit: change "VM" to "Vm" in all of these (https://google.github.io/styleguide/cppguide.html#Function_Names; these aren't functions but people have told me the same rule applies to constants in chromium) https://codereview.chromium.org/2218703006/diff/40001/chromeos/system/statist... chromeos/system/statistics_provider.h:93: CHROMEOS_EXPORT bool isRunningOnChromeOSVM(); s/is/Is/ (see naming conventions in style guide) also get rid of "ChromeOS" in the name and just call it "IsRunningOnVm". please also make this be a non-static method on StatisticsProvider.
Description was changed from ========== Clean up handling of invalid HWID when running ChromeOS in VMs Move logic handling the detection of a VM (crossystem reports HWID "(error)" and FWID "nonchrome") to statistics_provider. This makes calling code simpler. Cleanup: - trim whitespaces (CR) in system vendor field - More consistent naming BUG=chromium:632303 BUG=chromium:585514 TEST=Ran by hand on cyan board and cyan image in QEMU ========== to ========== Gracefully handle cases where Chrome OS is running in a VM Read from crossystem whether Chrome OS is running inside a VM or not. If that's the case, log it in hwid_checker but do not fail. Add isRunningOnVm() method similar to isRunningOnChromeOS(). Cleanup: - trim whitespaces (CR) in system vendor field - More consistent naming BUG=chromium:632303 BUG=chromium:585514 TEST=Ran by hand on cyan board and cyan image in QEMU ==========
Fixed patchset after review.
lgtm
derat@chromium.org changed reviewers: + stevenjb@chromium.org
https://codereview.chromium.org/2218703006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/hwid_checker.cc (right): https://codereview.chromium.org/2218703006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/hwid_checker.cc:112: bool IsMachineHWIDCorrect() { it's not your fault, but would you mind adding tests for this method to hwid_checker_unittest.cc? i think that you'd need to instantiate a chromeos::system::ScopedFakeStatisticsProvider on the stack at the start of the test, and then you could configure what it returns and check IsMachineHWIDCorrect's return values. note that the test should probably be ifdef-ed for GOOGLE_CHROME_BUILD and also call base::SysInfo::SetChromeOSVersionInfoForTest to ensure that the IsRunningOnChromeOS check below returns an expected value. https://codereview.chromium.org/2218703006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/hwid_checker.cc:139: << system_vendor << "'."; nit: fix indenting ('<<' should line up with first '<<' on previous line) re LOG(WARNING), is this actually a problem? if not, please either use VLOG(1), or better, just switch this to a comment. https://codereview.chromium.org/2218703006/diff/80001/chromeos/system/statist... File chromeos/system/statistics_provider.cc (right): https://codereview.chromium.org/2218703006/diff/80001/chromeos/system/statist... chromeos/system/statistics_provider.cc:165: const char kIsCrosVmKey[] = "inside_cros_vm"; this code is only built for chrome os, so including "cros" here (in both the constant names and in this value) seems a bit redundant. mind switching to just something like: const char kIsVmKey[] = "is_vm"; ? https://codereview.chromium.org/2218703006/diff/80001/chromeos/system/statist... File chromeos/system/statistics_provider.h (right): https://codereview.chromium.org/2218703006/diff/80001/chromeos/system/statist... chromeos/system/statistics_provider.h:54: // Empty on non-x86 architectures, most meaningful in VM environments nit: add trailing period https://codereview.chromium.org/2218703006/diff/80001/chromeos/system/statist... chromeos/system/statistics_provider.h:119: // Returns true if the machine is a VM nit: add trailing period
https://codereview.chromium.org/2218703006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/hwid_checker.cc (right): https://codereview.chromium.org/2218703006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/hwid_checker.cc:139: << system_vendor << "'."; On 2016/08/10 16:32:28, Daniel Erat wrote: > nit: fix indenting ('<<' should line up with first '<<' on previous line) > > re LOG(WARNING), is this actually a problem? if not, please either use VLOG(1), > or better, just switch this to a comment. We had introduced this logging because we weren't sure what the system vendor was on GCE instances. As Dan says, VLOG is fine. A comment seems unnecessary since IsRunningOnVm is self-explanatory I think? https://codereview.chromium.org/2218703006/diff/80001/chromeos/system/fake_st... File chromeos/system/fake_statistics_provider.h (right): https://codereview.chromium.org/2218703006/diff/80001/chromeos/system/fake_st... chromeos/system/fake_statistics_provider.h:31: bool IsRunningOnVm() override; Feels like this should be const, but I guess GetMachineStatistic is not const, so never mind. https://codereview.chromium.org/2218703006/diff/80001/chromeos/system/statist... File chromeos/system/statistics_provider.cc (right): https://codereview.chromium.org/2218703006/diff/80001/chromeos/system/statist... chromeos/system/statistics_provider.cc:507: machine_info_[kSystemVendorKey] = system_vendor; Do we actually care about this? https://codereview.chromium.org/2218703006/diff/80001/chromeos/system/statist... File chromeos/system/statistics_provider.h (right): https://codereview.chromium.org/2218703006/diff/80001/chromeos/system/statist... chromeos/system/statistics_provider.h:55: CHROMEOS_EXPORT extern const char kSystemVendorKey[]; We should just get rid of kSystemVendorKey. I believe the only application is figuring out if we're in a VM.
Description was changed from ========== Gracefully handle cases where Chrome OS is running in a VM Read from crossystem whether Chrome OS is running inside a VM or not. If that's the case, log it in hwid_checker but do not fail. Add isRunningOnVm() method similar to isRunningOnChromeOS(). Cleanup: - trim whitespaces (CR) in system vendor field - More consistent naming BUG=chromium:632303 BUG=chromium:585514 TEST=Ran by hand on cyan board and cyan image in QEMU ========== to ========== Gracefully handle cases where Chrome OS is running in a VM Read from crossystem whether Chrome OS is running inside a VM or not. Do not report an error if the HWID is invalid but Chrome OS is in a VM, it is expected. Add isRunningOnVm() method similar to isRunningOnChromeOS(). Cleanup: - more consistent naming - add unit tests to hwid_checker BUG=chromium:632303 BUG=chromium:585514 TEST=Ran by hand on cyan board and cyan image in QEMU ==========
New patchset, ptal :-) https://codereview.chromium.org/2218703006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/hwid_checker.cc (right): https://codereview.chromium.org/2218703006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/hwid_checker.cc:112: bool IsMachineHWIDCorrect() { On 2016/08/10 16:32:28, Daniel Erat wrote: > it's not your fault, but would you mind adding tests for this method to > hwid_checker_unittest.cc? > > i think that you'd need to instantiate a > chromeos::system::ScopedFakeStatisticsProvider on the stack at the start of the > test, and then you could configure what it returns and check > IsMachineHWIDCorrect's return values. > > note that the test should probably be ifdef-ed for GOOGLE_CHROME_BUILD and also > call base::SysInfo::SetChromeOSVersionInfoForTest to ensure that the > IsRunningOnChromeOS check below returns an expected value. Done. https://codereview.chromium.org/2218703006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/hwid_checker.cc:139: << system_vendor << "'."; On 2016/08/10 18:24:58, achuithb wrote: > On 2016/08/10 16:32:28, Daniel Erat wrote: > > nit: fix indenting ('<<' should line up with first '<<' on previous line) > > > > re LOG(WARNING), is this actually a problem? if not, please either use > VLOG(1), > > or better, just switch this to a comment. > > We had introduced this logging because we weren't sure what the system vendor > was on GCE instances. As Dan says, VLOG is fine. A comment seems unnecessary > since IsRunningOnVm is self-explanatory I think? Done. https://codereview.chromium.org/2218703006/diff/80001/chromeos/system/fake_st... File chromeos/system/fake_statistics_provider.h (right): https://codereview.chromium.org/2218703006/diff/80001/chromeos/system/fake_st... chromeos/system/fake_statistics_provider.h:31: bool IsRunningOnVm() override; On 2016/08/10 18:24:58, achuithb wrote: > Feels like this should be const, but I guess GetMachineStatistic is not const, > so never mind. Yes, we have to const-ify all the way down :-( https://codereview.chromium.org/2218703006/diff/80001/chromeos/system/statist... File chromeos/system/statistics_provider.cc (right): https://codereview.chromium.org/2218703006/diff/80001/chromeos/system/statist... chromeos/system/statistics_provider.cc:165: const char kIsCrosVmKey[] = "inside_cros_vm"; On 2016/08/10 16:32:28, Daniel Erat wrote: > this code is only built for chrome os, so including "cros" here (in both the > constant names and in this value) seems a bit redundant. mind switching to just > something like: > > const char kIsVmKey[] = "is_vm"; > > ? Done. https://codereview.chromium.org/2218703006/diff/80001/chromeos/system/statist... chromeos/system/statistics_provider.cc:507: machine_info_[kSystemVendorKey] = system_vendor; On 2016/08/10 18:24:58, achuithb wrote: > Do we actually care about this? Removed system_vendor altogether https://codereview.chromium.org/2218703006/diff/80001/chromeos/system/statist... File chromeos/system/statistics_provider.h (right): https://codereview.chromium.org/2218703006/diff/80001/chromeos/system/statist... chromeos/system/statistics_provider.h:55: CHROMEOS_EXPORT extern const char kSystemVendorKey[]; On 2016/08/10 18:24:58, achuithb wrote: > We should just get rid of kSystemVendorKey. I believe the only application is > figuring out if we're in a VM. Done. https://codereview.chromium.org/2218703006/diff/80001/chromeos/system/statist... chromeos/system/statistics_provider.h:119: // Returns true if the machine is a VM On 2016/08/10 16:32:28, Daniel Erat wrote: > nit: add trailing period Done.
lgtm thanks for the tests!
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2218703006/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/hwid_checker.cc (right): https://codereview.chromium.org/2218703006/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/hwid_checker.cc:129: if (!(chromeos::IsHWIDCorrect(hwid) || stats->IsRunningOnVm())) { I wonder if it's clearer to explicitly do something like if (stats->IsRunningOnVm()) return true; after line 123, similiar to our treatment of IsRunningOnChromeOS on line 119. https://codereview.chromium.org/2218703006/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/hwid_checker_unittest.cc (right): https://codereview.chromium.org/2218703006/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/hwid_checker_unittest.cc:139: TEST(MachineHWIDCheckerTest, TestSwitch) { I think adding a comment describing each test would be useful, though the test names are a good start. Also a few newlines in each test would make them more readable - it's currently very dense. https://codereview.chromium.org/2218703006/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/hwid_checker_unittest.cc:143: ::switches::kTestType); Isn't this always true for unittests? Apparently not, but I don't understand it. Maybe it's only always true for browser tests? https://codereview.chromium.org/2218703006/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/hwid_checker_unittest.cc:187: // GIVEN The HWID is valid nit: period
New patchset after review https://codereview.chromium.org/2218703006/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/hwid_checker.cc (right): https://codereview.chromium.org/2218703006/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/hwid_checker.cc:129: if (!(chromeos::IsHWIDCorrect(hwid) || stats->IsRunningOnVm())) { On 2016/08/11 18:40:15, achuithb wrote: > I wonder if it's clearer to explicitly do something like > if (stats->IsRunningOnVm()) > return true; > > after line 123, similiar to our treatment of IsRunningOnChromeOS on line 119. Done. https://codereview.chromium.org/2218703006/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/hwid_checker_unittest.cc (right): https://codereview.chromium.org/2218703006/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/hwid_checker_unittest.cc:139: TEST(MachineHWIDCheckerTest, TestSwitch) { On 2016/08/11 18:40:16, achuithb wrote: > I think adding a comment describing each test would be useful, though the test > names are a good start. > > Also a few newlines in each test would make them more readable - it's currently > very dense. Done. https://codereview.chromium.org/2218703006/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/hwid_checker_unittest.cc:187: // GIVEN The HWID is valid On 2016/08/11 18:40:16, achuithb wrote: > nit: period Done.
https://codereview.chromium.org/2218703006/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/hwid_checker.cc (right): https://codereview.chromium.org/2218703006/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/hwid_checker.cc:130: if (stats->IsRunningOnVm()) { Does this need to be nested inside the !IsHWIDCorrect stmt?
https://codereview.chromium.org/2218703006/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/hwid_checker.cc (right): https://codereview.chromium.org/2218703006/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/hwid_checker.cc:130: if (stats->IsRunningOnVm()) { On 2016/08/12 20:39:02, achuithb wrote: > Does this need to be nested inside the !IsHWIDCorrect stmt? Dunno, that order looks right to me. The complete absence of the 'hardware_class' property is a classic "This should never happen" so I don't like to preempt it with IsRunningOnVm(), it's the kind of condition we really want to know about. And once we've retrieved the HWID it makes sense to check its correctness before falling back to the VM check in case the HWID is invalid.
On 2016/08/13 00:07:00, norvez wrote: > https://codereview.chromium.org/2218703006/diff/140001/chrome/browser/chromeo... > File chrome/browser/chromeos/login/hwid_checker.cc (right): > > https://codereview.chromium.org/2218703006/diff/140001/chrome/browser/chromeo... > chrome/browser/chromeos/login/hwid_checker.cc:130: if (stats->IsRunningOnVm()) { > On 2016/08/12 20:39:02, achuithb wrote: > > Does this need to be nested inside the !IsHWIDCorrect stmt? > > Dunno, that order looks right to me. The complete absence of the > 'hardware_class' property is a classic "This should never happen" so I don't > like to preempt it with IsRunningOnVm(), it's the kind of condition we really > want to know about. And once we've retrieved the HWID it makes sense to check > its correctness before falling back to the VM check in case the HWID is > invalid. What do you think? OK to submit to the CQ?
https://codereview.chromium.org/2218703006/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/hwid_checker.cc (right): https://codereview.chromium.org/2218703006/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/hwid_checker.cc:130: if (stats->IsRunningOnVm()) { On 2016/08/13 00:06:59, norvez wrote: > On 2016/08/12 20:39:02, achuithb wrote: > > Does this need to be nested inside the !IsHWIDCorrect stmt? > > Dunno, that order looks right to me. The complete absence of the > 'hardware_class' property is a classic "This should never happen" so I don't > like to preempt it with IsRunningOnVm(), it's the kind of condition we really > want to know about. And once we've retrieved the HWID it makes sense to check > its correctness before falling back to the VM check in case the HWID is > invalid. No, to be honest I would prefer this to be moved to line 123. Nesting this inside IsHWIDCorrect only makes sense to people aware that the current implementation of IsRunningOnVm uses HWID to determine if we're in a VM. First, this is an implementation detail in the chromeos source, and secondly, this could change, for instance, we could use image_to_vm.sh to write a special file instead of relying on hwid.
On 2016/08/15 17:15:05, achuithb wrote: > https://codereview.chromium.org/2218703006/diff/140001/chrome/browser/chromeo... > File chrome/browser/chromeos/login/hwid_checker.cc (right): > > https://codereview.chromium.org/2218703006/diff/140001/chrome/browser/chromeo... > chrome/browser/chromeos/login/hwid_checker.cc:130: if (stats->IsRunningOnVm()) { > On 2016/08/13 00:06:59, norvez wrote: > > On 2016/08/12 20:39:02, achuithb wrote: > > > Does this need to be nested inside the !IsHWIDCorrect stmt? > > > > Dunno, that order looks right to me. The complete absence of the > > 'hardware_class' property is a classic "This should never happen" so I don't > > like to preempt it with IsRunningOnVm(), it's the kind of condition we really > > want to know about. And once we've retrieved the HWID it makes sense to check > > its correctness before falling back to the VM check in case the HWID is > > invalid. > > No, to be honest I would prefer this to be moved to line 123. > > Nesting this inside IsHWIDCorrect only makes sense to people aware that the > current implementation of IsRunningOnVm uses HWID to determine if we're in a VM. > First, this is an implementation detail in the chromeos source, and secondly, > this could change, for instance, we could use image_to_vm.sh to write a special > file instead of relying on hwid. Makes sense, I'll modify the CL tomorrow.
On 2016/08/15 22:24:58, norvez wrote: > On 2016/08/15 17:15:05, achuithb wrote: > > > https://codereview.chromium.org/2218703006/diff/140001/chrome/browser/chromeo... > > File chrome/browser/chromeos/login/hwid_checker.cc (right): > > > > > https://codereview.chromium.org/2218703006/diff/140001/chrome/browser/chromeo... > > chrome/browser/chromeos/login/hwid_checker.cc:130: if (stats->IsRunningOnVm()) > { > > On 2016/08/13 00:06:59, norvez wrote: > > > On 2016/08/12 20:39:02, achuithb wrote: > > > > Does this need to be nested inside the !IsHWIDCorrect stmt? > > > > > > Dunno, that order looks right to me. The complete absence of the > > > 'hardware_class' property is a classic "This should never happen" so I don't > > > like to preempt it with IsRunningOnVm(), it's the kind of condition we > really > > > want to know about. And once we've retrieved the HWID it makes sense to > check > > > its correctness before falling back to the VM check in case the HWID is > > > invalid. > > > > No, to be honest I would prefer this to be moved to line 123. > > > > Nesting this inside IsHWIDCorrect only makes sense to people aware that the > > current implementation of IsRunningOnVm uses HWID to determine if we're in a > VM. > > First, this is an implementation detail in the chromeos source, and secondly, > > this could change, for instance, we could use image_to_vm.sh to write a > special > > file instead of relying on hwid. > > Makes sense, I'll modify the CL tomorrow. Thank you!
Description was changed from ========== Gracefully handle cases where Chrome OS is running in a VM Read from crossystem whether Chrome OS is running inside a VM or not. Do not report an error if the HWID is invalid but Chrome OS is in a VM, it is expected. Add isRunningOnVm() method similar to isRunningOnChromeOS(). Cleanup: - more consistent naming - add unit tests to hwid_checker BUG=chromium:632303 BUG=chromium:585514 TEST=Ran by hand on cyan board and cyan image in QEMU ========== to ========== Gracefully handle cases where Chrome OS is running in a VM Read from crossystem whether Chrome OS is running inside a VM or not. Do not report an error if Chrome OS is in a VM, even if the HWID is invalid, it is expected. Add isRunningOnVm() method similar to isRunningOnChromeOS(). Cleanup: - more consistent naming - add unit tests to hwid_checker BUG=chromium:632303 BUG=chromium:585514 TEST=Ran by hand on cyan board and cyan image in QEMU ==========
New patchset moving the VM check and updating the relevant unit tests. https://codereview.chromium.org/2218703006/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/hwid_checker.cc (right): https://codereview.chromium.org/2218703006/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/hwid_checker.cc:130: if (stats->IsRunningOnVm()) { On 2016/08/15 17:15:05, achuithb wrote: > On 2016/08/13 00:06:59, norvez wrote: > > On 2016/08/12 20:39:02, achuithb wrote: > > > Does this need to be nested inside the !IsHWIDCorrect stmt? > > > > Dunno, that order looks right to me. The complete absence of the > > 'hardware_class' property is a classic "This should never happen" so I don't > > like to preempt it with IsRunningOnVm(), it's the kind of condition we really > > want to know about. And once we've retrieved the HWID it makes sense to check > > its correctness before falling back to the VM check in case the HWID is > > invalid. > > No, to be honest I would prefer this to be moved to line 123. > > Nesting this inside IsHWIDCorrect only makes sense to people aware that the > current implementation of IsRunningOnVm uses HWID to determine if we're in a VM. > First, this is an implementation detail in the chromeos source, and secondly, > this could change, for instance, we could use image_to_vm.sh to write a special > file instead of relying on hwid. Done.
lgtm https://codereview.chromium.org/2218703006/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/login/hwid_checker.cc (right): https://codereview.chromium.org/2218703006/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/hwid_checker.cc:124: if (stats->IsRunningOnVm()) { nit: don't need {} https://codereview.chromium.org/2218703006/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/login/hwid_checker_unittest.cc (right): https://codereview.chromium.org/2218703006/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/hwid_checker_unittest.cc:224: // GIVEN The OS is Chrome OS. nit: s/The/the
Description was changed from ========== Gracefully handle cases where Chrome OS is running in a VM Read from crossystem whether Chrome OS is running inside a VM or not. Do not report an error if Chrome OS is in a VM, even if the HWID is invalid, it is expected. Add isRunningOnVm() method similar to isRunningOnChromeOS(). Cleanup: - more consistent naming - add unit tests to hwid_checker BUG=chromium:632303 BUG=chromium:585514 TEST=Ran by hand on cyan board and cyan image in QEMU ========== to ========== Gracefully handle cases where Chrome OS is running in a VM Read from crossystem whether Chrome OS is running inside a VM or not. Do not report an error if Chrome OS is in a VM, even if the HWID is invalid, it is expected. Add IsRunningOnVm() method similar to IsRunningOnChromeOS(). Cleanup: - more consistent naming - add unit tests to hwid_checker BUG=chromium:632303 BUG=chromium:585514 TEST=Ran by hand on cyan board and cyan image in QEMU ==========
Thanks for the reviews, latest patchset should address pending comments. https://codereview.chromium.org/2218703006/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/login/hwid_checker.cc (right): https://codereview.chromium.org/2218703006/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/hwid_checker.cc:124: if (stats->IsRunningOnVm()) { On 2016/08/16 17:59:33, achuithb wrote: > nit: don't need {} Done. https://codereview.chromium.org/2218703006/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/login/hwid_checker_unittest.cc (right): https://codereview.chromium.org/2218703006/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/hwid_checker_unittest.cc:224: // GIVEN The OS is Chrome OS. On 2016/08/16 17:59:33, achuithb wrote: > nit: s/The/the Done. Also made the same change throughout.
The CQ bit was checked by norvez@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xdai@chromium.org, derat@chromium.org, stevenjb@chromium.org, achuith@chromium.org Link to the patchset: https://codereview.chromium.org/2218703006/#ps180001 (title: "fix after review comments (no functional changes)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Gracefully handle cases where Chrome OS is running in a VM Read from crossystem whether Chrome OS is running inside a VM or not. Do not report an error if Chrome OS is in a VM, even if the HWID is invalid, it is expected. Add IsRunningOnVm() method similar to IsRunningOnChromeOS(). Cleanup: - more consistent naming - add unit tests to hwid_checker BUG=chromium:632303 BUG=chromium:585514 TEST=Ran by hand on cyan board and cyan image in QEMU ========== to ========== Gracefully handle cases where Chrome OS is running in a VM Read from crossystem whether Chrome OS is running inside a VM or not. Do not report an error if Chrome OS is in a VM, even if the HWID is invalid, it is expected. Add IsRunningOnVm() method similar to IsRunningOnChromeOS(). Cleanup: - more consistent naming - add unit tests to hwid_checker BUG=chromium:632303 BUG=chromium:585514 TEST=Ran by hand on cyan board and cyan image in QEMU Committed: https://crrev.com/b923b70a31b4fb3eb286f6a5e4482b844afa4a40 Cr-Commit-Position: refs/heads/master@{#413097} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/b923b70a31b4fb3eb286f6a5e4482b844afa4a40 Cr-Commit-Position: refs/heads/master@{#413097} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
