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

[Vladislav Valtchev <vladislav.valtchev@gmail.com>]

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