Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit b46bad3

Browse filesBrowse 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
Copy full SHA for b46bad3

File tree

Expand file treeCollapse file tree

4 files changed

+93
-78
lines changed
Filter options
Expand file treeCollapse file tree

4 files changed

+93
-78
lines changed

‎src/node_credentials.cc

Copy file name to clipboardExpand all lines: src/node_credentials.cc
+1-9Lines 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

Copy file name to clipboardExpand all lines: src/node_env_var.cc
+65-45Lines 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

Copy file name to clipboardExpand all lines: src/node_internals.h
+5-2Lines 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

Copy file name to clipboardExpand all lines: test/parallel/test-trace-env.js
+22-22Lines 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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.