Clean up handling of invalid HWID when running ChromeOS in VMs

chrome/browser/chromeos/login/hwid_checker.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/login/hwid_checker.h"
 
 #include <cstdio>
 
 #include "base/command_line.h"
 #include "base/logging.h"
@@ skipping 91 matching lines @@
 
 } // anonymous namespace
 
 namespace chromeos {
 
 bool IsHWIDCorrect(const std::string& hwid) {
   return IsCorrectHWIDv2(hwid) || IsCorrectExceptionalHWID(hwid) ||
       IsCorrectHWIDv3(hwid);
 }
 
 bool IsMachineHWIDCorrect() {

[Daniel Erat, 2016/08/10 16:32:28]

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.

[norvez, 2016/08/11 17:10:34]

Done.

 #if !defined(GOOGLE_CHROME_BUILD)
   return true;
 #endif
   base::CommandLine* cmd_line = base::CommandLine::ForCurrentProcess();
   if (cmd_line->HasSwitch(::switches::kTestType))
     return true;
   if (!base::SysInfo::IsRunningOnChromeOS())
     return true;
   chromeos::system::StatisticsProvider* stats =
       chromeos::system::StatisticsProvider::GetInstance();
 
   std::string hwid;
   if (!stats->GetMachineStatistic(chromeos::system::kHardwareClassKey, &hwid)) {
     LOG(ERROR) << "Couldn't get machine statistic 'hardware_class'.";
     return false;
   }
   if (!chromeos::IsHWIDCorrect(hwid)) {
-    // Log the system vendor info to see what the system vendor is on the GCE
-    // VMs. This info will be used to filter out error messages on VMs. See
-    // http://crbug.com/585514 and http://crbug.com/585515 for more info.
     std::string system_vendor;
     stats->GetMachineStatistic(chromeos::system::kSystemVendorKey,
                                &system_vendor);
-    std::string firmware_type;
-    const bool non_chrome_firmware =
-        stats->GetMachineStatistic(chromeos::system::kFirmwareTypeKey,
-                                   &firmware_type) &&
-        firmware_type == system::kFirmwareTypeValueNonchrome;
-    if (non_chrome_firmware) {
-      LOG(WARNING) << "Detected non-Chrome firmware with malformed HWID '"
-                   << hwid << "', assuming VM environment. "
-                   << "The system vendor is '" << system_vendor << "'.";
-    } else {
+    if (!stats->IsRunningOnVm()) {
       LOG(ERROR) << "Machine has malformed HWID '" << hwid << "'. "
                  << "The system vendor is '" << system_vendor << "'.";
       return false;
+    } else {
+      LOG(WARNING) << "The system is a VM. The system vendor is '"
+                << system_vendor << "'.";

[Daniel Erat, 2016/08/10 16:32:28]

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.

[achuithb, 2016/08/10 18:24:58]

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?

[norvez, 2016/08/11 17:10:34]

Done.

     }
   }
   return true;
 }
 
 } // namespace chromeos