Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: add CheckOptions to Options classes #22943

Closed
wants to merge 2 commits into from
Closed

src: add CheckOptions to Options classes #22943

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cc9e954
src: add CheckOptions to Options classes
danbev Sep 19, 2018
0c61a6d
squash: make CheckOptions delegate
danbev Sep 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 6 additions & 27 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
}

Expand All @@ -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(),
Expand Down
13 changes: 7 additions & 6 deletions src/node_options-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
Expand All @@ -288,7 +288,7 @@ void OptionsParser<Options>::Parse(

if (arg == "--") {
if (required_env_settings == kAllowedInEnvironment)
*error = NotAllowedInEnvErr("--");
errors->push_back(NotAllowedInEnvErr("--"));
break;
}

Expand Down Expand Up @@ -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;
}

Expand All @@ -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 {
Expand Down Expand Up @@ -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;
Expand All @@ -424,6 +424,7 @@ void OptionsParser<Options>::Parse(
UNREACHABLE();
}
}
options->CheckOptions(errors);
}

} // namespace options_parser
Expand Down
36 changes: 31 additions & 5 deletions src/node_options.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,30 @@ using v8::Undefined;
using v8::Value;

namespace node {

void PerProcessOptions::CheckOptions(std::vector<std::string>* errors) {
// Check any per-process options here.
#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);
per_isolate->per_env->CheckOptions(errors);
per_isolate->per_env->debug_options->CheckOptions(errors);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it maybe make sense to nest these directly, like callling debug_options->CheckOptions(errors) from EnvironmentOptions::CheckOptions() and so on?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I like that, I'll give that a try. Thanks

}

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");
}
}

namespace options_parser {

// XXX: If you add an option here, please also add it to doc/node.1 and
Expand Down Expand Up @@ -302,18 +326,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);
Expand All @@ -329,11 +355,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:
Expand Down
20 changes: 14 additions & 6 deletions src/node_options.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -91,17 +96,18 @@ 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();
};

class PerProcessOptions {
class PerProcessOptions : public Options {
public:
std::shared_ptr<PerIsolateOptions> per_isolate { new PerIsolateOptions() };

Expand Down Expand Up @@ -138,13 +144,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 {
Expand Down Expand Up @@ -254,7 +262,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
Expand Down