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@xxxxxxxxx> 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