Skip to content

Commit b46bad3

Browse files
joyeecheungaduh95
joyeecheung
authored and
aduh95
committed
src: refactor --trace-env to reuse option selection and handling
This reduces the duplication in the code base, also makes makes the log messages more concise since the `[--trace-env]` prefix should provide enough indication about the fact that it's about environment variable access. PR-URL: #56293 Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
1 parent 19b4aa4 commit b46bad3

File tree

4 files changed

+93
-78
lines changed

4 files changed

+93
-78
lines changed

‎src/node_credentials.cc‎

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -99,15 +99,7 @@ bool SafeGetenv(const char* key, std::string* text, Environment* env) {
9999
*text = value.value();
100100
}
101101

102-
auto options =
103-
(env != nullptr ? env->options()
104-
: per_process::cli_options->per_isolate->per_env);
105-
106-
if (options->trace_env) {
107-
fprintf(stderr, "[--trace-env] get environment variable \"%s\"\n", key);
108-
109-
PrintTraceEnvStack(options);
110-
}
102+
TraceEnvVar(env, "get", key);
111103

112104
return has_env;
113105
}

‎src/node_env_var.cc‎

Lines changed: 65 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -338,19 +338,73 @@ Maybe<void> KVStore::AssignToObject(v8::Isolate* isolate,
338338
return JustVoid();
339339
}
340340

341-
void PrintTraceEnvStack(Environment* env) {
342-
PrintTraceEnvStack(env->options());
343-
}
341+
struct TraceEnvVarOptions {
342+
bool print_message : 1 = 0;
343+
bool print_js_stack : 1 = 0;
344+
bool print_native_stack : 1 = 0;
345+
};
344346

