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.
  • Loading branch information
@joyeecheung
joyeecheung committed Dec 17, 2024
1 parent b171afe commit 3c65551
Show file tree
Hide file tree
Showing 4 changed files with 88 additions and 79 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);
}
TraceEnv(env, "get", key);

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

void PrintTraceEnvStack(Environment* env) {
PrintTraceEnvStack(env->options());
}
struct TraceEnvOptions {
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 TraceEnvImpl(Environment* env,
TraceEnvOptions 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);
}
}

TraceEnvOptions GetTraceEnvOptions(Environment* env) {
TraceEnvOptions 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 TraceEnv(Environment* env, const char* message) {
TraceEnvImpl(env, GetTraceEnvOptions(env), "[--trace-env] %s\n", message);
}

void TraceEnv(Environment* env, const char* message, const char* key) {
TraceEnvImpl(
env, GetTraceEnvOptions(env), "[--trace-env] %s \"%s\"\n", message, key);
}

void TraceEnv(Environment* env,
const char* message,
v8::Local<v8::String> key) {
TraceEnvOptions options = GetTraceEnvOptions(env);
if (options.print_message) {
Utf8Value key_utf8(env->isolate(), key);
TraceEnvImpl(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 @@ -363,14 +413,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);
}
TraceEnv(env, "get", property.As<String>());

if (has_env) {
info.GetReturnValue().Set(value_string.ToLocalChecked());
Expand Down Expand Up @@ -410,14 +453,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);
}
TraceEnv(env, "set", key);

return Intercepted::kYes;
}
Expand All @@ -429,16 +465,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);
}
TraceEnv(env, "query", property.As<String>());
if (has_env) {
// Return attributes for the property.
info.GetReturnValue().Set(v8::None);
Expand All @@ -455,14 +482,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);
}
TraceEnv(env, "delete", property.As<String>());
}

// process.env never has non-configurable properties, so always
Expand All @@ -475,11 +495,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);
}
TraceEnv(env, "enumerate environment variables");

info.GetReturnValue().Set(
env->env_vars()->Enumerate(env->isolate()));
Expand Down
5 changes: 3 additions & 2 deletions src/node_internals.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -324,8 +324,9 @@ 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 TraceEnv(Environment* env, const char* message);
void TraceEnv(Environment* env, const char* message, const char* key);
void TraceEnv(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
46 changes: 23 additions & 23 deletions test/parallel/test-trace-env.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,33 +9,33 @@ const fixtures = require('../common/fixtures');
// Check reads from the Node.js internals.
spawnSyncAndAssert(process.execPath, ['--trace-env', fixtures.path('empty.js')], {
stderr(output) {
// This is a non-exhaustive list of the environment variables checked
// This is a non-exhaustive list of thes checked
// during startup of an empty script at the time this test was written.
// 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 3c65551

Please sign in to comment.