Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1433813 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [patch] crypto: sha256-mb - cleanup a || vs | typo Dan Carpenter <dan.carpenter@oracle.com> - 2016-06-29 16:50 +0200
    Re: [patch] crypto: sha256-mb - cleanup a || vs | typo "H. Peter Anvin" <hpa@zytor.com> - 2016-06-29 19:10 +0200
      Re: [patch] crypto: sha256-mb - cleanup a || vs | typo Dan Carpenter <dan.carpenter@oracle.com> - 2016-06-30 10:00 +0200
        Re: [patch] crypto: sha256-mb - cleanup a || vs | typo Joe Perches <joe@perches.com> - 2016-06-30 13:20 +0200
          Re: [patch] crypto: sha256-mb - cleanup a || vs | typo walter harms <wharms@bfs.de> - 2016-06-30 13:50 +0200
            Re: [patch] crypto: sha256-mb - cleanup a || vs | typo Dan Carpenter <dan.carpenter@oracle.com> - 2016-06-30 14:40 +0200
      Re: [patch] crypto: sha256-mb - cleanup a || vs | typo Tim Chen <tim.c.chen@linux.intel.com> - 2016-06-30 22:50 +0200
        Re: [patch] crypto: sha256-mb - cleanup a || vs | typo Dan Carpenter <dan.carpenter@oracle.com> - 2016-07-01 00:20 +0200
        Re: [patch] crypto: sha256-mb - cleanup a || vs | typo Ingo Molnar <mingo@kernel.org> - 2016-07-01 10:00 +0200
          Re: [patch] crypto: sha256-mb - cleanup a || vs | typo Herbert Xu <herbert@gondor.apana.org.au> - 2016-07-01 11:30 +0200
            Re: [patch] crypto: sha256-mb - cleanup a || vs | typo Ingo Molnar <mingo@kernel.org> - 2016-07-01 13:20 +0200
              Re: [patch] crypto: sha256-mb - cleanup a || vs | typo Tim Chen <tim.c.chen@linux.intel.com> - 2016-07-08 18:30 +0200
                Re: [patch] crypto: sha256-mb - cleanup a || vs | typo Herbert Xu <herbert@gondor.apana.org.au> - 2016-07-08 18:50 +0200
                  Re: [patch] crypto: sha256-mb - cleanup a || vs | typo Linus Torvalds <torvalds@linux-foundation.org> - 2016-07-08 19:20 +0200
                  Re: [patch] crypto: sha256-mb - cleanup a || vs | typo Tim Chen <tim.c.chen@linux.intel.com> - 2016-07-08 19:20 +0200

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

|| 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
 		 */

[toc] | [next] | [standalone]

#1433918

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

[toc] | [prev] | [next] | [standalone]

#1434317

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

[toc] | [prev] | [next] | [standalone]

#1434470

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.

[toc] | [prev] | [next] | [standalone]

#1434481

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

[toc] | [prev] | [next] | [standalone]

#1434513

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

regards,
dan carpenter

[toc] | [prev] | [next] | [standalone]

#1434838

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

[toc] | [prev] | [next] | [standalone]

#1434877

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

[toc] | [prev] | [next] | [standalone]

#1435095

* 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

[toc] | [prev] | [next] | [standalone]

#1435206

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

[toc] | [prev] | [next] | [standalone]

#1435261

* 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

[toc] | [prev] | [next] | [standalone]

#1439630

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

[toc] | [prev] | [next] | [standalone]

#1439675

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

[toc] | [prev] | [next] | [standalone]

#1439698

[ 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

[toc] | [prev] | [next] | [standalone]

#1439699

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web