arc-bridge: Add IPC message definitions

chromeos/arc/bridge/common/arc_bridge.cc

+// Copyright 2015 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 "arc_bridge.h"

[hidehiko, 2015/10/26 09:15:00]

nit: IIUC, probably we should use
#include "chromeos/arc/bridge/common/arc_bridge.h"

[Luis Héctor Chávez, 2015/10/26 21:03:27]

This code should be able to be built from within ARC, so the path would not
match there. Ideally this should behave more as a third_party/ directory, but I
decided against putting this code there since we are the only users of this
code. Once an actual shared repository exists, I can move it to third_party.

[satorux1, 2015/10/27 07:10:16]

The relative paths look worrisome to me. tools/git/move_source_file.py assumes
include paths are from the top level directory. There may be other tools that
has the same assumption.

Would it be difficult to get this code compile-able within ARC while using
verbose paths like "chromeos/arc/bridge/common/arc_bridge.h"?


Also, the "This code should be able to be built from within ARC" requirement is
unusual for code in Chrome. Maybe nice to add a README.md file in the directory
and explain this?

[Shuhei Takahashi, 2015/10/27 07:14:25]

Maybe this is another reason to avoid having ArcBridgeEndpoint implementations
in libarcbridge and provide IPC message definitions only instead.

[lhc(google), 2015/10/27 08:00:23]

Yes, it would be very difficult to compile this code within ARC with the verbose
paths. I guess there's no choice but to only have the IPC messages here.

+
+#include <stdint.h>
+
+#include <base/bind_helpers.h>

[hidehiko, 2015/10/26 09:15:00]

nit: #include "base/bind_helpers.h"
Ditto for below.

[Luis Héctor Chávez, 2015/10/26 21:03:27]

Done.

+#include <base/files/file_util.h>
+#include <ipc/ipc_channel.h>
+
+#include "arc_host_messages.h"

[hidehiko, 2015/10/26 09:15:00]

ditto. And sort in lexicographical order (with mixing base/... ipc/...).

[Luis Héctor Chávez, 2015/10/26 21:03:27]

Since we can't use the full path, I'm a bit wary of mixing the includes.

+#include "arc_instance_messages.h"
+
+namespace arc {
+
+BridgeEndpoint::BridgeEndpoint() : ipc_thread_("ARC bridge listener") {
+  ipc_thread_.StartWithOptions(
+      base::Thread::Options(base::MessageLoop::TYPE_IO, 0));
+}
+
+BridgeEndpoint::~BridgeEndpoint() {}
+
+bool BridgeEndpoint::Connect(const IPC::ChannelHandle& handle,
+                             IPC::Channel::Mode mode) {
+  channel_ = IPC::ChannelProxy::Create(handle, mode, this,
+                                       ipc_thread_.task_runner().get());
+  if (!channel_)
+    return false;
+
+  return true;
+}
+
+BridgeInstanceEndpoint::BridgeInstanceEndpoint() : BridgeEndpoint() {}
+
+BridgeInstanceEndpoint::~BridgeInstanceEndpoint() {}
+
+bool BridgeInstanceEndpoint::SocketConnect(const std::string& socket_path) {
+  return BridgeEndpoint::Connect(IPC::ChannelHandle(socket_path),
+                                 IPC::Channel::MODE_NAMED_CLIENT);
+}
+
+bool BridgeInstanceEndpoint::InstanceReady() {
+  return channel_->Send(new ArcHostMsg_InstanceReady());
+}
+
+bool BridgeInstanceEndpoint::OnMessageReceived(const IPC::Message& message) {
+  bool handled = true;
+
+  IPC_BEGIN_MESSAGE_MAP(BridgeInstanceEndpoint, message)
+    IPC_MESSAGE_HANDLER(ArcInstanceMsg_Ping, OnPing)
+    IPC_MESSAGE_UNHANDLED(handled = false)
+  IPC_END_MESSAGE_MAP()
+
+  if (!handled)
+    LOG(ERROR) << "Invalid message with type = " << message.type();
+  return handled;
+}
+
+void BridgeInstanceEndpoint::OnPing() {
+  channel_->Send(new ArcHostMsg_Pong());
+}
+
+BridgeHostEndpoint::BridgeHostEndpoint() : BridgeEndpoint() {}
+
+BridgeHostEndpoint::~BridgeHostEndpoint() {}
+
+bool BridgeHostEndpoint::SocketConnect(const std::string& socket_path) {
+  base::FilePath path = base::FilePath::FromUTF8Unsafe(socket_path);
+  if (!base::CreateDirectory(path.DirName()))

[hidehiko, 2015/10/26 09:15:00]

Clarification: Which thread will this be invoked on? File operation may not be
allowed. (Note: on the IO thread, it is not allowed conceptually (despite of its
name...)).

[Shuhei Takahashi, 2015/10/26 11:13:40]

And one more question: Is this really needed for our purpose? I remember init
scripts create the base directory.

[Luis Héctor Chávez, 2015/10/26 21:03:27]

This is run on the browser main thread. I'll add a comment.
The scripts do, but we need to stop using the global socket and part of the init
scripts. Part of the initialization needs to be per-profile and only after user
opt-in.

To expedite the code review, I'll remove this and note that the caller must
ensure the directory exists.

+    return false;
+
+  if (!BridgeEndpoint::Connect(IPC::ChannelHandle(path.value()),
+                               IPC::Channel::MODE_OPEN_NAMED_SERVER))
+    return false;
+  // TODO(lhchavez): Tighten the security around the socket by tying it to the

[Shuhei Takahashi, 2015/10/26 11:13:40]

Maybe better to file a bug so we don't remember to remove this?

+  // user the instance will run as.
+  if (HANDLE_EINTR(chmod(socket_path.c_str(), 0777)) != 0)
+    return false;
+  return true;
+}
+
+bool BridgeHostEndpoint::Ping() {
+  return channel_->Send(new ArcInstanceMsg_Ping());
+}
+
+bool BridgeHostEndpoint::OnMessageReceived(const IPC::Message& message) {
+  bool handled = true;
+
+  IPC_BEGIN_MESSAGE_MAP(BridgeHostEndpoint, message)
+    IPC_MESSAGE_HANDLER(ArcHostMsg_Pong, OnPong)
+    IPC_MESSAGE_HANDLER(ArcHostMsg_InstanceReady, OnInstanceReady)
+    IPC_MESSAGE_UNHANDLED(handled = false)
+  IPC_END_MESSAGE_MAP()
+
+  if (!handled)
+    LOG(ERROR) << "Invalid message with type = " << message.type();
+  return handled;
+}
+
+}  // namespace arc