From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752554AbcGALMG (ORCPT ); Fri, 1 Jul 2016 07:12:06 -0400 Received: from mail-wm0-f67.google.com ([74.125.82.67]:36171 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752065AbcGALMC (ORCPT ); Fri, 1 Jul 2016 07:12:02 -0400 Date: Fri, 1 Jul 2016 12:13:30 +0200 From: Ingo Molnar To: Herbert Xu Cc: Tim Chen , "H. Peter Anvin" , Dan Carpenter , "David S. Miller" , Thomas Gleixner , Ingo Molnar , x86@kernel.org, Megha Dey , "Wang, Rui Y" , Denys Vlasenko , Xiaodong Liu , linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org, Linus Torvalds , Andrew Morton , Peter Zijlstra Subject: Re: [patch] crypto: sha256-mb - cleanup a || vs | typo Message-ID: <20160701101329.GA3833@gmail.com> References: <20160629144242.GE22818@mwanda> <8538242a-eab7-127e-e47e-26027fee4f6d@zytor.com> <1467319339.22178.256.camel@linux.intel.com> <20160701075559.GA11902@gmail.com> <20160701092806.GA13424@gondor.apana.org.au> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20160701092806.GA13424@gondor.apana.org.au> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Herbert Xu wrote: > On Fri, Jul 01, 2016 at 09:55:59AM +0200, Ingo Molnar wrote: > > > > Plus: > > > > > > >   /* Compute how many bytes to copy from user buffer into > > > > >    * extra block > > > > >    */ > > > > please use the customary (multi-line) comment style: > > This is the customary comment style of the networking stack and > the crypto API. So please don't change it. Guys, do you even read your own code?? That 'standard' is not being enforced consistently at all. Even in this very series there's an example of that weird comment not being followed: +++ b/arch/x86/crypto/sha1-mb/sha1_mb.c @@ -304,7 +304,7 @@ static struct sha1_hash_ctx *sha1_ctx_mgr_submit(struct sha1_ctx_mgr *mgr, /* * Compute how many bytes to copy from user buffer into * extra block See how this comment block uses the standard coding style, while the next patch has this weird coding style: - if ((ctx->partial_block_buffer_length) | (len < SHA256_BLOCK_SIZE)) { + if ((ctx->partial_block_buffer_length) || (len < SHA256_BLOCK_SIZE)) { /* Compute how many bytes to copy from user buffer into * extra block */ The networking code's "exceptionalism" regarding the standard comment style is super distracting and in this particular example it resulted in: - inconsistent comment styles next to each other, - the questionable '|' pattern hiding right next to: - pointless parantheses around the (ctx->partial_block_buffer_length), - which field name is also a misnomer. So anyone doing security review of that weird '|' pattern first has to figure out whether the 4 ugly code patterns amount to a security problem or not... One thing that is more harmful that any of the coding styles: the inconsistent coding style used by this code. Btw., as a historic reference, there is nothing sacred about the 'networking comments coding style': I was there (way too many years ago) when that comment style was introduced by Alan Cox's first TCP/IP code drop, and it was little more than just a random inconsistency that people are now treating as gospel... Thanks, Ingo