device/bluetooth: Add I/O watcher for audio data retrieval triggered by state change.

chromeos/dbus/fake_bluetooth_media_transport_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/fake_bluetooth_media_transport_client.h"
 
+#include <unistd.h>
+#include <sys/socket.h>
+
 #include <sstream>
 
 #include "base/bind.h"
 #include "base/stl_util.h"
 #include "chromeos/dbus/bluetooth_media_client.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/fake_bluetooth_adapter_client.h"
 #include "chromeos/dbus/fake_bluetooth_media_client.h"
 #include "chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.h"
 
 using dbus::ObjectPath;
 
 namespace {
 
 // TODO(mcchou): Remove this constants once it is in cros_system_api.
 const char kBluetoothMediaTransportInterface[] = "org.bluez.MediaTransport1";
 const char kNotImplemented[] = "org.bluez.NotImplemented";
+const char kNotAuthorized[] = "org.bluez.NotAuthorized";
+const char kFailed[] = "org.bluez.Failed";
+const char kNotAvailable[] = "org.bluez.NotAvailable";
+
+const uint16_t kReadMtu = 20;
+const uint16_t kWriteMtu = 25;
 
 ObjectPath GenerateTransportPath() {
   static unsigned int sequence_number = 0;
   ++sequence_number;
   std::stringstream path;
   path << chromeos::FakeBluetoothAdapterClient::kAdapterPath
        << chromeos::FakeBluetoothMediaTransportClient::kTransportDevicePath
        << "/fd" << sequence_number;
   return ObjectPath(path.str());
 }
@@ skipping 48 matching lines @@
   properties.reset(transport_properties);
 }
 
 FakeBluetoothMediaTransportClient::Transport::~Transport() {
 }
 
 FakeBluetoothMediaTransportClient::FakeBluetoothMediaTransportClient() {
 }
 
 FakeBluetoothMediaTransportClient::~FakeBluetoothMediaTransportClient() {
-  for (auto& it : endpoint_to_transport_map_)
-    delete it.second;
-  endpoint_to_transport_map_.clear();
+  STLDeleteValues(&endpoint_to_transport_map_);
 }
 
 // DBusClient override.
 void FakeBluetoothMediaTransportClient::Init(dbus::Bus* bus) {
 }
 
 void FakeBluetoothMediaTransportClient::AddObserver(
     BluetoothMediaTransportClient::Observer* observer) {
   observers_.AddObserver(observer);
 }
 
 void FakeBluetoothMediaTransportClient::RemoveObserver(
     BluetoothMediaTransportClient::Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
 FakeBluetoothMediaTransportClient::Properties*
-    FakeBluetoothMediaTransportClient::GetProperties(
-        const ObjectPath& object_path) {
+FakeBluetoothMediaTransportClient::GetProperties(
+    const ObjectPath& object_path) {
   ObjectPath endpoint_path = GetEndpointPath(object_path);
-  if (!endpoint_path.IsValid() ||
-      !ContainsKey(endpoint_to_transport_map_, endpoint_path))
+  Transport* transport = GetTransport(endpoint_path);
+  if (!transport)
     return nullptr;
-  return endpoint_to_transport_map_[endpoint_path]->properties.get();
+  return transport->properties.get();
 }
 
 void FakeBluetoothMediaTransportClient::Acquire(
     const ObjectPath& object_path,
     const AcquireCallback& callback,
     const ErrorCallback& error_callback) {
-  error_callback.Run(kNotImplemented, "");
+  VLOG(1) << "Acquire - transport path: " << object_path.value();
+
+  ObjectPath endpoint_path = GetEndpointPath(object_path);

[armansito, 2015/03/12 03:42:54]

const ObjectPath&

[Miao, 2015/03/12 22:33:31]

Done.

+  Transport* transport = GetTransport(endpoint_path);
+  if (!transport) {
+    error_callback.Run(kFailed, "");
+    return;
+  }
+
+  std::string state = transport->properties->state.value();
+  if (state == "active") {
+    error_callback.Run(kNotAuthorized, "");
+    return;
+  }
+  if (state != "pending") {
+    error_callback.Run(kFailed, "");
+    return;
+  }
+
+  int fds[2];
+  if (socketpair(AF_UNIX, SOCK_STREAM, 0, fds) < 0) {
+    transport->input_fd.reset();
+    error_callback.Run(kFailed, "");
+    return;
+  }

[armansito, 2015/03/12 03:42:54]

Maybe add a DCHECK here that both fds[0] and fd[1] are positive.

[Miao, 2015/03/12 22:33:31]

Done.

+  transport->input_fd.reset(new base::File(fds[0]));
+
+  callback.Run(fds[1], kReadMtu, kWriteMtu);
+  SetState(endpoint_path, "active");
 }
 
 void FakeBluetoothMediaTransportClient::TryAcquire(
     const ObjectPath& object_path,
     const AcquireCallback& callback,
     const ErrorCallback& error_callback) {
-  error_callback.Run(kNotImplemented, "");
+  VLOG(1) << "TryAcquire - transport path: " << object_path.value();
+
+  ObjectPath endpoint_path = GetEndpointPath(object_path);

[armansito, 2015/03/12 03:42:54]

const ObjectPath&

[Miao, 2015/03/12 22:33:31]

Done.

+  Transport* transport = GetTransport(endpoint_path);

[armansito, 2015/03/12 03:42:54]

Maybe you want a helper like "GetTransportByPath" which returns a Transport
object using a transport path.

+  if (!transport) {
+    error_callback.Run(kFailed, "");
+    return;
+  }
+
+  std::string state = transport->properties->state.value();
+  if (state == "active") {
+    error_callback.Run(kNotAuthorized, "");
+    return;
+  }
+
+  // NotAvailable error is an extra error type for TryAcquire.
+  if (state != "pending") {
+    error_callback.Run(kNotAvailable, "");
+    return;
+  }
+
+  int fds[2];
+  if (socketpair(AF_UNIX, SOCK_STREAM, 0, fds) < 0) {
+    transport->input_fd.reset();
+    error_callback.Run(kFailed, "");
+    return;
+  }
+  transport->input_fd.reset(new base::File(fds[0]));
+
+  callback.Run(fds[1], kReadMtu, kWriteMtu);
+  SetState(endpoint_path, "active");

[armansito, 2015/03/12 03:42:54]

A lot of the code between TryAcquire and Acquire is the same. Perhaps you can
unify them in a helper?

[Miao, 2015/03/12 22:33:31]

Acquire and TryAcquire have different error messages for |error_callback|, so I
prefer not to unify them.

[armansito, 2015/03/12 23:59:07]

I don't think that's a good enough excuse to have so much duplicate code.

 }
 
 void FakeBluetoothMediaTransportClient::Release(
     const ObjectPath& object_path,
     const base::Closure& callback,
     const ErrorCallback& error_callback) {
   error_callback.Run(kNotImplemented, "");
 }
 
 void FakeBluetoothMediaTransportClient::SetValid(
@@ skipping 24 matching lines @@
     properties->state.ReplaceValue(BluetoothMediaTransportClient::kStateIdle);
     properties->delay.ReplaceValue(kTransportDelay);
     properties->volume.ReplaceValue(kTransportVolume);
 
     endpoint_to_transport_map_[endpoint_path] =
         new Transport(transport_path, properties.release());
     transport_to_endpoint_map_[transport_path] = endpoint_path;
     return;
   }
 
-  if (!ContainsKey(endpoint_to_transport_map_, endpoint_path))
+  Transport* transport = GetTransport(endpoint_path);
+  if (!transport)
     return;

[armansito, 2015/03/12 03:42:54]

nit: new line after 'return'

+  ObjectPath transport_path = transport->path;

[armansito, 2015/03/12 03:42:54]

const ObjectPath&

[Miao, 2015/03/12 22:33:31]

Done.

 
   // Notifies observers about the state change of the transport.
   FOR_EACH_OBSERVER(BluetoothMediaTransportClient::Observer, observers_,
-                    MediaTransportRemoved(GetTransportPath(endpoint_path)));
+                    MediaTransportRemoved(transport_path));
 
-  endpoint->ClearConfiguration(GetTransportPath(endpoint_path));
-  transport_to_endpoint_map_.erase(GetTransportPath(endpoint_path));
-  delete endpoint_to_transport_map_[endpoint_path];
+  endpoint->ClearConfiguration(transport_path);
+  delete transport;
   endpoint_to_transport_map_.erase(endpoint_path);
+  transport_to_endpoint_map_.erase(transport_path);
 }
 
 void FakeBluetoothMediaTransportClient::SetState(
-    const dbus::ObjectPath& endpoint_path,
+    const ObjectPath& endpoint_path,
     const std::string& state) {
-  if (!ContainsKey(endpoint_to_transport_map_, endpoint_path))
+  VLOG(1) << "SetState - state: " << state;
+
+  Transport* transport = GetTransport(endpoint_path);
+  if (!transport)
     return;
 
-  endpoint_to_transport_map_[endpoint_path]
-      ->properties->state.ReplaceValue(state);
+  transport->properties->state.ReplaceValue(state);
   FOR_EACH_OBSERVER(BluetoothMediaTransportClient::Observer, observers_,
                     MediaTransportPropertyChanged(
-                        GetTransportPath(endpoint_path),
+                        transport->path,
                         BluetoothMediaTransportClient::kStateProperty));
 }
 
 void FakeBluetoothMediaTransportClient::SetVolume(
-    const dbus::ObjectPath& endpoint_path,
+    const ObjectPath& endpoint_path,
     const uint16_t& volume) {
-  if (!ContainsKey(endpoint_to_transport_map_, endpoint_path))
+  Transport* transport = GetTransport(endpoint_path);
+  if (!transport)
     return;
 
-  endpoint_to_transport_map_[endpoint_path]->properties->volume.ReplaceValue(
-      volume);
+  transport->properties->volume.ReplaceValue(volume);
   FOR_EACH_OBSERVER(BluetoothMediaTransportClient::Observer, observers_,
                     MediaTransportPropertyChanged(
-                        GetTransportPath(endpoint_path),
+                        transport->path,
                         BluetoothMediaTransportClient::kVolumeProperty));
 }
 
+void FakeBluetoothMediaTransportClient::WriteData(
+    const ObjectPath& endpoint_path, const std::vector<char>& bytes) {
+  VLOG(1) << "WriteData";

[armansito, 2015/03/12 03:42:54]

Maybe log the number of bytes here? Might be useful for debugging.

[Miao, 2015/03/12 22:33:31]

Done.

+
+  Transport* transport = GetTransport(endpoint_path);
+
+  if (!transport || transport->properties->state.value() != "active") {
+    VLOG(1) << "WriteData - write operation rejected on endpoint: "

[armansito, 2015/03/12 03:42:54]

You should probably log here that this was rejected because the state isn't
"active". Will save you time later when reading the logs :)

[Miao, 2015/03/12 22:33:31]

Done.

+            << endpoint_path.value();
+    return;
+  }
+
+  if (!transport->input_fd.get()) {
+    VLOG(1) << "WriteData - invalid file descriptors";

[armansito, 2015/03/12 03:42:54]

nit: say "invalid input file descriptor"?

[Miao, 2015/03/12 22:33:31]

Done.

+    return;
+  }
+
+  ssize_t written_len =
+      write(transport->input_fd->GetPlatformFile(), bytes.data(), bytes.size());

[armansito, 2015/03/12 03:42:54]

