Skip to content

Commit

Permalink
src: refactor --trace-env to reuse option selection and handling
Browse files Browse the repository at this point in the history 
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
  • Loading branch information
@joyeecheung
joyeecheung authored Jan 9, 2025
1 parent 8975748 commit 6b3937a
Show file tree
Hide file tree
Showing 4 changed files with 93 additions and 78 deletions.
10 changes: 1 addition & 9 deletions src/node_credentials.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,15 +99,7 @@ bool SafeGetenv(const char* key, std::string* text, Environment* env) {
*text = value.value();
}

auto options =
(env != nullptr ? env->options()
: per_process::cli_options->per_isolate->per_env);

if (options->trace_env) {
fprintf(stderr, "[--trace-env] get environment variable \"%s\"\n", key);

PrintTraceEnvStack(options);
}
TraceEnvVar(env, "get", key);

return has_env;
}
Expand Down
110 changes: 65 additions & 45 deletions src/node_env_var.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -338,19 +338,73 @@ Maybe<void> KVStore::AssignToObject(v8::Isolate* isolate,
return JustVoid();
}

void PrintTraceEnvStack(Environment* env) {
PrintTraceEnvStack(env->options());
}
struct TraceEnvVarOptions {
bool print_message : 1 = 0;
bool print_js_stack : 1 = 0;
bool print_native_stack : 1 = 0;
};

