lkml.org 
[lkml]   [2017]   [Nov]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 3/3] trace-cmd: Making stat to report when the stack tracer is ON
From
Date
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




\
 
 \ /
  Last update: 2017-11-23 13:33    [W:0.061 / U:4.672 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site