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/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);
}
}