[PATCH 00/10] md/dm-crypt: Fine-tuning for five function implementations

[PATCH 00/10] md/dm-crypt: Fine-tuning for five function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 28 Sep 2016 17:25:17 +0200

Some update suggestions were taken into account
from static source code analysis.

Markus Elfring (10):
  Use kcalloc() in crypt_alloc_tfms()
  Reduce the scope for a variable in crypt_alloc_tfms()
  Rename a jump label in crypt_message()
  Delete an unnecessary variable initialisation in crypt_message()
  Rename a jump label in crypt_set_key()
  Delete an unnecessary variable initialisation in crypt_set_key()
  Rename a jump label in crypt_iv_tcw_whitening()
  Return directly after a failed crypto_alloc_ahash() in crypt_iv_essiv_ctr()
  Two checks and one function call less in crypt_iv_essiv_ctr() after error detection
  Delete unnecessary variable initialisations in crypt_iv_essiv_ctr()

 drivers/md/dm-crypt.c | 51 ++++++++++++++++++++++-----------------------------
 1 file changed, 22 insertions(+), 29 deletions(-)

-- 
2.10.0

[PATCH 01/10] md/dm-crypt: Use kcalloc() in crypt_alloc_tfms()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 28 Sep 2016 13:26:22 +0200

* A multiplication for the size determination of a memory allocation
  indicated that an array data structure should be processed.
  Thus reuse the corresponding function "kcalloc".

  This issue was detected by using the Coccinelle software.

* Replace the specification of a data type by a pointer dereference
  to make the corresponding size determination a bit safer according to
  the Linux coding style convention.

Fixes: 5d0be84ec0cacfc7a6d6ea548afdd07d481324cd ("dm crypt: fix free of bad values after tfm allocation failure")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/md/dm-crypt.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index a276883..68971a2 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -1454,8 +1454,7 @@ static int crypt_alloc_tfms(struct crypt_config *cc, char *ciphermode)
 	unsigned i;
 	int err;
 
-	cc->tfms = kzalloc(cc->tfms_count * sizeof(struct crypto_skcipher *),
-			   GFP_KERNEL);
+	cc->tfms = kcalloc(cc->tfms_count, sizeof(*cc->tfms), GFP_KERNEL);
 	if (!cc->tfms)
 		return -ENOMEM;
 
-- 
2.10.0

Re: [PATCH 00/10] md/dm-crypt: Fine-tuning for five function implementations

[Mike Snitzer]

On Wed, Sep 28 2016 at 11:34am -0400,
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 28 Sep 2016 17:25:17 +0200
> 
> Some update suggestions were taken into account
> from static source code analysis.
> 
> Markus Elfring (10):
>   Use kcalloc() in crypt_alloc_tfms()
>   Reduce the scope for a variable in crypt_alloc_tfms()
>   Rename a jump label in crypt_message()
>   Delete an unnecessary variable initialisation in crypt_message()
>   Rename a jump label in crypt_set_key()
>   Delete an unnecessary variable initialisation in crypt_set_key()
>   Rename a jump label in crypt_iv_tcw_whitening()
>   Return directly after a failed crypto_alloc_ahash() in crypt_iv_essiv_ctr()
>   Two checks and one function call less in crypt_iv_essiv_ctr() after error detection
>   Delete unnecessary variable initialisations in crypt_iv_essiv_ctr()
> 
>  drivers/md/dm-crypt.c | 51 ++++++++++++++++++++++-----------------------------
>  1 file changed, 22 insertions(+), 29 deletions(-)

These are _not_ the kind of changes I want to be seeing.  Churn in the
name of "fine-tuning".

Nack.

[PATCH 02/10] md/dm-crypt: Reduce the scope for a variable in crypt_alloc_tfms()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 28 Sep 2016 14:05:25 +0200

