From: SF Markus Elfring <hidden> Date: 2016-09-29 09:03:30
From: Markus Elfring <redacted>
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
From: SF Markus Elfring <hidden> Date: 2016-09-29 09:08:20
From: Markus Elfring <redacted>
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 <redacted>
---
drivers/md/dm-snap.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
@@ -326,8 +326,9 @@ static int init_origin_hash(void){inti;-_origins=kmalloc(ORIGIN_HASH_SIZE*sizeof(structlist_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(structlist_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);
From: Paul Bolle <hidden> Date: 2016-09-29 09:54:40
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:
@@ -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){
From: Joe Perches <joe@perches.com> Date: 2016-09-29 10:03:06
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:
quoted
* 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.
From: Paul Bolle <hidden> Date: 2016-09-29 11:13:16
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
From: Joe Perches <joe@perches.com> Date: 2016-09-29 20:56:41
On Thu, 2016-09-29 at 22:39 +0200, Paul Bolle wrote:
On Thu, 2016-09-29 at 13:24 -0700, Joe Perches wrote:
quoted
On Thu, 2016-09-29 at 21:43 +0200, Paul Bolle wrote:
quoted
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 [off-list ref]
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 [off-list ref]
Cc: Christoph Lameter [off-list ref]
Cc: Pekka Enberg [off-list ref]
Cc: David Rientjes [off-list ref]
Cc: Joonsoo Kim [off-list ref]
Signed-off-by: Andrew Morton [off-list ref]
Signed-off-by: Linus Torvalds [off-list ref]
From: Paul Bolle <hidden> Date: 2016-09-29 21:15:07
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 [off-list ref]
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 [off-list ref]
Cc: Christoph Lameter [off-list ref]
Cc: Pekka Enberg [off-list ref]
Cc: David Rientjes [off-list ref]
Cc: Joonsoo Kim [off-list ref]
Signed-off-by: Andrew Morton [off-list ref]
Signed-off-by: Linus Torvalds [off-list ref]
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
@@ -5835,8 +5835,8 @@ sub process {if($^V&&$^Vge5.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($oldfunceq"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;}}
From: Paul Bolle <hidden> Date: 2016-09-29 21:42:59
On Thu, 2016-09-29 at 14:21 -0700, Joe Perches wrote:
quoted
quoted
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
From: SF Markus Elfring <hidden> Date: 2016-09-30 07:15:36
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
From: SF Markus Elfring <hidden> Date: 2016-09-29 11:46:17
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
On Thu, Sep 29, 2016 at 01:45:41PM +0200, SF Markus Elfring wrote:
quoted
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?
@@ -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;}
From: SF Markus Elfring <hidden> Date: 2016-09-29 09:11:47
From: Markus Elfring <redacted>
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 <redacted>
---
drivers/md/dm-snap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: SF Markus Elfring <hidden> Date: 2016-09-29 09:13:36
From: Markus Elfring <redacted>
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 <redacted>
---
drivers/md/dm-snap.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
From: SF Markus Elfring <hidden> Date: 2016-09-29 09:14:39
From: Markus Elfring <redacted>
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 <redacted>
---
drivers/md/dm-snap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: SF Markus Elfring <hidden> Date: 2016-09-29 09:15:47
From: Markus Elfring <redacted>
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 <redacted>
---
drivers/md/dm-snap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: SF Markus Elfring <hidden> Date: 2016-09-29 09:16:37
From: Markus Elfring <redacted>
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 <redacted>
---
drivers/md/dm-snap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: SF Markus Elfring <hidden> Date: 2016-09-29 09:17:35
From: Markus Elfring <redacted>
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 <redacted>
---
drivers/md/dm-snap.c | 21 +++++++--------------
1 file changed, 7 insertions(+), 14 deletions(-)
@@ -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",(unsignedlonglong)old_chunk);return-EINVAL;}
@@ -886,8 +883,7 @@ static int __remove_single_exception_chunk(struct dm_snapshot *s,e->new_chunk++;}elseif(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]",(unsignedlonglong)old_chunk,(unsignedlonglong)e->old_chunk,(unsignedlonglong)
@@ -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;}elseif(!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);
From: SF Markus Elfring <hidden> Date: 2016-09-29 09:18:58
From: Markus Elfring <redacted>
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 <redacted>
---
drivers/md/dm-snap.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
@@ -2068,7 +2068,7 @@ static int __origin_write(struct list_head *snapshots, sector_t sector,chunk_tchunk;/* Do all the snapshots on this origin */-list_for_each_entry(snap,snapshots,list){+list_for_each_entry(snap,snapshots,list){/**Don'tmakenewexceptionsinamergingsnapshot*becauseithaseffectivelybeendeleted