linux-kernel - Re: [PATCH v9 1/4] ftrace: Implement fs notification for tracing_max

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 15 Oct 2019 21:17:41 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Joel Fernandes <joel@...lfernandes.org>
Cc:     "Viktor Rosendahl (BMW)" <viktor.rosendahl@...il.com>,
        Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v9 1/4] ftrace: Implement fs notification for
 tracing_max_latency

On Tue, 15 Oct 2019 20:41:20 -0400
Joel Fernandes <joel@...lfernandes.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