Copilot commented on code in PR #3124:
URL: https://github.com/apache/brpc/pull/3124#discussion_r2458416325
##########
src/bthread/task_tracer.h:
##########
@@ -100,7 +103,7 @@ class TaskTracer {
Result ContextTrace(bthread_fcontext_t fcontext);
static Result TraceByLibunwind(unw_cursor_t& cursor);
Review Comment:
The method signature changed from static to non-static, which is a
significant API change affecting how this method is called. This change should
be documented with a comment explaining why it needs to be non-static (i.e., to
access the instance variable _signal_num).
```suggestion
// This method must be non-static in order to access the instance
variable _signal_num.
```
##########
src/bthread/task_tracer.cpp:
##########
@@ -35,6 +35,10 @@ namespace bthread {
DEFINE_uint32(signal_trace_timeout_ms, 50, "Timeout for signal trace in ms");
BUTIL_VALIDATE_GFLAG(signal_trace_timeout_ms,
butil::PositiveInteger<uint32_t>);
+// Note that SIGURG handler may be registered by some library such as cgo
+// so let the signal number be configurable
+DEFINE_int32(signal_number_for_trace, SIGURG,
+ "signal number used for stack trace, default to SIGURG");
Review Comment:
The flag definition lacks validation to ensure the signal number is valid.
Invalid signal numbers could cause runtime errors. Consider adding a validation
function similar to BUTIL_VALIDATE_GFLAG used for signal_trace_timeout_ms above
to check that the value is a valid signal number (e.g., between 1 and NSIG-1 or
within the set of real-time signals).
```suggestion
// Validation function for signal_number_for_trace
inline bool IsValidSignalNumber(int signal_number) {
// Valid signal numbers are between 1 and NSIG-1 (inclusive)
return signal_number >= 1 && signal_number < NSIG;
}
DEFINE_uint32(signal_trace_timeout_ms, 50, "Timeout for signal trace in ms");
BUTIL_VALIDATE_GFLAG(signal_trace_timeout_ms,
butil::PositiveInteger<uint32_t>);
// Note that SIGURG handler may be registered by some library such as cgo
// so let the signal number be configurable
DEFINE_int32(signal_number_for_trace, SIGURG,
"signal number used for stack trace, default to SIGURG");
BUTIL_VALIDATE_GFLAG(signal_number_for_trace, bthread::IsValidSignalNumber);
```
--
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]
