src: add CheckOptions to Options classes

This commit adds a CheckOptions function that the options classes can optionally implement to check that options specified are correct (dependencies between options are met or options that are mutually exclusive). In the process of doing this the error pointer passed to Parse was changed to be of type vector so that potentially multiple options check failures can be reported. PR-URL: #22943 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
nodejs · Sep 23, 2018 · b6cd185 · b6cd185
1 parent 9c36827
commit b6cd185
Show file tree

Hide file tree

Showing 4 changed files with 61 additions and 44 deletions.
diff --git a/src/node.cc b/src/node.cc
@@ -2467,7 +2467,7 @@ void ProcessArgv(std::vector<std::string>* args,
                  bool is_env) {
   // Parse a few arguments which are specific to Node.
   std::vector<std::string> v8_args;
-  std::string error;
+  std::vector<std::string> errors{};
 
   {
     // TODO(addaleax): The mutex here should ideally be held during the
@@ -2479,11 +2479,13 @@ void ProcessArgv(std::vector<std::string>* args,
         &v8_args,
         per_process_opts.get(),
         is_env ? kAllowedInEnvironment : kDisallowedInEnvironment,
-        &error);
+        &errors);
   }
 
-  if (!error.empty()) {
-    fprintf(stderr, "%s: %s\n", args->at(0).c_str(), error.c_str());
+  if (!errors.empty()) {
+    for (auto const& error : errors) {
+      fprintf(stderr, "%s: %s\n", args->at(0).c_str(), error.c_str());
+    }
     exit(9);
   }
 
@@ -2500,30 +2502,7 @@ void ProcessArgv(std::vector<std::string>* args,
   for (const std::string& cve : per_process_opts->security_reverts)
     Revert(cve.c_str());
 
-  // TODO(addaleax): Move this validation to the option parsers.
   auto env_opts = per_process_opts->per_isolate->per_env;
-  if (!env_opts->userland_loader.empty() &&
-      !env_opts->experimental_modules) {
-    fprintf(stderr, "%s: --loader requires --experimental-modules be enabled\n",
-            args->at(0).c_str());
-    exit(9);
-  }
-
-  if (env_opts->syntax_check_only && env_opts->has_eval_string) {
-    fprintf(stderr, "%s: either --check or --eval can be used, not both\n",
-            args->at(0).c_str());
-    exit(9);
-  }
-
-#if HAVE_OPENSSL
-  if (per_process_opts->use_openssl_ca && per_process_opts->use_bundled_ca) {
-    fprintf(stderr, "%s: either --use-openssl-ca or --use-bundled-ca can be "
-                    "used, not both\n",
-            args->at(0).c_str());
-    exit(9);
-  }
-#endif
-
   if (std::find(v8_args.begin(), v8_args.end(),
                 "--abort-on-uncaught-exception") != v8_args.end() ||
       std::find(v8_args.begin(), v8_args.end(),

diff --git a/src/node_options-inl.h b/src/node_options-inl.h
@@ -270,7 +270,7 @@ void OptionsParser<Options>::Parse(
     std::vector<std::string>* const v8_args,
     Options* const options,
     OptionEnvvarSettings required_env_settings,
-    std::string* const error) {
+    std::vector<std::string>* const errors) {
   ArgsInfo args(orig_args, exec_args);
 
   // The first entry is the process name. Make sure it ends up in the V8 argv,
@@ -279,7 +279,7 @@ void OptionsParser<Options>::Parse(
   if (v8_args->empty())
     v8_args->push_back(args.program_name());
 
-  while (!args.empty() && error->empty()) {
+  while (!args.empty() && errors->empty()) {
     if (args.first().size() <= 1 || args.first()[0] != '-') break;
 
     // We know that we're either going to consume this
@@ -288,7 +288,7 @@ void OptionsParser<Options>::Parse(
 
     if (arg == "--") {
       if (required_env_settings == kAllowedInEnvironment)
-        *error = NotAllowedInEnvErr("--");
+        errors->push_back(NotAllowedInEnvErr("--"));
       break;
     }
 
@@ -357,7 +357,7 @@ void OptionsParser<Options>::Parse(
     if ((it == options_.end() ||
          it->second.env_setting == kDisallowedInEnvironment) &&
         required_env_settings == kAllowedInEnvironment) {
-      *error = NotAllowedInEnvErr(original_name);
+      errors->push_back(NotAllowedInEnvErr(original_name));
       break;
     }
 
@@ -379,7 +379,7 @@ void OptionsParser<Options>::Parse(
         value = arg.substr(equals_index + 1);
         if (value.empty()) {
         missing_argument:
-          *error = RequiresArgumentErr(original_name);
+          errors->push_back(RequiresArgumentErr(original_name));
           break;
         }
       } else {
@@ -413,7 +413,7 @@ void OptionsParser<Options>::Parse(
         break;
       case kHostPort:
         Lookup<HostPort>(info.field, options)
-            ->Update(SplitHostPort(value, error));
+            ->Update(SplitHostPort(value, errors));
         break;
       case kNoOp:
         break;
@@ -424,6 +424,7 @@ void OptionsParser<Options>::Parse(
         UNREACHABLE();
     }
   }
+  options->CheckOptions(errors);
 }
 
 }  // namespace options_parser

diff --git a/src/node_options.cc b/src/node_options.cc
@@ -16,6 +16,32 @@ using v8::Undefined;
 using v8::Value;
 
 namespace node {
+
+void PerProcessOptions::CheckOptions(std::vector<std::string>* errors) {
+#if HAVE_OPENSSL
+  if (use_openssl_ca && use_bundled_ca) {
+    errors->push_back("either --use-openssl-ca or --use-bundled-ca can be "
+                      "used, not both");
+  }
+#endif
+  per_isolate->CheckOptions(errors);
+}
+
+void PerIsolateOptions::CheckOptions(std::vector<std::string>* errors) {
+  per_env->CheckOptions(errors);
+}
+
+void EnvironmentOptions::CheckOptions(std::vector<std::string>* errors) {
+  if (!userland_loader.empty() && !experimental_modules) {
+    errors->push_back("--loader requires --experimental-modules be enabled");
+  }
+
+  if (syntax_check_only && has_eval_string) {
+    errors->push_back("either --check or --eval can be used, not both");
+  }
+  debug_options->CheckOptions(errors);
+}
+
 namespace options_parser {
 
 // XXX: If you add an option here, please also add it to doc/node.1 and
@@ -307,18 +333,20 @@ inline std::string RemoveBrackets(const std::string& host) {
     return host;
 }
 
-inline int ParseAndValidatePort(const std::string& port, std::string* error) {
+inline int ParseAndValidatePort(const std::string& port,
+                                std::vector<std::string>* errors) {
   char* endptr;
   errno = 0;
   const long result = strtol(port.c_str(), &endptr, 10);  // NOLINT(runtime/int)
   if (errno != 0 || *endptr != '\0'||
       (result != 0 && result < 1024) || result > 65535) {
-    *error = "Port must be 0 or in range 1024 to 65535.";
+    errors->push_back(" must be 0 or in range 1024 to 65535.");
   }
   return static_cast<int>(result);
 }
 
-HostPort SplitHostPort(const std::string& arg, std::string* error) {
+HostPort SplitHostPort(const std::string& arg,
+                      std::vector<std::string>* errors) {
   // remove_brackets only works if no port is specified
   // so if it has an effect only an IPv6 address was specified.
   std::string host = RemoveBrackets(arg);
@@ -334,11 +362,11 @@ HostPort SplitHostPort(const std::string& arg, std::string* error) {
         return HostPort { arg, -1 };
       }
     }
-    return HostPort { "", ParseAndValidatePort(arg, error) };
+    return HostPort { "", ParseAndValidatePort(arg, errors) };
   }
   // Host and port found:
   return HostPort { RemoveBrackets(arg.substr(0, colon)),
-                    ParseAndValidatePort(arg.substr(colon + 1), error) };
+                    ParseAndValidatePort(arg.substr(colon + 1), errors) };
 }
 
 // Usage: Either:

diff --git a/src/node_options.h b/src/node_options.h
@@ -21,11 +21,16 @@ struct HostPort {
   }
 };
 
+class Options {
+ public:
+  virtual void CheckOptions(std::vector<std::string>* errors) {}
+};
+
 // These options are currently essentially per-Environment, but it can be nice
 // to keep them separate since they are a group of options applying to a very
 // specific part of Node. It might also make more sense for them to be
 // per-Isolate, rather than per-Environment.
-class DebugOptions {
+class DebugOptions : public Options {
  public:
   bool inspector_enabled = false;
   bool deprecated_debug = false;
@@ -57,7 +62,7 @@ class DebugOptions {
   }
 };
 
-class EnvironmentOptions {
+class EnvironmentOptions : public Options {
  public:
   std::shared_ptr<DebugOptions> debug_options { new DebugOptions() };
   bool abort_on_uncaught_exception = false;
@@ -91,17 +96,19 @@ class EnvironmentOptions {
   std::vector<std::string> user_argv;
 
   inline DebugOptions* get_debug_options();
+  void CheckOptions(std::vector<std::string>* errors);
 };
 
-class PerIsolateOptions {
+class PerIsolateOptions : public Options {
  public:
   std::shared_ptr<EnvironmentOptions> per_env { new EnvironmentOptions() };
   bool track_heap_objects = false;
 
   inline EnvironmentOptions* get_per_env_options();
+  void CheckOptions(std::vector<std::string>* errors);
 };
 
-class PerProcessOptions {
+class PerProcessOptions : public Options {
  public:
   std::shared_ptr<PerIsolateOptions> per_isolate { new PerIsolateOptions() };
 
@@ -139,13 +146,15 @@ class PerProcessOptions {
 #endif
 
   inline PerIsolateOptions* get_per_isolate_options();
+  void CheckOptions(std::vector<std::string>* errors);
 };
 
 // The actual options parser, as opposed to the structs containing them:
 
 namespace options_parser {
 
-HostPort SplitHostPort(const std::string& arg, std::string* error);
+HostPort SplitHostPort(const std::string& arg,
+    std::vector<std::string>* errors);
 void GetOptions(const v8::FunctionCallbackInfo<v8::Value>& args);
 
 enum OptionEnvvarSettings {
@@ -255,7 +264,7 @@ class OptionsParser {
                      std::vector<std::string>* const v8_args,
                      Options* const options,
                      OptionEnvvarSettings required_env_settings,
-                     std::string* const error);
+                     std::vector<std::string>* const errors);
 
  private:
   // We support the wide variety of different option types by remembering