From 3c6555166e19d90d9f7df0f43c0ca24763e325e1 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 17 Dec 2024 13:17:45 +0100 Subject: [PATCH] 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. --- src/node_credentials.cc | 10 +-- src/node_env_var.cc | 106 ++++++++++++++++++-------------- src/node_internals.h | 5 +- test/parallel/test-trace-env.js | 46 +++++++------- 4 files changed, 88 insertions(+), 79 deletions(-) diff --git a/src/node_credentials.cc b/src/node_credentials.cc index 0152f6269c83ef..50ae6cd1cc5802 100644 --- a/src/node_credentials.cc +++ b/src/node_credentials.cc @@ -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; } diff --git a/src/node_env_var.cc b/src/node_env_var.cc index 82b8231c435775..600697bd064d5a 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -337,19 +337,69 @@ Maybe 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 options) { - if (options->trace_env_native_stack) { +template +inline void TraceEnvImpl(Environment* env, + TraceEnvOptions options, + const char* format, + Args&&... args) { + if (options.print_message) { + fprintf(stderr, format, std::forward(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 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(key_utf8.length()), + key_utf8.out()); + } +} + static Intercepted EnvGetter(Local property, const PropertyCallbackInfo& info) { Environment* env = Environment::GetCurrent(info); @@ -363,14 +413,7 @@ static Intercepted EnvGetter(Local property, env->env_vars()->Get(env->isolate(), property.As()); bool has_env = !value_string.IsEmpty(); - if (env->options()->trace_env) { - Utf8Value key(env->isolate(), property.As()); - fprintf(stderr, - "[--trace-env] get environment variable \"%.*s\"\n", - static_cast(key.length()), - key.out()); - PrintTraceEnvStack(env); - } + TraceEnv(env, "get", property.As()); if (has_env) { info.GetReturnValue().Set(value_string.ToLocalChecked()); @@ -410,14 +453,7 @@ static Intercepted EnvSetter(Local 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(key_utf8.length()), - key_utf8.out()); - PrintTraceEnvStack(env); - } + TraceEnv(env, "set", key); return Intercepted::kYes; } @@ -429,16 +465,7 @@ static Intercepted EnvQuery(Local property, if (property->IsString()) { int32_t rc = env->env_vars()->Query(env->isolate(), property.As()); bool has_env = (rc != -1); - - if (env->options()->trace_env) { - Utf8Value key_utf8(env->isolate(), property.As()); - fprintf(stderr, - "[--trace-env] query environment variable \"%.*s\": %s\n", - static_cast(key_utf8.length()), - key_utf8.out(), - has_env ? "is set" : "is not set"); - PrintTraceEnvStack(env); - } + TraceEnv(env, "query", property.As()); if (has_env) { // Return attributes for the property. info.GetReturnValue().Set(v8::None); @@ -455,14 +482,7 @@ static Intercepted EnvDeleter(Local property, if (property->IsString()) { env->env_vars()->Delete(env->isolate(), property.As()); - if (env->options()->trace_env) { - Utf8Value key_utf8(env->isolate(), property.As()); - fprintf(stderr, - "[--trace-env] delete environment variable \"%.*s\"\n", - static_cast(key_utf8.length()), - key_utf8.out()); - PrintTraceEnvStack(env); - } + TraceEnv(env, "delete", property.As()); } // process.env never has non-configurable properties, so always @@ -475,11 +495,7 @@ static void EnvEnumerator(const PropertyCallbackInfo& 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())); diff --git a/src/node_internals.h b/src/node_internals.h index 000ba16303740d..52e3e5dfc6ba48 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -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 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 key); void DefineZlibConstants(v8::Local target); v8::Isolate* NewIsolate(v8::Isolate::CreateParams* params, diff --git a/test/parallel/test-trace-env.js b/test/parallel/test-trace-env.js index c67924e5c610d3..6c72ec0209d18b 100644 --- a/test/parallel/test-trace-env.js +++ b/test/parallel/test-trace-env.js @@ -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"/); } }); @@ -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"/); } }); @@ -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"/); } });