Make update over cellular an option for user

chromeos/dbus/update_engine_client.cc

 // Copyright (c) 2012 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 "chromeos/dbus/update_engine_client.h"
 
 #include <stdint.h>
 
 #include <algorithm>
 
@@ skipping 51 matching lines @@
   if (str == update_engine::kUpdateStatusVerifying)
     return UpdateEngineClient::UPDATE_STATUS_VERIFYING;
   if (str == update_engine::kUpdateStatusFinalizing)
     return UpdateEngineClient::UPDATE_STATUS_FINALIZING;
   if (str == update_engine::kUpdateStatusUpdatedNeedReboot)
     return UpdateEngineClient::UPDATE_STATUS_UPDATED_NEED_REBOOT;
   if (str == update_engine::kUpdateStatusReportingErrorEvent)
     return UpdateEngineClient::UPDATE_STATUS_REPORTING_ERROR_EVENT;
   if (str == update_engine::kUpdateStatusAttemptingRollback)
     return UpdateEngineClient::UPDATE_STATUS_ATTEMPTING_ROLLBACK;
+  if (str == update_engine::kUpdateStatusNeedPermissionToUpdate)
+    return UpdateEngineClient::UPDATE_STATUS_NEED_PERMISSION_TO_UPDATE;
   return UpdateEngineClient::UPDATE_STATUS_ERROR;
 }
 
 // Used in UpdateEngineClient::EmptyUpdateCheckCallback().
 void EmptyUpdateCheckCallbackBody(
     UpdateEngineClient::UpdateCheckResult unused_result) {
 }
 
 bool IsValidChannel(const std::string& channel) {
   return channel == kReleaseChannelDev || channel == kReleaseChannelBeta ||
@@ skipping 156 matching lines @@
 
     VLOG(1) << "Requesting UpdateEngine to " << (allowed ? "allow" : "prohibit")
             << " updates over cellular.";
 
     return update_engine_proxy_->CallMethod(
         &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
         base::Bind(&UpdateEngineClientImpl::OnSetUpdateOverCellularPermission,
                    weak_ptr_factory_.GetWeakPtr(), callback));
   }
 
+  void SetUpdateOverCellularTarget(const std::string& target_version,
+                                   int64_t target_size,
+                                   const SetTargetCallback& callback) override {
+    dbus::MethodCall method_call(update_engine::kUpdateEngineInterface,
+                                 update_engine::kSetUpdateOverCellularTarget);
+    dbus::MessageWriter writer(&method_call);
+    writer.AppendString(target_version);
+    writer.AppendInt64(target_size);
+
+    VLOG(1) << "Requesting UpdateEngine to allow updates over cellular "
+            << "to target version: \"" << target_version << "\" "
+            << "target_size: " << target_size;
+
+    return update_engine_proxy_->CallMethod(
+        &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+        base::Bind(&UpdateEngineClientImpl::OnSetUpdateOverCellularTarget,
+                   weak_ptr_factory_.GetWeakPtr(), callback));
+  }
+
  protected:
   void Init(dbus::Bus* bus) override {
     update_engine_proxy_ = bus->GetObjectProxy(
         update_engine::kUpdateEngineServiceName,
         dbus::ObjectPath(update_engine::kUpdateEngineServicePath));
     update_engine_proxy_->ConnectToSignal(
         update_engine::kUpdateEngineInterface,
         update_engine::kStatusUpdate,
         base::Bind(&UpdateEngineClientImpl::StatusUpdateReceived,
                    weak_ptr_factory_.GetWeakPtr()),
@@ skipping 193 matching lines @@
       }
     } else {
       LOG(ERROR) << kFailureMessage << "No response from DBus.";
     }
 
     // Callback should run anyway, regardless of whether DBus call to enable
     // update over cellular succeeded or failed.
     callback.Run();
   }
 
