Copilot commented on code in PR #3523:
URL: https://github.com/apache/brpc/pull/3523#discussion_r3936082209
##########
src/brpc/builtin/pprof_service.cpp:
##########
@@ -68,7 +68,7 @@ static int ReadSeconds(Controller* cntl) {
}
}
- return seconds;
+ return std::min(seconds, FLAGS_max_profiling_seconds);
}
Review Comment:
`ReadSeconds()` now clamps to `FLAGS_max_profiling_seconds`, but `profile()`
treats any `sleep_sec <= 0` as "missing seconds" unless the controller is
already failed. If `-max_profiling_seconds` is set to 0 (allowed by the
validator) and the user passes `?seconds=N`, this will return 0 and produce a
misleading error message. Consider validating the clamped value and setting a
clear error when `seconds` is out of range.
##########
src/bvar/variable.cpp:
##########
@@ -54,11 +54,35 @@ DEFINE_bool(bvar_abort_on_same_name, false, "Abort when
names of bvar are same")
BUTIL_VALIDATE_GFLAG(bvar_abort_on_same_name,
validate_bvar_abort_on_same_name);
-DEFINE_bool(bvar_log_dumpped, false,
- "[For debugging] print dumpped info"
- " into logstream before call Dumpper");
+DEFINE_bool(bvar_log_dumpped, false, "[For debugging] print dumpped info "
+ "into logstream before call Dumpper");
BUTIL_VALIDATE_GFLAG(bvar_log_dumpped, butil::PassValidate);
+DEFINE_bool(bvar_dump, false, "Create a background thread dumping all bvar
periodically, "
+ "all bvar_dump_* flags are not effective when
this flag is off");
+DEFINE_int32(bvar_dump_interval, 10, "Seconds between consecutive dump");
+DEFINE_string(bvar_dump_file, "monitor/bvar.<app>.data",
+ "Dump bvar into this file, not settable at runtime");
+DEFINE_string(bvar_dump_include, "", "Dump bvar matching these wildcards,
separated "
+ "by semicolon(;), empty means including
all");
Review Comment:
The help string for `bvar_dump_include` has a double space after the comma
("wildcards, separated"), which looks unintentional in a user-facing flag
description.
##########
src/bvar/variable.cpp:
##########
@@ -951,22 +951,16 @@ static bool wakeup_dumping_thread(const char*, const
std::string&) {
return true;
}
-const bool ALLOW_UNUSED dummy_bvar_dump_file =
GFLAGS_NAMESPACE::RegisterFlagValidator(
- &FLAGS_bvar_dump_file, wakeup_dumping_thread);
const bool ALLOW_UNUSED dummy_bvar_dump_filter =
GFLAGS_NAMESPACE::RegisterFlagValidator(
&FLAGS_bvar_dump_include, wakeup_dumping_thread);
const bool ALLOW_UNUSED dummy_bvar_dump_exclude =
GFLAGS_NAMESPACE::RegisterFlagValidator(
&FLAGS_bvar_dump_exclude, wakeup_dumping_thread);
const bool ALLOW_UNUSED dummy_bvar_dump_prefix =
GFLAGS_NAMESPACE::RegisterFlagValidator(
&FLAGS_bvar_dump_prefix, wakeup_dumping_thread);
-const bool ALLOW_UNUSED dummy_bvar_dump_tabs =
GFLAGS_NAMESPACE::RegisterFlagValidator(
- &FLAGS_bvar_dump_tabs, wakeup_dumping_thread);
BUTIL_VALIDATE_GFLAG(mbvar_dump, validate_bvar_dump);
const bool ALLOW_UNUSED dummy_mbvar_dump_prefix =
GFLAGS_NAMESPACE::RegisterFlagValidator(
&FLAGS_mbvar_dump_prefix, wakeup_dumping_thread);
-const bool ALLOW_UNUSED dump_mbvar_dump_file =
GFLAGS_NAMESPACE::RegisterFlagValidator(
- &FLAGS_mbvar_dump_file, wakeup_dumping_thread);
Review Comment:
This change relies on removing validators so `/flags/...?...setvalue=` can
no longer modify `bvar_dump_file`, `bvar_dump_tabs`, and `mbvar_dump_file`.
There are existing unit tests for `/flags` behavior, but none assert that these
specific flags are now rejected; adding coverage would help prevent regressions
of the security fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
