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 "dbus/bus.h"
+#include "dbus/message.h"
+#include "dbus/object_proxy.h"
+#include "third_party/cros_system_api/dbus/service_constants.h"
+
+namespace shill {
+
+const char kFlimflamThirdPartyVpnInterface[] =
+    "org.chromium.flimflam.ThirdPartyVpn";
+const char kSetParametersFunction[] = "SetParameters";
+const char kSendPacketFunction[] = "SendPacket";
+const char kUpdateConnectionStateFunction[] = "UpdateConnectionState";
+const char kOnPacketReceivedFunction[] = "OnPacketReceived";
+const char kOnPlatformMessageFunction[] = "OnPlatformMessage";
+const char* kSetParametersKeyList[] = {"address",
+                                       "broadcast_address",
+                                       "gateway",
+                                       "bypass_tunnel_for_ip",
+                                       "subnet_prefix",
+                                       "mtu",
+                                       "domain_search",
+                                       "dns_servers"};

[stevenjb, 2014/10/29 18:14:56]

Can we make these part of the ONC spec? Regardless, these keys should be defined
individually in a separate header so that other code can use the consts.

[kaliamoorthi, 2014/10/30 13:09:05]

We discussed ONC aspect, these values would be set by an VPN extension and the
values may not be known upfront so ONC would be an optional addition rather than
the only option. In normal operation mode, we would expect these values to be
set by the extension possibly controlled by policy for extensions.

I added a TODO to move it to service_constants.h. Initially it would be here but
once all the constants are finalized I'll move it to service_constants.h

[stevenjb, 2014/10/30 20:55:36]

It just occurred to me that this is lower level than what any extension API will
be using, so for now ONC or not doesn't really mater.

If Shill needs to parse these properties, then they definitely need to be part
of service_constants. We really should define all of these there first, even if
they might change later, but for expediency I am OK with defining them here
first, but they really need to be defined as individual constants, not just as
an array. (And the array itself should be defined in the anonymous namespace).

+}  // namespace shill
+
+namespace chromeos {
+
+namespace {
+
+// The ShillThirdPartyVpnDriverClient implementation.
+class ShillThirdPartyVpnDriverClientImpl
+    : public ShillThirdPartyVpnDriverClient {
+ public:
+  ShillThirdPartyVpnDriverClientImpl();
+  virtual ~ShillThirdPartyVpnDriverClientImpl();

[stevenjb, 2014/10/29 18:14:56]

Use the new override style:
~ShillThirdPartyVpnDriverClientImpl() override;
+ no 'virtual' in the overrides below.

[kaliamoorthi, 2014/10/30 13:09:05]

Done.

+
+  virtual void AddShillThirdPartyVpnObserver(
+      const dbus::ObjectPath& object_path,
+      ShillThirdPartyVpnObserver* observer) override;
+
+  virtual void RemoveShillThirdPartyVpnObserver(
+      const dbus::ObjectPath& object_path) override;
+
+  virtual void SetParameters(const dbus::ObjectPath& object_path,
+                             const base::DictionaryValue& parameters,
+                             const VoidDBusMethodCallback& callback) override;
+
+  virtual void UpdateConnectionState(
+      const dbus::ObjectPath& object_path,
+      const uint32 connection_state,
+      const VoidDBusMethodCallback& callback) override;
+
+  virtual void SendPacket(const dbus::ObjectPath& object_path,
+                          const std::vector<uint8>& ip_packet,
+                          const VoidDBusMethodCallback& callback) override;
+
+ protected:
+  virtual void Init(dbus::Bus* bus) override { bus_ = bus; }
+
+ private:
+  struct HelperInfo {
+    explicit HelperInfo(dbus::ObjectProxy* object_proxy);
+    ShillClientHelper helper;
+    ShillThirdPartyVpnObserver* observer;
+
+    ShillClientHelper* get_helper() { return &helper; }

[stevenjb, 2014/10/29 18:14:56]

Don't add this, just use &(info->helper), and see other comments.

[kaliamoorthi, 2014/10/30 13:09:05]

Done.

+  };
+  typedef std::map<std::string, HelperInfo*> HelperMap;
+
+  void ReleaseHelper(const dbus::ObjectPath& object_path);
+
+  static void OnPacketReceived(HelperInfo* helper_info, dbus::Signal* signal);
+  static void OnPlatformMessage(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 profile.
+  ShillClientHelper* GetHelper(const dbus::ObjectPath& service_path) {
+    HelperMap::iterator it = helpers_.find(service_path.value());
+    if (it != helpers_.end())
+      return it->second->get_helper();

[stevenjb, 2014/10/29 18:14:56]

I would have this return a HelperInfo*, otherwise tracking ownership/lifetime of
the result of this becomes difficult (i.e. we need to ensure that the result
can't be used after the HelperInfo object is destroyed, which is easier to do if
we just pass a pointer to the HelperInfo object itself).

[kaliamoorthi, 2014/10/30 13:09:04]

Maybe I don't understand this comment, HelperIno and helper get destroyed
together. The only inconsistent state is if HelperInfo is destroyed and there is
still a pointer to it in helpers_. Is this the condition you are talking about?
This should not happen since when helper_info is deleted, it is also removed
from helpers_.

We'll discuss this offline.

[stevenjb, 2014/10/30 20:55:36]

Yes, I understand that ShillClientHelper is owned by HelperInfo and will only be
deleted with it, but at first glance this is confusing. Using
GetHelperInfo()->helper would make it clear that this function is managing
HelperInfo instances, and 'helper' is a member of that. Alternatively you could
split this into GetHelperInfo and have GetHelper() return
&GetHelperInfo()->helper, but the current code confused me initially.

+
+    // There is no helper for the profile, create it.
+    dbus::ObjectProxy* object_proxy =
+        bus_->GetObjectProxy(shill::kFlimflamServiceName, service_path);
+    HelperInfo* helper_info = new HelperInfo(object_proxy);
+    helpers_.insert(HelperMap::value_type(service_path.value(), helper_info));
+    return helper_info->get_helper();
+  }
+
+  dbus::Bus* bus_;
+  HelperMap helpers_;
+  std::set<std::string> valid_keys;

[stevenjb, 2014/10/29 18:14:56]

valid_keys_

[kaliamoorthi, 2014/10/30 13:09:04]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(ShillThirdPartyVpnDriverClientImpl);
+};
+
+ShillThirdPartyVpnDriverClientImpl::HelperInfo::HelperInfo(
+    dbus::ObjectProxy* object_proxy)
+    : helper(object_proxy), observer(NULL) {

[stevenjb, 2014/10/29 18:14:56]

One line per member initializer (google_clang_format is your friend)

[kaliamoorthi, 2014/10/30 13:09:04]

I use clang format already and it formatted it this way. I verified the coding
guideline as well, it says if the entire list can fit in a line, it is okay.
Otherwise put it in seperate line.

If you still insist, I can make this change in the end. As I am modifying the
code, I would rely on clang format and it would put it in a single line again,
making me repeat the work.

[stevenjb, 2014/10/30 20:55:36]

Yeah, no, that's a semi recent change with clang-format that I'd forgotten, this
is fine.

+}
+
+ShillThirdPartyVpnDriverClientImpl::ShillThirdPartyVpnDriverClientImpl()
+    : bus_(NULL) {
+  for (uint32 i = 0; i < sizeof(shill::kSetParametersKeyList) /
+                          sizeof(shill::kSetParametersKeyList[0]);
+       ++i) {
+    valid_keys.insert(shill::kSetParametersKeyList[i]);
+  }
+}
+
+ShillThirdPartyVpnDriverClientImpl::~ShillThirdPartyVpnDriverClientImpl() {
+  for (HelperMap::iterator iter = helpers_.begin(); iter != helpers_.end();
+       ++iter) {
+    HelperInfo* helper_info = iter->second;
+    bus_->RemoveObjectProxy(
+        shill::kFlimflamServiceName,
+        helper_info->get_helper()->object_proxy()->object_path(),
+        base::Bind(&base::DoNothing));
+    delete helper_info;
+  }
+}
+
+void ShillThirdPartyVpnDriverClientImpl::AddShillThirdPartyVpnObserver(
+    const dbus::ObjectPath& object_path,
+    ShillThirdPartyVpnObserver* observer) {
+  bus_->AssertOnOriginThread();
+  HelperMap::iterator it = helpers_.find(object_path.value());
+  CHECK(it != helpers_.end());
+  if (it != helpers_.end()) {

[stevenjb, 2014/10/29 18:14:56]

Either {D}CHECK or add a test, not both. (This if would never fail).
Since the CHECK could potentially introduce edge cases that result in a crash, I
would suggest using LOG(ERROR) and an early return if it == helpers_.end().

[kaliamoorthi, 2014/10/30 13:09:05]

Done.

+    HelperInfo* helper_info = it->second;
+    CHECK(helper_info->observer == NULL);
+    helper_info->observer = observer;
+    dbus::ObjectProxy* proxy = const_cast<dbus::ObjectProxy*>(
+        helper_info->get_helper()->object_proxy());
+
+    proxy->ConnectToSignal(
+        shill::kFlimflamThirdPartyVpnInterface,
+        shill::kOnPlatformMessageFunction,
+        base::Bind(&ShillThirdPartyVpnDriverClientImpl::OnPlatformMessage,
+                   helper_info),
+        base::Bind(&ShillThirdPartyVpnDriverClientImpl::OnSignalConnected));
+
+    proxy->ConnectToSignal(
+        shill::kFlimflamThirdPartyVpnInterface,
+        shill::kOnPacketReceivedFunction,
+        base::Bind(&ShillThirdPartyVpnDriverClientImpl::OnPacketReceived,
+                   helper_info),
+        base::Bind(&ShillThirdPartyVpnDriverClientImpl::OnSignalConnected));
+  }
+}
+
+void ShillThirdPartyVpnDriverClientImpl::RemoveShillThirdPartyVpnObserver(
+    const dbus::ObjectPath& object_path) {
+  bus_->AssertOnOriginThread();
+  HelperMap::iterator it = helpers_.find(object_path.value());
+  CHECK(it != helpers_.end());
+  if (it != helpers_.end()) {

[stevenjb, 2014/10/29 18:14:56]

Same here.

[kaliamoorthi, 2014/10/30 13:09:04]

Done.

+    HelperInfo* helper_info = it->second;
+    CHECK(helper_info->observer);
+    helper_info->observer = NULL;
+    ReleaseHelper(object_path);
+  }
+}
+
+void ShillThirdPartyVpnDriverClientImpl::ReleaseHelper(
+    const dbus::ObjectPath& object_path) {
+  HelperMap::iterator it = helpers_.find(object_path.value());
+  if (it != helpers_.end()) {

[stevenjb, 2014/10/29 18:14:56]

Early exit.

[kaliamoorthi, 2014/10/30 13:09:05]

Done.

+    HelperInfo* helper_info = it->second;
+    bus_->RemoveObjectProxy(shill::kFlimflamServiceName, object_path,
+                            base::Bind(&base::DoNothing));
+    helpers_.erase(it);
+    delete helper_info;
+  }
+}
+
+void ShillThirdPartyVpnDriverClientImpl::SetParameters(
+    const dbus::ObjectPath& object_path,
+    const base::DictionaryValue& parameters,
+    const VoidDBusMethodCallback& 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;
+    if (valid_keys.find(it.key()) != valid_keys.end() &&
+        it.value().GetAsString(&value)) {
+      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)->CallVoidMethod(&method_call, callback);
+}
+
+void ShillThirdPartyVpnDriverClientImpl::UpdateConnectionState(
+    const dbus::ObjectPath& object_path,
+    const uint32 connection_state,
+    const VoidDBusMethodCallback& callback) {
+  dbus::MethodCall method_call(shill::kFlimflamThirdPartyVpnInterface,
+                               shill::kUpdateConnectionStateFunction);
+  dbus::MessageWriter writer(&method_call);
+  writer.AppendUint32(connection_state);
+  GetHelper(object_path)->CallVoidMethod(&method_call, callback);
+}
+
+void ShillThirdPartyVpnDriverClientImpl::SendPacket(
+    const dbus::ObjectPath& object_path,
+    const std::vector<uint8>& ip_packet,
+    const VoidDBusMethodCallback& callback) {
+  dbus::MethodCall method_call(shill::kFlimflamThirdPartyVpnInterface,
+                               shill::kSendPacketFunction);
+  dbus::MessageWriter writer(&method_call);
+  writer.AppendArrayOfBytes(ip_packet.data(), ip_packet.size());
+  GetHelper(object_path)->CallVoidMethod(&method_call, callback);
+}
+
+// static
+void ShillThirdPartyVpnDriverClientImpl::OnPacketReceived(
+    ShillThirdPartyVpnDriverClientImpl::HelperInfo* helper_info,
+    dbus::Signal* signal) {
+  if (helper_info->observer) {

[stevenjb, 2014/10/29 18:14:56]

Early exit.

[kaliamoorthi, 2014/10/30 13:09:04]

Done.

+    dbus::MessageReader reader(signal);
+    const uint8* data;
+    size_t length;
+    if (reader.PopArrayOfBytes(&data, &length))
+      helper_info->observer->OnPacketReceived(data, length);
+  }
+}
+
+// static
+void ShillThirdPartyVpnDriverClientImpl::OnPlatformMessage(
+    ShillThirdPartyVpnDriverClientImpl::HelperInfo* helper_info,
+    dbus::Signal* signal) {
+  if (helper_info->observer) {

[stevenjb, 2014/10/29 18:14:56]

Early exit.

[kaliamoorthi, 2014/10/30 13:09:05]

Done.

+    dbus::MessageReader reader(signal);
+    uint32 platform_message;
+    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