linux-kernel - Re: [Linux-eng] [RFC 0/3] Abstract empty functions with STUB

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]

Message-ID: <20190118170117.GC8120@e119886-lin.cambridge.arm.com>
Date:   Fri, 18 Jan 2019 17:01:17 +0000
From:   Andrew Murray <andrew.murray@....com>
To:     Dave Martin <Dave.Martin@....com>
Cc:     Russell King - ARM Linux admin <linux@...linux.org.uk>,
        Kees Cook <keescook@...omium.org>,
        Arnd Bergmann <arnd@...db.de>,
        Catalin Marinas <catalin.marinas@....com>, rjw@...ysocki.net,
        linux-kernel@...r.kernel.org, Will Deacon <will.deacon@....com>,
        Steven Price <Steven.Price@....com>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>,
        Grant Likely <Grant.Likely@....com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [Linux-eng] [RFC 0/3] Abstract empty functions with STUB_UNLESS
 macro

On Fri, Jan 18, 2019 at 04:44:25PM +0000, Dave Martin wrote:
> On Fri, Jan 18, 2019 at 04:37:36PM +0000, Russell King - ARM Linux admin wrote:
> > On Fri, Jan 18, 2019 at 04:00:27PM +0000, Andrew Murray wrote:
> > > A common pattern found in header files is a function declaration dependent
> > > on a CONFIG_ option being enabled, followed by an empty function for when
> > > that option isn't enabled. This boilerplate code can often take up a lot
> > > of space and impact code readability.
> > > 
> > > This series introduces a STUB_UNLESS macro that simplifies header files as
> > > follows:
> > > 
> > > STUB_UNLESS(CONFIG_FOO, [body], prototype)
> > 
> > Can you explain the desire to make the second argument optional,
> > rather than having the mandatory arguments first and the optional body
> > last?  It will mean more lines at each site, but I don't think that's
> > a bad thing:

My intent was to make the function prototype look like an ordinary prototype
but with all this special macro stuff on the preceding line. Much like this:

> > 
> > STUB_UNLESS(CONFIG_HAVE_HW_BREAKPOINT,
> > void hw_breakpoint_thread_switch(struct task_struct *next));

Besides the extra ')' at the end it looks like a normal prototype. I felt this
may be important as existing tooling (ctags etc) might have a better chance of
recognising it and it wouldn't be so alien to new developers. I feared that if
the 'prototype' argument was in the middle then it would get lost in all the
other arguments and be less readable as a prototype.

> > 
> > STUB_UNLESS(CONFIG_CPU_FREQ,
> > struct cpufreq_policy *cpufreq_cpu_get_raw(unsigned int cpu), return NULL);
> > 
> > or:
> > 
> > STUB_UNLESS(CONFIG_CPU_FREQ,
> > struct cpufreq_policy *cpufreq_cpu_get_raw(unsigned int cpu),
> > 	return NULL);

As you indicate here, it's possible to spread this to three lines and keep the
readability of the prototype - though I was keen to condense it to as few lines
as possible (I was probably putting too much focus on the diff stat).

> > 
> > Seems to be more readable in terms of the flow.
> 
> Hmmm, looking at that, I probably prefer that too.

Feedback I've had so far suggests that there is a preference to putting the
optional argument at the end, I have no objection to this.

> 
> In the unlikely case that <body> uses the function arguments it would be
> quite confusing to have the body before the function prototype.
> 
> If we can keep this down to two lines so much the better, but still
> seems fine.

This is the compromise - having the optional argument after the prototype
will likely result in wrapping to the next line as prototypes tend to be
long. Perhaps this is more readable.

> 
> Provided we don't end up needing a trailing comma in the void case, to
> supply the empty body argument, that is.

No this isn't necessary.

Thanks,

Andrew Murray

> 
> Cheers
> ---Dave