[patch] crypto: sha256-mb - cleanup a || vs | typo

[patch] crypto: sha256-mb - cleanup a || vs | typo

[Dan Carpenter]

|| and | behave basically the same here but || is intended.  It causes a
static checker warning to mix up bitwise and logical operations.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/arch/x86/crypto/sha256-mb/sha256_mb.c b/arch/x86/crypto/sha256-mb/sha256_mb.c
index c9d5dcc..4ec895a 100644
--- a/arch/x86/crypto/sha256-mb/sha256_mb.c
+++ b/arch/x86/crypto/sha256-mb/sha256_mb.c
@@ -299,7 +299,7 @@ static struct sha256_hash_ctx *sha256_ctx_mgr_submit(struct sha256_ctx_mgr *mgr,
 	 * Or if the user's buffer contains less than a whole block,
 	 * append as much as possible to the extra block.
 	 */
-	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
 		 */

Re: [patch] crypto: sha256-mb - cleanup a || vs | typo

[H. Peter Anvin]

On 06/29/16 07:42, Dan Carpenter wrote:
> || and | behave basically the same here but || is intended.  It causes a
> static checker warning to mix up bitwise and logical operations.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/arch/x86/crypto/sha256-mb/sha256_mb.c b/arch/x86/crypto/sha256-mb/sha256_mb.c
> index c9d5dcc..4ec895a 100644
> --- a/arch/x86/crypto/sha256-mb/sha256_mb.c
> +++ b/arch/x86/crypto/sha256-mb/sha256_mb.c
> @@ -299,7 +299,7 @@ static struct sha256_hash_ctx *sha256_ctx_mgr_submit(struct sha256_ctx_mgr *mgr,
>  	 * Or if the user's buffer contains less than a whole block,
>  	 * append as much as possible to the extra block.
>  	 */
> -	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
>  		 */
> 

As far as I know the | was an intentional optimization, so you may way
to look at the generated code.

	-hpa

Re: [patch] crypto: sha256-mb - cleanup a || vs | typo

[Dan Carpenter]

On Wed, Jun 29, 2016 at 10:05:53AM -0700, H. Peter Anvin wrote:
> On 06/29/16 07:42, Dan Carpenter wrote:
> > || and | behave basically the same here but || is intended.  It causes a
> > static checker warning to mix up bitwise and logical operations.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > diff --git a/arch/x86/crypto/sha256-mb/sha256_mb.c b/arch/x86/crypto/sha256-mb/sha256_mb.c
> > index c9d5dcc..4ec895a 100644
> > --- a/arch/x86/crypto/sha256-mb/sha256_mb.c
> > +++ b/arch/x86/crypto/sha256-mb/sha256_mb.c
> > @@ -299,7 +299,7 @@ static struct sha256_hash_ctx *sha256_ctx_mgr_submit(struct sha256_ctx_mgr *mgr,
> >  	 * Or if the user's buffer contains less than a whole block,
> >  	 * append as much as possible to the extra block.
> >  	 */
> > -	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
> >  		 */
> > 
> 
> As far as I know the | was an intentional optimization, so you may way
> to look at the generated code.

I know how the rules work.  I just thought it looked more like a typo
than an optimization.  It's normally a typo.  It's hard to tell the
intent.

I think I'll modify my static checker to ignore these since the typo is
harmless.

regards,
dan carpenter

Re: [patch] crypto: sha256-mb - cleanup a || vs | typo

[Joe Perches]

On Thu, 2016-06-30 at 10:50 +0300, Dan Carpenter wrote:
> On Wed, Jun 29, 2016 at 10:05:53AM -0700, H. Peter Anvin wrote:
> > On 06/29/16 07:42, Dan Carpenter wrote:
> > > > > and | behave basically the same here but || is intended.  It causes a
> > > static checker warning to mix up bitwise and logical operations.
> > > 
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > 
> > > diff --git a/arch/x86/crypto/sha256-mb/sha256_mb.c b/arch/x86/crypto/sha256-mb/sha256_mb.c
[]
> > > @@ -299,7 +299,7 @@ static struct sha256_hash_ctx *sha256_ctx_mgr_submit(struct sha256_ctx_mgr *mgr,
> > >  	 * Or if the user's buffer contains less than a whole block,
> > >  	 * append as much as possible to the extra block.
> > >  	 */
> > > -	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
> > >  		 */
> > > 
> > As far as I know the | was an intentional optimization, so you may way
> > to look at the generated code.
> I know how the rules work.  I just thought it looked more like a typo
> than an optimization.  It's normally a typo.  It's hard to tell the
> intent.

The compiler could potentially emit the same code when
optimizing but at least gcc 5.3 doesn't.

It's probably useful to add a comment for the specific intent
here rather than change a potentially useful static checker.

Re: [patch] crypto: sha256-mb - cleanup a || vs | typo

[walter harms]

Am 30.06.2016 13:16, schrieb Joe Perches:
> On Thu, 2016-06-30 at 10:50 +0300, Dan Carpenter wrote:
>> On Wed, Jun 29, 2016 at 10:05:53AM -0700, H. Peter Anvin wrote:
>>> On 06/29/16 07:42, Dan Carpenter wrote:
>>>>>> and | behave basically the same here but || is intended.  It causes a
>>>> static checker warning to mix up bitwise and logical operations.
>>>>
>>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>>
>>>> diff --git a/arch/x86/crypto/sha256-mb/sha256_mb.c b/arch/x86/crypto/sha256-mb/sha256_mb.c
> []
>>>> @@ -299,7 +299,7 @@ static struct sha256_hash_ctx *sha256_ctx_mgr_submit(struct sha256_ctx_mgr *mgr,
>>>>  	 * Or if the user's buffer contains less than a whole block,
>>>>  	 * append as much as possible to the extra block.
>>>>  	 */
>>>> -	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
>>>>  		 */
>>>>
>>> As far as I know the | was an intentional optimization, so you may way
>>> to look at the generated code.
>> I know how the rules work.  I just thought it looked more like a typo
>> than an optimization.  It's normally a typo.  It's hard to tell the
>> intent.
> 
> The compiler could potentially emit the same code when
> optimizing but at least gcc 5.3 doesn't.
> 
> It's probably useful to add a comment for the specific intent
> here rather than change a potentially useful static checker.
> 

perhaps we can agree not to play tricks with a compiler.
Everything may be true for a certain version of CC but the next compiler is different.

just my 2 cents,
 wh

Re: [patch] crypto: sha256-mb - cleanup a || vs | typo

[Dan Carpenter]

The difference between | and || is that || has ordering constraints.
It's from the C standard, and not the compiler version.

regards,
dan carpenter

Re: [patch] crypto: sha256-mb - cleanup a || vs | typo

[Tim Chen]

On Wed, 2016-06-29 at 10:05 -0700, H. Peter Anvin wrote:
> On 06/29/16 07:42, Dan Carpenter wrote:
> > 
> > > 
> > > > 
> > > > and | behave basically the same here but || is intended.  It causes a
> > static checker warning to mix up bitwise and logical operations.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > diff --git a/arch/x86/crypto/sha256-mb/sha256_mb.c b/arch/x86/crypto/sha256-mb/sha256_mb.c
> > index c9d5dcc..4ec895a 100644
> > --- a/arch/x86/crypto/sha256-mb/sha256_mb.c
> > +++ b/arch/x86/crypto/sha256-mb/sha256_mb.c
> > @@ -299,7 +299,7 @@ static struct sha256_hash_ctx *sha256_ctx_mgr_submit(struct sha256_ctx_mgr *mgr,
> >  	 * Or if the user's buffer contains less than a whole block,
> >  	 * append as much as possible to the extra block.
> >  	 */
> > -	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
> >  		 */
> > 
> As far as I know the | was an intentional optimization, so you may way
> to look at the generated code.
> 
> 	-hpa
> 

Yes, this is an intentional optimization.  Is there any scenario where things may
break with the compiler?

Tim

Re: [patch] crypto: sha256-mb - cleanup a || vs | typo

[Dan Carpenter]

On Thu, Jun 30, 2016 at 01:42:19PM -0700, Tim Chen wrote:
> On Wed, 2016-06-29 at 10:05 -0700, H. Peter Anvin wrote:
> > On 06/29/16 07:42, Dan Carpenter wrote:
> > > 
> > > > 
> > > > > 
> > > > > and | behave basically the same here but || is intended.  It causes a
> > > static checker warning to mix up bitwise and logical operations.
> > > 
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > 
> > > diff --git a/arch/x86/crypto/sha256-mb/sha256_mb.c b/arch/x86/crypto/sha256-mb/sha256_mb.c
> > > index c9d5dcc..4ec895a 100644
> > > --- a/arch/x86/crypto/sha256-mb/sha256_mb.c
> > > +++ b/arch/x86/crypto/sha256-mb/sha256_mb.c
> > > @@ -299,7 +299,7 @@ static struct sha256_hash_ctx *sha256_ctx_mgr_submit(struct sha256_ctx_mgr *mgr,
> > >  	 * Or if the user's buffer contains less than a whole block,
> > >  	 * append as much as possible to the extra block.
> > >  	 */
> > > -	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
> > >  		 */
> > > 
> > As far as I know the | was an intentional optimization, so you may way
> > to look at the generated code.
> > 
> > 	-hpa
> > 
> 
> Yes, this is an intentional optimization.  Is there any scenario where things may
> break with the compiler?

No.  I'm going to remove the warning from the static checker like I said
earlier.  It should only complain for && vs & typos, || vs | is
harmless.

regards,
dan carpenter

Re: [patch] crypto: sha256-mb - cleanup a || vs | typo

[Ingo Molnar]

* Tim Chen <tim.c.chen@linux.intel.com> wrote:

> On Wed, 2016-06-29 at 10:05 -0700, H. Peter Anvin wrote:
> > On 06/29/16 07:42, Dan Carpenter wrote:
> > > 
> > > > 
> > > > > 
> > > > > and | behave basically the same here but || is intended.  It causes a
> > > static checker warning to mix up bitwise and logical operations.
> > > 
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > 
> > > diff --git a/arch/x86/crypto/sha256-mb/sha256_mb.c b/arch/x86/crypto/sha256-mb/sha256_mb.c
> > > index c9d5dcc..4ec895a 100644
> > > --- a/arch/x86/crypto/sha256-mb/sha256_mb.c
> > > +++ b/arch/x86/crypto/sha256-mb/sha256_mb.c
> > > @@ -299,7 +299,7 @@ static struct sha256_hash_ctx *sha256_ctx_mgr_submit(struct sha256_ctx_mgr *mgr,
> > >  	 * Or if the user's buffer contains less than a whole block,
> > >  	 * append as much as possible to the extra block.
> > >  	 */
> > > -	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
> > >  		 */
> > > 
> > As far as I know the | was an intentional optimization, so you may way
> > to look at the generated code.
> > 
> > 	-hpa
> > 
> 
> Yes, this is an intentional optimization. [...]

Please don't do intentional optimizations while mixing them with a very ugly 
coding style:

	if ((ctx->partial_block_buffer_length) | (len < SHA256_BLOCK_SIZE)) {

The extra, unnecessary parantheses around ctx->partial_block_buffer_length will 
make the ordinary reader assume that the person who wrote the code was unsure 
about basic C syntax details and typoed the '|' as well ...

Also, for heaven's (and readability's) sake, pick shorter structure field names. 
What's wrong with ctx->partial_block_buf_len?

Also, even if the '|' was intentional - wouldn't it result in better code to use 
'||'?

Plus:

> > >  		/* Compute how many bytes to copy from user buffer into
> > >  		 * extra block
> > >  		 */

please use the customary (multi-line) comment style:

  /*
   * Comment .....
   * ...... goes here.
   */

specified in Documentation/CodingStyle.

Thanks,

        Ingo

Re: [patch] crypto: sha256-mb - cleanup a || vs | typo

[Herbert Xu]

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.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [patch] crypto: sha256-mb - cleanup a || vs | typo

[Ingo Molnar]

* Herbert Xu <herbert@gondor.apana.org.au> 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

Re: [patch] crypto: sha256-mb - cleanup a || vs | typo

[Tim Chen]

On Fri, Jul 01, 2016 at 12:13:30PM +0200, Ingo Molnar wrote:
> 
> * Herbert Xu <herbert@gondor.apana.org.au> 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)) {

Sorry I was on vacation and didn't get to respond earlier.
Let's switch the above from | to || so the code logic is
clearer.  Also clean up various multi-line comment style
inconsistencies in patch below.

Thanks.

Tim

---

From: Tim Chen <tim.c.chen@linux.intel.com>
Subject: [PATCH] crypto: Cleanup sha multi-buffer code to use || instead of |
 for condition comparison and cleanup multiline comment style

In sha*_ctx_mgr_submit, we currently use the | operator instead of ||
((ctx->partial_block_buffer_length) | (len < SHA1_BLOCK_SIZE))

Switching it to || and remove extraneous paranthesis to
adhere to coding style.

Also cleanup inconsistent multiline comment style.

Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
---
 arch/x86/crypto/sha1-mb/sha1_mb.c     |  2 +-
 arch/x86/crypto/sha256-mb/sha256_mb.c | 11 +++++++----
 arch/x86/crypto/sha512-mb/sha512_mb.c | 11 +++++++----
 3 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/arch/x86/crypto/sha1-mb/sha1_mb.c b/arch/x86/crypto/sha1-mb/sha1_mb.c
index 561b286..9e5b671 100644
--- a/arch/x86/crypto/sha1-mb/sha1_mb.c
+++ 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,
 	 * Or if the user's buffer contains less than a whole block,
 	 * append as much as possible to the extra block.
 	 */
-	if ((ctx->partial_block_buffer_length) | (len < SHA1_BLOCK_SIZE)) {
+	if (ctx->partial_block_buffer_length || len < SHA1_BLOCK_SIZE) {
 		/*
 		 * Compute how many bytes to copy from user buffer into
 		 * extra block
diff --git a/arch/x86/crypto/sha256-mb/sha256_mb.c b/arch/x86/crypto/sha256-mb/sha256_mb.c
index c9d5dcc..89fa85e 100644
--- a/arch/x86/crypto/sha256-mb/sha256_mb.c
+++ b/arch/x86/crypto/sha256-mb/sha256_mb.c
@@ -283,7 +283,8 @@ static struct sha256_hash_ctx *sha256_ctx_mgr_submit(struct sha256_ctx_mgr *mgr,
 	ctx->incoming_buffer = buffer;
 	ctx->incoming_buffer_length = len;
 
-	/* Store the user's request flags and mark this ctx as currently
+	/*
+	 * Store the user's request flags and mark this ctx as currently
 	 * being processed.
 	 */
 	ctx->status = (flags & HASH_LAST) ?
@@ -299,8 +300,9 @@ static struct sha256_hash_ctx *sha256_ctx_mgr_submit(struct sha256_ctx_mgr *mgr,
 	 * Or if the user's buffer contains less than a whole block,
 	 * append as much as possible to the extra block.
 	 */
-	if ((ctx->partial_block_buffer_length) | (len < SHA256_BLOCK_SIZE)) {
-		/* Compute how many bytes to copy from user buffer into
+	if (ctx->partial_block_buffer_length || len < SHA256_BLOCK_SIZE) {
+		/*
+		 * Compute how many bytes to copy from user buffer into
 		 * extra block
 		 */
 		uint32_t copy_len = SHA256_BLOCK_SIZE -
@@ -323,7 +325,8 @@ static struct sha256_hash_ctx *sha256_ctx_mgr_submit(struct sha256_ctx_mgr *mgr,
 		/* The extra block should never contain more than 1 block */
 		assert(ctx->partial_block_buffer_length <= SHA256_BLOCK_SIZE);
 
-		/* If the extra block buffer contains exactly 1 block,
+		/*
+		 * If the extra block buffer contains exactly 1 block,
 		 * it can be hashed.
 		 */
 		if (ctx->partial_block_buffer_length >= SHA256_BLOCK_SIZE) {
diff --git a/arch/x86/crypto/sha512-mb/sha512_mb.c b/arch/x86/crypto/sha512-mb/sha512_mb.c
index 676f0f2..f4cf5b7 100644
--- a/arch/x86/crypto/sha512-mb/sha512_mb.c
+++ b/arch/x86/crypto/sha512-mb/sha512_mb.c
@@ -253,7 +253,8 @@ static struct sha512_hash_ctx
 					  int flags)
 {
 	if (flags & (~HASH_ENTIRE)) {
-		/* User should not pass anything other than FIRST, UPDATE, or
+		/*
+		 * User should not pass anything other than FIRST, UPDATE, or
 		 * LAST
 		 */
 		ctx->error = HASH_CTX_ERROR_INVALID_FLAGS;
@@ -284,7 +285,8 @@ static struct sha512_hash_ctx
 		ctx->partial_block_buffer_length = 0;
 	}
 
-	/* If we made it here, there were no errors during this call to
+	/*
+	 * If we made it here, there were no errors during this call to
 	 * submit
 	 */
 	ctx->error = HASH_CTX_ERROR_NONE;
@@ -293,7 +295,8 @@ static struct sha512_hash_ctx
 	ctx->incoming_buffer = buffer;
 	ctx->incoming_buffer_length = len;
 
-	/* Store the user's request flags and mark this ctx as currently being
+	/*
+	 * Store the user's request flags and mark this ctx as currently being
 	 * processed.
 	 */
 	ctx->status = (flags & HASH_LAST) ?
@@ -309,7 +312,7 @@ static struct sha512_hash_ctx
 	 * Or if the user's buffer contains less than a whole block,
 	 * append as much as possible to the extra block.
 	 */
-	if ((ctx->partial_block_buffer_length) | (len < SHA512_BLOCK_SIZE)) {
+	if (ctx->partial_block_buffer_length || len < SHA512_BLOCK_SIZE) {
 		/* Compute how many bytes to copy from user buffer into extra
 		 * block
 		 */
-- 
2.5.5

Re: [patch] crypto: sha256-mb - cleanup a || vs | typo

[Herbert Xu]

On Fri, Jul 08, 2016 at 09:28:03AM -0700, Tim Chen wrote:
>
> Sorry I was on vacation and didn't get to respond earlier.
> Let's switch the above from | to || so the code logic is
> clearer.  Also clean up various multi-line comment style
> inconsistencies in patch below.

Nack.  As I said the commenting style in the crypto API is the
same as the network stack.  So unless we decide to change both
please stick to the current style.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [patch] crypto: sha256-mb - cleanup a || vs | typo

[Tim Chen]

On Sat, 2016-07-09 at 00:45 +0800, Herbert Xu wrote:
> On Fri, Jul 08, 2016 at 09:28:03AM -0700, Tim Chen wrote:
> > 
> > 
> > Sorry I was on vacation and didn't get to respond earlier.
> > Let's switch the above from | to || so the code logic is
> > clearer.  Also clean up various multi-line comment style
> > inconsistencies in patch below.
> Nack.  As I said the commenting style in the crypto API is the
> same as the network stack.  So unless we decide to change both
> please stick to the current style.
> 

Will you like a patch with just the | to || change, or leave
the code as is?

Tim

Re: [patch] crypto: sha256-mb - cleanup a || vs | typo

[Linus Torvalds]

[ rare comment rant. I think I'll do this once, and then ignore the discussion ]

On Fri, Jul 8, 2016 at 9:45 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> Nack.  As I said the commenting style in the crypto API is the
> same as the network stack.  So unless we decide to change both
> please stick to the current style.

Can we please get rid of the brain-damaged stupid networking comment
syntax style, PLEASE?

If the networking people cannot handle the pure awesomeness that is a
balanced and symmetric traditional multi-line C style comments, then
instead of the disgusting unbalanced crap that you guys use now,
please just go all the way to the C++ mode.

In other words, these three models are good:

 (a)
      /* This is a comment *./

 (b)
      /*
       * This is also a comment, but it can now be cleanly
       * split over multiple lines
       */

 (c)
      // This can be a single line. Or many. Your choice.

and they are all obviously visually balanced. Sometimes you want (b)
even for a single line, if you want the white-space to make it stand
out more, but you can obviously do that with (c) too, by just
surrounding it with two empty (comment) lines.

The (c) form is particularly good for things like enum or structure
member comments at the end of code, where you might want to align
things up, but the ending comment marker ends up being visually pretty
distracting (and lining _that_ up is too much make-believe work).

There's also another acceptablr traditional multi-line style that
you'll find in some places, but it's not the common kernel style:

 (d)
      /* This is an alternate multi-line format
         that isn't horrible, but not kernel style */

Note how all the above comment styles have a certain visual symmatry
and balance.

But no, the networking code picked *none* of the above sane formats.
Instead, it picked these two models that are just half-arsed
shit-for-brains:

 (no)
     /* This is disgusting drug-induced
       * crap, and should die
       */

  (no-no-no)
      /* This is also very nasty
       * and visually unbalanced */

Please. The networking code actually has the *worst* possible comment
style. You can literally find that (no-no-no) style, which is just
really horribly disgusting and worse than the otherwise fairly similar
(d) in pretty much every way.

I'm not even going to start talking about the people who prefer to
"box in" their comments, and line up both ends and have fancy boxes of
stars around the whole thing. I'm sure that looks really nice if you
are out of your mind on LSD, and have nothing better to do than to
worry about the right alignment of the asterisks.

I'd be happy to start moving the whole kernel over to the C++ style,
it's been many many years since we had compatibility issues and we are
all used to it by now, even if we weren't all fans originally.

I really don't understand why the networking people think that their
particularly ugly styles are fine. They are the most visually
unbalanced version of _all_ the common comment styles, and have no
actual advantages.

So just get rid of the (no-no) and (no-no-no) forms. Not in one big
go, but as people touch the code, just fix that mess up.

                 Linus

Re: [patch] crypto: sha256-mb - cleanup a || vs | typo

[Geert Uytterhoeven]

Hi Linus,

On Fri, Jul 8, 2016 at 7:19 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>  (c)
>       // This can be a single line. Or many. Your choice.

> The (c) form is particularly good for things like enum or structure
> member comments at the end of code, where you might want to align
> things up, but the ending comment marker ends up being visually pretty
> distracting (and lining _that_ up is too much make-believe work).

While I'm a fan of the (c) form myself, I became used to not using it for
kernel code. Except for internal comments that are not intended to be sent
out. This works fine, as checkpatch will complain if I ever forget to remove
them while preparing patches.

The alternative would be to teach checkpatch to complain about FIXME, TODO,
and XXX in comments...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [patch] crypto: sha256-mb - cleanup a || vs | typo

[Pavel Machek]

On Fri 2016-07-08 10:19:26, Linus Torvalds wrote:
> [ rare comment rant. I think I'll do this once, and then ignore the discussion ]
> 
> On Fri, Jul 8, 2016 at 9:45 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> >
> > Nack.  As I said the commenting style in the crypto API is the
> > same as the network stack.  So unless we decide to change both
> > please stick to the current style.
> 
> Can we please get rid of the brain-damaged stupid networking comment
> syntax style, PLEASE?

Please? :-). Having different comment styles between networking and
the rest is confusing.

And yes, this uglyness is documented for net/ and drivers/net/, but
not for crypto/, so at the very least Documentation/CodingStyle should
be updated.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [patch] crypto: sha256-mb - cleanup a || vs | typo

[Tim Chen]

On Mon, 2016-07-18 at 10:59 +0200, Pavel Machek wrote:
> On Fri 2016-07-08 10:19:26, Linus Torvalds wrote:
> > 
> > [ rare comment rant. I think I'll do this once, and then ignore the discussion ]
> > 
> > On Fri, Jul 8, 2016 at 9:45 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > > 
> > > 
> > > Nack.  As I said the commenting style in the crypto API is the
> > > same as the network stack.  So unless we decide to change both
> > > please stick to the current style.
> > Can we please get rid of the brain-damaged stupid networking comment
> > syntax style, PLEASE?
> Please? :-). Having different comment styles between networking and
> the rest is confusing.
> 
> And yes, this uglyness is documented for net/ and drivers/net/, but
> not for crypto/, so at the very least Documentation/CodingStyle should
> be updated.
> 
> 									Pavel

The cleanup patch has already been merged.  Thanks.

Tim

Re: [patch] crypto: sha256-mb - cleanup a || vs | typo

[Herbert Xu]

On Fri, Jul 08, 2016 at 09:28:03AM -0700, Tim Chen wrote:
> 
> From: Tim Chen <tim.c.chen@linux.intel.com>
> Subject: [PATCH] crypto: Cleanup sha multi-buffer code to use || instead of |
>  for condition comparison and cleanup multiline comment style
> 
> In sha*_ctx_mgr_submit, we currently use the | operator instead of ||
> ((ctx->partial_block_buffer_length) | (len < SHA1_BLOCK_SIZE))
> 
> Switching it to || and remove extraneous paranthesis to
> adhere to coding style.
> 
> Also cleanup inconsistent multiline comment style.
> 
> Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>

Patch applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt