New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor the kernel depth option and documentation #1759
Conversation
Hello, @ChoKyuWon. Thank you for the first patch on OSSCA 2023!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but we could improve the message a little bit.
uftrace.c
Outdated
@@ -655,14 +655,15 @@ static int parse_option(struct uftrace_opts *opts, int key, char *arg) | |||
|
|||
case 'k': | |||
opts->kernel = true; | |||
opts->kernel_depth = 1; | |||
break; | |||
|
|||
case 'K': | |||
opts->kernel = true; | |||
opts->kernel_depth = strtol(arg, NULL, 0); | |||
if (opts->kernel_depth < 1 || opts->kernel_depth > 50) { | |||
pr_use("invalid kernel depth: %s (ignoring...)\n", arg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can say "set to 1" instead of "(ignoring...)".
98687fd
to
e9c3205
Compare
initialize the opts->kernel_depth to 1 in '-k' and remove the ternary operator in setup_writers. Signed-off-by: ChoKyuWon <kyuwoncho18@gmail.com>
Signed-off-by: ChoKyuWon <kyuwoncho18@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
To process the
-k
option, thekernel_depth
in opt field is not set and is set to 1 when used (setup_writers
).This PR initializes the
opts->kernel_depth
to 1 at the case-k
and-K
if the input value is invalid.This refactoring clarifies the code semantic when the user only uses the
-k
, and invalid-K
.