345-
void PrintTraceEnvStack(std::shared_ptr<EnvironmentOptions> options) {
346-
if (options->trace_env_native_stack) {
347+
template <typename... Args>
348+
inline void TraceEnvVarImpl(Environment* env,
349+
TraceEnvVarOptions options,
350+
const char* format,
351+
Args&&... args) {
352+
if (options.print_message) {
353+
fprintf(stderr, format, std::forward<Args>(args)...);
354+
}
355+
if (options.print_native_stack) {
347356
DumpNativeBacktrace(stderr);
348357
}
349-
if (options->trace_env_js_stack) {
358+
if (options.print_js_stack) {
350359
DumpJavaScriptBacktrace(stderr);
351360
}
352361
}
353362

363+
TraceEnvVarOptions GetTraceEnvVarOptions(Environment* env) {
364+
TraceEnvVarOptions options;
365+
auto cli_options = env != nullptr
366+
? env->options()
367+
: per_process::cli_options->per_isolate->per_env;
368+
if (cli_options->trace_env) {
369+
options.print_message = 1;
370+
};
371+
if (cli_options->trace_env_js_stack) {
372+
options.print_js_stack = 1;
373+
};
374+
if (cli_options->trace_env_native_stack) {
375+
options.print_native_stack = 1;
376+
};
377+
return options;
378+
}
379+
380+
void TraceEnvVar(Environment* env, const char* message) {
381+
TraceEnvVarImpl(
382+
env, GetTraceEnvVarOptions(env), "[--trace-env] %s\n", message);
383+
}
384+
385+
void TraceEnvVar(Environment* env, const char* message, const char* key) {
386+
TraceEnvVarImpl(env,
387+
GetTraceEnvVarOptions(env),
388+
"[--trace-env] %s \"%s\"\n",
389+
message,
390+
key);
391+
}
392+
393+
void TraceEnvVar(Environment* env,
394+
const char* message,
395+
v8::Local<v8::String> key) {
396+
TraceEnvVarOptions options = GetTraceEnvVarOptions(env);
397+
if (options.print_message) {
398+
Utf8Value key_utf8(env->isolate(), key);
399+
TraceEnvVarImpl(env,
400+
options,
401+
"[--trace-env] %s \"%.*s\"\n",
402+
message,
403+
static_cast<int>(key_utf8.length()),
404+
key_utf8.out());
405+
}
406+
}
407+
354408
static Intercepted EnvGetter(Local<Name> property,
355409
const PropertyCallbackInfo<Value>& info) {
356410
Environment* env = Environment::GetCurrent(info);
@@ -364,14 +418,7 @@ static Intercepted EnvGetter(Local<Name> property,
364418
env->env_vars()->Get(env->isolate(), property.As<String>());
365419

366420
bool has_env = !value_string.IsEmpty();
367-
if (env->options()->trace_env) {
368-
Utf8Value key(env->isolate(), property.As<String>());
369-
fprintf(stderr,
370-
"[--trace-env] get environment variable \"%.*s\"\n",
371-
static_cast<int>(key.length()),
372-
key.out());
373-
PrintTraceEnvStack(env);
374-
}
421+
TraceEnvVar(env, "get", property.As<String>());
375422

376423
if (has_env) {
377424
info.GetReturnValue().Set(value_string.ToLocalChecked());
@@ -411,14 +458,7 @@ static Intercepted EnvSetter(Local<Name> property,
411458
}
412459

413460
env->env_vars()->Set(env->isolate(), key, value_string);
414-
if (env->options()->trace_env) {
415-
Utf8Value key_utf8(env->isolate(), key);
416-
fprintf(stderr,
417-
"[--trace-env] set environment variable \"%.*s\"\n",
418-
static_cast<int>(key_utf8.length()),
419-
key_utf8.out());
420-
PrintTraceEnvStack(env);
421-
}
461+
TraceEnvVar(env, "set", key);
422462

423463
return Intercepted::kYes;
424464
}
@@ -430,16 +470,7 @@ static Intercepted EnvQuery(Local<Name> property,
430470
if (property->IsString()) {
431471
int32_t rc = env->env_vars()->Query(env->isolate(), property.As<String>());
432472
bool has_env = (rc != -1);
433-
434-
if (env->options()->trace_env) {
435-
Utf8Value key_utf8(env->isolate(), property.As<String>());
436-
fprintf(stderr,
437-
"[--trace-env] query environment variable \"%.*s\": %s\n",
438-
static_cast<int>(key_utf8.length()),
439-
key_utf8.out(),
440-
has_env ? "is set" : "is not set");
441-
PrintTraceEnvStack(env);
442-
}
473+
TraceEnvVar(env, "query", property.As<String>());
443474
if (has_env) {
444475
// Return attributes for the property.
445476
info.GetReturnValue().Set(v8::None);
@@ -456,14 +487,7 @@ static Intercepted EnvDeleter(Local<Name> property,
456487
if (property->IsString()) {
457488
env->env_vars()->Delete(env->isolate(), property.As<String>());
458489

459-
if (env->options()->trace_env) {
460-
Utf8Value key_utf8(env->isolate(), property.As<String>());
461-
fprintf(stderr,
462-
"[--trace-env] delete environment variable \"%.*s\"\n",
463-
static_cast<int>(key_utf8.length()),
464-
key_utf8.out());
465-
PrintTraceEnvStack(env);
466-
}
490+
TraceEnvVar(env, "delete", property.As<String>());
467491
}
468492

469493
// process.env never has non-configurable properties, so always
@@ -476,11 +500,7 @@ static void EnvEnumerator(const PropertyCallbackInfo<Array>& info) {
476500
Environment* env = Environment::GetCurrent(info);
477501
CHECK(env->has_run_bootstrapping_code());
478502

479-
if (env->options()->trace_env) {
480-
fprintf(stderr, "[--trace-env] enumerate environment variables\n");
481-
482-
PrintTraceEnvStack(env);
483-
}
503+
TraceEnvVar(env, "enumerate environment variables");
484504

485505
info.GetReturnValue().Set(
486506
env->env_vars()->Enumerate(env->isolate()));

‎src/node_internals.h‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -324,8 +324,11 @@ namespace credentials {
324324
bool SafeGetenv(const char* key, std::string* text, Environment* env = nullptr);
325325
} // namespace credentials
326326

327-
void PrintTraceEnvStack(Environment* env);
328-
void PrintTraceEnvStack(std::shared_ptr<EnvironmentOptions> options);
327+
void TraceEnvVar(Environment* env, const char* message);
328+
void TraceEnvVar(Environment* env, const char* message, const char* key);
329+
void TraceEnvVar(Environment* env,
330+
const char* message,
331+
v8::Local<v8::String> key);
329332

330333
void DefineZlibConstants(v8::Local<v8::Object> target);
331334
v8::Isolate* NewIsolate(v8::Isolate::CreateParams* params,

‎test/parallel/test-trace-env.js‎

Lines changed: 22 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -14,28 +14,28 @@ spawnSyncAndAssert(process.execPath, ['--trace-env', fixtures.path('empty.js')],
1414
// If the internals remove any one of them, the checks here can be updated
1515
// accordingly.
1616
if (common.hasIntl) {
17-
assert.match(output, /get environment variable "NODE_ICU_DATA"/);
17+
assert.match(output, /get "NODE_ICU_DATA"/);
1818
}
1919
if (common.hasCrypto) {
20-
assert.match(output, /get environment variable "NODE_EXTRA_CA_CERTS"/);
20+
assert.match(output, /get "NODE_EXTRA_CA_CERTS"/);
2121
}
2222
if (common.hasOpenSSL3) {
23-
assert.match(output, /get environment variable "OPENSSL_CONF"/);
23+
assert.match(output, /get "OPENSSL_CONF"/);
2424
}
25-
assert.match(output, /get environment variable "NODE_DEBUG_NATIVE"/);
26-
assert.match(output, /get environment variable "NODE_COMPILE_CACHE"/);
27-
assert.match(output, /get environment variable "NODE_NO_WARNINGS"/);
28-
assert.match(output, /get environment variable "NODE_V8_COVERAGE"/);
29-
assert.match(output, /get environment variable "NODE_DEBUG"/);
30-
assert.match(output, /get environment variable "NODE_CHANNEL_FD"/);
31-
assert.match(output, /get environment variable "NODE_UNIQUE_ID"/);
25+
assert.match(output, /get "NODE_DEBUG_NATIVE"/);
26+
assert.match(output, /get "NODE_COMPILE_CACHE"/);
27+
assert.match(output, /get "NODE_NO_WARNINGS"/);
28+
assert.match(output, /get "NODE_V8_COVERAGE"/);
29+
assert.match(output, /get "NODE_DEBUG"/);
30+
assert.match(output, /get "NODE_CHANNEL_FD"/);
31+
assert.match(output, /get "NODE_UNIQUE_ID"/);
3232
if (common.isWindows) {
33-
assert.match(output, /get environment variable "USERPROFILE"/);
33+
assert.match(output, /get "USERPROFILE"/);
3434
} else {
35-
assert.match(output, /get environment variable "HOME"/);
35+
assert.match(output, /get "HOME"/);
3636
}
37-
assert.match(output, /get environment variable "NODE_PATH"/);
38-
assert.match(output, /get environment variable "WATCH_REPORT_DEPENDENCIES"/);
37+
assert.match(output, /get "NODE_PATH"/);
38+
assert.match(output, /get "WATCH_REPORT_DEPENDENCIES"/);
3939
}
4040
});
4141

@@ -48,22 +48,22 @@ spawnSyncAndAssert(process.execPath, ['--trace-env', fixtures.path('process-env'
4848
}
4949
}, {
5050
stderr(output) {
51-
assert.match(output, /get environment variable "FOO"/);
52-
assert.match(output, /get environment variable "BAR"/);
51+
assert.match(output, /get "FOO"/);
52+
assert.match(output, /get "BAR"/);
5353
}
5454
});
5555

5656
// Check set from user land.
5757
spawnSyncAndAssert(process.execPath, ['--trace-env', fixtures.path('process-env', 'set.js') ], {
5858
stderr(output) {
59-
assert.match(output, /set environment variable "FOO"/);
59+
assert.match(output, /set "FOO"/);
6060
}
6161
});
6262

6363
// Check define from user land.
6464
spawnSyncAndAssert(process.execPath, ['--trace-env', fixtures.path('process-env', 'define.js') ], {
6565
stderr(output) {
66-
assert.match(output, /set environment variable "FOO"/);
66+
assert.match(output, /set "FOO"/);
6767
}
6868
});
6969

@@ -77,16 +77,16 @@ spawnSyncAndAssert(process.execPath, ['--trace-env', fixtures.path('process-env'
7777
}
7878
}, {
7979
stderr(output) {
80-
assert.match(output, /query environment variable "FOO": is set/);
81-
assert.match(output, /query environment variable "BAR": is not set/);
82-
assert.match(output, /query environment variable "BAZ": is not set/);
80+
assert.match(output, /query "FOO"/);
81+
assert.match(output, /query "BAR"/);
82+
assert.match(output, /query "BAZ"/);
8383
}
8484
});
8585

8686
// Check delete from user land.
8787
spawnSyncAndAssert(process.execPath, ['--trace-env', fixtures.path('process-env', 'delete.js') ], {
8888
stderr(output) {
89-
assert.match(output, /delete environment variable "FOO"/);
89+
assert.match(output, /delete "FOO"/);
9090
}
9191
});
9292

0 commit comments

Comments
 (0)