Skip to content

Commit 36bdd19

Browse files
danbevaddaleax
danbev
authored and
addaleax
committed
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: nodejs#22943 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
1 parent 1f4d4c0 commit 36bdd19

4 files changed

Lines changed: 61 additions & 44 deletions

File tree

‎src/node.cc‎

Lines changed: 6 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -2460,7 +2460,7 @@ void ProcessArgv(std::vector<std::string>* args,
24602460
bool is_env) {
24612461
// Parse a few arguments which are specific to Node.
24622462
std::vector<std::string> v8_args;
2463-
std::string error;
2463+
std::vector<std::string> errors{};
24642464

24652465
{
24662466
// TODO(addaleax): The mutex here should ideally be held during the
@@ -2472,11 +2472,13 @@ void ProcessArgv(std::vector<std::string>* args,
24722472
&v8_args,
24732473
per_process_opts.get(),
24742474
is_env ? kAllowedInEnvironment : kDisallowedInEnvironment,
2475-
&error);
2475+
&errors);
24762476
}
24772477

2478-
if (!error.empty()) {
2479-
fprintf(stderr, "%s: %s\n", args->at(0).c_str(), error.c_str());
2478+
if (!errors.empty()) {
2479+
for (auto const& error : errors) {
2480+
fprintf(stderr, "%s: %s\n", args->at(0).c_str(), error.c_str());
2481+
}
24802482
exit(9);
24812483
}
24822484

@@ -2493,30 +2495,7 @@ void ProcessArgv(std::vector<std::string>* args,
24932495
for (const std::string& cve : per_process_opts->security_reverts)
24942496
Revert(cve.c_str());
24952497

2496-
// TODO(addaleax): Move this validation to the option parsers.
24972498
auto env_opts = per_process_opts->per_isolate->per_env;
2498-
if (!env_opts->userland_loader.empty() &&
2499-
!env_opts->experimental_modules) {
2500-
fprintf(stderr, "%s: --loader requires --experimental-modules be enabled\n",
2501-
args->at(0).c_str());
2502-
exit(9);
2503-
}
2504-
2505-
if (env_opts->syntax_check_only && env_opts->has_eval_string) {
2506-
fprintf(stderr, "%s: either --check or --eval can be used, not both\n",
2507-
args->at(0).c_str());
2508-
exit(9);
2509-
}
2510-
2511-
#if HAVE_OPENSSL
2512-
if (per_process_opts->use_openssl_ca && per_process_opts->use_bundled_ca) {
2513-
fprintf(stderr, "%s: either --use-openssl-ca or --use-bundled-ca can be "
2514-
"used, not both\n",
2515-
args->at(0).c_str());
2516-
exit(9);
2517-
}
2518-
#endif
2519-
25202499
if (std::find(v8_args.begin(), v8_args.end(),
25212500
"--abort-on-uncaught-exception") != v8_args.end() ||
25222501
std::find(v8_args.begin(), v8_args.end(),

‎src/node_options-inl.h‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -270,7 +270,7 @@ void OptionsParser<Options>::Parse(
270270
std::vector<std::string>* const v8_args,
271271
Options* const options,
272272
OptionEnvvarSettings required_env_settings,
273-
std::string* const error) {
273+
std::vector<std::string>* const errors) {
274274
ArgsInfo args(orig_args, exec_args);
275275

276276
// The first entry is the process name. Make sure it ends up in the V8 argv,
@@ -279,7 +279,7 @@ void OptionsParser<Options>::Parse(
279279
if (v8_args->empty())
280280
v8_args->push_back(args.program_name());
281281

282-
while (!args.empty() && error->empty()) {
282+
while (!args.empty() && errors->empty()) {
283283
if (args.first().size() <= 1 || args.first()[0] != '-') break;
284284

285285
// We know that we're either going to consume this
@@ -288,7 +288,7 @@ void OptionsParser<Options>::Parse(
288288

289289
if (arg == "--") {
290290
if (required_env_settings == kAllowedInEnvironment)
291-
*error = NotAllowedInEnvErr("--");
291+
errors->push_back(NotAllowedInEnvErr("--"));
292292
break;
293293
}
294294

@@ -357,7 +357,7 @@ void OptionsParser<Options>::Parse(
357357
if ((it == options_.end() ||
358358
it->second.env_setting == kDisallowedInEnvironment) &&
359359
required_env_settings == kAllowedInEnvironment) {
360-
*error = NotAllowedInEnvErr(original_name);
360+
errors->push_back(NotAllowedInEnvErr(original_name));
361361
break;
362362
}
363363

@@ -379,7 +379,7 @@ void OptionsParser<Options>::Parse(
379379
value = arg.substr(equals_index + 1);
380380
if (value.empty()) {
381381
missing_argument:
382-
*error = RequiresArgumentErr(original_name);
382+
errors->push_back(RequiresArgumentErr(original_name));
383383
break;
384384
}
385385
} else {
@@ -413,7 +413,7 @@ void OptionsParser<Options>::Parse(
413413
break;
414414
case kHostPort:
415415
Lookup<HostPort>(info.field, options)
416-
->Update(SplitHostPort(value, error));
416+
->Update(SplitHostPort(value, errors));
417417
break;
418418
case kNoOp:
419419
break;
@@ -424,6 +424,7 @@ void OptionsParser<Options>::Parse(
424424
UNREACHABLE();
425425
}
426426
}
427+
options->CheckOptions(errors);
427428
}
428429

429430
} // namespace options_parser

‎src/node_options.cc‎

Lines changed: 33 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,32 @@ using v8::Undefined;
1616
using v8::Value;
1717

1818
namespace node {
19+
20+
void PerProcessOptions::CheckOptions(std::vector<std::string>* errors) {
21+
#if HAVE_OPENSSL
22+
if (use_openssl_ca && use_bundled_ca) {
23+
errors->push_back("either --use-openssl-ca or --use-bundled-ca can be "
24+
"used, not both");
25+
}
26+
#endif
27+
per_isolate->CheckOptions(errors);
28+
}
29+
30+
void PerIsolateOptions::CheckOptions(std::vector<std::string>* errors) {
31+
per_env->CheckOptions(errors);
32+
}
33+
34+
void EnvironmentOptions::CheckOptions(std::vector<std::string>* errors) {
35+
if (!userland_loader.empty() && !experimental_modules) {
36+
errors->push_back("--loader requires --experimental-modules be enabled");
37+
}
38+
39+
if (syntax_check_only && has_eval_string) {
40+
errors->push_back("either --check or --eval can be used, not both");
41+
}
42+
debug_options->CheckOptions(errors);
43+
}
44+
1945
namespace options_parser {
2046

2147
// XXX: If you add an option here, please also add it to doc/node.1 and
@@ -305,18 +331,20 @@ inline std::string RemoveBrackets(const std::string& host) {
305331
return host;
306332
}
307333

308-
inline int ParseAndValidatePort(const std::string& port, std::string* error) {
334+
inline int ParseAndValidatePort(const std::string& port,
335+
std::vector<std::string>* errors) {
309336
char* endptr;
310337
errno = 0;
311338
const long result = strtol(port.c_str(), &endptr, 10); // NOLINT(runtime/int)
312339
if (errno != 0 || *endptr != '\0'||
313340
(result != 0 && result < 1024) || result > 65535) {
314-
*error = "Port must be 0 or in range 1024 to 65535.";
341+
errors->push_back(" must be 0 or in range 1024 to 65535.");
315342
}
316343
return static_cast<int>(result);
317344
}
318345

319-
HostPort SplitHostPort(const std::string& arg, std::string* error) {
346+
HostPort SplitHostPort(const std::string& arg,
347+
std::vector<std::string>* errors) {
320348
// remove_brackets only works if no port is specified
321349
// so if it has an effect only an IPv6 address was specified.
322350
std::string host = RemoveBrackets(arg);
@@ -332,11 +360,11 @@ HostPort SplitHostPort(const std::string& arg, std::string* error) {
332360
return HostPort { arg, -1 };
333361
}
334362
}
335-
return HostPort { "", ParseAndValidatePort(arg, error) };
363+
return HostPort { "", ParseAndValidatePort(arg, errors) };
336364
}
337365
// Host and port found:
338366
return HostPort { RemoveBrackets(arg.substr(0, colon)),
339-
ParseAndValidatePort(arg.substr(colon + 1), error) };
367+
ParseAndValidatePort(arg.substr(colon + 1), errors) };
340368
}
341369

342370
// Usage: Either:

‎src/node_options.h‎

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,11 +21,16 @@ struct HostPort {
2121
}
2222
};
2323

24+
class Options {
25+
public:
26+
virtual void CheckOptions(std::vector<std::string>* errors) {}
27+
};
28+
2429
// These options are currently essentially per-Environment, but it can be nice
2530
// to keep them separate since they are a group of options applying to a very
2631
// specific part of Node. It might also make more sense for them to be
2732
// per-Isolate, rather than per-Environment.
28-
class DebugOptions {
33+
class DebugOptions : public Options {
2934
public:
3035
bool inspector_enabled = false;
3136
bool deprecated_debug = false;
@@ -57,7 +62,7 @@ class DebugOptions {
5762
}
5863
};
5964

60-
class EnvironmentOptions {
65+
class EnvironmentOptions : public Options {
6166
public:
6267
std::shared_ptr<DebugOptions> debug_options { new DebugOptions() };
6368
bool abort_on_uncaught_exception = false;
@@ -91,17 +96,19 @@ class EnvironmentOptions {
9196
std::vector<std::string> user_argv;
9297

9398
inline DebugOptions* get_debug_options();
99+
void CheckOptions(std::vector<std::string>* errors);
94100
};
95101

96-
class PerIsolateOptions {
102+
class PerIsolateOptions : public Options {
97103
public:
98104
std::shared_ptr<EnvironmentOptions> per_env { new EnvironmentOptions() };
99105
bool track_heap_objects = false;
100106

101107
inline EnvironmentOptions* get_per_env_options();
108+
void CheckOptions(std::vector<std::string>* errors);
102109
};
103110

104-
class PerProcessOptions {
111+
class PerProcessOptions : public Options {
105112
public:
106113
std::shared_ptr<PerIsolateOptions> per_isolate { new PerIsolateOptions() };
107114

@@ -139,13 +146,15 @@ class PerProcessOptions {
139146
#endif
140147

141148
inline PerIsolateOptions* get_per_isolate_options();
149+
void CheckOptions(std::vector<std::string>* errors);
142150
};
143151

144152
// The actual options parser, as opposed to the structs containing them:
145153

146154
namespace options_parser {
147155

148-
HostPort SplitHostPort(const std::string& arg, std::string* error);
156+
HostPort SplitHostPort(const std::string& arg,
157+
std::vector<std::string>* errors);
149158
void GetOptions(const v8::FunctionCallbackInfo<v8::Value>& args);
150159

151160
enum OptionEnvvarSettings {
@@ -255,7 +264,7 @@ class OptionsParser {
255264
std::vector<std::string>* const v8_args,
256265
Options* const options,
257266
OptionEnvvarSettings required_env_settings,
258-
std::string* const error);
267+
std::vector<std::string>* const errors);
259268

260269
private:
261270
// We support the wide variety of different option types by remembering

0 commit comments

Comments
 (0)