+  // Called when a response for SetUpdateOverCellularPermission() is received.
+  void OnSetUpdateOverCellularTarget(const SetTargetCallback& callback,
+                                     dbus::Response* response) {
+    constexpr char kFailureMessage[] =
+        "Failed to set UpdateEngine to allow updates over cellular to a given "

[Daniel Erat, 2017/05/12 13:37:03]

nit: s/UpdateEngine/update_engine/

[weidongg, 2017/05/12 16:33:16]

Done.

+        "target: ";
+
+    if (response) {

[Daniel Erat, 2017/05/12 13:37:03]

you don't need this lengthy error-handling code. see the implementation in
dbus/object_proxy.cc. i think the only case where you need to log an error
yourself is if response is null, e.g.

  if (!response) {
    LOG(ERROR) << update_engine::kSetUpdateOverCellularTarget
               << " call failed";
  }

[weidongg, 2017/05/12 16:33:16]

Done, should I modify the OnSetUpdateOverCellularPermission() using the same
strategy?

[Daniel Erat, 2017/05/12 16:44:48]

yes, please do. unnecessary error handling code like this just increases the
binary size. thanks!

please avoid copying-and-pasting code in general, too. if you actually needed
this, the right approach would be to refactor the common code into a new helper
function in the anonymous namespace.

[weidongg, 2017/05/12 17:00:15]

Ok, I see. Good to know.

+      switch (response->GetMessageType()) {
+        case dbus::Message::MESSAGE_ERROR:
+          LOG(ERROR) << kFailureMessage
+                     << "DBus responded with error: " << response->ToString();
+          break;
+        case dbus::Message::MESSAGE_INVALID:
+          LOG(ERROR) << kFailureMessage
+                     << "Invalid response from DBus (cannot be parsed).";
+          break;
+        default:
+          VLOG(1) << "Successfully set UpdateEngine to allow update over cell "
+                     "to a given target.";
+          callback.Run(true);
+          return;
+      }
+    } else {
+      LOG(ERROR) << kFailureMessage << "No response from DBus.";
+    }
+    callback.Run(false);
+  }
+
   // Called when a status update signal is received.
   void StatusUpdateReceived(dbus::Signal* signal) {
     VLOG(1) << "Status update signal received: " << signal->ToString();
     dbus::MessageReader reader(signal);
     int64_t last_checked_time = 0;
     double progress = 0.0;
     std::string current_operation;
     std::string new_version;
     int64_t new_size = 0;
     if (!(reader.PopInt64(&last_checked_time) &&
@@ skipping 83 matching lines @@
       callback.Run(target_channel_);
   }
 
   void GetEolStatus(const GetEolStatusCallback& callback) override {
     callback.Run(update_engine::EndOfLifeStatus::kSupported);
   }
 
   void SetUpdateOverCellularPermission(bool allowed,
                                        const base::Closure& callback) override {
     callback.Run();
   }

[Daniel Erat, 2017/05/12 13:37:03]

put blank lines between method definitions

[weidongg, 2017/05/12 16:33:16]

Done.

+  void SetUpdateOverCellularTarget(const std::string& target_version,
+                                   int64_t target_size,
+                                   const SetTargetCallback& callback) override {
+    // Do nothing.

[Daniel Erat, 2017/05/12 13:37:03]

delete comment

[weidongg, 2017/05/12 16:33:16]

Done.

+  }
 
   std::string current_channel_;
   std::string target_channel_;
 };
 
 // The UpdateEngineClient implementation used on Linux desktop, which
 // tries to emulate real update engine client.
 class UpdateEngineClientFakeImpl : public UpdateEngineClientStubImpl {
  public:
   UpdateEngineClientFakeImpl() : weak_factory_(this) {
@@ skipping 37 matching lines @@
  private:
   void StateTransition() {
     UpdateStatusOperation next_status = UPDATE_STATUS_ERROR;
     int delay_ms = kStateTransitionDefaultDelayMs;
     switch (last_status_.status) {
       case UPDATE_STATUS_ERROR:
       case UPDATE_STATUS_IDLE:
       case UPDATE_STATUS_UPDATED_NEED_REBOOT:
       case UPDATE_STATUS_REPORTING_ERROR_EVENT:
       case UPDATE_STATUS_ATTEMPTING_ROLLBACK:
+      case UPDATE_STATUS_NEED_PERMISSION_TO_UPDATE:
         return;
       case UPDATE_STATUS_CHECKING_FOR_UPDATE:
         next_status = UPDATE_STATUS_UPDATE_AVAILABLE;
         break;
       case UPDATE_STATUS_UPDATE_AVAILABLE:
         next_status = UPDATE_STATUS_DOWNLOADING;
         break;
       case UPDATE_STATUS_DOWNLOADING:
         if (last_status_.download_progress >= 1.0) {
           next_status = UPDATE_STATUS_VERIFYING;
@@ skipping 62 matching lines @@
   const char** cix = std::find(
       kReleaseChannelsList,
       kReleaseChannelsList + arraysize(kReleaseChannelsList), current_channel);
   const char** tix = std::find(
       kReleaseChannelsList,
       kReleaseChannelsList + arraysize(kReleaseChannelsList), target_channel);
   return tix > cix;
 }
 
 }  // namespace chromeos