JDWP agent parameters manager

Current implementation uses string comparison which makes it hard to
work with parameters. Using a dedicated object improves readability
and maintainability.

This refactor also prepare the codebase to automatically fix filename
to use an absolute "data/data/..." path when needed (see next CL).

Test: NA
Bug: 286291596
Change-Id: I93ce1bfc63cd12a1fb41d64f151afd2c7ff5ef59
diff --git a/adbconnection/Android.bp b/adbconnection/Android.bp
index ecad650..ed8fa38 100644
--- a/adbconnection/Android.bp
+++ b/adbconnection/Android.bp
@@ -28,7 +28,10 @@
 cc_defaults {
     name: "adbconnection-defaults",
     host_supported: true,
-    srcs: ["adbconnection.cc"],
+    srcs: [
+        "adbconnection.cc",
+        "jdwpargs.cc",
+    ],
     defaults: ["art_defaults"],
 
     // Note that this tool needs to be built for both 32-bit and 64-bit since it requires
diff --git a/adbconnection/adbconnection.cc b/adbconnection/adbconnection.cc
index 239fe31..3ca9166 100644
--- a/adbconnection/adbconnection.cc
+++ b/adbconnection/adbconnection.cc
@@ -14,12 +14,19 @@
  * limitations under the License.
  */
 
+#include "adbconnection.h"
+
+#include <jni.h>
+#include <sys/eventfd.h>
+#include <sys/ioctl.h>
+#include <sys/socket.h>
+#include <sys/uio.h>
+#include <sys/un.h>
+
 #include <array>
 #include <cstddef>
 #include <iterator>
 
-#include "adbconnection.h"
-
 #include "adbconnection/client.h"
 #include "android-base/endian.h"
 #include "android-base/stringprintf.h"
@@ -32,27 +39,19 @@
 #include "base/mutex.h"
 #include "base/socket_peer_is_trusted.h"
 #include "debugger.h"
+#include "fd_transport.h"
+#include "jdwpargs.h"
 #include "jni/java_vm_ext.h"
 #include "jni/jni_env_ext.h"
 #include "mirror/class-alloc-inl.h"
 #include "mirror/throwable.h"
 #include "nativehelper/scoped_local_ref.h"
+#include "poll.h"
 #include "runtime-inl.h"
 #include "runtime_callbacks.h"
 #include "scoped_thread_state_change-inl.h"
 #include "well_known_classes.h"
 
-#include "fd_transport.h"
-
-#include "poll.h"
-
-#include <sys/ioctl.h>
-#include <sys/socket.h>
-#include <sys/uio.h>
-#include <sys/un.h>
-#include <sys/eventfd.h>
-#include <jni.h>
-
 namespace adbconnection {
 
 static constexpr size_t kJdwpHeaderLen = 11U;
@@ -863,16 +862,29 @@
 std::string AdbConnectionState::MakeAgentArg() {
   const std::string& opts = art::Runtime::Current()->GetJdwpOptions();
   DCHECK(ValidateJdwpOptions(opts));
+
+  VLOG(jdwp) << "Raw jdwp options '" + opts + "'";
+  JdwpArgs parameters = JdwpArgs(opts);
+
+  // See the comment above for why we need to be server=y. Since the agent defaults to server=n
+  // we must always set it.
+  parameters.put("server", "y");
+
+  // See the comment above for why we need to be suspend=n. Since the agent defaults to
+  // suspend=y we must always set it.
+  parameters.put("suspend", "n");
+
+  std::string ddm_already_active = "n";
+  if (notified_ddm_active_) {
+    ddm_already_active = "y";
+  }
+  parameters.put("ddm_already_active", ddm_already_active);
+
+  parameters.put("transport", "dt_fd_forward");
+  parameters.put("address", std::to_string(remote_agent_control_sock_));
+
   // TODO Get agent_name_ from something user settable?
-  return agent_name_ + "=" + opts + (opts.empty() ? "" : ",") +
-      "ddm_already_active=" + (notified_ddm_active_ ? "y" : "n") + "," +
-      // See the comment above for why we need to be server=y. Since the agent defaults to server=n
-      // we will add it if it wasn't already present for the convenience of the user.
-      (ContainsArgument(opts, "server=y") ? "" : "server=y,") +
-      // See the comment above for why we need to be suspend=n. Since the agent defaults to
-      // suspend=y we will add it if it wasn't already present.
-      (ContainsArgument(opts, "suspend=n") ? "" : "suspend=n,") +
-      "transport=dt_fd_forward,address=" + std::to_string(remote_agent_control_sock_);
+  return agent_name_ + "=" + parameters.join();
 }
 
 void AdbConnectionState::StopDebuggerThreads() {
diff --git a/adbconnection/jdwpargs.cc b/adbconnection/jdwpargs.cc
new file mode 100644
index 0000000..732713e
--- /dev/null
+++ b/adbconnection/jdwpargs.cc
@@ -0,0 +1,75 @@
+/*
+ * Copyright (C) 2023 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "jdwpargs.h"
+
+#include <algorithm>
+#include <sstream>
+
+#include "base/logging.h"  // For VLOG.
+
+namespace adbconnection {
+
+JdwpArgs::JdwpArgs(const std::string& opts) {
+  std::stringstream ss(opts);
+
+  // Split on ',' character
+  while (!ss.eof()) {
+    std::string w;
+    getline(ss, w, ',');
+
+    // Trim spaces
+    w.erase(std::remove_if(w.begin(), w.end(), ::isspace), w.end());
+
+    // Extract key=value
+    auto pos = w.find('=');
+
+    // Check for bad format such as no '=' or '=' at either extremity
+    if (pos == std::string::npos || w.back() == '=' || w.front() == '=') {
+      VLOG(jdwp) << "Skipping jdwp parameters '" << opts << "', token='" << w << "'";
+      continue;
+    }
+
+    // Set
+    std::string key = w.substr(0, pos);
+    std::string value = w.substr(pos + 1);
+    put(key, value);
+    VLOG(jdwp) << "Found jdwp parameters '" << key << "'='" << value << "'";
+  }
+}
+
+void JdwpArgs::put(const std::string& key, const std::string& value) {
+  if (store.find(key) == store.end()) {
+    keys.emplace_back(key);
+  }
+
+  store[key] = value;
+}
+
+std::string JdwpArgs::join() {
+  std::string opts;
+  for (const auto& key : keys) {
+    opts += key + "=" + store[key] + ",";
+  }
+
+  // Remove the trailing comma if there is one
+  if (opts.length() >= 2) {
+    opts = opts.substr(0, opts.length() - 1);
+  }
+
+  return opts;
+}
+}  // namespace adbconnection
diff --git a/adbconnection/jdwpargs.h b/adbconnection/jdwpargs.h
new file mode 100644
index 0000000..f3ade44
--- /dev/null
+++ b/adbconnection/jdwpargs.h
@@ -0,0 +1,50 @@
+/*
+ * Copyright (C) 2023 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef ART_ADBCONNECTION_JDWPARGS_H_
+#define ART_ADBCONNECTION_JDWPARGS_H_
+
+#include <string>
+#include <unordered_map>
+#include <vector>
+
+namespace adbconnection {
+
+// A key/value store which respects order of insertion when join the values.
+// This is necessary for jdwp agent parameter. e.g.: key "transport", must be
+// issued before "address", otherwise oj-libjdpw will crash.
+//
+// If a key were to be re-inserted (a.k.a overwritten), the first insertion
+// will be used for order.
+class JdwpArgs {
+ public:
+  explicit JdwpArgs(const std::string& opts);
+  ~JdwpArgs() = default;
+
+  // Add a key / value
+  void put(const std::string& key, const std::string& value);
+
+  // Concatenate all key/value into a command separated list of "key=value" entries.
+  std::string join();
+
+ private:
+  std::vector<std::string> keys;
+  std::unordered_map<std::string, std::string> store;
+};
+
+}  // namespace adbconnection
+
+#endif  // ART_ADBCONNECTION_JDWPARGS_H_