Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 15 Oct 2019 21:17:41 -0400
From:   Steven Rostedt <rostedt@goodmis.org>
To:     Joel Fernandes <joel@joelfernandes.org>
Cc:     "Viktor Rosendahl (BMW)" <viktor.rosendahl@gmail.com>,
        Ingo Molnar <mingo@redhat.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v9 1/4] ftrace: Implement fs notification for
 tracing_max_latency
Message-ID: <20191015211741.3ff2f3a1@gandalf.local.home>
In-Reply-To: <20191016004120.GD89937@google.com>
References: <20191015111910.4496-1-viktor.rosendahl@gmail.com>
        <20191015111910.4496-2-viktor.rosendahl@gmail.com>
        <20191016004120.GD89937@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, 15 Oct 2019 20:41:20 -0400
Joel Fernandes <joel@joelfernandes.org> wrote:

> > +static const struct file_operations tracing_max_lat_fops;
> > +
> > +#if (defined(CONFIG_TRACER_MAX_TRACE) || defined(CONFIG_HWLAT_TRACER)) && \
> > +	defined(CONFIG_FSNOTIFY)  
> 
> Something bothers me. If you dropped support for HWLAT_TRACER as you
> mentioned in the cover letter, then why does this #if look for the CONFIG
> option?
> 
> (and similar comment on the rest of the patch..)

Also, if you have a complex if statement like this, it is better to
create a new define to set it in a header file:

#if (defined(CONFIG_TRACER_MAX_TRACE) || defined(CONFIG_HWLAT_TRACER)) && \
	defined(CONFIG_FSNOTIFY)
# define HAVE_TRACER_FSNOTIFY
#endif

And then just use that:

#ifdef HAVE_TRACER_FSNOTIFY
...
#endif

It keeps things a bit more manageable.

-- Steve