void PrintTraceEnvStack(std::shared_ptr<EnvironmentOptions> options) {
if (options->trace_env_native_stack) {
template <typename... Args>
inline void TraceEnvVarImpl(Environment* env,
TraceEnvVarOptions options,
const char* format,
Args&&... args) {
if (options.print_message) {
fprintf(stderr, format, std::forward<Args>(args)...);
}
if (options.print_native_stack) {
DumpNativeBacktrace(stderr);
}
if (options->trace_env_js_stack) {
if (options.print_js_stack) {
DumpJavaScriptBacktrace(stderr);
}
}

TraceEnvVarOptions GetTraceEnvVarOptions(Environment* env) {
TraceEnvVarOptions options;
auto cli_options = env != nullptr
? env->options()
: per_process::cli_options->per_isolate->per_env;
if (cli_options->trace_env) {
options.print_message = 1;
};
if (cli_options->trace_env_js_stack) {
options.print_js_stack = 1;
};
if (cli_options->trace_env_native_stack) {
options.print_native_stack = 1;
};
return options;
}

void TraceEnvVar(Environment* env, const char* message) {
TraceEnvVarImpl(
env, GetTraceEnvVarOptions(env), "[--trace-env] %s\n", message);
}

void TraceEnvVar(Environment* env, const char* message, const char* key) {
TraceEnvVarImpl(env,
GetTraceEnvVarOptions(env),
"[--trace-env] %s \"%s\"\n",
message,
key);
}

void TraceEnvVar(Environment* env,
const char* message,
v8::Local<v8::String> key) {
TraceEnvVarOptions options = GetTraceEnvVarOptions(env);
if (options.print_message) {
Utf8Value key_utf8(env->isolate(), key);
TraceEnvVarImpl(env,
options,
"[--trace-env] %s \"%.*s\"\n",
message,
static_cast<int>(key_utf8.length()),
key_utf8.out());
}
}

static Intercepted EnvGetter(Local<Name> property,
const PropertyCallbackInfo<Value>& info) {
Environment* env = Environment::GetCurrent(info);
Expand All @@ -364,14 +418,7 @@ static Intercepted EnvGetter(Local<Name> property,
env->env_vars()->Get(env->isolate(), property.As<String>());

bool has_env = !value_string.IsEmpty();
if (env->options()->trace_env) {
Utf8Value key(env->isolate(), property.As<String>());
fprintf(stderr,
"[--trace-env] get environment variable \"%.*s\"\n",
static_cast<int>(key.length()),
key.out());
PrintTraceEnvStack(env);
}
TraceEnvVar(env, "get", property.As<String>());

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

env->env_vars()->Set(env->isolate(), key, value_string);
if (env->options()->trace_env) {
Utf8Value key_utf8(env->isolate(), key);
fprintf(stderr,
"[--trace-env] set environment variable \"%.*s\"\n",
static_cast<int>(key_utf8.length()),
key_utf8.out());
PrintTraceEnvStack(env);
}
TraceEnvVar(env, "set", key);

return Intercepted::kYes;
}
Expand All @@ -430,16 +470,7 @@ static Intercepted EnvQuery(Local<Name> property,
if (property->IsString()) {
int32_t rc = env->env_vars()->Query(env->isolate(), property.As<String>());
bool has_env = (rc != -1);

if (env->options()->trace_env) {
Utf8Value key_utf8(env->isolate(), property.As<String>());
fprintf(stderr,
"[--trace-env] query environment variable \"%.*s\": %s\n",
static_cast<int>(key_utf8.length()),
key_utf8.out(),
has_env ? "is set" : "is not set");
PrintTraceEnvStack(env);
}
TraceEnvVar(env, "query", property.As<String>());
if (has_env) {
// Return attributes for the property.
info.GetReturnValue().Set(v8::None);
Expand All @@ -456,14 +487,7 @@ static Intercepted EnvDeleter(Local<Name> property,
if (property->IsString()) {
env->env_vars()->Delete(env->isolate(), property.As<String>());

if (env->options()->trace_env) {
Utf8Value key_utf8(env->isolate(), property.As<String>());
fprintf(stderr,
"[--trace-env] delete environment variable \"%.*s\"\n",
static_cast<int>(key_utf8.length()),
key_utf8.out());
PrintTraceEnvStack(env);
}
TraceEnvVar(env, "delete", property.As<String>());
}

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

if (env->options()->trace_env) {
fprintf(stderr, "[--trace-env] enumerate environment variables\n");

PrintTraceEnvStack(env);
}
TraceEnvVar(env, "enumerate environment variables");

info.GetReturnValue().Set(
env->env_vars()->Enumerate(env->isolate()));
Expand Down
7 changes: 5 additions & 2 deletions src/node_internals.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -324,8 +324,11 @@ namespace credentials {
bool SafeGetenv(const char* key, std::string* text, Environment* env = nullptr);
} // namespace credentials

void PrintTraceEnvStack(Environment* env);
void PrintTraceEnvStack(std::shared_ptr<EnvironmentOptions> options);
void TraceEnvVar(Environment* env, const char* message);
void TraceEnvVar(Environment* env, const char* message, const char* key);
void TraceEnvVar(Environment* env,
const char* message,
v8::Local<v8::String> key);

void DefineZlibConstants(v8::Local<v8::Object> target);
v8::Isolate* NewIsolate(v8::Isolate::CreateParams* params,
Expand Down
44 changes: 22 additions & 22 deletions test/parallel/test-trace-env.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,28 +14,28 @@ spawnSyncAndAssert(process.execPath, ['--trace-env', fixtures.path('empty.js')],
// If the internals remove any one of them, the checks here can be updated
// accordingly.
if (common.hasIntl) {
assert.match(output, /get environment variable "NODE_ICU_DATA"/);
assert.match(output, /get "NODE_ICU_DATA"/);
}
if (common.hasCrypto) {
assert.match(output, /get environment variable "NODE_EXTRA_CA_CERTS"/);
assert.match(output, /get "NODE_EXTRA_CA_CERTS"/);
}
if (common.hasOpenSSL3) {
assert.match(output, /get environment variable "OPENSSL_CONF"/);
assert.match(output, /get "OPENSSL_CONF"/);
}
assert.match(output, /get environment variable "NODE_DEBUG_NATIVE"/);
assert.match(output, /get environment variable "NODE_COMPILE_CACHE"/);
assert.match(output, /get environment variable "NODE_NO_WARNINGS"/);
assert.match(output, /get environment variable "NODE_V8_COVERAGE"/);
assert.match(output, /get environment variable "NODE_DEBUG"/);
assert.match(output, /get environment variable "NODE_CHANNEL_FD"/);
assert.match(output, /get environment variable "NODE_UNIQUE_ID"/);
assert.match(output, /get "NODE_DEBUG_NATIVE"/);
assert.match(output, /get "NODE_COMPILE_CACHE"/);
assert.match(output, /get "NODE_NO_WARNINGS"/);
assert.match(output, /get "NODE_V8_COVERAGE"/);
assert.match(output, /get "NODE_DEBUG"/);
assert.match(output, /get "NODE_CHANNEL_FD"/);
assert.match(output, /get "NODE_UNIQUE_ID"/);
if (common.isWindows) {
assert.match(output, /get environment variable "USERPROFILE"/);
assert.match(output, /get "USERPROFILE"/);
} else {
assert.match(output, /get environment variable "HOME"/);
assert.match(output, /get "HOME"/);
}
assert.match(output, /get environment variable "NODE_PATH"/);
assert.match(output, /get environment variable "WATCH_REPORT_DEPENDENCIES"/);
assert.match(output, /get "NODE_PATH"/);
assert.match(output, /get "WATCH_REPORT_DEPENDENCIES"/);
}
});

Expand All @@ -48,22 +48,22 @@ spawnSyncAndAssert(process.execPath, ['--trace-env', fixtures.path('process-env'
}
}, {
stderr(output) {
assert.match(output, /get environment variable "FOO"/);
assert.match(output, /get environment variable "BAR"/);
assert.match(output, /get "FOO"/);
assert.match(output, /get "BAR"/);
}
});

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

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

Expand All @@ -77,16 +77,16 @@ spawnSyncAndAssert(process.execPath, ['--trace-env', fixtures.path('process-env'
}
}, {
stderr(output) {
assert.match(output, /query environment variable "FOO": is set/);
assert.match(output, /query environment variable "BAR": is not set/);
assert.match(output, /query environment variable "BAZ": is not set/);
assert.match(output, /query "FOO"/);
assert.match(output, /query "BAR"/);
assert.match(output, /query "BAZ"/);
}
});

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

Expand Down

0 comments on commit 6b3937a

Please sign in to comment.