Messages in this thread | | | Subject | Re: [PATCH 3/3] trace-cmd: Making stat to report when the stack tracer is ON | From | Vladislav Valtchev <> | Date | Thu, 23 Nov 2017 14:32:32 +0200 |
| |
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
| |