Addressing review comments that came after merging previous CL.
Minor updates to naming conventions and comments.
BUG=chromium-os:34299
TEST=Retested on ZGB. Re-ran unit tests.
Change-Id: I7db665d4f69969a972ee801f0e0cea9cf33437a6
Reviewed-on: https://gerrit.chromium.org/gerrit/36531
Reviewed-by: Gilad Arnold <garnold@chromium.org>
Commit-Ready: Jay Srinivasan <jaysri@chromium.org>
Reviewed-by: Jay Srinivasan <jaysri@chromium.org>
Tested-by: Jay Srinivasan <jaysri@chromium.org>
diff --git a/action_processor.h b/action_processor.h
index 0dff359..e193c67 100644
--- a/action_processor.h
+++ b/action_processor.h
@@ -63,27 +63,29 @@
kActionCodeOmahaErrorInHTTPResponse = 37,
kActionCodeDownloadOperationHashMissingError = 38,
kActionCodeDownloadInvalidMetadataSize = 39,
- kActionCodeDownloadInvalidMetadataSignature = 39,
- kActionCodeDownloadMetadataSignatureMissingError = 40,
+ kActionCodeDownloadInvalidMetadataSignature = 40,
+ kActionCodeDownloadMetadataSignatureMissingError = 41,
// Any code above this is sent to both Omaha and UMA as-is.
// Codes/flags below this line is sent only to Omaha and not to UMA.
- // kNumBucketsForUMAMetrics is not an error code per se, it's just the count
+ // kActionCodeUmaReportedMax is not an error code per se, it's just the count
// of the number of enums above. Add any new errors above this line if you
// want them to show up on UMA. Stuff below this line will not be sent to UMA
// but is used for other errors that are sent to Omaha. We don't assign any
// particular value for this enum so that it's just one more than the last
// one above and thus always represents the correct count of UMA metrics
// buckets, even when new enums are added above this line in future. See
- // utils::SendErrorCodeToUMA on how this enum is used.
- kNumBucketsForUMAMetrics,
+ // utils::SendErrorCodeToUma on how this enum is used.
+ kActionCodeUmaReportedMax,
// use the 2xxx range to encode HTTP errors. These errors are available in
// Dremel with the individual granularity. But for UMA purposes, all these
// errors are aggregated into one: kActionCodeOmahaErrorInHTTPResponse.
kActionCodeOmahaRequestHTTPResponseBase = 2000, // + HTTP response code
+ // TODO(jaysri): Move out all the bit masks into separate constants
+ // outside the enum as part of fixing bug 34369.
// Bit flags. Remember to update the mask below for new bits.
kActionCodeResumedFlag = 1 << 30, // Set if resuming an interruped update.
kActionCodeBootModeFlag = 1 << 31, // Set if boot mode not normal.
diff --git a/delta_performer.cc b/delta_performer.cc
index 59311b9..e771da0 100644
--- a/delta_performer.cc
+++ b/delta_performer.cc
@@ -254,10 +254,11 @@
<< "Actual = " << *metadata_size;
// Send a UMA Stat here to help with the decision to enforce
// this check in a future release, as mentioned below.
- SendUMAStat(kActionCodeDownloadInvalidMetadataSize);
+ SendUmaStat(kActionCodeDownloadInvalidMetadataSize);
// TODO(jaysri): VALIDATION: Initially we don't want to make this a fatal
- // error. But in the next release, we should uncomment the lines below.
+ // error. But in the next release, we should uncomment the lines below
+ // and remove the SendUmaStat call above.
// *error = kActionCodeDownloadInvalidManifest;
// return kMetadataParseError;
}
@@ -284,11 +285,11 @@
if (*error != kActionCodeSuccess) {
// Send a UMA Stat here to help with the decision to enforce
// this check in a future release, as mentioned below.
- SendUMAStat(*error);
+ SendUmaStat(*error);
// TODO(jaysri): VALIDATION: Initially we don't want to make this a fatal
// error. But in the next release, we should remove the line below and
- // return an error.
+ // return an error. We should also remove the SendUmaStat call above.
*error = kActionCodeSuccess;
}
@@ -369,11 +370,12 @@
// Send a UMA stat to indicate that an operation hash failed to be
// validated as expected.
- SendUMAStat(*error);
+ SendUmaStat(*error);
// TODO(jaysri): VALIDATION: For now, we don't treat this as fatal.
// But once we're confident that the new code works fine in the field,
- // we should uncomment the line below.
+ // we should uncomment the line below. We should also remove the
+ // SendUmaStat call above.
// return false;
LOG(INFO) << "Ignoring operation validation errors for now";
}
@@ -695,18 +697,19 @@
const char* metadata, uint64_t metadata_size) {
if (install_plan_->metadata_signature.empty()) {
- // If this is not present, we cannot validate the manifest. This should
- // never happen in normal circumstances, but this can be used as a
- // release-knob to turn off the new code path that verify per-operation
- // hashes. So, for now, we should not treat this as a failure. Once we are
- // confident this path is bug-free, we should treat this as a failure so
- // that we remain robust even if the connection to Omaha is subjected to
- // any SSL attack.
+ // TODO(jaysri): If this is not present, we cannot validate the manifest.
+ // This should never happen in normal circumstances, but this can be used
+ // as a release-knob to turn off the new code path that verify
+ // per-operation hashes. So, for now, we should not treat this as a
+ // failure. Once we are confident this path is bug-free, we should treat
+ // this as a failure so that we remain robust even if the connection to
+ // Omaha is subjected to any SSL attack. Once we make this an error, we
+ // should remove the SendUmaStat call below.
LOG(WARNING) << "Cannot validate metadata as the signature is empty";
// Send a UMA stat here so we're aware of any man-in-the-middle attempts to
// bypass these checks.
- SendUMAStat(kActionCodeDownloadMetadataSignatureMissingError);
+ SendUmaStat(kActionCodeDownloadMetadataSignatureMissingError);
return kActionCodeSuccess;
}
@@ -769,14 +772,14 @@
// Send a UMA stat here so we're aware of any man-in-the-middle attempts to
// bypass these checks.
- SendUMAStat(kActionCodeDownloadOperationHashMissingError);
+ SendUmaStat(kActionCodeDownloadOperationHashMissingError);
// TODO(jaysri): VALIDATION: no hash is present for the operation. This
// shouldn't happen normally for any client that has this code, because the
// corresponding update should have been produced with the operation
// hashes. But if it happens it's likely that we've turned this feature off
// in Omaha rule for some reason. Once we make these hashes mandatory, we
- // should return an error here.
+ // should return an error here. We should also remove the SendUmaStat call.
// One caveat though: The last operation is a dummy signature operation
// that doesn't have a hash at the time the manifest is created. So we
// should not complaint about that operation. This operation can be
@@ -1109,9 +1112,9 @@
return true;
}
-void DeltaPerformer::SendUMAStat(ActionExitCode code) {
+void DeltaPerformer::SendUmaStat(ActionExitCode code) {
if (system_state_) {
- utils::SendErrorCodeToUMA(system_state_->metrics_lib(), code);
+ utils::SendErrorCodeToUma(system_state_->metrics_lib(), code);
}
}
diff --git a/delta_performer.h b/delta_performer.h
index 0bdf7ca..2010765 100644
--- a/delta_performer.h
+++ b/delta_performer.h
@@ -217,7 +217,7 @@
bool PrimeUpdateState();
// Sends UMA statistics for the given error code.
- void SendUMAStat(ActionExitCode code);
+ void SendUmaStat(ActionExitCode code);
// Update Engine preference store.
PrefsInterface* prefs_;
diff --git a/download_action.cc b/download_action.cc
index b2055c8..8aa9e35 100644
--- a/download_action.cc
+++ b/download_action.cc
@@ -24,12 +24,12 @@
SystemState* system_state,
HttpFetcher* http_fetcher)
: prefs_(prefs),
- writer_(NULL),
+ system_state_(system_state),
http_fetcher_(http_fetcher),
+ writer_(NULL),
code_(kActionCodeSuccess),
delegate_(NULL),
- bytes_received_(0),
- system_state_(system_state) {}
+ bytes_received_(0) {}
DownloadAction::~DownloadAction() {}
diff --git a/download_action.h b/download_action.h
index 98cde46..6e334f6 100644
--- a/download_action.h
+++ b/download_action.h
@@ -100,15 +100,18 @@
// Update Engine preference store.
PrefsInterface* prefs_;
+ // Global context for the system.
+ SystemState* system_state_;
+
+ // Pointer to the HttpFetcher that does the http work.
+ scoped_ptr<HttpFetcher> http_fetcher_;
+
// The FileWriter that downloaded data should be written to. It will
// either point to *decompressing_file_writer_ or *delta_performer_.
FileWriter* writer_;
scoped_ptr<DeltaPerformer> delta_performer_;
- // Pointer to the HttpFetcher that does the http work.
- scoped_ptr<HttpFetcher> http_fetcher_;
-
// Used by TransferTerminated to figure if this action terminated itself or
// was terminated by the action processor.
ActionExitCode code_;
@@ -117,9 +120,6 @@
DownloadActionDelegate* delegate_;
uint64_t bytes_received_;
- // Global context for the system.
- SystemState* system_state_;
-
DISALLOW_COPY_AND_ASSIGN(DownloadAction);
};
diff --git a/update_attempter.cc b/update_attempter.cc
index 0c12ea7..e6a897a 100644
--- a/update_attempter.cc
+++ b/update_attempter.cc
@@ -605,7 +605,7 @@
// Also report the success code so that the percentiles can be
// interpreted properly for the remaining error codes in UMA.
- utils::SendErrorCodeToUMA(system_state_->metrics_lib(), code);
+ utils::SendErrorCodeToUma(system_state_->metrics_lib(), code);
return;
}
@@ -879,7 +879,7 @@
return false;
LOG(INFO) << "Update failed -- reporting the error event.";
- utils::SendErrorCodeToUMA(system_state_->metrics_lib(),
+ utils::SendErrorCodeToUma(system_state_->metrics_lib(),
error_event_->error_code);
shared_ptr<OmahaRequestAction> error_event_action(
new OmahaRequestAction(prefs_,
diff --git a/utils.cc b/utils.cc
index 1165db5..1267256 100644
--- a/utils.cc
+++ b/utils.cc
@@ -31,7 +31,6 @@
#include <rootdev/rootdev.h>
#include "update_engine/file_writer.h"
-#include "update_engine/install_plan.h"
#include "update_engine/omaha_request_params.h"
#include "update_engine/subprocess.h"
@@ -699,26 +698,29 @@
exp_time.second);
}
-void SendErrorCodeToUMA(MetricsLibraryInterface* metrics_lib,
+void SendErrorCodeToUma(MetricsLibraryInterface* metrics_lib,
ActionExitCode code)
{
- string metric = utils::IsNormalBootMode() ? "UpdateEngine.NormalErrorCodes" :
- "UpdateEngine.DevModeErrorCodes";
+ string metric = utils::IsNormalBootMode() ? "Installer.NormalErrorCodes" :
+ "Installer.DevModeErrorCodes";
// Ignore the higher order bits in the code by applying the mask as
// we want the enumerations to be in the small contiguous range
- // with values less than kNumBucketsForUMAMetrics.
- int actual_code = code & kActualCodeMask;
+ // with values less than kActionCodeUmaReportedMax.
+ int reported_code = code & kActualCodeMask;
// Make additional adjustments required for UMA.
- if (actual_code >= kActionCodeOmahaRequestHTTPResponseBase) {
+ // TODO(jaysri): Move this logic to UeErrorCode.cc when we
+ // fix BUG 34369.
+ if (reported_code >= kActionCodeOmahaRequestHTTPResponseBase) {
// Since we want to keep the enums to a small value, aggregate all HTTP
// errors into this one bucket for UMA purposes.
- actual_code = kActionCodeOmahaErrorInHTTPResponse;
+ reported_code = kActionCodeOmahaErrorInHTTPResponse;
}
- LOG(INFO) << "Sending " << actual_code << " to UMA metric: " << metric;
- metrics_lib->SendEnumToUMA(metric, actual_code, kNumBucketsForUMAMetrics);
+ LOG(INFO) << "Sending error code " << reported_code
+ << " to UMA metric: " << metric;
+ metrics_lib->SendEnumToUMA(metric, reported_code, kActionCodeUmaReportedMax);
}
diff --git a/utils.h b/utils.h
index e7bf288..555f54c 100644
--- a/utils.h
+++ b/utils.h
@@ -23,8 +23,6 @@
namespace chromeos_update_engine {
-class InstallPlan;
-
namespace utils {
// Returns true if this is an official Chrome OS build, false otherwise.
@@ -281,7 +279,7 @@
// Sends the error code to the appropriate bucket in UMA using the metrics_lib
// interface. This method also massages the error code to be suitable for UMA
// purposes.
-void SendErrorCodeToUMA(MetricsLibraryInterface* metrics_lib,
+void SendErrorCodeToUma(MetricsLibraryInterface* metrics_lib,
ActionExitCode code);
} // namespace utils