Add new shill client for VPN

chromeos/dbus/shill_third_party_vpn_driver_client.cc

+// Copyright 2014 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/shill_third_party_vpn_driver_client.h"
+
+#include "base/bind.h"
+#include "base/macros.h"

[pneubeck (no reviews), 2014/11/10 16:44:07]

nit: not required if included in the header (see comment there)

[kaliamoorthi, 2014/11/11 14:58:33]

Done.

+#include "chromeos/dbus/shill_third_party_vpn_observer.h"
+#include "dbus/bus.h"
+#include "dbus/message.h"
+#include "dbus/object_proxy.h"
+#include "third_party/cros_system_api/dbus/service_constants.h"
+
+namespace chromeos {
+
+namespace {
+
+const char* kSetParametersKeyList[] = {
+    shill::kAddressParameterThirdPartyVpn,
+    shill::kBroadcastAddressParameterThirdPartyVpn,
+    shill::kGatewayParameterThirdPartyVpn,
+    shill::kBypassTunnelForIpParameterThirdPartyVpn,
+    shill::kSubnetPrefixParameterThirdPartyVpn,
+    shill::kMtuParameterThirdPartyVpn,
+    shill::kDomainSearchParameterThirdPartyVpn,
+    shill::kDnsServersParameterThirdPartyVpn};
+
+// The ShillThirdPartyVpnDriverClient implementation.
+class ShillThirdPartyVpnDriverClientImpl
+    : public ShillThirdPartyVpnDriverClient {
+ public:
+  ShillThirdPartyVpnDriverClientImpl();
+  ~ShillThirdPartyVpnDriverClientImpl() override;
+
+  // ShillThirdPartyVpnDriverClient overrides
+  void AddShillThirdPartyVpnObserver(
+      const dbus::ObjectPath& object_path,
+      ShillThirdPartyVpnObserver* observer) override;
+
+  void RemoveShillThirdPartyVpnObserver(
+      const dbus::ObjectPath& object_path) override;
+
+  void SetParameters(
+      const dbus::ObjectPath& object_path,
+      const base::DictionaryValue& parameters,
+      const base::Closure& callback,
+      const ShillClientHelper::ErrorCallback& error_callback) override;
+
+  void UpdateConnectionState(
+      const dbus::ObjectPath& object_path,
+      const uint32 connection_state,
+      const base::Closure& callback,
+      const ShillClientHelper::ErrorCallback& error_callback) override;
+
+  void SendPacket(
+      const dbus::ObjectPath& object_path,
+      const std::string& ip_packet,
+      const base::Closure& callback,
+      const ShillClientHelper::ErrorCallback& error_callback) override;
+
+ protected:
+  void Init(dbus::Bus* bus) override { bus_ = bus; }
+
+  ShillThirdPartyVpnDriverClient::TestInterface* GetTestInterface() override {
+    return NULL;

[pneubeck (no reviews), 2014/11/10 16:44:07]

nit: NULL -> nullptr , also everywhere else where you use NULL of pointer type

[kaliamoorthi, 2014/11/11 14:58:33]

Done.

[pneubeck (no reviews), 2014/11/12 14:21:07]

not done?
please run s/NULL/nullptr/ over your whole CL and change it where NULL is ptr.

+  }
+
+ private:
+  class HelperInfo {
+   public:
+    explicit HelperInfo(dbus::ObjectProxy* object_proxy);
+    ShillClientHelper* helper() { return &helper_; }
+    ShillThirdPartyVpnObserver* observer() { return observer_; }
+
+    void set_observer(ShillThirdPartyVpnObserver* observer) {
+      observer_ = observer;
+    }
+
+    base::WeakPtr<HelperInfo> GetWeakPtr() {
+      return weak_ptr_factory_.GetWeakPtr();
+    }
+
+   private:
+    ShillClientHelper helper_;
+    ShillThirdPartyVpnObserver* observer_;
+
+    base::WeakPtrFactory<HelperInfo> weak_ptr_factory_;
+  };
+  typedef std::map<std::string, HelperInfo*> HelperMap;

[pneubeck (no reviews), 2014/11/10 16:44:08]

typedef -> using

[kaliamoorthi, 2014/11/11 14:58:33]

Done.

+
+  void ReleaseHelper(const dbus::ObjectPath& object_path);

[pneubeck (no reviews), 2014/11/10 16:44:07]

please make it consistent whether you define functions inline or not.
considering the size of this, I'd suggest making nothing inline.

[pneubeck (no reviews), 2014/11/10 16:44:08]

'Release' is misleading. This term is usually used if ownership is released but
the object is not deleted.
Please rename to Remove/Delete

[kaliamoorthi, 2014/11/11 14:58:33]

Done.

[kaliamoorthi, 2014/11/11 14:58:33]

Done.

+
+  static void OnPacketReceived(base::WeakPtr<HelperInfo> helper_info,
+                               dbus::Signal* signal);
+  static void OnPlatformMessage(base::WeakPtr<HelperInfo> helper_info,
+                                dbus::Signal* signal);
+  static void OnSignalConnected(const std::string& interface,
+                                const std::string& signal,
+                                bool success);
+
+  // Returns the corresponding ShillClientHelper for the object_path.

[pneubeck (no reviews), 2014/11/10 16:44:07]

'Returns or creates'

[kaliamoorthi, 2014/11/11 14:58:33]

Done.

+  ShillClientHelper* GetHelper(const dbus::ObjectPath& object_path) {
+    return GetHelperInfo(object_path)->helper();
+  }
+
+  HelperInfo* FindHelperInfo(const dbus::ObjectPath& object_path) {
+    HelperMap::iterator it = helpers_.find(object_path.value());
+    return (it != helpers_.end()) ? it->second : NULL;
+  }
+
+  // Returns the corresponding HelperInfo for the object_path.

[pneubeck (no reviews), 2014/11/10 16:44:07]

'Returns or creates'

[kaliamoorthi, 2014/11/11 14:58:33]

Done.

+  HelperInfo* GetHelperInfo(const dbus::ObjectPath& object_path) {
+    HelperInfo* helper_info = FindHelperInfo(object_path);
+    if (helper_info)
+      return helper_info;
+
+    // There is no helper for the profile, create it.
+    dbus::ObjectProxy* object_proxy =
+        bus_->GetObjectProxy(shill::kFlimflamServiceName, object_path);
+    helper_info = new HelperInfo(object_proxy);
+    helpers_.insert(HelperMap::value_type(object_path.value(), helper_info));

[pneubeck (no reviews), 2014/11/10 16:44:08]

nit: using operator[] seems easier to read

[kaliamoorthi, 2014/11/11 14:58:33]

Done.

+    return helper_info;
+  }
+
+  dbus::Bus* bus_;
+  HelperMap helpers_;
+  std::set<std::string> valid_keys_;
+
+  DISALLOW_COPY_AND_ASSIGN(ShillThirdPartyVpnDriverClientImpl);
+};
+
+ShillThirdPartyVpnDriverClientImpl::HelperInfo::HelperInfo(
+    dbus::ObjectProxy* object_proxy)
+    : helper_(object_proxy), observer_(NULL), weak_ptr_factory_(this) {
+}
+
+ShillThirdPartyVpnDriverClientImpl::ShillThirdPartyVpnDriverClientImpl()
+    : bus_(NULL) {
+  for (uint32 i = 0; i < arraysize(kSetParametersKeyList); ++i) {

[pneubeck (no reviews), 2014/11/10 16:44:07]

nit: braces are not required

[kaliamoorthi, 2014/11/11 14:58:33]

I guess this is a matter of style. I prefer it this way.

+    valid_keys_.insert(kSetParametersKeyList[i]);
+  }
+}
+
+ShillThirdPartyVpnDriverClientImpl::~ShillThirdPartyVpnDriverClientImpl() {
+  for (auto& iter : helpers_) {
+    HelperInfo* helper_info = iter.second;
+    bus_->RemoveObjectProxy(
+        shill::kFlimflamServiceName,
+        helper_info->helper()->object_proxy()->object_path(),
+        base::Bind(&base::DoNothing));
+    delete helper_info;
+  }
+}
+
+void ShillThirdPartyVpnDriverClientImpl::AddShillThirdPartyVpnObserver(
+    const dbus::ObjectPath& object_path,
+    ShillThirdPartyVpnObserver* observer) {
+  HelperInfo* helper_info = GetHelperInfo(object_path);
+  if (helper_info->observer()) {
+    LOG(ERROR) << "Observer exists for " << object_path.value();
+    return;
+  }
+
+  helper_info->set_observer(observer);
+  dbus::ObjectProxy* proxy =
+      const_cast<dbus::ObjectProxy*>(helper_info->helper()->object_proxy());

[pneubeck (no reviews), 2014/11/10 16:44:07]

please try to get rid of the const_cast.
e.g. make a separate CL to remove the 'const' attribute from the object_proxy()
function

[kaliamoorthi, 2014/11/11 14:58:33]

I agree that this needs to be done. However, I am not sure about the order, so I
am introducing a TODO to fix this.

+
+  proxy->ConnectToSignal(
+      shill::kFlimflamThirdPartyVpnInterface, shill::kOnPlatformMessageFunction,
+      base::Bind(&ShillThirdPartyVpnDriverClientImpl::OnPlatformMessage,
+                 helper_info->GetWeakPtr()),
+      base::Bind(&ShillThirdPartyVpnDriverClientImpl::OnSignalConnected));
+
+  proxy->ConnectToSignal(
+      shill::kFlimflamThirdPartyVpnInterface, shill::kOnPacketReceivedFunction,
+      base::Bind(&ShillThirdPartyVpnDriverClientImpl::OnPacketReceived,
+                 helper_info->GetWeakPtr()),
+      base::Bind(&ShillThirdPartyVpnDriverClientImpl::OnSignalConnected));
+}
+
+void ShillThirdPartyVpnDriverClientImpl::RemoveShillThirdPartyVpnObserver(
+    const dbus::ObjectPath& object_path) {
+  HelperInfo* helper_info = FindHelperInfo(object_path);
+  if (!helper_info) {
+    LOG(ERROR) << "Unknown object_path " << object_path.value();
+    return;
+  }
+
+  CHECK(helper_info->observer());
+  helper_info->set_observer(NULL);
+  ReleaseHelper(object_path);

[pneubeck (no reviews), 2014/11/10 16:44:08]

potential BUG!

ReleaseHelper will remove the helper object and the contained ShillClientHelper
helper_, even if there is still a callback to the same object_path running!
Probably you should unify the ownership model with ShillClientHelper's reference
counting.

[kaliamoorthi, 2014/11/11 14:58:33]

We discussed this. But thanks for the thorough review.

[pneubeck (no reviews), 2014/11/12 14:21:07]

Consider that in the case that I explained, the callback from SetParameters will
not be run anymore without good reason.
This is still a bug, just not an as problematic one ('not calling a callback'
vs. 'crash')

[pneubeck (no reviews), 2014/11/13 14:06:11]

Note that as long as this is not clarified this CL shouldn't land.
There are several solutions. Either
 - add a clarifying comment and ideally a unit test that ensures that the
mentioned case works
 - change the implementation, so that it's obvious that it works
 - give examples that show that this usage/pattern is very common (I didn't find
any from a quick search, e.g. other ShillServiceClient does _not_ do this)

Note that the latter is very controversial, as the code should be understandable
to unfamiliar developers.

+}
+
+void ShillThirdPartyVpnDriverClientImpl::ReleaseHelper(
+    const dbus::ObjectPath& object_path) {
+  HelperInfo* helper_info = FindHelperInfo(object_path);
+  if (!helper_info) {
+    LOG(ERROR) << "Unknown object_path " << object_path.value();
+    return;
+  }
+
+  bus_->RemoveObjectProxy(shill::kFlimflamServiceName, object_path,
+                          base::Bind(&base::DoNothing));
+  helpers_.erase(helpers_.find(object_path.value()));
+  delete helper_info;
+}
+
+void ShillThirdPartyVpnDriverClientImpl::SetParameters(
+    const dbus::ObjectPath& object_path,
+    const base::DictionaryValue& parameters,
+    const base::Closure& callback,
+    const ShillClientHelper::ErrorCallback& error_callback) {
+  dbus::MethodCall method_call(shill::kFlimflamThirdPartyVpnInterface,
+                               shill::kSetParametersFunction);
+  dbus::MessageWriter writer(&method_call);
+  dbus::MessageWriter array_writer(NULL);
+  writer.OpenArray("{ss}", &array_writer);
+  for (base::DictionaryValue::Iterator it(parameters); !it.IsAtEnd();
+       it.Advance()) {
+    std::string value;

[pneubeck (no reviews), 2014/11/10 16:44:07]

nit: move to first usage

[kaliamoorthi, 2014/11/11 14:58:33]

Done.

+    if (valid_keys_.find(it.key()) == valid_keys_.end()) {
+      LOG(WARNING) << "Unknown key " << it.key();
+      continue;
+    }
+    if (!it.value().GetAsString(&value)) {
+      LOG(WARNING) << "Non string value " << it.value();
+      continue;
+    }
+    dbus::MessageWriter entry_writer(NULL);
+    array_writer.OpenDictEntry(&entry_writer);
+    entry_writer.AppendString(it.key());
+    entry_writer.AppendString(value);
+    array_writer.CloseContainer(&entry_writer);
+  }
+  writer.CloseContainer(&array_writer);
+  GetHelper(object_path)
+      ->CallVoidMethodWithErrorCallback(&method_call, callback, error_callback);

[pneubeck (no reviews), 2014/11/10 16:44:07]

here you're creating a helper that you only delete in
~ShillThirdPartyVpnDriverClientImpl or if RemoveShillThirdPartyVpnObserver is
used at some time for the same object_path.

You could use SetReleasedCallback, or at a minimum document that you're creating
a helper here and not deleting the helper when the callback returns.

[kaliamoorthi, 2014/11/11 14:58:33]

We discussed this. To many object creation an deletion is not necessary here. 

+}
+
+void ShillThirdPartyVpnDriverClientImpl::UpdateConnectionState(
+    const dbus::ObjectPath& object_path,
+    const uint32 connection_state,
+    const base::Closure& callback,
+    const ShillClientHelper::ErrorCallback& error_callback) {
+  dbus::MethodCall method_call(shill::kFlimflamThirdPartyVpnInterface,
+                               shill::kUpdateConnectionStateFunction);
+  dbus::MessageWriter writer(&method_call);
+  writer.AppendUint32(connection_state);
+  GetHelper(object_path)
+      ->CallVoidMethodWithErrorCallback(&method_call, callback, error_callback);
+}
+
+void ShillThirdPartyVpnDriverClientImpl::SendPacket(
+    const dbus::ObjectPath& object_path,
+    const std::string& ip_packet,
+    const base::Closure& callback,
+    const ShillClientHelper::ErrorCallback& error_callback) {
+  dbus::MethodCall method_call(shill::kFlimflamThirdPartyVpnInterface,
+                               shill::kSendPacketFunction);
+  dbus::MessageWriter writer(&method_call);
+  writer.AppendArrayOfBytes(reinterpret_cast<const uint8*>(ip_packet.data()),
+                            ip_packet.size());
+  GetHelper(object_path)
+      ->CallVoidMethodWithErrorCallback(&method_call, callback, error_callback);
+}
+
+// static
+void ShillThirdPartyVpnDriverClientImpl::OnPacketReceived(
+    base::WeakPtr<HelperInfo> helper_info,
+    dbus::Signal* signal) {
+  if (!helper_info || !helper_info->observer())
+    return;
+
+  dbus::MessageReader reader(signal);
+  const uint8* data;

[pneubeck (no reviews), 2014/11/10 16:44:07]

please initialize all primitives explicitly, just to be double sure that stuff
is deterministic

[kaliamoorthi, 2014/11/11 14:58:33]

Done.

+  size_t length;

[pneubeck (no reviews), 2014/11/10 16:44:08]

ditto: initialize

[kaliamoorthi, 2014/11/11 14:58:33]

Done.

+  if (reader.PopArrayOfBytes(&data, &length))
+    helper_info->observer()->OnPacketReceived(data, length);
+}
+
+// static
+void ShillThirdPartyVpnDriverClientImpl::OnPlatformMessage(
+    base::WeakPtr<HelperInfo> helper_info,
+    dbus::Signal* signal) {
+  if (!helper_info || !helper_info->observer())
+    return;
+
+  dbus::MessageReader reader(signal);
+  uint32 platform_message;

[pneubeck (no reviews), 2014/11/10 16:44:07]

ditto: initialize

[kaliamoorthi, 2014/11/11 14:58:33]

Done.

+  if (reader.PopUint32(&platform_message))
+    helper_info->observer()->OnPlatformMessage(platform_message);
+}
+
+// static
+void ShillThirdPartyVpnDriverClientImpl::OnSignalConnected(
+    const std::string& interface,
+    const std::string& signal,
+    bool success) {
+  LOG_IF(ERROR, !success) << "Connect to " << interface << " " << signal
+                          << " failed.";
+}
+
+}  // namespace
+
+ShillThirdPartyVpnDriverClient::ShillThirdPartyVpnDriverClient() {
+}
+
+ShillThirdPartyVpnDriverClient::~ShillThirdPartyVpnDriverClient() {
+}
+
+// static
+ShillThirdPartyVpnDriverClient* ShillThirdPartyVpnDriverClient::Create() {
+  return new ShillThirdPartyVpnDriverClientImpl();
+}
+
+}  // namespace chromeos