Message-ID: <1512041215.4897.160.camel@gmail.com>
Date:   Thu, 30 Nov 2017 13:26:55 +0200
From:   Vladislav Valtchev <vladislav.valtchev@...il.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] trace-cmd: Making stat to report when the stack
 tracer is ON

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

Powered by blists - more mailing lists