I wonder if you should prevent sending more bytes than the current read MTU? At
least this would make the behavior consistent with BlueZ (but I guess you
wouldn't be able to test invalid behavior in that case, so maybe what you have
already is better).

[Miao, 2015/03/12 22:33:31]

I'll keep this for now.

[armansito, 2015/03/12 23:59:07]

sgtm

+  if (written_len < 0) {
+    VLOG(1) << "WriteData - failed to write to the socket";
+    return;
+  }
+
+  VLOG(1) << "WriteData - wrote " << written_len << " bytes to the socket";
+}
+
+FakeBluetoothMediaTransportClient::Transport*
+FakeBluetoothMediaTransportClient::GetTransport(
+    const ObjectPath& endpoint_path) {
+  auto it = endpoint_to_transport_map_.find(endpoint_path);

[armansito, 2015/03/12 03:42:54]

const auto& it

[Miao, 2015/03/12 22:33:31]

Done.

[Miao, 2015/03/12 22:33:31]

Done.

+  return (it != endpoint_to_transport_map_.end()) ? it->second : nullptr;
+}
+
+
 ObjectPath FakeBluetoothMediaTransportClient::GetTransportPath(
     const ObjectPath& endpoint_path) {
-  if (ContainsKey(endpoint_to_transport_map_, endpoint_path))
-    return endpoint_to_transport_map_[endpoint_path]->path;
-  return ObjectPath("");
+  Transport* transport = GetTransport(endpoint_path);
+  return transport ? transport->path : ObjectPath("");
 }
 
 ObjectPath FakeBluetoothMediaTransportClient::GetEndpointPath(
     const ObjectPath& transport_path) {
-  if (ContainsKey(transport_to_endpoint_map_, transport_path))
-    return transport_to_endpoint_map_[transport_path];
-  return ObjectPath("");
+  auto it = transport_to_endpoint_map_.find(transport_path);

[armansito, 2015/03/12 03:42:54]

const auto& it

[Miao, 2015/03/12 22:33:31]

Done.

+  return it != transport_to_endpoint_map_.end() ? it->second : ObjectPath("");
 }
 
 void FakeBluetoothMediaTransportClient::OnPropertyChanged(
     const std::string& property_name) {
   VLOG(1) << "Property " << property_name << " changed";
 }
 
 }  // namespace chromeos