inspector: move options parsing

As inspector functionality expands, more options will need to be added.
Currently this requires changing adding function arguments, etc. This
change packs the veriables into a single class that can be extended
without changing APIs.

PR-URL: https://github.com/nodejs/node/pull/9691
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

node.gyp

@@ -154,6 +154,7 @@
         'src/node_config.cc',
         'src/node_constants.cc',
         'src/node_contextify.cc',
+        'src/node_debug_options.cc',
         'src/node_file.cc',
         'src/node_http_parser.cc',
         'src/node_javascript.cc',
@@ -194,6 +195,7 @@
         'src/node.h',
         'src/node_buffer.h',
         'src/node_constants.h',
+        'src/node_debug_options.h',
         'src/node_file.h',
         'src/node_http_parser.h',
         'src/node_internals.h',

src/debug-agent.cc

@@ -50,7 +50,6 @@ using v8::Value;
 
 
 Agent::Agent(Environment* env) : state_(kNone),
-                                 port_(5858),
                                  wait_(false),
                                  parent_env_(env),
                                  child_env_(nullptr),
@@ -69,7 +68,7 @@ Agent::~Agent() {
 }
 
 
-bool Agent::Start(const char* host, int port, bool wait) {
+bool Agent::Start(const DebugOptions& options) {
   int err;
 
   if (state_ == kRunning)
@@ -85,9 +84,8 @@ bool Agent::Start(const char* host, int port, bool wait) {
     goto async_init_failed;
   uv_unref(reinterpret_cast<uv_handle_t*>(&child_signal_));
 
-  host_ = host;
+  options_ = options;
-  port_ = port;
+  wait_ = options_.wait_for_connect();
-  wait_ = wait;
 
   err = uv_thread_create(&thread_,
                          reinterpret_cast<uv_thread_cb>(ThreadCb),
@@ -210,9 +208,11 @@ void Agent::InitAdaptor(Environment* env) {
 
   api->Set(String::NewFromUtf8(isolate, "host",
                                NewStringType::kNormal).ToLocalChecked(),
-           String::NewFromUtf8(isolate, host_.data(), NewStringType::kNormal,
+           String::NewFromUtf8(isolate, options_.host_name().data(),
-                               host_.size()).ToLocalChecked());
+                               NewStringType::kNormal,
-  api->Set(String::NewFromUtf8(isolate, "port"), Integer::New(isolate, port_));
+                               options_.host_name().size()).ToLocalChecked());
+  api->Set(String::NewFromUtf8(isolate, "port"),
+           Integer::New(isolate, options_.port()));
 
   env->process_object()->Set(String::NewFromUtf8(isolate, "_debugAPI"), api);
   api_.Reset(env->isolate(), api);

src/debug-agent.h

@@ -25,6 +25,7 @@
 #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
 
 #include "node_mutex.h"
+#include "node_debug_options.h"
 #include "util.h"
 #include "util-inl.h"
 #include "uv.h"
@@ -76,7 +77,7 @@ class Agent {
   typedef void (*DispatchHandler)(node::Environment* env);
 
   // Start the debugger agent thread
-  bool Start(const char* host, int port, bool wait);
+  bool Start(const DebugOptions& options);
   // Listen for debug events
   void Enable();
   // Stop the debugger agent
@@ -114,9 +115,8 @@ class Agent {
   };
 
   State state_;
+  DebugOptions options_;
 
-  std::string host_;
-  int port_;
   bool wait_;
 
   uv_sem_t start_sem_;

src/inspector_agent.cc

@@ -132,7 +132,8 @@ class AgentImpl {
   ~AgentImpl();
 
   // Start the inspector agent thread
-  bool Start(v8::Platform* platform, const char* path, int port, bool wait);
+  bool Start(v8::Platform* platform, const char* path,
+             const DebugOptions& options);
   // Stop the inspector agent
   void Stop();
 
@@ -169,6 +170,7 @@ class AgentImpl {
   void NotifyMessageReceived();
   State ToState(State state);
 
+  DebugOptions options_;
   uv_sem_t start_sem_;
   ConditionVariable incoming_message_cond_;
   Mutex state_lock_;
@@ -176,8 +178,6 @@ class AgentImpl {
   uv_loop_t child_loop_;
 
   InspectorAgentDelegate* delegate_;
-
-  int port_;
   bool wait_;
   bool shutting_down_;
   State state_;
@@ -194,6 +194,8 @@ class AgentImpl {
   InspectorSocketServer* server_;
 
   std::string script_name_;
+  std::string script_path_;
+  const std::string id_;
 
   friend class ChannelImpl;
   friend class DispatchOnInspectorBackendTask;
@@ -318,7 +320,6 @@ class V8NodeInspector : public v8_inspector::V8InspectorClient {
 };
 
 AgentImpl::AgentImpl(Environment* env) : delegate_(nullptr),
-                                         port_(0),
                                          wait_(false),
                                          shutting_down_(false),
                                          state_(State::kNew),
@@ -409,7 +410,10 @@ void InspectorWrapConsoleCall(const v8::FunctionCallbackInfo<v8::Value>& args) {
 }
 
 bool AgentImpl::Start(v8::Platform* platform, const char* path,
-                      int port, bool wait) {
+                      const DebugOptions& options) {
+  options_ = options;
+  wait_ = options.wait_for_connect();
+
   auto env = parent_env_;
   inspector_ = new V8NodeInspector(this, env, platform);
   platform_ = platform;
@@ -421,9 +425,6 @@ bool AgentImpl::Start(v8::Platform* platform, const char* path,
   int err = uv_loop_init(&child_loop_);
   CHECK_EQ(err, 0);
 
-  port_ = port;
-  wait_ = wait;
-
   err = uv_thread_create(&thread_, AgentImpl::ThreadCbIO, this);
   CHECK_EQ(err, 0);
   uv_sem_wait(&start_sem_);
@@ -433,7 +434,7 @@ bool AgentImpl::Start(v8::Platform* platform, const char* path,
     return false;
   }
   state_ = State::kAccepting;
-  if (wait) {
+  if (options_.wait_for_connect()) {
     DispatchMessages();
   }
   return true;
@@ -561,7 +562,7 @@ void AgentImpl::WorkerRunIO() {
   }
   InspectorAgentDelegate delegate(this, script_path, script_name_, wait_);
   delegate_ = &delegate;
-  InspectorSocketServer server(&delegate, port_);
+  InspectorSocketServer server(&delegate, options_.port());
   if (!server.Start(&child_loop_)) {
     fprintf(stderr, "Unable to open devtools socket: %s\n", uv_strerror(err));
     state_ = State::kError;  // Safe, main thread is waiting on semaphore
@@ -681,8 +682,8 @@ Agent::~Agent() {
 }
 
 bool Agent::Start(v8::Platform* platform, const char* path,
-                  int port, bool wait) {
+                  const DebugOptions& options) {
-  return impl->Start(platform, path, port, wait);
+  return impl->Start(platform, path, options);
 }
 
 void Agent::Stop() {

src/inspector_agent.h

@@ -7,6 +7,8 @@
 #error("This header can only be used when inspector is enabled")
 #endif
 
+#include "node_debug_options.h"
+
 // Forward declaration to break recursive dependency chain with src/env.h.
 namespace node {
 class Environment;
@@ -31,7 +33,8 @@ class Agent {
   ~Agent();
 
   // Start the inspector agent thread
-  bool Start(v8::Platform* platform, const char* path, int port, bool wait);
+  bool Start(v8::Platform* platform, const char* path,
+             const DebugOptions& options);
   // Stop the inspector agent
   void Stop();
 

src/node.cc

@@ -7,6 +7,7 @@
 #include "node_version.h"
 #include "node_internals.h"
 #include "node_revert.h"
+#include "node_debug_options.h"
 
 #if defined HAVE_PERFCTR
 #include "node_counters.h"
@@ -140,17 +141,6 @@ static bool track_heap_objects = false;
 static const char* eval_string = nullptr;
 static unsigned int preload_module_count = 0;
 static const char** preload_modules = nullptr;
-#if HAVE_INSPECTOR
-static bool use_inspector = false;
-#else
-static const bool use_inspector = false;
-#endif
-static bool use_debug_agent = false;
-static bool debug_wait_connect = false;
-static std::string* debug_host;  // coverity[leaked_storage]
-static const int default_debugger_port = 5858;
-static const int default_inspector_port = 9229;
-static int debug_port = -1;
 static const int v8_default_thread_pool_size = 4;
 static int v8_thread_pool_size = v8_default_thread_pool_size;
 static bool prof_process = false;
@@ -197,6 +187,8 @@ static uv_async_t dispatch_debug_messages_async;
 static Mutex node_isolate_mutex;
 static v8::Isolate* node_isolate;
 
+static node::DebugOptions debug_options;
+
 static struct {
 #if NODE_USE_V8_PLATFORM
   void Initialize(int thread_pool_size) {
@@ -213,14 +205,12 @@ static struct {
     platform_ = nullptr;
   }
 
-  bool StartInspector(Environment *env, const char* script_path,
-                      int port, bool wait) {
 #if HAVE_INSPECTOR
-    return env->inspector_agent()->Start(platform_, script_path, port, wait);
+  bool StartInspector(Environment *env, const char* script_path,
-#else
+                      const node::DebugOptions& options) {
-    return true;
+    return env->inspector_agent()->Start(platform_, script_path, options);
-#endif  // HAVE_INSPECTOR
   }
+#endif  // HAVE_INSPECTOR
 
   v8::Platform* platform_;
 #else  // !NODE_USE_V8_PLATFORM
@@ -2520,7 +2510,7 @@ void FatalException(Isolate* isolate,
 
   if (exit_code) {
 #if HAVE_INSPECTOR
-    if (use_inspector) {
+    if (debug_options.inspector_enabled()) {
       env->inspector_agent()->FatalException(error, message);
     }
 #endif
@@ -2924,17 +2914,14 @@ static Local<Object> GetFeatures(Environment* env) {
 
 static void DebugPortGetter(Local<Name> property,
                             const PropertyCallbackInfo<Value>& info) {
-  int port = debug_port;
+  info.GetReturnValue().Set(debug_options.port());
-  if (port < 0)
-    port = use_inspector ? default_inspector_port : default_debugger_port;
-  info.GetReturnValue().Set(port);
 }
 
 
 static void DebugPortSetter(Local<Name> property,
                             Local<Value> value,
                             const PropertyCallbackInfo<void>& info) {
-  debug_port = value->Int32Value();
+  debug_options.set_port(value->Int32Value());
 }
 
 
@@ -3277,7 +3264,7 @@ void SetupProcessObject(Environment* env,
   }
 
   // --debug-brk
-  if (debug_wait_connect) {
+  if (debug_options.wait_for_connect()) {
     READONLY_PROPERTY(process, "_debugWaitConnect", True(env->isolate()));
   }
 
@@ -3469,90 +3456,6 @@ void LoadEnvironment(Environment* env) {
   f->Call(Null(env->isolate()), 1, &arg);
 }
 
-
-static void PrintHelp();
-
-static bool ParseDebugOpt(const char* arg) {
-  const char* port = nullptr;
-
-  if (!strcmp(arg, "--debug")) {
-    use_debug_agent = true;
-  } else if (!strncmp(arg, "--debug=", sizeof("--debug=") - 1)) {
-    use_debug_agent = true;
-    port = arg + sizeof("--debug=") - 1;
-  } else if (!strcmp(arg, "--debug-brk")) {
-    use_debug_agent = true;
-    debug_wait_connect = true;
-  } else if (!strncmp(arg, "--debug-brk=", sizeof("--debug-brk=") - 1)) {
-    use_debug_agent = true;
-    debug_wait_connect = true;
-    port = arg + sizeof("--debug-brk=") - 1;
-  } else if (!strncmp(arg, "--debug-port=", sizeof("--debug-port=") - 1)) {
-    // XXX(bnoordhuis) Misnomer, configures port and listen address.
-    port = arg + sizeof("--debug-port=") - 1;
-#if HAVE_INSPECTOR
-  // Specifying both --inspect and --debug means debugging is on, using Chromium
-  // inspector.
-  } else if (!strcmp(arg, "--inspect")) {
-    use_debug_agent = true;
-    use_inspector = true;
-  } else if (!strncmp(arg, "--inspect=", sizeof("--inspect=") - 1)) {
-    use_debug_agent = true;
-    use_inspector = true;
-    port = arg + sizeof("--inspect=") - 1;
-#else
-  } else if (!strncmp(arg, "--inspect", sizeof("--inspect") - 1)) {
-    fprintf(stderr,
-            "Inspector support is not available with this Node.js build\n");
-    return false;
-#endif
-  } else {
-    return false;
-  }
-
-  if (port == nullptr) {
-    return true;
-  }
-
-  // FIXME(bnoordhuis) Move IPv6 address parsing logic to lib/net.js.
-  // It seems reasonable to support [address]:port notation
-  // in net.Server#listen() and net.Socket#connect().
-  const size_t port_len = strlen(port);
-  if (port[0] == '[' && port[port_len - 1] == ']') {
-    debug_host = new std::string(port + 1, port_len - 2);
-    return true;
-  }
-
-  const char* const colon = strrchr(port, ':');
-  if (colon == nullptr) {
-    // Either a port number or a host name.  Assume that
-    // if it's not all decimal digits, it's a host name.
-    for (size_t n = 0; port[n] != '\0'; n += 1) {
-      if (port[n] < '0' || port[n] > '9') {
-        debug_host = new std::string(port);
-        return true;
-      }
-    }
-  } else {
-    const bool skip = (colon > port && port[0] == '[' && colon[-1] == ']');
-    debug_host = new std::string(port + skip, colon - skip);
-  }
-
-  char* endptr;
-  errno = 0;
-  const char* const digits = colon != nullptr ? colon + 1 : port;
-  const long result = strtol(digits, &endptr, 10);  // NOLINT(runtime/int)
-  if (errno != 0 || *endptr != '\0' || result < 1024 || result > 65535) {
-    fprintf(stderr, "Debug port must be in range 1024 to 65535.\n");
-    PrintHelp();
-    exit(12);
-  }
-
-  debug_port = static_cast<int>(result);
-
-  return true;
-}
-
 static void PrintHelp() {
   // XXX: If you add an option here, please also add it to doc/node.1 and
   // doc/api/cli.md
@@ -3666,8 +3569,8 @@ static void ParseArgs(int* argc,
     const char* const arg = argv[index];
     unsigned int args_consumed = 1;
 
-    if (ParseDebugOpt(arg)) {
+    if (debug_options.ParseOption(arg)) {
-      // Done, consumed by ParseDebugOpt().
+      // Done, consumed by DebugOptions::ParseOption().
     } else if (strcmp(arg, "--version") == 0 || strcmp(arg, "-v") == 0) {
       printf("%s\n", NODE_VERSION);
       exit(0);
@@ -3809,22 +3712,21 @@ static void DispatchMessagesDebugAgentCallback(Environment* env) {
 }
 
 
-static void StartDebug(Environment* env, const char* path, bool wait) {
+static void StartDebug(Environment* env, const char* path,
+                       DebugOptions debug_options) {
   CHECK(!debugger_running);
-  if (use_inspector) {
+#if HAVE_INSPECTOR
-    debugger_running = v8_platform.StartInspector(env, path,
+  if (debug_options.inspector_enabled())
-        debug_port >= 0 ? debug_port : default_inspector_port, wait);
+    debugger_running = v8_platform.StartInspector(env, path, debug_options);
-  } else {
+#endif  // HAVE_INSPECTOR
+  if (debug_options.debugger_enabled()) {
     env->debugger_agent()->set_dispatch_handler(
           DispatchMessagesDebugAgentCallback);
-    const char* host = debug_host ? debug_host->c_str() : "127.0.0.1";
+    debugger_running = env->debugger_agent()->Start(debug_options);
-    int port = debug_port >= 0 ? debug_port : default_debugger_port;
-    debugger_running =
-        env->debugger_agent()->Start(host, port, wait);
     if (debugger_running == false) {
-      fprintf(stderr, "Starting debugger on %s:%d failed\n", host, port);
+      fprintf(stderr, "Starting debugger on %s:%d failed\n",
+              debug_options.host_name().c_str(), debug_options.port());
       fflush(stderr);
-      return;
     }
   }
 }
@@ -3834,7 +3736,7 @@ static void StartDebug(Environment* env, const char* path, bool wait) {
 static void EnableDebug(Environment* env) {
   CHECK(debugger_running);
 
-  if (use_inspector) {
+  if (!debug_options.debugger_enabled()) {
     return;
   }
 
@@ -3875,8 +3777,8 @@ static void DispatchDebugMessagesAsyncCallback(uv_async_t* handle) {
       HandleScope scope(isolate);
       Environment* env = Environment::GetCurrent(isolate);
       Context::Scope context_scope(env->context());
-
+      debug_options.EnableDebugAgent(DebugAgentType::kDebugger);
-      StartDebug(env, nullptr, false);
+      StartDebug(env, nullptr, debug_options);
       EnableDebug(env);
     }
 
@@ -4130,7 +4032,7 @@ static void DebugEnd(const FunctionCallbackInfo<Value>& args) {
   if (debugger_running) {
     Environment* env = Environment::GetCurrent(args);
 #if HAVE_INSPECTOR
-    if (use_inspector) {
+    if (debug_options.inspector_enabled()) {
       env->inspector_agent()->Stop();
     } else {
 #endif
@@ -4302,7 +4204,7 @@ void Init(int* argc,
   const char no_typed_array_heap[] = "--typed_array_max_size_in_heap=0";
   V8::SetFlagsFromString(no_typed_array_heap, sizeof(no_typed_array_heap) - 1);
 
-  if (!use_debug_agent) {
+  if (!debug_options.debugger_enabled() && !debug_options.inspector_enabled()) {
     RegisterDebugSignalHandler();
   }
 
@@ -4419,11 +4321,14 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data,
   Environment env(isolate_data, context);
   env.Start(argc, argv, exec_argc, exec_argv, v8_is_profiling);
 
+  bool debug_enabled =
+      debug_options.debugger_enabled() || debug_options.inspector_enabled();
+
   // Start debug agent when argv has --debug
-  if (use_debug_agent) {
+  if (debug_enabled) {
     const char* path = argc > 1 ? argv[1] : nullptr;
-    StartDebug(&env, path, debug_wait_connect);
+    StartDebug(&env, path, debug_options);
-    if (use_inspector && !debugger_running)
+    if (debug_options.debugger_enabled() && !debugger_running)
       return 12;  // Signal internal error.
   }
 
@@ -4435,7 +4340,7 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data,
   env.set_trace_sync_io(trace_sync_io);
 
   // Enable debugger
-  if (use_debug_agent)
+  if (debug_enabled)
     EnableDebug(&env);
 
   {

src/node_debug_options.cc

@@ -0,0 +1,144 @@
+#include "node_debug_options.h"
+
+#include <errno.h>
+#include <stdlib.h>
+#include <string.h>
+#include "util.h"
+
+namespace node {
+
+namespace {
+const int default_debugger_port = 5858;
+const int default_inspector_port = 9229;
+
+inline std::string remove_brackets(const std::string& host) {
+  if (!host.empty() && host.front() == '[' && host.back() == ']')
+    return host.substr(1, host.size() - 2);
+  else
+    return host;
+}
+
+int parse_and_validate_port(const std::string& port) {
+  char* endptr;
+  errno = 0;
+  const long result = strtol(port.c_str(), &endptr, 10);  // NOLINT(runtime/int)
+  if (errno != 0 || *endptr != '\0'|| result < 1024 || result > 65535) {
+    fprintf(stderr, "Debug port must be in range 1024 to 65535.\n");
+    exit(12);
+  }
+  return static_cast<int>(result);
+}
+
+std::pair<std::string, int> split_host_port(const std::string& arg) {
+  // IPv6, no port
+  std::string host = remove_brackets(arg);
+  if (host.length() < arg.length())
+    return {host, -1};
+
+  size_t colon = arg.rfind(':');
+  if (colon == std::string::npos) {
+    // Either a port number or a host name.  Assume that
+    // if it's not all decimal digits, it's a host name.
+    for (char c : arg) {
+      if (c < '0' || c > '9') {
+        return {arg, -1};
+      }
+    }
+    return {"", parse_and_validate_port(arg)};
+  }
+  return std::make_pair(remove_brackets(arg.substr(0, colon)),
+                        parse_and_validate_port(arg.substr(colon + 1)));
+}
+
+}  // namespace
+
+DebugOptions::DebugOptions() : debugger_enabled_(false),
+#if HAVE_INSPECTOR
+                               inspector_enabled_(false),
+#endif  // HAVE_INSPECTOR
+                               wait_connect_(false), http_enabled_(false),
+                               host_name_("127.0.0.1"), port_(-1) { }
+
+void DebugOptions::EnableDebugAgent(DebugAgentType tool) {
+  switch (tool) {
+#if HAVE_INSPECTOR
+    case DebugAgentType::kInspector:
+      inspector_enabled_ = true;
+      debugger_enabled_ = true;
+      break;
+#endif  // HAVE_INSPECTOR
+    case DebugAgentType::kDebugger:
+      debugger_enabled_ = true;
+      break;
+    case DebugAgentType::kNone:
+      break;
+  }
+}
+
+bool DebugOptions::ParseOption(const std::string& option) {
+  bool enable_inspector = false;
+  bool has_argument = false;
+  std::string option_name;
+  std::string argument;
+
+  auto pos = option.find("=");
+  if (pos == std::string::npos) {
+    option_name = option;
+  } else {
+    has_argument = true;
+    option_name = option.substr(0, pos);
+    argument = option.substr(pos + 1);
+  }
+
+  if (option_name == "--debug") {
+    debugger_enabled_ = true;
+  } else if (option_name == "--debug-brk") {
+    debugger_enabled_ = true;
+    wait_connect_ = true;
+  } else if (option_name == "--inspect") {
+    debugger_enabled_ = true;
+    enable_inspector = true;
+  } else if (option_name != "--debug-port" || !has_argument) {
+    return false;
+  }
+
+  if (enable_inspector) {
+#if HAVE_INSPECTOR
+    inspector_enabled_ = true;
+#else
+    fprintf(stderr,
+            "Inspector support is not available with this Node.js build\n");
+    return false;
+#endif
+  }
+
+  if (!has_argument) {
+    return true;
+  }
+
+  // FIXME(bnoordhuis) Move IPv6 address parsing logic to lib/net.js.
+  // It seems reasonable to support [address]:port notation
+  // in net.Server#listen() and net.Socket#connect().
+  std::pair<std::string, int> host_port = split_host_port(argument);
+  if (!host_port.first.empty()) {
+    host_name_ = host_port.first;
+  }
+  if (host_port.second >= 0) {
+    port_ = host_port.second;
+  }
+  return true;
+}
+
+int DebugOptions::port() const {
+  int port = port_;
+  if (port < 0) {
+#if HAVE_INSPECTOR
+    port = inspector_enabled_ ? default_inspector_port : default_debugger_port;
+#else
+    port = default_debugger_port;
+#endif  // HAVE_INSPECTOR
+  }
+  return port;
+}
+
+}  // namespace node

src/node_debug_options.h

@@ -0,0 +1,51 @@
+#ifndef SRC_NODE_DEBUG_OPTIONS_H_
+#define SRC_NODE_DEBUG_OPTIONS_H_
+
+#include <string>
+
+// Forward declaration to break recursive dependency chain with src/env.h.
+namespace node {
+
+enum class DebugAgentType {
+  kNone,
+  kDebugger,
+#if HAVE_INSPECTOR
+  kInspector
+#endif  // HAVE_INSPECTOR
+};
+
+class DebugOptions {
+ public:
+  DebugOptions();
+  bool ParseOption(const std::string& option);
+  bool debugger_enabled() const {
+    return debugger_enabled_ && !inspector_enabled();
+  }
+  bool inspector_enabled() const {
+#if HAVE_INSPECTOR
+    return inspector_enabled_;
+#else
+    return false;
+#endif  // HAVE_INSPECTOR
+  }
+  void EnableDebugAgent(DebugAgentType type);
+  bool ToolsServerEnabled();
+  bool wait_for_connect() const { return wait_connect_; }
+  std::string host_name() const { return host_name_; }
+  int port() const;
+  void set_port(int port) { port_ = port; }
+
+ private:
+  bool debugger_enabled_;
+#if HAVE_INSPECTOR
+  bool inspector_enabled_;
+#endif  // HAVE_INSPECTOR
+  bool wait_connect_;
+  bool http_enabled_;
+  std::string host_name_;
+  int port_;
+};
+
+}  // namespace node
+
+#endif  // SRC_NODE_DEBUG_OPTIONS_H_