Linux pairing registry delegate implementation

remoting/host/pairing_registry_delegate_linux.cc

Index: remoting/host/pairing_registry_delegate_linux.cc
diff --git a/remoting/host/pairing_registry_delegate_linux.cc b/remoting/host/pairing_registry_delegate_linux.cc
new file mode 100644
index 0000000000000000000000000000000000000000..cdeef48bbf35820bf69f02c57736fa816a715ab7
--- /dev/null
+++ b/remoting/host/pairing_registry_delegate_linux.cc
@@ -0,0 +1,146 @@
+// Copyright 2013 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 "remoting/host/pairing_registry_delegate_linux.h"
+
+#include "base/bind.h"
+#include "base/environment.h"
+#include "base/file_util.h"
+#include "base/json/json_file_value_serializer.h"
+#include "base/location.h"
+#include "base/task_runner.h"
+#include "base/values.h"
+
+namespace {
+const char kConfigDir[] = ".config/chrome-remote-desktop";

[Sergey Ulanov, 2013/05/31 18:45:46]

There is remoting::GetConfigDir() that returns path to the config directory.

[Jamie, 2013/06/19 17:56:44]

Done.

+const char kRegistryFilename[] = "paired-clients";

[Sergey Ulanov, 2013/05/31 18:45:46]

Maybe paired-clients.json?
For host config we add a hash of the host name to deal with the case when home
directory is shared between multiple machines. I think it might be useful here
too. 
Config file path with the suffix normally comes from command line. One possible
solution is to replace --host-config option with --config-suffix and use the
suffix for both files.

[Jamie, 2013/06/19 17:56:44]

Done.
I think we're planning on moving to a model where the config directory is under
/var, in which case the suffix won't be needed, so I prefer not to add one.

+const char kCreatedTimeLowKey[] = "created-time-low";
+const char kCreatedTimeHighKey[] = "created-time-high";
+const char kClientIdKey[] = "client-id";
+const char kClientNameKey[] = "client-name";
+const char kSharedSecretKey[] = "shared-secret";
+}  // namespace
+
+
+namespace remoting {
+namespace protocol {
+
+PairingRegistryDelegateLinux::PairingRegistryDelegateLinux(
+    scoped_refptr<base::TaskRunner> task_runner)
+    : task_runner_(task_runner),
+      weak_factory_(this) {
+}
+
+void PairingRegistryDelegateLinux::Save(
+    const PairingRegistry::PairedClients& paired_clients) {
+  task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&PairingRegistryDelegateLinux::SaveOnCurrentThread,
+                 weak_factory_.GetWeakPtr(), paired_clients));
+}
+
+void PairingRegistryDelegateLinux::SaveOnCurrentThread(

[Sergey Ulanov, 2013/05/31 18:45:46]

Why is it "OnCurrentThread"? Should it be something like OnIoThread? Or maybe
just call it DoSave()

[Jamie, 2013/06/19 17:56:44]

The new interface has different functions, but I've adopted the Do* naming
convention for the implementation as you suggest.

+    const PairingRegistry::PairedClients& paired_clients) {
+  base::FilePath registry_path;
+  if (!GetRegistryFilePath(&registry_path)) {
+    return;
+  }
+
+  base::ListValue root;
+  for (PairingRegistry::PairedClients::const_iterator i =

[Sergey Ulanov, 2013/05/31 18:45:46]

It looks to me that most of the code in this class can be shared between
platforms. All formatting/parsing code can be shared. Is it possible to avoid
duplication? E.g. PairingRegistry could format the data, so Save() would accept
already formatted base::Value and so it would just need to save it at the right
place.
But I'm actually not sure why delegate needs to be platform specific. The only
part that's differs between platforms is the path to the file, everything else
will be the same, right?

[Lambros, 2013/05/31 18:51:59]

This serialization code looks like it would be the same on Linux/Mac/Win. Only
difference between Linux and Mac is the choice of filename. I guess on Windows
we might store the list in the Registry somehow, but wouldn't we just save the
whole JSON string into a Registry key? If it's too big for that, we'd probably
want to save it as a file on Windows too?

Maybe just have per-platform SaveJson()/LoadJson() implementations, and keep the
rest of the code cross-platform?

[Jamie, 2013/06/19 17:56:44]

I was concerned about permissions. For example, on Mac we can't support pairing
at all right now because the process needing write access to it might be running
as root or as the user who started the host. Similarly, on Windows, we'll
probably need a completely different implementation that works with the
multi-process architecture (based on a short discussion with Alex, it's likely
that this will be registry-based).

[Jamie, 2013/06/19 17:56:44]

I don't think all platforms will use JSON. On Windows we might use the registry,
which has support for structured data.

[Sergey Ulanov, 2013/06/19 18:23:50]

What are the benefits of using registry instead of storing that data in the file
system on windows too?
We already use JSON file to store host config on all platforms. Similarly chrome
doesn't store settings in the registry. So I think it's better to store the
pairing data in the file system for consistency, and that will allow more code
to be reused across platforms (meaning there will be fewer bugs and it will be
easier to maintain).

+           paired_clients.begin(); i != paired_clients.end(); ++i) {
+    base::DictionaryValue* pairing = new base::DictionaryValue();
+    int64 created_time = i->second.created_time.ToInternalValue();
+    pairing->SetInteger(kCreatedTimeLowKey, created_time & 0xffffffff);
+    pairing->SetInteger(kCreatedTimeHighKey, created_time >> 32);
+    pairing->SetString(kClientNameKey, i->second.client_name);
+    pairing->SetString(kClientIdKey, i->second.client_id);
+    pairing->SetString(kSharedSecretKey, i->second.shared_secret);
+    root.Append(pairing);
+  }
+
+  JSONFileValueSerializer registry(registry_path);

[Sergey Ulanov, 2013/05/31 18:45:46]

It's better to use base::ImportantFileWriter - it helps to avoid damaging the
file in case something goes wrong.

[Jamie, 2013/06/19 17:56:44]

Done.

+  registry.Serialize(root);

[Lambros, 2013/05/31 18:51:59]

I don't think JSONFileValueSerializer creates the directory. If you need to
create it here, you can use file_util::CreateDirectory() which looks like it
also sets the permissions we want.

[Jamie, 2013/06/19 17:56:44]

Done.

+}
+
+PairingRegistry::PairedClients
+PairingRegistryDelegateLinux::LoadOnCurrentThread() {
+  PairingRegistry::PairedClients result;
+  base::FilePath registry_path;
+  if (!GetRegistryFilePath(&registry_path)) {
+    return result;
+  }
+
+  JSONFileValueSerializer registry(registry_path);
+  int error_code;
+  std::string error_message;
+  scoped_ptr<base::Value> root(
+      registry.Deserialize(&error_code, &error_message));
+  if (!root) {
+    LOG(ERROR) << "Failed to load paired clients: " << error_message
+               << " (" << error_code << ").";
+    return result;
+  }
+
+  base::ListValue* root_list = NULL;
+  if (!root->GetAsList(&root_list)) {
+    LOG(ERROR) << "Failed to load paired clients: root node is not a list.";
+    return result;
+  }
+
+  for (size_t i = 0; i < root_list->GetSize(); ++i) {
+    base::DictionaryValue* pairing = NULL;
+    std::string client_id, client_name, shared_secret;
+    int created_time_low, created_time_high;
+    if (root_list->GetDictionary(i, &pairing) &&
+        pairing->GetInteger(kCreatedTimeLowKey, &created_time_low) &&
+        pairing->GetInteger(kCreatedTimeHighKey, &created_time_high) &&
+        pairing->GetString(kClientIdKey, &client_id) &&
+        pairing->GetString(kClientNameKey, &client_name) &&
+        pairing->GetString(kSharedSecretKey, &shared_secret)) {
+      PairingRegistry::Pairing pairing_struct;
+      pairing_struct.created_time = base::Time::FromInternalValue(
+          (static_cast<int64>(created_time_high) << 32) + created_time_low);

[Sergey Ulanov, 2013/05/31 18:45:46]

I'm not sure this will always work correctly. Particularly when 32'nd bit is set
- you'll get a negative value for the low part. It's better to use double to
store time instead of pair of integers.  

[Lambros, 2013/05/31 19:55:26]

Ah yes, I missed this! I think created_time_low/high need to be declared as
uint32.

Or serialize as decimal string using base/strings/string_number_conversions.h.

[Jamie, 2013/06/19 17:56:44]

Done.

[Jamie, 2013/06/19 17:56:44]

I've followed Lambros's suggestion of using string_number_conversions.h.

+      pairing_struct.client_id = client_id;
+      pairing_struct.client_name = client_name;
+      pairing_struct.shared_secret = shared_secret;
+      result[client_id] = pairing_struct;
+    } else {
+      LOG(ERROR) << "Paired client " << i << " has unexpected format.";
+    }
+  }
+
+  return result;
+}
+
+bool PairingRegistryDelegateLinux::GetRegistryFilePath(base::FilePath* path) {
+  if (!filename_for_testing_.empty()) {
+    *path = filename_for_testing_;
+    return true;
+  }
+
+  base::FilePath home = file_util::GetHomeDir();
+  if (home.empty()) {
+    LOG(ERROR) << "Could not locate user's home directory.";
+    return false;
+  }
+
+  *path = home.Append(kConfigDir).Append(kRegistryFilename);
+  return true;
+}
+
+void PairingRegistryDelegateLinux::SetFilenameForTesting(
+    const base::FilePath &filename) {
+  filename_for_testing_ = filename;
+}
+
+
+scoped_ptr<PairingRegistry::Delegate> PairingRegistry::CreateDelegate(
+    scoped_refptr<base::TaskRunner> task_runner) {
+  return scoped_ptr<PairingRegistry::Delegate>(
+      new PairingRegistryDelegateLinux(task_runner));
+}
+
+}  // namespace protocol
+}  // namespace remoting