Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <ec43cf0faa4bfeaa4495b4e1f1c61e617d468591.camel@HansenPartnership.com>
Subject: Re: [PATCH] locks: remove trailing semicolon in macro definition
From:   James Bottomley <James.Bottomley@HansenPartnership.com>
To:     Randy Dunlap <rdunlap@infradead.org>, Tom Rix <trix@redhat.com>,
        Matthew Wilcox <willy@infradead.org>
Cc:     jlayton@kernel.org, bfields@fieldses.org, viro@zeniv.linux.org.uk,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Date:   Sun, 29 Nov 2020 10:15:32 -0800
In-Reply-To: <d65cd737-61a5-4b31-7f25-e72f0a7f4ec2@infradead.org>
References: <20201127190707.2844580-1-trix@redhat.com>
         <20201127195323.GZ4327@casper.infradead.org>
         <8e7c0d56-64f3-d0b6-c1cf-9f285c59f169@redhat.com>
         <d65cd737-61a5-4b31-7f25-e72f0a7f4ec2@infradead.org>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.34.4 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Sun, 2020-11-29 at 09:52 -0800, Randy Dunlap wrote:
> On 11/29/20 9:47 AM, Tom Rix wrote:
> > On 11/27/20 11:53 AM, Matthew Wilcox wrote:
> > > On Fri, Nov 27, 2020 at 11:07:07AM -0800, trix@redhat.com wrote:
> > > > +++ b/fs/fcntl.c
> > > > @@ -526,7 +526,7 @@ SYSCALL_DEFINE3(fcntl64, unsigned int, fd,
> > > > unsigned int, cmd,
> > > >  	(dst)->l_whence = (src)->l_whence;	\
> > > >  	(dst)->l_start = (src)->l_start;	\
> > > >  	(dst)->l_len = (src)->l_len;		\
> > > > -	(dst)->l_pid = (src)->l_pid;
> > > > +	(dst)->l_pid = (src)->l_pid
> > > This should be wrapped in a do { } while (0).
> > > 
> > > Look, this warning is clearly great at finding smelly code, but
> > > the
> > > fixes being generated to shut up the warning are low quality.
> > > 
> > Multiline macros not following the do {} while (0) pattern are
> > likely a larger problem.
> > 
> > I'll take a look.
> 
> Could it become a static inline function instead?
> or that might expand its scope too much?

I think nowadays we should always use static inlines for argument
checking unless we're capturing debug information like __FILE__ or
__LINE__ or something that a static inline can't.  Even in the latter
case the pattern should probably be single line #define that captures
the information and passes it to a static inline.

There was a time when we had problems with compiler expansion of static
inlines, so we shouldn't go back and churn the code base to change it
because there's thousands of these and possibly some old compiler used
for an obscure architecture that still needs the define.

James