[PATCH 0/3] trace-cmd: fixing three minor user experience issues

[PATCH 0/3] trace-cmd: fixing three minor user experience issues

[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

[PATCH 1/3] trace-cmd: Fix err msg for record w/o the -e option

[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

[PATCH 2/3] trace-cmd: s/plugin/tracer/ in record's man page

[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

[PATCH 3/3] trace-cmd: Making stat to report when the stack tracer is ON

[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

Re: [PATCH 3/3] trace-cmd: Making stat to report when the stack tracer is ON

[Steven Rostedt]

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);
>  }

Re: [PATCH 3/3] trace-cmd: Making stat to report when the stack tracer is ON

[Vladislav Valtchev]

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


 

Re: [PATCH 3/3] trace-cmd: Making stat to report when the stack tracer is ON

[Steven Rostedt]

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

Re: [PATCH 3/3] trace-cmd: Making stat to report when the stack tracer is ON

[Vladislav Valtchev]

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

Re: [PATCH 3/3] trace-cmd: Making stat to report when the stack tracer is ON

[Steven Rostedt]

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

Re: [PATCH 3/3] trace-cmd: Making stat to report when the stack tracer is ON

[Vladislav Valtchev]

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

Re: [PATCH 3/3] trace-cmd: Making stat to report when the stack tracer is ON

[Steven Rostedt]

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