crypto: sha256-mb - cleanup a || vs | typo

PATCH In review Dan Carpenter 10 participants 19 messages Jun 29, 2016 – Jul 18, 2016

Subsystems (3)

x86 Kernel janitors Crypto API

lore

Share & tools

lore

Dan Carpenter

author

2016-06-29 14:42 UTC #0 lore raw

PATCH 1/1 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 <[email protected]>

arch/x86/crypto/sha256-mb/sha256_mb.c +1 -1

index c9d5dcc..4ec895a 100644
@@ -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
 		 */

H. Peter Anvin

reviewer

2016-06-29 17:05 UTC #1 lore raw

Quoted 11 lines · 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 <[email protected]>
arch/x86/crypto/sha256-mb/sha256_mb.c +0 -0
index c9d5dcc..4ec895a 100644

arch/x86/crypto/sha256-mb/sha256_mb.c static struct sha256_hash_ctx *sha256_ctx_mgr_submit(struct sha256_ctx_mgr *mgr, · line 305

Quoted 8 lines

arch/x86/crypto/sha256-mb/sha256_mb.c +1 -1

@@ -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

Dan Carpenter

author

2016-06-30 07:50 UTC #2 lore raw

Quoted 15 lines · H. Peter Anvin

Quoted 14 lines · 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 <[email protected]>
arch/x86/crypto/sha256-mb/sha256_mb.c +0 -1
index c9d5dcc..4ec895a 100644
@@ -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)) {

Quoted 8 lines

arch/x86/crypto/sha256-mb/sha256_mb.c static struct sha256_hash_ctx *sha256_ctx_mgr_submit(struct sha256_ctx_mgr *mgr, · line 305
Quoted 5 lines
arch/x86/crypto/sha256-mb/sha256_mb.c +1 -0
+	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

Joe Perches

2016-06-30 11:16 UTC #3 lore raw

Quoted 8 lines · Dan Carpenter

Quoted 7 lines · H. Peter Anvin
arch/x86/crypto/sha256-mb/sha256_mb.c
Quoted 6 lines · Dan Carpenter
Quoted 1 line
Quoted 1 line
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 <[email protected]>
arch/x86/crypto/sha256-mb/sha256_mb.c +0 -0

[]

Quoted 12 lines

Quoted 4 lines
Quoted 4 lines
arch/x86/crypto/sha256-mb/sha256_mb.c +0 -0
@@ -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.
  	 */
Quoted 8 lines
arch/x86/crypto/sha256-mb/sha256_mb.c static struct sha256_hash_ctx *sha256_ctx_mgr_submit(struct sha256_ctx_mgr *mgr, · line 305
Quoted 6 lines
arch/x86/crypto/sha256-mb/sha256_mb.c +1 -1
-	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.

Quoted 3 lines

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.

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

walter harms

2016-06-30 11:45 UTC #4 lore raw

Am 30.06.2016 13:16, schrieb Joe Perches:

Quoted 26 lines · Dan Carpenter

Quoted 8 lines · H. Peter Anvin
Quoted 7 lines · Dan Carpenter
arch/x86/crypto/sha256-mb/sha256_mb.c
Quoted 6 lines
Quoted 1 line
Quoted 1 line
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 <[email protected]>
arch/x86/crypto/sha256-mb/sha256_mb.c +0 -0
[]
Quoted 12 lines
Quoted 4 lines
Quoted 4 lines
arch/x86/crypto/sha256-mb/sha256_mb.c +0 -0
@@ -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.
 	 */
Quoted 8 lines
arch/x86/crypto/sha256-mb/sha256_mb.c static struct sha256_hash_ctx *sha256_ctx_mgr_submit(struct sha256_ctx_mgr *mgr, · line 305
Quoted 6 lines
arch/x86/crypto/sha256-mb/sha256_mb.c +1 -1
-	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.
Quoted 3 lines
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.

Quoted 6 lines

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

Dan Carpenter

author

2016-06-30 12:33 UTC #5 lore raw

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

regards,
dan carpenter

Tim Chen

2016-06-30 20:42 UTC #6 lore raw

In reply to H. Peter Anvin

Quoted 26 lines · H. Peter Anvin

Quoted 14 lines · Dan Carpenter
Quoted 4 lines
Quoted 4 lines
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 <[email protected]>
arch/x86/crypto/sha256-mb/sha256_mb.c +0 -0
index c9d5dcc..4ec895a 100644
arch/x86/crypto/sha256-mb/sha256_mb.c static struct sha256_hash_ctx *sha256_ctx_mgr_submit(struct sha256_ctx_mgr *mgr, · line 305
Quoted 8 lines
arch/x86/crypto/sha256-mb/sha256_mb.c +1 -1
@@ -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.

Quoted 2 lines

-hpa

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

Tim

Dan Carpenter

author

2016-06-30 22:16 UTC #7 lore raw

Quoted 27 lines · Tim Chen

Quoted 26 lines · H. Peter Anvin
Quoted 14 lines · Dan Carpenter
Quoted 4 lines
Quoted 4 lines
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 <[email protected]>
arch/x86/crypto/sha256-mb/sha256_mb.c +0 -0
index c9d5dcc..4ec895a 100644
arch/x86/crypto/sha256-mb/sha256_mb.c static struct sha256_hash_ctx *sha256_ctx_mgr_submit(struct sha256_ctx_mgr *mgr, · line 305
Quoted 8 lines
arch/x86/crypto/sha256-mb/sha256_mb.c +1 -1
@@ -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.

Quoted 5 lines

Quoted 2 lines
-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

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Ingo Molnar

2016-07-01 07:55 UTC #8 lore raw

In reply to Tim Chen

Quoted 27 lines · Tim Chen

Quoted 26 lines · H. Peter Anvin
Quoted 14 lines · Dan Carpenter
Quoted 4 lines
Quoted 4 lines
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 <[email protected]>
arch/x86/crypto/sha256-mb/sha256_mb.c +0 -0
index c9d5dcc..4ec895a 100644
arch/x86/crypto/sha256-mb/sha256_mb.c static struct sha256_hash_ctx *sha256_ctx_mgr_submit(struct sha256_ctx_mgr *mgr, · line 305
Quoted 8 lines
arch/x86/crypto/sha256-mb/sha256_mb.c +1 -1
@@ -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.

Quoted 4 lines

Quoted 2 lines
-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:

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
/* 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
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Herbert Xu

maintainer

2016-07-01 09:28 UTC #9 lore raw

Quoted 8 lines · Ingo Molnar

Plus:
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
/* 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,

Signature · 7 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ingo Molnar

2016-07-01 10:13 UTC #10 lore raw

Quoted 4 lines · Herbert Xu

Quoted 3 lines · Ingo Molnar
Plus:

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
/* 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
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Tim Chen

2016-07-08 16:28 UTC #11 lore raw

Quoted 30 lines · Ingo Molnar

Quoted 4 lines · Herbert Xu
Quoted 3 lines · Ingo Molnar
Plus:
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
/* 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
(quoted diff) +0 -0
@@ -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:

Review

Quoted 2 lines

(quoted diff) +1 -1

-       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 <[email protected]>
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 <[email protected]>

---
 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(-)

arch/x86/crypto/sha1-mb/sha1_mb.c +1 -1

index 561b286..9e5b671 100644
@@ -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

arch/x86/crypto/sha256-mb/sha256_mb.c +7 -4

index c9d5dcc..89fa85e 100644
@@ -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) {

arch/x86/crypto/sha512-mb/sha512_mb.c +7 -4

index 676f0f2..f4cf5b7 100644
@@ -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
 		 */

Signature · 6 lines

2.5.5

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Herbert Xu

maintainer

2016-07-08 16:45 UTC #12 lore raw

Quoted 5 lines · Tim Chen

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,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Tim Chen

2016-07-08 17:17 UTC #13 lore raw

Quoted 11 lines · Herbert Xu

Quoted 6 lines · Tim Chen
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

Linus Torvalds

2016-07-08 17:19 UTC #14 lore raw

In reply to Herbert Xu

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

Quoted 4 lines · Herbert Xu

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

Geert Uytterhoeven

2016-07-11 06:40 UTC #15 lore raw

Hi Linus,

Quoted 7 lines · Linus Torvalds

(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 -- [email protected]

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

Pavel Machek

2016-07-18 08:59 UTC #16 lore raw

In reply to Linus Torvalds

Quoted 10 lines · Linus Torvalds

[ rare comment rant. I think I'll do this once, and then ignore the discussion ]
Quoted 4 lines · Herbert Xu
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

Signature · 2 lines

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

Tim Chen

2016-07-18 22:12 UTC #17 lore raw

Quoted 12 lines · Pavel Machek

Quoted 11 lines · Linus Torvalds
[ rare comment rant. I think I'll do this once, and then ignore the discussion ]
Quoted 5 lines · Herbert Xu
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?

Quoted 8 lines

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

Herbert Xu

maintainer

2016-07-11 10:09 UTC #18 lore raw

In reply to Tim Chen

Quoted 8 lines · Tim Chen

From: Tim Chen <[email protected]>
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))

Quoted 6 lines

Switching it to || and remove extraneous paranthesis to adhere to coding style.
Also cleanup inconsistent multiline comment style.
Signed-off-by Tim Chen <[email protected]>

Patch applied. Thanks.

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt