From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752750AbdKWMcg (ORCPT ); Thu, 23 Nov 2017 07:32:36 -0500 Received: from mail-wm0-f67.google.com ([74.125.82.67]:40053 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752405AbdKWMcf (ORCPT ); Thu, 23 Nov 2017 07:32:35 -0500 X-Google-Smtp-Source: AGs4zMZc4GAqyou/ud1jCp2TFprzig3Wh+vp2VvsCaaV+FVsDgblDuZtkZRenFP5q6Q9BiOt4Z1K+Q== Message-ID: <1511440352.2719.34.camel@gmail.com> Subject: Re: [PATCH 3/3] trace-cmd: Making stat to report when the stack tracer is ON From: Vladislav Valtchev To: Steven Rostedt Cc: linux-kernel@vger.kernel.org Date: Thu, 23 Nov 2017 14:32:32 +0200 In-Reply-To: <20171122145002.17c39637@gandalf.local.home> References: <20171122180202.9519-1-vladislav.valtchev@gmail.com> <20171122180202.9519-4-vladislav.valtchev@gmail.com> <20171122145002.17c39637@gandalf.local.home> Organization: VMware Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.26.1-1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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