Move the definition for the variable "err" (including its declaration)
into an if branch (so that the corresponding setting will only be performed
if a call of the function "crypto_alloc_skcipher" failed as before
this refactoring).

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/md/dm-crypt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 68971a2..88a3b62 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -1452,7 +1452,6 @@ static void crypt_free_tfms(struct crypt_config *cc)
 static int crypt_alloc_tfms(struct crypt_config *cc, char *ciphermode)
 {
 	unsigned i;
-	int err;
 
 	cc->tfms = kcalloc(cc->tfms_count, sizeof(*cc->tfms), GFP_KERNEL);
 	if (!cc->tfms)
@@ -1461,7 +1460,8 @@ static int crypt_alloc_tfms(struct crypt_config *cc, char *ciphermode)
 	for (i = 0; i < cc->tfms_count; i++) {
 		cc->tfms[i] = crypto_alloc_skcipher(ciphermode, 0, 0);
 		if (IS_ERR(cc->tfms[i])) {
-			err = PTR_ERR(cc->tfms[i]);
+			int err = PTR_ERR(cc->tfms[i]);
+
 			crypt_free_tfms(cc);
 			return err;
 		}
-- 
2.10.0

[PATCH 03/10] md/dm-crypt: Rename a jump label in crypt_message()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 28 Sep 2016 14:54:39 +0200

Adjust a jump label according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/md/dm-crypt.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 88a3b62..08e3de2 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -2016,7 +2016,7 @@ static int crypt_message(struct dm_target *ti, unsigned argc, char **argv)
 	int ret = -EINVAL;
 
 	if (argc < 2)
-		goto error;
+		goto show_warning;
 
 	if (!strcasecmp(argv[0], "key")) {
 		if (!test_bit(DM_CRYPT_SUSPENDED, &cc->flags)) {
@@ -2040,8 +2040,7 @@ static int crypt_message(struct dm_target *ti, unsigned argc, char **argv)
 			return crypt_wipe_key(cc);
 		}
 	}
-
-error:
+show_warning:
 	DMWARN("unrecognised message received.");
 	return -EINVAL;
 }
-- 
2.10.0

[PATCH 04/10] md/dm-crypt: Delete an unnecessary variable initialisation in crypt_message()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 28 Sep 2016 15:06:05 +0200

The local variable "ret" will be set to an appropriate value in if branches.
Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/md/dm-crypt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 08e3de2..7778e9b 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -2013,7 +2013,7 @@ static void crypt_resume(struct dm_target *ti)
 static int crypt_message(struct dm_target *ti, unsigned argc, char **argv)
 {
 	struct crypt_config *cc = ti->private;
-	int ret = -EINVAL;
+	int ret;
 
 	if (argc < 2)
 		goto show_warning;
-- 
2.10.0

[PATCH 05/10] md/dm-crypt: Rename a jump label in crypt_set_key()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 28 Sep 2016 15:21:18 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/md/dm-crypt.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 7778e9b..7e0fd82 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -1496,20 +1496,19 @@ static int crypt_set_key(struct crypt_config *cc, char *key)
 
 	/* The key size may not be changed. */
 	if (cc->key_size != (key_string_len >> 1))
-		goto out;
+		goto set_memory;
 
 	/* Hyphen (which gives a key_size of zero) means there is no key. */
 	if (!cc->key_size && strcmp(key, "-"))
-		goto out;
+		goto set_memory;
 
 	if (cc->key_size && crypt_decode_key(cc->key, key, cc->key_size) < 0)
-		goto out;
+		goto set_memory;
 
 	set_bit(DM_CRYPT_KEY_VALID, &cc->flags);
 
 	r = crypt_setkey_allcpus(cc);
-
-out:
+set_memory:
 	/* Hex key string not needed after here, so wipe it. */
 	memset(key, '0', key_string_len);
 
-- 
2.10.0

[PATCH 06/10] md/dm-crypt: Delete an unnecessary variable initialisation in crypt_set_key()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 28 Sep 2016 15:24:13 +0200

The local variable "r" will be set to an appropriate value a bit later.
Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/md/dm-crypt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 7e0fd82..3bc54c1 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -1491,7 +1491,7 @@ static int crypt_setkey_allcpus(struct crypt_config *cc)
 
 static int crypt_set_key(struct crypt_config *cc, char *key)
 {
-	int r = -EINVAL;
+	int r;
 	int key_string_len = strlen(key);
 
 	/* The key size may not be changed. */
-- 
2.10.0

[PATCH 07/10] md/dm-crypt: Rename a jump label in crypt_iv_tcw_whitening()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 28 Sep 2016 15:32:15 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/md/dm-crypt.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 3bc54c1..c457b5e 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -698,13 +698,13 @@ static int crypt_iv_tcw_whitening(struct crypt_config *cc,
 	for (i = 0; i < 4; i++) {
 		r = crypto_shash_init(desc);
 		if (r)
-			goto out;
+			goto zero_memory;
 		r = crypto_shash_update(desc, &buf[i * 4], 4);
 		if (r)
-			goto out;
+			goto zero_memory;
 		r = crypto_shash_final(desc, &buf[i * 4]);
 		if (r)
-			goto out;
+			goto zero_memory;
 	}
 	crypto_xor(&buf[0], &buf[12], 4);
 	crypto_xor(&buf[4], &buf[8], 4);
@@ -712,7 +712,7 @@ static int crypt_iv_tcw_whitening(struct crypt_config *cc,
 	/* apply whitening (8 bytes) to whole sector */
 	for (i = 0; i < ((1 << SECTOR_SHIFT) / 8); i++)
 		crypto_xor(data + i * 8, buf, 8);
-out:
+zero_memory:
 	memzero_explicit(buf, sizeof(buf));
 	return r;
 }
-- 
2.10.0

[PATCH 08/10] md/dm-crypt: Return directly after a failed crypto_alloc_ahash() in crypt_iv_essiv_ctr()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 28 Sep 2016 15:55:47 +0200

Return directly after a call of the function "crypto_alloc_ahash"
failed here.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/md/dm-crypt.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index c457b5e..47f6265 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -375,8 +375,7 @@ static int crypt_iv_essiv_ctr(struct crypt_config *cc, struct dm_target *ti,
 	hash_tfm = crypto_alloc_ahash(opts, 0, CRYPTO_ALG_ASYNC);
 	if (IS_ERR(hash_tfm)) {
 		ti->error = "Error initializing ESSIV hash";
-		err = PTR_ERR(hash_tfm);
-		goto bad;
+		return PTR_ERR(hash_tfm);
 	}
 
 	salt = kzalloc(crypto_ahash_digestsize(hash_tfm), GFP_KERNEL);
-- 
2.10.0

[PATCH 09/10] md/dm-crypt: Two checks and one function call less in crypt_iv_essiv_ctr() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 28 Sep 2016 16:38:37 +0200

The kfree() function was called in one case by the crypt_iv_essiv_ctr()
function during error handling even if the passed variable "salt"
contained a null pointer.

* Adjust a jump target according to the Linux coding style convention.

* Delete this function call and a condition check which became unnecessary
  with this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/md/dm-crypt.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 47f6265..53a9155 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -382,7 +382,7 @@ static int crypt_iv_essiv_ctr(struct crypt_config *cc, struct dm_target *ti,
 	if (!salt) {
 		ti->error = "Error kmallocing salt storage in ESSIV";
 		err = -ENOMEM;
-		goto bad;
+		goto free_hash;
 	}
 
 	cc->iv_gen_private.essiv.salt = salt;
@@ -397,11 +397,8 @@ static int crypt_iv_essiv_ctr(struct crypt_config *cc, struct dm_target *ti,
 	cc->iv_private = essiv_tfm;
 
 	return 0;
-
-bad:
-	if (hash_tfm && !IS_ERR(hash_tfm))
-		crypto_free_ahash(hash_tfm);
-	kfree(salt);
+free_hash:
+	crypto_free_ahash(hash_tfm);
 	return err;
 }
 
-- 
2.10.0

[PATCH 10/10] md/dm-crypt: Delete unnecessary variable initialisations in crypt_iv_essiv_ctr()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 28 Sep 2016 16:44:32 +0200

Three local variables will be set to an appropriate pointer a bit later.
Thus omit the explicit initialisation which became unnecessary with
a previous update step.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/md/dm-crypt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 53a9155..d27716e 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -361,9 +361,9 @@ static void crypt_iv_essiv_dtr(struct crypt_config *cc)
 static int crypt_iv_essiv_ctr(struct crypt_config *cc, struct dm_target *ti,
 			      const char *opts)
 {
-	struct crypto_cipher *essiv_tfm = NULL;
-	struct crypto_ahash *hash_tfm = NULL;
-	u8 *salt = NULL;
+	struct crypto_cipher *essiv_tfm;
+	struct crypto_ahash *hash_tfm;
+	u8 *salt;
 	int err;
 
 	if (!opts) {
-- 
2.10.0

[PATCH 00/10] dm snapshot: Fine-tuning for several function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 29 Sep 2016 10:35:43 +0200

Some update suggestions were taken into account
from static source code analysis.

Markus Elfring (10):
  Use kmalloc_array() in init_origin_hash()
  Delete two error messages for a failed memory allocation
  Delete an unnecessary variable initialisation in snapshot_map()
  Rename a jump label in pending_complete()
  Delete unnecessary variable initialisations in pending_complete()
  Delete an unnecessary variable initialisation in snapshot_ctr()
  Delete an unnecessary variable initialisation in merge_callback()
  Delete an unnecessary variable initialisation in remove_single_exception_chunk()
  Combine substrings for seven error messages
  Delete five unwanted spaces behind "list_for_each_entry"

 drivers/md/dm-snap.c | 69 +++++++++++++++++++++++-----------------------------
 1 file changed, 30 insertions(+), 39 deletions(-)

-- 
2.10.0

[PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 28 Sep 2016 22:20:08 +0200

* Multiplications for the size determination of memory allocations
  indicated that array data structures should be processed.
  Thus use the corresponding function "kmalloc_array".

  This issue was detected by using the Coccinelle software.

* Replace the specification of data structures by pointer dereferences
  to make the corresponding size determination a bit safer according to
  the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/md/dm-snap.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/md/dm-snap.c b/drivers/md/dm-snap.c
index c65feea..f262f7e 100644
--- a/drivers/md/dm-snap.c
+++ b/drivers/md/dm-snap.c
@@ -326,8 +326,9 @@ static int init_origin_hash(void)
 {
 	int i;
 
-	_origins = kmalloc(ORIGIN_HASH_SIZE * sizeof(struct list_head),
-			   GFP_KERNEL);
+	_origins = kmalloc_array(ORIGIN_HASH_SIZE,
+				 sizeof(*_origins),
+				 GFP_KERNEL);
 	if (!_origins) {
 		DMERR("unable to allocate memory for _origins");
 		return -ENOMEM;
@@ -335,8 +336,9 @@ static int init_origin_hash(void)
 	for (i = 0; i < ORIGIN_HASH_SIZE; i++)
 		INIT_LIST_HEAD(_origins + i);
 
-	_dm_origins = kmalloc(ORIGIN_HASH_SIZE * sizeof(struct list_head),
-			      GFP_KERNEL);
+	_dm_origins = kmalloc_array(ORIGIN_HASH_SIZE,
+				    sizeof(*_dm_origins),
+				    GFP_KERNEL);
 	if (!_dm_origins) {
 		DMERR("unable to allocate memory for _dm_origins");
 		kfree(_origins);
-- 
2.10.0

[PATCH 02/10] dm snapshot: Delete two error messages for a failed memory allocation

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 28 Sep 2016 22:33:09 +0200

Omit extra messages for a memory allocation failure in this function.

Link: http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/md/dm-snap.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/md/dm-snap.c b/drivers/md/dm-snap.c
index f262f7e..7d81390 100644
--- a/drivers/md/dm-snap.c
+++ b/drivers/md/dm-snap.c
@@ -329,10 +329,8 @@ static int init_origin_hash(void)
 	_origins = kmalloc_array(ORIGIN_HASH_SIZE,
 				 sizeof(*_origins),
 				 GFP_KERNEL);
-	if (!_origins) {
-		DMERR("unable to allocate memory for _origins");
+	if (!_origins)
 		return -ENOMEM;
-	}
 	for (i = 0; i < ORIGIN_HASH_SIZE; i++)
 		INIT_LIST_HEAD(_origins + i);
 
@@ -340,7 +338,6 @@ static int init_origin_hash(void)
 				    sizeof(*_dm_origins),
 				    GFP_KERNEL);
 	if (!_dm_origins) {
-		DMERR("unable to allocate memory for _dm_origins");
 		kfree(_origins);
 		return -ENOMEM;
 	}
-- 
2.10.0

[PATCH 03/10] dm snapshot: Delete an unnecessary variable initialisation in snapshot_map()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 29 Sep 2016 08:00:29 +0200

The local variable "pe" will be set to an appropriate pointer a bit later.
Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/md/dm-snap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/dm-snap.c b/drivers/md/dm-snap.c
index 7d81390..82b7604 100644
--- a/drivers/md/dm-snap.c
+++ b/drivers/md/dm-snap.c
@@ -1675,7 +1675,7 @@ static int snapshot_map(struct dm_target *ti, struct bio *bio)
 	struct dm_snapshot *s = ti->private;
 	int r = DM_MAPIO_REMAPPED;
 	chunk_t chunk;
-	struct dm_snap_pending_exception *pe = NULL;
+	struct dm_snap_pending_exception *pe;
 
 	init_tracked_chunk(bio);
 
-- 
2.10.0

[PATCH 04/10] dm snapshot: Rename a jump label in pending_complete()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 29 Sep 2016 08:18:51 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/md/dm-snap.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/md/dm-snap.c b/drivers/md/dm-snap.c
index 82b7604..a6b797f 100644
--- a/drivers/md/dm-snap.c
+++ b/drivers/md/dm-snap.c
@@ -1460,7 +1460,7 @@ static void pending_complete(void *context, int success)
 		down_write(&s->lock);
 		__invalidate_snapshot(s, -EIO);
 		error = 1;
-		goto out;
+		goto remove_exception;
 	}
 
 	e = alloc_completed_exception(GFP_NOIO);
@@ -1468,7 +1468,7 @@ static void pending_complete(void *context, int success)
 		down_write(&s->lock);
 		__invalidate_snapshot(s, -ENOMEM);
 		error = 1;
-		goto out;
+		goto remove_exception;
 	}
 	*e = pe->e;
 
@@ -1476,7 +1476,7 @@ static void pending_complete(void *context, int success)
 	if (!s->valid) {
 		free_completed_exception(e);
 		error = 1;
-		goto out;
+		goto remove_exception;
 	}
 
 	/* Check for conflicting reads */
@@ -1487,8 +1487,7 @@ static void pending_complete(void *context, int success)
 	 * in-flight exception from the list.
 	 */
 	dm_insert_exception(&s->complete, e);
-
-out:
+remove_exception:
 	dm_remove_exception(&pe->e);
 	snapshot_bios = bio_list_get(&pe->snapshot_bios);
 	origin_bios = bio_list_get(&pe->origin_bios);
-- 
2.10.0

[PATCH 05/10] dm snapshot: Delete unnecessary variable initialisations in pending_complete()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 29 Sep 2016 08:25:47 +0200

Three local variables will be set to an appropriate pointer a bit later.
Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/md/dm-snap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/md/dm-snap.c b/drivers/md/dm-snap.c
index a6b797f..7bbd3c4 100644
--- a/drivers/md/dm-snap.c
+++ b/drivers/md/dm-snap.c
@@ -1450,9 +1450,9 @@ static void pending_complete(void *context, int success)
 	struct dm_snap_pending_exception *pe = context;
 	struct dm_exception *e;
 	struct dm_snapshot *s = pe->snap;
-	struct bio *origin_bios = NULL;
-	struct bio *snapshot_bios = NULL;
-	struct bio *full_bio = NULL;
+	struct bio *origin_bios;
+	struct bio *snapshot_bios;
+	struct bio *full_bio;
 	int error = 0;
 
 	if (!success) {
-- 
2.10.0

[PATCH 06/10] dm snapshot: Delete an unnecessary variable initialisation in snapshot_ctr()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 29 Sep 2016 08:38:29 +0200

The local variable "r" will be set to an appropriate value a bit later.
Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/md/dm-snap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/dm-snap.c b/drivers/md/dm-snap.c
index 7bbd3c4..06b1b69 100644
--- a/drivers/md/dm-snap.c
+++ b/drivers/md/dm-snap.c
@@ -1102,7 +1102,7 @@ static int snapshot_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 {
 	struct dm_snapshot *s;
 	int i;
-	int r = -EINVAL;
+	int r;
 	char *origin_path, *cow_path;
 	dev_t origin_dev, cow_dev;
 	unsigned args_used, num_flush_bios = 1;
-- 
2.10.0

[PATCH 07/10] dm snapshot: Delete an unnecessary variable initialisation in merge_callback()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 29 Sep 2016 09:00:06 +0200

The local variable "b" will be set to an appropriate pointer a bit later.
Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/md/dm-snap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/dm-snap.c b/drivers/md/dm-snap.c
index 06b1b69..1d90131 100644
--- a/drivers/md/dm-snap.c
+++ b/drivers/md/dm-snap.c
@@ -1046,7 +1046,7 @@ static void error_bios(struct bio *bio);
 static void merge_callback(int read_err, unsigned long write_err, void *context)
 {
 	struct dm_snapshot *s = context;
-	struct bio *b = NULL;
+	struct bio *b;
 
 	if (read_err || write_err) {
 		if (read_err)
-- 
2.10.0

[PATCH 08/10] dm snapshot: Delete an unnecessary variable initialisation in remove_single_exception_chunk()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 29 Sep 2016 09:06:37 +0200

The local variable "b" will be set to an appropriate pointer a bit later.
Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/md/dm-snap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/dm-snap.c b/drivers/md/dm-snap.c
index 1d90131..4966584 100644
--- a/drivers/md/dm-snap.c
+++ b/drivers/md/dm-snap.c
@@ -904,7 +904,7 @@ static void flush_bios(struct bio *bio);
 
 static int remove_single_exception_chunk(struct dm_snapshot *s)
 {
-	struct bio *b = NULL;
+	struct bio *b;
 	int r;
 	chunk_t old_chunk = s->first_merging_chunk + s->num_merging_chunks - 1;
 
-- 
2.10.0

[PATCH 09/10] dm snapshot: Combine substrings for seven error messages

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 29 Sep 2016 09:46:32 +0200

The script "checkpatch.pl" can point information out like the following.

WARNING: quoted string split across lines

Thus fix the affected source code places.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/md/dm-snap.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/md/dm-snap.c b/drivers/md/dm-snap.c
index 4966584..7593986 100644
--- a/drivers/md/dm-snap.c
+++ b/drivers/md/dm-snap.c
@@ -468,8 +468,7 @@ static int __validate_exception_handover(struct dm_snapshot *snap)
 	if ((__find_snapshots_sharing_cow(snap, &snap_src, &snap_dest,
 					  &snap_merge) == 2) ||
 	    snap_dest) {
-		snap->ti->error = "Snapshot cow pairing for exception "
-				  "table handover failed";
+		snap->ti->error = "Snapshot cow pairing for exception table handover failed";
 		return -EINVAL;
 	}
 
@@ -496,8 +495,7 @@ static int __validate_exception_handover(struct dm_snapshot *snap)
 
 	if (!snap_src->store->type->prepare_merge ||
 	    !snap_src->store->type->commit_merge) {
-		snap->ti->error = "Snapshot exception store does not "
-				  "support snapshot-merge.";
+		snap->ti->error = "Snapshot exception store does not support snapshot-merge.";
 		return -EINVAL;
 	}
 
@@ -858,8 +856,7 @@ static int __remove_single_exception_chunk(struct dm_snapshot *s,
 
 	e = dm_lookup_exception(&s->complete, old_chunk);
 	if (!e) {
-		DMERR("Corruption detected: exception for block %llu is "
-		      "on disk but not in memory",
+		DMERR("Corruption detected: exception for block %llu is on disk but not in memory",
 		      (unsigned long long)old_chunk);
 		return -EINVAL;
 	}
@@ -886,8 +883,7 @@ static int __remove_single_exception_chunk(struct dm_snapshot *s,
 		e->new_chunk++;
 	} else if (old_chunk != e->old_chunk +
 		   dm_consecutive_chunk_count(e)) {
-		DMERR("Attempt to merge block %llu from the "
-		      "middle of a chunk range [%llu - %llu]",
+		DMERR("Attempt to merge block %llu from the middle of a chunk range [%llu - %llu]",
 		      (unsigned long long)old_chunk,
 		      (unsigned long long)e->old_chunk,
 		      (unsigned long long)
@@ -980,8 +976,7 @@ static void snapshot_merge_next_chunks(struct dm_snapshot *s)
 						      &new_chunk);
 	if (linear_chunks <= 0) {
 		if (linear_chunks < 0) {
-			DMERR("Read error in exception store: "
-			      "shutting down merge");
+			DMERR("Read error in exception store: shutting down merge");
 			down_write(&s->lock);
 			s->merge_failed = 1;
 			up_write(&s->lock);
@@ -1877,12 +1872,10 @@ static int snapshot_preresume(struct dm_target *ti)
 	if (snap_src && snap_dest) {
 		down_read(&snap_src->lock);
 		if (s == snap_src) {
-			DMERR("Unable to resume snapshot source until "
-			      "handover completes.");
+			DMERR("Unable to resume snapshot source until handover completes.");
 			r = -EINVAL;
 		} else if (!dm_suspended(snap_src->ti)) {
-			DMERR("Unable to perform snapshot handover until "
-			      "source is suspended.");
+			DMERR("Unable to perform snapshot handover until source is suspended.");
 			r = -EINVAL;
 		}
 		up_read(&snap_src->lock);
-- 
2.10.0

[PATCH 10/10] dm snapshot: Delete five unwanted spaces behind "list_for_each_entry"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 29 Sep 2016 10:14:32 +0200

The script "checkpatch.pl" can point information out like the following.

WARNING: space prohibited between function name and open parenthesis '('

Thus fix the affected source code places.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/md/dm-snap.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/md/dm-snap.c b/drivers/md/dm-snap.c
index 7593986..1310652 100644
--- a/drivers/md/dm-snap.c
+++ b/drivers/md/dm-snap.c
@@ -366,7 +366,7 @@ static struct origin *__lookup_origin(struct block_device *origin)
 	struct origin *o;
 
 	ol = &_origins[origin_hash(origin)];
-	list_for_each_entry (o, ol, hash_list)
+	list_for_each_entry(o, ol, hash_list)
 		if (bdev_equal(o->bdev, origin))
 			return o;
 
@@ -385,7 +385,7 @@ static struct dm_origin *__lookup_dm_origin(struct block_device *origin)
 	struct dm_origin *o;
 
 	ol = &_dm_origins[origin_hash(origin)];
-	list_for_each_entry (o, ol, hash_list)
+	list_for_each_entry(o, ol, hash_list)
 		if (bdev_equal(o->dev->bdev, origin))
 			return o;
 
@@ -625,7 +625,7 @@ static void dm_exception_table_exit(struct dm_exception_table *et,
 	for (i = 0; i < size; i++) {
 		slot = et->table + i;
 
-		list_for_each_entry_safe (ex, next, slot, hash_list)
+		list_for_each_entry_safe(ex, next, slot, hash_list)
 			kmem_cache_free(mem, ex);
 	}
 
@@ -653,7 +653,7 @@ static struct dm_exception *dm_lookup_exception(struct dm_exception_table *et,
 	struct dm_exception *e;
 
 	slot = &et->table[exception_hash(et, chunk)];
-	list_for_each_entry (e, slot, hash_list)
+	list_for_each_entry(e, slot, hash_list)
 		if (chunk >= e->old_chunk &&
 		    chunk <= e->old_chunk + dm_consecutive_chunk_count(e))
 			return e;
@@ -2068,7 +2068,7 @@ static int __origin_write(struct list_head *snapshots, sector_t sector,
 	chunk_t chunk;
 
 	/* Do all the snapshots on this origin */
-	list_for_each_entry (snap, snapshots, list) {
+	list_for_each_entry(snap, snapshots, list) {
 		/*
 		 * Don't make new exceptions in a merging snapshot
 		 * because it has effectively been deleted
-- 
2.10.0

Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()

[Paul Bolle]

Andy, Joe,

On Thu, 2016-09-29 at 11:07 +0200, SF Markus Elfring wrote:
> * Multiplications for the size determination of memory allocations
>   indicated that array data structures should be processed.
>   Thus use the corresponding function "kmalloc_array".
> 
>   This issue was detected by using the Coccinelle software.

We have no hope of fixing Markus' homegrown coccinelle script. But we
could try to fix the checkpatch false positive here. Something like:

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 206a6b3..b47201d 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5693,7 +5693,7 @@ sub process {
 				$r2 = $a1;
 			}
 			if ($r1 !~ /^sizeof\b/ && $r2 =~ /^sizeof\s*\S/ &&
-			    !($r1 =~ /^$Constant$/ || $r1 =~ /^[A-Z_][A-Z0-9_]*$/)) {
+			    !($r1 =~ /^$Constant$/ || $r1 =~ /^[A-Z_][A-Z0-9_]*\b/)) {
 				if (WARN("ALLOC_WITH_MULTIPLY",
 					 "Prefer $newfunc over $oldfunc with multiply\n" . $herecurr) &&
 				    $fix) {

Does that work for you too?

> --- a/drivers/md/dm-snap.c
> +++ b/drivers/md/dm-snap.c
> @@ -326,8 +326,9 @@ static int init_origin_hash(void)
>  {
>  	int i;
>  
> -	_origins = kmalloc(ORIGIN_HASH_SIZE * sizeof(struct list_head),
> -			   GFP_KERNEL);
> +	_origins = kmalloc_array(ORIGIN_HASH_SIZE,
> +				 sizeof(*_origins),
> +				 GFP_KERNEL);
>  	if (!_origins) {
>  		DMERR("unable to allocate memory for _origins");
>  		return -ENOMEM;
> @@ -335,8 +336,9 @@ static int init_origin_hash(void)
>  	for (i = 0; i < ORIGIN_HASH_SIZE; i++)
>  		INIT_LIST_HEAD(_origins + i);
>  
> -	_dm_origins = kmalloc(ORIGIN_HASH_SIZE * sizeof(struct list_head),
> -			      GFP_KERNEL);
> +	_dm_origins = kmalloc_array(ORIGIN_HASH_SIZE,
> +				    sizeof(*_dm_origins),
> +				    GFP_KERNEL);
>  	if (!_dm_origins) {
>  		DMERR("unable to allocate memory for _dm_origins");
>  		kfree(_origins);

Thanks,


Paul Bolle

Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()

[Joe Perches]

On Thu, 2016-09-29 at 11:54 +0200, Paul Bolle wrote:
> Andy, Joe,
> 
> On Thu, 2016-09-29 at 11:07 +0200, SF Markus Elfring wrote:
> > * Multiplications for the size determination of memory allocations
> >   indicated that array data structures should be processed.
> >   Thus use the corresponding function "kmalloc_array".
> > 
> >   This issue was detected by using the Coccinelle software.
> 
> 
> We have no hope of fixing Markus' homegrown coccinelle script. But we
> could try to fix the checkpatch false positive here.

What's the false positive?

I get:

$ ./scripts/checkpatch.pl -f drivers/md/dm-snap.c --show-types --types=alloc_with_multiply
WARNING:ALLOC_WITH_MULTIPLY: Prefer kmalloc_array over kmalloc with multiply
#329: FILE: drivers/md/dm-snap.c:329:
+	_origins = kmalloc(ORIGIN_HASH_SIZE * sizeof(struct list_head),

WARNING:ALLOC_WITH_MULTIPLY: Prefer kmalloc_array over kmalloc with multiply
#338: FILE: drivers/md/dm-snap.c:338:
+	_dm_origins = kmalloc(ORIGIN_HASH_SIZE * sizeof(struct list_head),

total: 0 errors, 2 warnings, 2490 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

drivers/md/dm-snap.c has style problems, please review.

NOTE: Used message types: ALLOC_WITH_MULTIPLY

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()

[Paul Bolle]

On Thu, 2016-09-29 at 03:02 -0700, Joe Perches wrote:
> What's the false positive?
> 
> I get:
> 
> $ ./scripts/checkpatch.pl -f drivers/md/dm-snap.c --show-types --types=alloc_with_multiply
> WARNING:ALLOC_WITH_MULTIPLY: Prefer kmalloc_array over kmalloc with multiply
> #329: FILE: drivers/md/dm-snap.c:329:
> +	_origins = kmalloc(ORIGIN_HASH_SIZE * sizeof(struct list_head),
> 
> WARNING:ALLOC_WITH_MULTIPLY: Prefer kmalloc_array over kmalloc with multiply
> #338: FILE: drivers/md/dm-snap.c:338:
> +	_dm_origins = kmalloc(ORIGIN_HASH_SIZE * sizeof(struct list_head),
> 
> total: 0 errors, 2 warnings, 2490 lines checked
> 
> NOTE: For some of the reported defects, checkpatch may be able to
>       mechanically convert to the typical style using --fix or --fix-inplace.
> 
> drivers/md/dm-snap.c has style problems, please review.
> 
> NOTE: Used message types: ALLOC_WITH_MULTIPLY
> 
> NOTE: If any of the errors are false positives, please report
>       them to the maintainer, see CHECKPATCH in MAINTAINERS.

It seems it was intended to be silent about multiplying with constants,
where things that look like preprocessor defines are also considered
constants. Or did I misread that test?


Paul Bolle

Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()

[Paul Bolle]

On Thu, 2016-09-29 at 13:12 +0200, Paul Bolle wrote:
> Or did I misread that test?

I finally did some digging: commit e367455a9f25 ("checkpatch: emit
fewer kmalloc_array/kcalloc conversion warnings") shows I didn't.


Paul Bolle

Re: dm snapshot: Use kmalloc_array() in init_origin_hash()

[SF Markus Elfring]

> We have no hope of fixing Markus' homegrown coccinelle script.

I have got an other impression. I see further possibilities
to clarify involved communication and software development challenges
for a few source code search patterns.

How do you think about to discuss the corresponding collateral evolution
a bit more?

Are there any more constraints to consider?

Regards,
Markus

Re: [dm-devel] [PATCH 03/10] md/dm-crypt: Rename a jump label in crypt_message()

[Theodore Ts'o]

On Wed, Sep 28, 2016 at 05:40:14PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 28 Sep 2016 14:54:39 +0200
> 
> Adjust a jump label according to the current Linux coding style convention.

In what bizzaro world is the "current Linux coding style convention"

> -
> -error:
> +show_warning:
>  	DMWARN("unrecognised message received.");
>  	return -EINVAL;
>  }

"show_warning" is better than "error" when the net result of the goto
is that the function returns -EINVAL?!?

Please give it up with these drive-by shooting of auto-generated
patches.  You're just embarassing yourself.

							- Ted

Re: [dm-devel] [PATCH 05/10] md/dm-crypt: Rename a jump label in crypt_set_key()

[Theodore Ts'o]

On Wed, Sep 28, 2016 at 05:42:28PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 28 Sep 2016 15:21:18 +0200
> 
> Adjust jump labels according to the current Linux coding style convention.
> 
> -
> -out:
> +set_memory:
>  	/* Hex key string not needed after here, so wipe it. */
>  	memset(key, '0', key_string_len);

Also not "current Linux coding style convetion".

							- Ted

Re: dm snapshot: Use kmalloc_array() in init_origin_hash()

[Theodore Ts'o]

On Thu, Sep 29, 2016 at 01:45:41PM +0200, SF Markus Elfring wrote:
> > We have no hope of fixing Markus' homegrown coccinelle script.
> 
> I have got an other impression. I see further possibilities
> to clarify involved communication and software development challenges
> for a few source code search patterns.
> 
> How do you think about to discuss the corresponding collateral evolution
> a bit more?

Here's my suggestion:

	https://www.youtube.com/watch?v=xAnVNXaa5oA

Regards,

					- Ted

Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()

[Joe Perches]

On Thu, 2016-09-29 at 13:45 +0200, Paul Bolle wrote:
> On Thu, 2016-09-29 at 13:12 +0200, Paul Bolle wrote:
> > Or did I misread that test?
> I finally did some digging: commit e367455a9f25 ("checkpatch: emit
> fewer kmalloc_array/kcalloc conversion warnings") shows I didn't.

You still misread it a little.
I think it's fine as-is.

$Constant there is any number and the match regex is
any upper case variable.

Re: [dm-devel] [PATCH 03/10] md/dm-crypt: Rename a jump label in crypt_message()

[SF Markus Elfring]

> In what bizzaro world is the "current Linux coding style convention"

Do you look at the evolution for a document like "CodingStyle"?


>> -
>> -error:
>> +show_warning:
>>  	DMWARN("unrecognised message received.");
>>  	return -EINVAL;
>>  }
> 
> "show_warning" is better than "error"

I got such an impression.


> when the net result of the goto is that the function returns -EINVAL?!?

Do other identifiers fit better for the desired description of "what" and "why"
by jump labels?


> Please give it up with these drive-by shooting of auto-generated patches.

This update step was not auto-generated.

There are further change possibilities where special analysis tools
can help in the corresponding software development.


> You're just embarassing yourself.

Do you find any of my update suggestions worth for further considerations?

Regards,
Markus

Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()

[Paul Bolle]

On Thu, 2016-09-29 at 08:01 -0700, Joe Perches wrote:
> $Constant there is any number and the match regex is
> any upper case variable.

Why doesn't that regex match on "ORIGIN_HASH_SIZE"?


Paul Bolle

Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()

[Joe Perches]

On Thu, 2016-09-29 at 21:43 +0200, Paul Bolle wrote:
> On Thu, 2016-09-29 at 08:01 -0700, Joe Perches wrote:
> > $Constant there is any number and the match regex is
> > any upper case variable.
> Why doesn't that regex match on "ORIGIN_HASH_SIZE"?

It does match.

Did you see my earlier email?

$ ./scripts/checkpatch.pl -f drivers/md/dm-snap.c --show-types --types=alloc_with_multiply
WARNING:ALLOC_WITH_MULTIPLY: Prefer kmalloc_array over kmalloc with multiply
#329: FILE: drivers/md/dm-snap.c:329:
+       _origins = kmalloc(ORIGIN_HASH_SIZE * sizeof(struct list_head),

WARNING:ALLOC_WITH_MULTIPLY: Prefer kmalloc_array over kmalloc with multiply
#338: FILE: drivers/md/dm-snap.c:338:
+       _dm_origins = kmalloc(ORIGIN_HASH_SIZE * sizeof(struct list_head),

total: 0 errors, 2 warnings, 2490 lines checked

Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()

[Paul Bolle]

On Thu, 2016-09-29 at 13:24 -0700, Joe Perches wrote:
> On Thu, 2016-09-29 at 21:43 +0200, Paul Bolle wrote:
> > Why doesn't that regex match on "ORIGIN_HASH_SIZE"?
> 
> It does match.

If that regex does match, it being part of a negative test, the
specific checkpatch rule should be silent, shouldn't it?


Paul Bolle

Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()

[Joe Perches]

On Thu, 2016-09-29 at 22:39 +0200, Paul Bolle wrote:
> On Thu, 2016-09-29 at 13:24 -0700, Joe Perches wrote:
> > On Thu, 2016-09-29 at 21:43 +0200, Paul Bolle wrote:
> > > Why doesn't that regex match on "ORIGIN_HASH_SIZE"?
> > It does match.
> If that regex does match, it being part of a negative test, the
> specific checkpatch rule should be silent, shouldn't it?

'cause I forgot to trim() the original $4 and $10 matches.

Oh well.

It doesn't matter match either way to me.

The case for the unnecessary multiply with <= gcc 4.8 was
removed with:

commit 91c6a05f72a996bee5133e76374ab3ad7d3b9b72
Author: Alexey Dobriyan <adobriyan@gmail.com>
Date:   Tue Jul 26 15:22:08 2016 -0700

    mm: faster kmalloc_array(), kcalloc()
    
    When both arguments to kmalloc_array() or kcalloc() are known at compile
    time then their product is known at compile time but search for kmalloc
    cache happens at runtime not at compile time.
    
    Link: http://lkml.kernel.org/r/20160627213454.GA2440@p183.telecom.by
    Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
    Cc: Christoph Lameter <cl@linux.com>
    Cc: Pekka Enberg <penberg@kernel.org>
    Cc: David Rientjes <rientjes@google.com>
    Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()

[Paul Bolle]

On Thu, 2016-09-29 at 13:56 -0700, Joe Perches wrote:
> It doesn't matter match either way to me.
> 
> The case for the unnecessary multiply with <= gcc 4.8 was
> removed with:
> 
> commit 91c6a05f72a996bee5133e76374ab3ad7d3b9b72
> Author: Alexey Dobriyan <adobriyan@gmail.com>
> Date:   Tue Jul 26 15:22:08 2016 -0700
> 
>     mm: faster kmalloc_array(), kcalloc()
>     
>     When both arguments to kmalloc_array() or kcalloc() are known at compile
>     time then their product is known at compile time but search for kmalloc
>     cache happens at runtime not at compile time.
>     
>     Link: http://lkml.kernel.org/r/20160627213454.GA2440@p183.telecom.by
>     Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
>     Cc: Christoph Lameter <cl@linux.com>
>     Cc: Pekka Enberg <penberg@kernel.org>
>     Cc: David Rientjes <rientjes@google.com>
>     Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>     Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>     Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

You've lost me.

Why does this stop you fixing an apparently wrong checkpatch rule,
crude as parts of it are (ie, uppercase identifier must be a constant)?


Paul Bolle

Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()

[Joe Perches]

On Thu, 2016-09-29 at 23:14 +0200, Paul Bolle wrote:
> On Thu, 2016-09-29 at 13:56 -0700, Joe Perches wrote:
> > It doesn't matter match either way to me.
> Why does this stop you fixing an apparently wrong checkpatch rule,
> crude as parts of it are (ie, uppercase identifier must be a constant)?

It doesn't.  It just doesn't matter much (match) to me.
Here:
---
 scripts/checkpatch.pl | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 3373c65fef1c..fc931d89152e 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5835,8 +5835,8 @@ sub process {
 		if ($^V && $^V ge 5.10.0 &&
 		    $line =~ /\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*(k[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)\s*,/) {
 			my $oldfunc = $3;
-			my $a1 = $4;
-			my $a2 = $10;
+			my $a1 = trim($4);
+			my $a2 = trim($10);
 			my $newfunc = "kmalloc_array";
 			$newfunc = "kcalloc" if ($oldfunc eq "kzalloc");
 			my $r1 = $a1;
@@ -5850,7 +5850,7 @@ sub process {
 				if (WARN("ALLOC_WITH_MULTIPLY",
 					 "Prefer $newfunc over $oldfunc with multiply\n" . $herecurr) &&
 				    $fix) {
-					$fixed[$fixlinenr] =~ s/\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*(k[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)/$1 . ' = ' . "$newfunc(" . trim($r1) . ', ' . trim($r2)/e;
+					$fixed[$fixlinenr] =~ s/\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*(k[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)/$1 . ' = ' . "$newfunc(" . $r1 . ', ' . $r2/e;
 
 				}
 			}

Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()

[Paul Bolle]

On Thu, 2016-09-29 at 14:21 -0700, Joe Perches wrote:
> > > It doesn't matter match either way to me.
> > Why does this stop you fixing an apparently wrong checkpatch rule,
> > crude as parts of it are (ie, uppercase identifier must be a
> > constant)?
> 
> It doesn't.  It just doesn't matter much (match) to me.

Joe, please.

I've recently ping-ponged with the kernel's "resident wrong bot of the
day" over this very rule (kmalloc_array() is safer than kmalloc(), so
change your driver now!). Could we just give wrong bots a bit less
ammunition whenever that's feasible?

Even if you don't care about my ping-pong experiences: this checkpatch
test is broken, please just fix it!


Paul Bolle

Re: dm snapshot: Use kmalloc_array() in init_origin_hash() ?

[SF Markus Elfring]

> I've recently ping-ponged with the kernel's "resident wrong bot of the
> day" over this very rule (kmalloc_array() is safer than kmalloc(), so
> change your driver now!).

Your bot of the day is going to point more update candidates out
in various source files that can "accidentally" belong also to Linux. ;-)


> Could we just give wrong bots a bit less ammunition whenever that's feasible?

How do you think about to clarify constraints any further so that
the probability for false positives can be reduced as desired for
the involved source code analysis tools?


> Even if you don't care about my ping-pong experiences: this checkpatch
> test is broken, please just fix it!

I am curious how collateral software evolution will be continued.

Regards,
Markus

Re: [dm-devel] [PATCH 03/10] md/dm-crypt: Rename a jump label in crypt_message()

[Dan Carpenter]

On Thu, Sep 29, 2016 at 05:43:57PM +0200, SF Markus Elfring wrote:
> > In what bizzaro world is the "current Linux coding style convention"
> 
> Do you look at the evolution for a document like "CodingStyle"?
> 

Again, I wrote the paragraph in CodingStyle.  I just said that it's a
good idea to think about label names instead of using GW-BASIC style
numbered labels, I didn't say go around bothering everyone with waste
of time cleanup patches.

I specifically did not say that "out:" or "error:" labels are bad names.
Those are common style in the kernel.

Please stop sending these patches.

regards,
dan carpenter

Re: md/dm-crypt: Rename a jump label in crypt_message() ?

[SF Markus Elfring]

> Again, I wrote the paragraph in CodingStyle.

This is obvious from the corresponding commit "add some more error
handling guidelines" (ea04036032edda6f771c1381d03832d2ed0f6c31
on 2014-12-02).


> I just said that it's a good idea to think about label names

I agree also to such a desire.


> instead of using GW-BASIC style numbered labels,

Is this kind of wording another weakness in the discussed document?
How many guidance do programmers get from such a software specification?

I came a few source code places along where I proposed corresponding changes.


> I didn't say

You did not say anything about some details as it is often easier to express
several aspects in vague and general terms.


> go around bothering everyone with waste of time cleanup patches.

I find it still debatable if the shown software development efforts
are really "wasted".

It seems that also the Linux development community is mixed about
related interpretations.


> I specifically did not say that "out:" or "error:" labels are bad names.

Did you inform me once that you had also a special opinion about an identifier
like "out"?

The C compiler will accept them as usual. But do we occasionally prefer
to express implementation details a bit better there?


> Those are common style in the kernel.

* Which impressions can you get from a statement like "goto fail;"
  or "goto error;"?

* Do any exception handling implementations should be reconsidered
  at such places?


> Please stop sending these patches.

Could it happen that the change acceptance will increase also for
the suggested renaming of jump labels if maintainers from other subsystems
would dare to respond once more in a positive way for such a software refactoring?

Regards,
Markus

Re: md/dm-crypt: Rename a jump label in crypt_message() ?

[Bjørn Mork]

SF Markus Elfring <elfring@users.sourceforge.net> writes:

>> go around bothering everyone with waste of time cleanup patches.
>
> I find it still debatable if the shown software development efforts
> are really "wasted".

When someone tells you that you are wasting their time, then that is not
a subject for further discussion.


Bjørn

Re: md/dm-crypt: Rename a jump label in crypt_message() ?

[SF Markus Elfring]

> When someone tells you that you are wasting their time,

This information can be useful to some degree


> then that is not a subject for further discussion.

I got an other impression. I guess that there are constraints for such a response
which can become interesting for further considerations.

Is it just usual that other software changes are more welcome?

Regards,
Markus

Re: md/dm-crypt: Rename a jump label in crypt_message() ?

[Dan Carpenter]

On Fri, Sep 30, 2016 at 01:32:23PM +0200, SF Markus Elfring wrote:
> > I specifically did not say that "out:" or "error:" labels are bad names.
> 
> Did you inform me once that you had also a special opinion about an identifier
> like "out"?

I don't like out labels, but that's my opinion.  There is nothing in
CodingStyle which says you can't do it.

regards,
dan carpenter

Re: md/dm-crypt: Rename a jump label in crypt_message() ?

[Bjørn Mork]

SF Markus Elfring <elfring@users.sourceforge.net> writes:

>> When someone tells you that you are wasting their time,
>
> This information can be useful to some degree

Yes.  If you continue discussing after that point, then you make a clear
statement that it isn't an accident.  You are deliberately wasting their
time.

A lot of people already know this.  But you're right that it would be
useful to make it even clearer.  Maybe you could add a note about it to
each patch?  Something along "I will not listen.  I will not change.
Nothing you tell me will ever make it worth your time to do so"?

Just an idea...



Bjørn

Re: md/dm-crypt: Rename a jump label in crypt_message() ?

[SF Markus Elfring]

> I don't like out labels, but that's my opinion.

Thanks for this acknowledgement that you have still got a special opinion
about such an identifier.

 
> There is nothing in CodingStyle which says you can't do it.

Does the terse description there try to suggest also to choose
better identifiers for source code places?

Does the meaning of such a coding style specification include also
the selection of a more pleasing identifier than "error"
for this software module?

Regards,
Markus

Re: md/dm-crypt: Rename a jump label in crypt_message() ?

[SF Markus Elfring]

> If you continue discussing after that point,

I guess that such a condition is not needed.


> then you make a clear statement that it isn't an accident.

I hope that most of my software development activities are not "an accident".
Is the intent for any update suggestion (like the renaming of a jump label
in this case) reasonable to some degree?


> You are deliberately wasting their time.

I imagine that I do not really try to "waste" others time. But I am trying
also to change some "things".
There are circumstances when these contributions are interpreted as "wasted efforts".
Is the change acceptance usually higher for other update patterns?


> Something along "I will not listen.

I am listening while my responses might not fit to your current expectations.


> I will not change.

I have got also some personal change opportunities.


> Nothing you tell me will ever make it worth your time to do so"?

While you can be so clear about a rejection for this software module at the moment,
other contributors showed occasionally more positive information.

Regards,
Markus