* [PATCH 0/3] trace-cmd: fixing three minor user experience issues @ 2017-11-22 18:01 Vladislav Valtchev (VMware) 2017-11-22 18:02 ` [PATCH 1/3] trace-cmd: Fix err msg for record w/o the -e option Vladislav Valtchev (VMware) ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Vladislav Valtchev (VMware) @ 2017-11-22 18:01 UTC (permalink / raw) To: rostedt, linux-kernel; +Cc: Vladislav Valtchev (VMware) This short patch series fixes three minor user experience issues in trace-cmd: - an error message (when record is used without -e and -p) - the documentation of trace record (plugins vs tracers terminology) - the command 'stat' to report when the stack tracer is enabled The purpose of this effort is to improve the user experience of the tool. Vladislav Valtchev (VMware) (3): trace-cmd: Fix err msg for record w/o the -e option trace-cmd: s/plugin/tracer/ in record's man page trace-cmd: Making stat to report when the stack tracer is ON Documentation/trace-cmd-record.1.txt | 11 +++++------ trace-cmd.h | 3 +++ trace-record.c | 6 ++---- trace-stack.c | 10 ++++++---- trace-stat.c | 11 +++++++---- 5 files changed, 23 insertions(+), 18 deletions(-) -- 2.14.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] trace-cmd: Fix err msg for record w/o the -e option 2017-11-22 18:01 [PATCH 0/3] trace-cmd: fixing three minor user experience issues Vladislav Valtchev (VMware) @ 2017-11-22 18:02 ` Vladislav Valtchev (VMware) 2017-11-22 18:02 ` [PATCH 2/3] trace-cmd: s/plugin/tracer/ in record's man page Vladislav Valtchev (VMware) 2017-11-22 18:02 ` [PATCH 3/3] trace-cmd: Making stat to report when the stack tracer is ON Vladislav Valtchev (VMware) 2 siblings, 0 replies; 11+ messages in thread From: Vladislav Valtchev (VMware) @ 2017-11-22 18:02 UTC (permalink / raw) To: rostedt, linux-kernel; +Cc: Vladislav Valtchev (VMware) Currently, running just `trace-cmd record` without telling which events have to be traced (-e) nor which tracer to use (-p), trace-cmd dies with the message: No instances reference?? Which might not be helpful to new users of the tool. This small patch removes an early check in trace_record() allowing, in the same case, the execution to continue a few more statements and fail more gracefully in the function check_doing_something() with the following message: no event or plugin was specified... aborting Signed-off-by: Vladislav Valtchev (VMware) <vladislav.valtchev@gmail.com> --- trace-record.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/trace-record.c b/trace-record.c index 06fc0e6..e2fb731 100644 --- a/trace-record.c +++ b/trace-record.c @@ -4722,11 +4722,9 @@ void trace_record (int argc, char **argv) * If top_instance doesn't have any plugins or events, then * remove it from being processed. */ - if (!extract && !__check_doing_something(&top_instance)) { - if (!buffer_instances) - die("No instances reference??"); + if (!extract && !__check_doing_something(&top_instance)) first_instance = buffer_instances; - } else + else topt = 1; update_first_instance(instance, topt); -- 2.14.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] trace-cmd: s/plugin/tracer/ in record's man page 2017-11-22 18:01 [PATCH 0/3] trace-cmd: fixing three minor user experience issues Vladislav Valtchev (VMware) 2017-11-22 18:02 ` [PATCH 1/3] trace-cmd: Fix err msg for record w/o the -e option Vladislav Valtchev (VMware) @ 2017-11-22 18:02 ` Vladislav Valtchev (VMware) 2017-11-22 18:02 ` [PATCH 3/3] trace-cmd: Making stat to report when the stack tracer is ON Vladislav Valtchev (VMware) 2 siblings, 0 replies; 11+ messages in thread From: Vladislav Valtchev (VMware) @ 2017-11-22 18:02 UTC (permalink / raw) To: rostedt, linux-kernel; +Cc: Vladislav Valtchev (VMware) Currently, the man page of trace-cmd record states that: To see a list of available plugins, see trace-cmd-list(1). While `trace-cmd list` [w/o options] mentions "tracers", which is the proper term there. The inconsistency exists because the terminology in trace-cmd/ftrace changed over time: what we call today "tracers" use to be called "plugins" when ftrace was created. This simple patch updates trace-cmd record's man page accordingly, in order to avoid any confusion and improve the user experience. Signed-off-by: Vladislav Valtchev (VMware) <vladislav.valtchev@gmail.com> --- Documentation/trace-cmd-record.1.txt | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/Documentation/trace-cmd-record.1.txt b/Documentation/trace-cmd-record.1.txt index 5663447..68afa16 100644 --- a/Documentation/trace-cmd-record.1.txt +++ b/Documentation/trace-cmd-record.1.txt @@ -25,12 +25,11 @@ file that can later be read (see trace-cmd-report(1)). OPTIONS ------- -*-p* 'plugin':: - Specify a trace plugin. Plugins are special Ftrace tracers that usually do - more than just trace an event. Common plugins are *function*, - *function_graph*, *preemptirqsoff*, *irqsoff*, *preemptoff*, and *wakeup*. - A plugin must be supported by the running kernel. To see a list of - available plugins, see trace-cmd-list(1). +*-p* 'tracer':: + Specify a tracer. Tracers usually do more than just trace an event. + Common tracers are: *function*, *function_graph*, *preemptirqsoff*, + *irqsoff*, *preemptoff* and *wakeup*. A tracer must be supported by the + running kernel. To see a list of available tracers, see trace-cmd-list(1). *-e* 'event':: Specify an event to trace. Various static trace points have been added to -- 2.14.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] trace-cmd: Making stat to report when the stack tracer is ON 2017-11-22 18:01 [PATCH 0/3] trace-cmd: fixing three minor user experience issues Vladislav Valtchev (VMware) 2017-11-22 18:02 ` [PATCH 1/3] trace-cmd: Fix err msg for record w/o the -e option Vladislav Valtchev (VMware) 2017-11-22 18:02 ` [PATCH 2/3] trace-cmd: s/plugin/tracer/ in record's man page Vladislav Valtchev (VMware) @ 2017-11-22 18:02 ` Vladislav Valtchev (VMware) 2017-11-22 19:50 ` Steven Rostedt 2 siblings, 1 reply; 11+ messages in thread From: Vladislav Valtchev (VMware) @ 2017-11-22 18:02 UTC (permalink / raw) To: rostedt, linux-kernel; +Cc: Vladislav Valtchev (VMware) trace-cmd stat is a handy way for users to see what tracing is currently going on, but currently is does not say anything about the stack tracing. This simple patch makes the command to show a message when the stack tracer is ON. Signed-off-by: Vladislav Valtchev (VMware) <vladislav.valtchev@gmail.com> --- trace-cmd.h | 3 +++ trace-stack.c | 10 ++++++---- trace-stat.c | 11 +++++++---- 3 files changed, 16 insertions(+), 8 deletions(-) diff --git a/trace-cmd.h b/trace-cmd.h index 485907f..62c5ea7 100644 --- a/trace-cmd.h +++ b/trace-cmd.h @@ -351,6 +351,9 @@ struct hook_list { struct hook_list *tracecmd_create_event_hook(const char *arg); void tracecmd_free_hooks(struct hook_list *hooks); +/* Stack tracer public functions */ +int is_stack_tracer_enabled(void); + /* --- Hack! --- */ int tracecmd_blk_hack(struct tracecmd_input *handle); diff --git a/trace-stack.c b/trace-stack.c index aa79ae3..0bd43a8 100644 --- a/trace-stack.c +++ b/trace-stack.c @@ -49,7 +49,7 @@ static void test_available(void) die("stack tracer not configured on running kernel"); } -static char read_proc(void) +int is_stack_tracer_enabled(void) { char buf[1]; int fd; @@ -62,8 +62,10 @@ static char read_proc(void) close(fd); if (n != 1) die("error reading %s", PROC_FILE); + if (buf[0] != '0' && buf[0] != '1') + die("Invalid value '%c' in %s", buf[0], PROC_FILE); - return buf[0]; + return buf[0] == '1'; } static void start_stop_trace(char val) @@ -72,7 +74,7 @@ static void start_stop_trace(char val) int fd; int n; - buf[0] = read_proc(); + buf[0] = '0' + is_stack_tracer_enabled(); if (buf[0] == val) return; @@ -124,7 +126,7 @@ static void read_trace(void) size_t n; int r; - if (read_proc() == '1') + if (is_stack_tracer_enabled()) printf("(stack tracer running)\n"); else printf("(stack tracer not running)\n"); diff --git a/trace-stat.c b/trace-stat.c index fd16354..3094d25 100644 --- a/trace-stat.c +++ b/trace-stat.c @@ -617,7 +617,7 @@ static void report_graph_funcs(struct buffer_instance *instance) die("malloc"); list_functions(path, "Function Graph Filter"); - + tracecmd_put_tracing_file(path); path = get_instance_file(instance, "set_graph_notrace"); @@ -625,7 +625,7 @@ static void report_graph_funcs(struct buffer_instance *instance) die("malloc"); list_functions(path, "Function Graph No Trace"); - + tracecmd_put_tracing_file(path); } @@ -638,7 +638,7 @@ static void report_ftrace_filters(struct buffer_instance *instance) die("malloc"); list_functions(path, "Function Filter"); - + tracecmd_put_tracing_file(path); path = get_instance_file(instance, "set_ftrace_notrace"); @@ -646,7 +646,7 @@ static void report_ftrace_filters(struct buffer_instance *instance) die("malloc"); list_functions(path, "Function No Trace"); - + tracecmd_put_tracing_file(path); } @@ -928,5 +928,8 @@ void trace_stat (int argc, char **argv) stat_instance(instance); } + if (is_stack_tracer_enabled()) + printf("Stack tracing is enabled\n\n"); + exit(0); } -- 2.14.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] trace-cmd: Making stat to report when the stack tracer is ON 2017-11-22 18:02 ` [PATCH 3/3] trace-cmd: Making stat to report when the stack tracer is ON Vladislav Valtchev (VMware) @ 2017-11-22 19:50 ` Steven Rostedt 2017-11-23 12:32 ` Vladislav Valtchev 0 siblings, 1 reply; 11+ messages in thread From: Steven Rostedt @ 2017-11-22 19:50 UTC (permalink / raw) To: Vladislav Valtchev (VMware); +Cc: linux-kernel On Wed, 22 Nov 2017 20:02:02 +0200 "Vladislav Valtchev (VMware)" <vladislav.valtchev@gmail.com> wrote: > trace-cmd stat is a handy way for users to see what tracing is currently going > on, but currently is does not say anything about the stack tracing. This simple > patch makes the command to show a message when the stack tracer is ON. I applied the first two. Small comments about this one. > > Signed-off-by: Vladislav Valtchev (VMware) <vladislav.valtchev@gmail.com> > --- > trace-cmd.h | 3 +++ > trace-stack.c | 10 ++++++---- > trace-stat.c | 11 +++++++---- > 3 files changed, 16 insertions(+), 8 deletions(-) > > diff --git a/trace-cmd.h b/trace-cmd.h > index 485907f..62c5ea7 100644 > --- a/trace-cmd.h > +++ b/trace-cmd.h > @@ -351,6 +351,9 @@ struct hook_list { > struct hook_list *tracecmd_create_event_hook(const char *arg); > void tracecmd_free_hooks(struct hook_list *hooks); > > +/* Stack tracer public functions */ > +int is_stack_tracer_enabled(void); As this is now in the trace-cmd.h header, please rename it to: tracecmd_is_stack_tracer_enabled() > + > /* --- Hack! --- */ > int tracecmd_blk_hack(struct tracecmd_input *handle); > > diff --git a/trace-stack.c b/trace-stack.c > index aa79ae3..0bd43a8 100644 > --- a/trace-stack.c > +++ b/trace-stack.c > @@ -49,7 +49,7 @@ static void test_available(void) > die("stack tracer not configured on running kernel"); > } > > -static char read_proc(void) > +int is_stack_tracer_enabled(void) > { > char buf[1]; > int fd; > @@ -62,8 +62,10 @@ static char read_proc(void) > close(fd); > if (n != 1) > die("error reading %s", PROC_FILE); > + if (buf[0] != '0' && buf[0] != '1') > + die("Invalid value '%c' in %s", buf[0], PROC_FILE); Why kill it here? We are reading the proc file system. What happens if a new kernel does update this. We just broke this tool, and we don't break user space with kernel updates. But user space should also be robust for updates like this. Actually, what I suggest is to keep the static read_proc function, and simply add: bool tracecmd_is_stack_tracer_enabled(void) { char buf; buf = read_proc(); return buf != '0'; } Much easier change. And handles cases where the proc file is 2 or more. -- Steve > > - return buf[0]; > + return buf[0] == '1'; > } > > static void start_stop_trace(char val) > @@ -72,7 +74,7 @@ static void start_stop_trace(char val) > int fd; > int n; > > - buf[0] = read_proc(); > + buf[0] = '0' + is_stack_tracer_enabled(); > if (buf[0] == val) > return; > > @@ -124,7 +126,7 @@ static void read_trace(void) > size_t n; > int r; > > - if (read_proc() == '1') > + if (is_stack_tracer_enabled()) > printf("(stack tracer running)\n"); > else > printf("(stack tracer not running)\n"); > diff --git a/trace-stat.c b/trace-stat.c > index fd16354..3094d25 100644 > --- a/trace-stat.c > +++ b/trace-stat.c > @@ -617,7 +617,7 @@ static void report_graph_funcs(struct buffer_instance *instance) > die("malloc"); > > list_functions(path, "Function Graph Filter"); > - > + > tracecmd_put_tracing_file(path); > > path = get_instance_file(instance, "set_graph_notrace"); > @@ -625,7 +625,7 @@ static void report_graph_funcs(struct buffer_instance *instance) > die("malloc"); > > list_functions(path, "Function Graph No Trace"); > - > + > tracecmd_put_tracing_file(path); > } > > @@ -638,7 +638,7 @@ static void report_ftrace_filters(struct buffer_instance *instance) > die("malloc"); > > list_functions(path, "Function Filter"); > - > + > tracecmd_put_tracing_file(path); > > path = get_instance_file(instance, "set_ftrace_notrace"); > @@ -646,7 +646,7 @@ static void report_ftrace_filters(struct buffer_instance *instance) > die("malloc"); > > list_functions(path, "Function No Trace"); > - > + > tracecmd_put_tracing_file(path); > } > > @@ -928,5 +928,8 @@ void trace_stat (int argc, char **argv) > stat_instance(instance); > } > > + if (is_stack_tracer_enabled()) > + printf("Stack tracing is enabled\n\n"); > + > exit(0); > } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] trace-cmd: Making stat to report when the stack tracer is ON 2017-11-22 19:50 ` Steven Rostedt @ 2017-11-23 12:32 ` Vladislav Valtchev 2017-11-29 12:57 ` Steven Rostedt 0 siblings, 1 reply; 11+ messages in thread From: Vladislav Valtchev @ 2017-11-23 12:32 UTC (permalink / raw) To: Steven Rostedt; +Cc: linux-kernel On Wed, 2017-11-22 at 14:50 -0500, Steven Rostedt wrote: > > I applied the first two. Small comments about this one. Thanks, Steven. > > > > > > +/* Stack tracer public functions */ > > +int is_stack_tracer_enabled(void); > > As this is now in the trace-cmd.h header, please rename it to: > > tracecmd_is_stack_tracer_enabled() > > > > > -static char read_proc(void) > > +int is_stack_tracer_enabled(void) > > { > > char buf[1]; > > int fd; > > @@ -62,8 +62,10 @@ static char read_proc(void) > > close(fd); > > if (n != 1) > > die("error reading %s", PROC_FILE); > > + if (buf[0] != '0' && buf[0] != '1') > > + die("Invalid value '%c' in %s", buf[0], PROC_FILE); > > Why kill it here? We are reading the proc file system. What happens if > a new kernel does update this. We just broke this tool, and we don't > break user space with kernel updates. But user space should also be > robust for updates like this. > I perfectly understand that you might want to accept values > 1, in the future. I was concerned about using buf != '0' since that means to accept as enabled any kind of weird values like '?', ' ', 'x', '(' etc. plus non-printable chars as well: that feels kind-of an "unsafe" to me: if a kernel bug causes the tracing files to contain garbage, shouldn't we complain somehow? > Actually, what I suggest is to keep the static read_proc function, and > simply add: > > bool tracecmd_is_stack_tracer_enabled(void) > { > char buf; > > buf = read_proc(); > return buf != '0'; > } > > Much easier change. And handles cases where the proc file is 2 or more. > Agree. We might also add an if (!isdigit(buf)) die() before return, but I understand that, on the other side, we might not need to check the kernel's behavior this way. We might ultimately trust the kernel [every part of it] and save trace-cmd's code from having a ton of verbose sanity checks like this one. It's all about trade-offs, clearly. Therefore, I'm fine with whatever trade-off you believe is better for trace-cmd. Vlad ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] trace-cmd: Making stat to report when the stack tracer is ON 2017-11-23 12:32 ` Vladislav Valtchev @ 2017-11-29 12:57 ` Steven Rostedt 2017-11-29 14:00 ` Vladislav Valtchev 0 siblings, 1 reply; 11+ messages in thread From: Steven Rostedt @ 2017-11-29 12:57 UTC (permalink / raw) To: Vladislav Valtchev; +Cc: linux-kernel On Thu, 23 Nov 2017 14:32:32 +0200 Vladislav Valtchev <vladislav.valtchev@gmail.com> wrote: > Agree. > We might also add an if (!isdigit(buf)) die() before return, but I understand > that, on the other side, we might not need to check the kernel's behavior > this way. We might ultimately trust the kernel [every part of it] and save > trace-cmd's code from having a ton of verbose sanity checks like this one. > > It's all about trade-offs, clearly. > Therefore, I'm fine with whatever trade-off you believe is better for trace-cmd. Let's think about what the user wants. If you do a "trace-cmd stat" what are you looking for? You want to see what ftrace operations are available. Now let's say we do something weird, or someone has some weird modified kernel, and the stack tracer shows something that trace-cmd doesn't expect. With a die, it kills the tool. Would you like it if you ran "trace-cmd stat" and got it crashed with an error message saying the kernel is doing something it doesn't understand? To me, I'd be pissed. I would be cursing at trace-cmd saying "I don't give a frick about that, show me what you do know!" Now, do you think having a "die" is good there? The only "die"s I have in trace-cmd stat is failure to allocate. That's because the tool itself is then corrupted. In no case should trace-cmd stat die because it doesn't understand something. And really, that should be the entire case for trace-cmd. The only reason to call die is if there's a failure in the tool itself where it can't continue (failed memory allocations are usually severe). -- Steve ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] trace-cmd: Making stat to report when the stack tracer is ON 2017-11-29 12:57 ` Steven Rostedt @ 2017-11-29 14:00 ` Vladislav Valtchev 2017-11-29 16:18 ` Steven Rostedt 0 siblings, 1 reply; 11+ messages in thread From: Vladislav Valtchev @ 2017-11-29 14:00 UTC (permalink / raw) To: Steven Rostedt; +Cc: linux-kernel On Wed, 2017-11-29 at 07:57 -0500, Steven Rostedt wrote: > > Let's think about what the user wants. > > If you do a "trace-cmd stat" what are you looking for? You want to see > what ftrace operations are available. Now let's say we do something > weird, or someone has some weird modified kernel, and the stack tracer > shows something that trace-cmd doesn't expect. With a die, it kills the > tool. > > Would you like it if you ran "trace-cmd stat" and got it crashed with > an error message saying the kernel is doing something it doesn't > understand? To me, I'd be pissed. I would be cursing at trace-cmd > saying "I don't give a frick about that, show me what you do know!" > I'm surprised how different kind of users are we :-) To me as user, in case there is a kernel bug, the best thing I'd expect to see is the tool refusing to work and reporting that it does not really know the state of the tracer because of invalid data in tracefs. In other words, I expect a tool to behave like: "I don't know what is that, so I cannot take any decisions. Here's the detailed problem (err msg, data). Now only a human may help now". The other approach is instead: "I don't know what is that, but I'll guess by best trying to not piss off the user". Both approaches have PROs and CONs. It is evident that, in the first case the tool is pedantic and won't give even a try to do something. In the 2nd case instead, the tool might guess even correctly at first but, by not exposing the underlying issue, it might fail (if there's a mem corruption at kernel-level, likely it will) at any moment in a strange way. In any case, the user will be pissed. Just, in the first case he/she will benefit from an "early-stage" error, that might make the problem easier to find. Also, with the 2nd approach, the user won't figure out immediately that the tool is not guilty, while the kernel is: nobody should blame the poor tool when it had no chance to get its job done in the first place. > Now, do you think having a "die" is good there? I prefer the "fail-early" approach in general. For a tool like trace-cmd, I'd implement a layer validating all the input with an option for controlling validation in hot paths. BUT, since that is not the philosophy of the tool, adding a check like that only there, does not make much sense. It makes sense to take an approach and consistently follow it. So, I'll fix my patch as you suggested. I hope I'm not "pissing off" you with my long comments :-) Vlad ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] trace-cmd: Making stat to report when the stack tracer is ON 2017-11-29 14:00 ` Vladislav Valtchev @ 2017-11-29 16:18 ` Steven Rostedt 2017-11-30 11:26 ` Vladislav Valtchev 0 siblings, 1 reply; 11+ messages in thread From: Steven Rostedt @ 2017-11-29 16:18 UTC (permalink / raw) To: Vladislav Valtchev; +Cc: linux-kernel On Wed, 29 Nov 2017 16:00:54 +0200 Vladislav Valtchev <vladislav.valtchev@gmail.com> wrote: > On Wed, 2017-11-29 at 07:57 -0500, Steven Rostedt wrote: > > > > Let's think about what the user wants. > > > > If you do a "trace-cmd stat" what are you looking for? You want to see > > what ftrace operations are available. Now let's say we do something > > weird, or someone has some weird modified kernel, and the stack tracer > > shows something that trace-cmd doesn't expect. With a die, it kills the > > tool. > > > > Would you like it if you ran "trace-cmd stat" and got it crashed with > > an error message saying the kernel is doing something it doesn't > > understand? To me, I'd be pissed. I would be cursing at trace-cmd > > saying "I don't give a frick about that, show me what you do know!" > > > > I'm surprised how different kind of users are we :-) > > To me as user, in case there is a kernel bug, the best thing I'd expect > to see is the tool refusing to work and reporting that it does not really > know the state of the tracer because of invalid data in tracefs. > > In other words, I expect a tool to behave like: > "I don't know what is that, so I cannot take any decisions. > Here's the detailed problem (err msg, data). Now only a human may help now". > > The other approach is instead: > "I don't know what is that, but I'll guess by best trying to not piss off the user". No, I want "I don't know what this is (tell user about it) and carry on." The point being, trace-cmd stat does a lot more than check if the stack tracer is on. If it can't figure that out, it should warn that it got confused about it, but it should still report about all the other tracing that it does know about. And who said there was a bug? It could be a modified kernel that was done on purpose. Why should that kill trace-cmd? > > Both approaches have PROs and CONs. It is evident that, in the first case the tool is > pedantic and won't give even a try to do something. In the 2nd case instead, the tool > might guess even correctly at first but, by not exposing the underlying issue, it might > fail (if there's a mem corruption at kernel-level, likely it will) at any moment in a > strange way. In any case, the user will be pissed. Just, in the first case he/she will > benefit from an "early-stage" error, that might make the problem easier to find. > Also, with the 2nd approach, the user won't figure out immediately that the tool is not > guilty, while the kernel is: nobody should blame the poor tool when it had no chance > to get its job done in the first place. It should warn, and continue. It shouldn't die. Warning lets the user know that there was some kind of anomaly that trace-cmd doesn't know of, and the user can investigate further if they want to. Or the user could say "oh yeah, I modified this kernel, or I have an out of date trace-cmd, no problem, it still gives me the information I'm looking for." I see no CON with my approach, but I see many with yours. > > > Now, do you think having a "die" is good there? > > I prefer the "fail-early" approach in general. For a tool like trace-cmd, > I'd implement a layer validating all the input with an option for controlling > validation in hot paths. BUT, since that is not the philosophy of the tool, > adding a check like that only there, does not make much sense. > > It makes sense to take an approach and consistently follow it. > So, I'll fix my patch as you suggested. > > > I hope I'm not "pissing off" you with my long comments :-) Nope not at all :-) I'm just trying to educate you. Please note, the kernel itself does the same thing. And Linus has yelled at people for using BUG_ON() instead of WARN_ON(). He says, don't crash my kernel just because your code screwed up! ftrace itself has lots of self checks. It will shut itself down if it finds an anomaly, but it doesn't crash the kernel. There's one exception, and that's when it gets into a code path during function graph tracing where there's no place to return to. That happened once, and was due to a bug in gcc that caused function graph tracing to make all calls it called not return properly. There was no recovery. But that's the exception and not the rule. -- Steve ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] trace-cmd: Making stat to report when the stack tracer is ON 2017-11-29 16:18 ` Steven Rostedt @ 2017-11-30 11:26 ` Vladislav Valtchev 2017-11-30 14:56 ` Steven Rostedt 0 siblings, 1 reply; 11+ messages in thread From: Vladislav Valtchev @ 2017-11-30 11:26 UTC (permalink / raw) To: Steven Rostedt; +Cc: linux-kernel On Wed, 2017-11-29 at 11:18 -0500, Steven Rostedt wrote: > > In other words, I expect a tool to behave like: > > "I don't know what is that, so I cannot take any decisions. > > Here's the detailed problem (err msg, data). Now only a human may help now". > > > > The other approach is instead: > > "I don't know what is that, but I'll guess my best trying to not piss off the user". > > No, I want "I don't know what this is (tell user about it) and carry > on." Ah, OK. I'm sorry then. I got the impression that you wanted just buf != '0', no warnings. > The point being, trace-cmd stat does a lot more than check if the stack > tracer is on. If it can't figure that out, it should warn that it got > confused about it, but it should still report about all the other > tracing that it does know about. That makes perfect sense to me. It's a more verbose to implement than just die(), but it does not hide the problem and also will display other useful information to the user. > > And who said there was a bug? It could be a modified kernel that was > done on purpose. Why should that kill trace-cmd? > Agree. Proper unknown value handling + error reporting, makes sense to me. It offers the best user experience even if, not surprisingly, is the most expensive one in terms of amount of code necessary (compared to DIE/ASSERT/VERIFY and to just trying to silently ignore the problem). I proposed die() because, by looking at the original code of read_proc(): static char read_proc(void) { char buf[1]; int fd; int n; fd = open(PROC_FILE, O_RDONLY); if (fd < 0) die("reading %s", PROC_FILE); n = read(fd, buf, 1); close(fd); if (n != 1) die("error reading %s", PROC_FILE); return buf[0]; } I saw that trace-cmd dies in case: - the file cannot be opened [e.g. file not found, no permissions etc.] - the file is empty So I thought that the approach was: "if the contract is violated, I die" Now, do you want also that the cases when the PROC_FILE, /proc/sys/kernel/stack_tracer_enabled, cannot be opened or it is empty should be to gracefully handled showing a warning + unknown status for the stack tracer? I noticed also this function: static void test_available(void) { struct stat buf; int fd; fd = stat(PROC_FILE, &buf); if (fd < 0) die("stack tracer not configured on running kernel"); } Called by trace_stack(). I start to think: maybe it's fine for 'stat' to just assume that the stack tracer is not configured on the running kernel if the file does not exist, but it should show a warning + UNKNOWN status is the file is empty. Right? I re-write this patch to do that. > > I see no CON with my approach, but I see many with yours. > That specific approach seems good to me. The only CON I see here is more verbose code, but nothing really to worry about. > > > > I hope I'm not "pissing off" you with my long comments :-) > > Nope not at all :-) > > I'm just trying to educate you. Please note, the kernel itself does the > same thing. And Linus has yelled at people for using BUG_ON() instead > of WARN_ON(). He says, don't crash my kernel just because your code > screwed up! > Thanks for the patience in doing that. I'm trying my best to understand the philosophy of trace-cmd and follow it while writing my patches. But, as you see, sometimes it is different from the approaches that I'm used to, and it just will take time for me to fully understand and follow it. Vlad ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] trace-cmd: Making stat to report when the stack tracer is ON 2017-11-30 11:26 ` Vladislav Valtchev @ 2017-11-30 14:56 ` Steven Rostedt 0 siblings, 0 replies; 11+ messages in thread From: Steven Rostedt @ 2017-11-30 14:56 UTC (permalink / raw) To: Vladislav Valtchev; +Cc: linux-kernel On Thu, 30 Nov 2017 13:26:55 +0200 Vladislav Valtchev <vladislav.valtchev@gmail.com> wrote: > I proposed die() because, by looking at the original code of read_proc(): > > static char read_proc(void) > { > char buf[1]; > int fd; > int n; > > fd = open(PROC_FILE, O_RDONLY); > if (fd < 0) > die("reading %s", PROC_FILE); > n = read(fd, buf, 1); > close(fd); > if (n != 1) > die("error reading %s", PROC_FILE); > > return buf[0]; > } > > I saw that trace-cmd dies in case: > - the file cannot be opened [e.g. file not found, no permissions etc.] > - the file is empty Right, which perhaps it shouldn't die, but there shouldn't be a case where this happens. As for file not found, it should be checked before calling this function, as you noticed this is done below. > > So I thought that the approach was: > "if the contract is violated, I die" > > > Now, do you want also that the cases when the > PROC_FILE, /proc/sys/kernel/stack_tracer_enabled, cannot be opened > or it is empty should be to gracefully handled showing a warning + unknown > status for the stack tracer? Let's just have the trace-cmd stat say stack tracing is "indeterminate". > > I noticed also this function: > > static void test_available(void) > { > struct stat buf; > int fd; > > fd = stat(PROC_FILE, &buf); > if (fd < 0) > die("stack tracer not configured on running kernel"); > } > > Called by trace_stack(). I start to think: maybe it's fine for 'stat' to > just assume that the stack tracer is not configured on the running kernel > if the file does not exist, but it should show a warning + UNKNOWN status > is the file is empty. Right? Actually, if the file doesn't exist, then it isn't enabled for the kernel. In that case, we do nothing (don't report stack tracing). Stat is about what is enabled and configured into the kernel. Not what isn't configured into the kernel. Having stack tracing not enabled is common in some configurations. I don't want to add noise to the output. Unless we add a "-v" for verbose option. Then perhaps with verbose set, we can say what isn't enabled. Better yet, have: -v - show all that is configured into the kernel but not enabled -vv - show all that trace-cmd knows about but isn't configured into the kernel. -- Steve ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-11-30 14:56 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-11-22 18:01 [PATCH 0/3] trace-cmd: fixing three minor user experience issues Vladislav Valtchev (VMware) 2017-11-22 18:02 ` [PATCH 1/3] trace-cmd: Fix err msg for record w/o the -e option Vladislav Valtchev (VMware) 2017-11-22 18:02 ` [PATCH 2/3] trace-cmd: s/plugin/tracer/ in record's man page Vladislav Valtchev (VMware) 2017-11-22 18:02 ` [PATCH 3/3] trace-cmd: Making stat to report when the stack tracer is ON Vladislav Valtchev (VMware) 2017-11-22 19:50 ` Steven Rostedt 2017-11-23 12:32 ` Vladislav Valtchev 2017-11-29 12:57 ` Steven Rostedt 2017-11-29 14:00 ` Vladislav Valtchev 2017-11-29 16:18 ` Steven Rostedt 2017-11-30 11:26 ` Vladislav Valtchev 2017-11-30 14:56 ` Steven Rostedt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).