From: SF Markus Elfring <hidden> Date: 2017-01-20 18:18:32
From: Markus Elfring <redacted>
Date: Fri, 20 Jan 2017 19:07:21 +0100
Some update suggestions were taken into account
from static source code analysis.
Markus Elfring (11):
Move assignments for the variable "err" in kvm_htab_write()
Improve a size determination in kvmppc_alloc_hpt()
Move error code assignments in two functions
Use common error handling code in kvmppc_clr_passthru_irq()
Adjust nine checks for null pointers
Use kcalloc() in kvmppc_alloc_host_rm_ops()
Improve size determinations in five functions
Use seq_puts() in xics_debug_show()
Improve a size determination in two functions
Use kcalloc() in e500_mmu_host_init()
Return directly after a failed copy_from_user() in two functions
arch/powerpc/kvm/book3s_64_mmu_hv.c | 26 +++++++------
arch/powerpc/kvm/book3s_hv.c | 73 +++++++++++++++++++------------------
arch/powerpc/kvm/book3s_xics.c | 7 ++--
arch/powerpc/kvm/e500_mmu_host.c | 5 +--
arch/powerpc/kvm/powerpc.c | 48 ++++++++++--------------
5 files changed, 75 insertions(+), 84 deletions(-)
--
2.11.0
From: SF Markus Elfring <hidden> Date: 2017-01-20 18:20:37
From: Markus Elfring <redacted>
Date: Thu, 19 Jan 2017 22:45:56 +0100
* A local variable was set to an error code in five cases
before a concrete error situation was detected.
Thus move the corresponding assignments into
if branches to indicate a software failure there.
This issue was detected by using the Coccinelle software.
* Delete two zero assignments which became unnecessary with
this refactoring.
Signed-off-by: Markus Elfring <redacted>
---
arch/powerpc/kvm/book3s_64_mmu_hv.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
From: SF Markus Elfring <hidden> Date: 2017-01-20 18:21:47
From: Markus Elfring <redacted>
Date: Fri, 20 Jan 2017 10:00:13 +0100
Replace the specification of a data structure by a pointer dereference
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.
Signed-off-by: Markus Elfring <redacted>
---
arch/powerpc/kvm/book3s_64_mmu_hv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: SF Markus Elfring <hidden> Date: 2017-01-20 18:22:16
From: Markus Elfring <redacted>
Date: Fri, 20 Jan 2017 10:30:26 +0100
A local variable was set to an error code in a few cases before a concrete
error situation was detected. Thus move the corresponding assignments into
if branches to indicate a software failure there.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <redacted>
---
arch/powerpc/kvm/book3s_hv.c | 25 +++++++++++++++----------
1 file changed, 15 insertions(+), 10 deletions(-)
@@ -2967,15 +2967,17 @@ static int kvm_vm_ioctl_get_dirty_log_hv(struct kvm *kvm,mutex_lock(&kvm->slots_lock);-r=-EINVAL;-if(log->slot>=KVM_USER_MEM_SLOTS)+if(log->slot>=KVM_USER_MEM_SLOTS){+r=-EINVAL;gotoout;+}slots=kvm_memslots(kvm);memslot=id_to_memslot(slots,log->slot);-r=-ENOENT;-if(!memslot->dirty_bitmap)+if(!memslot->dirty_bitmap){+r=-ENOENT;gotoout;+}n=kvm_dirty_bitmap_bytes(memslot);memset(memslot->dirty_bitmap,0,n);
@@ -2984,9 +2986,10 @@ static int kvm_vm_ioctl_get_dirty_log_hv(struct kvm *kvm,if(r)gotoout;-r=-EFAULT;-if(copy_to_user(log->dirty_bitmap,memslot->dirty_bitmap,n))+if(copy_to_user(log->dirty_bitmap,memslot->dirty_bitmap,n)){+r=-EFAULT;gotoout;+}r=0;out:
@@ -3127,9 +3130,10 @@ static int kvmppc_hv_setup_htab_rma(struct kvm_vcpu *vcpu)memslot=gfn_to_memslot(kvm,0);/* We must have some memory at 0 by now */-err=-EINVAL;-if(!memslot||(memslot->flags&KVM_MEMSLOT_INVALID))+if(!memslot||(memslot->flags&KVM_MEMSLOT_INVALID)){+err=-EINVAL;gotoout_srcu;+}/* Look up the VMA for the start of this memory slot */hva=memslot->userspace_addr;
@@ -3144,10 +3148,11 @@ static int kvmppc_hv_setup_htab_rma(struct kvm_vcpu *vcpu)up_read(¤t->mm->mmap_sem);/* We can handle 4k, 64k or 16M pages in the VRMA */-err=-EINVAL;if(!(psize==0x1000||psize==0x10000||-psize==0x1000000))+psize==0x1000000)){+err=-EINVAL;gotoout_srcu;+}senc=slb_pgsize_encoding(psize);kvm->arch.vrma_slb_v=senc|SLB_VSID_B_1T|
From: SF Markus Elfring <hidden> Date: 2017-01-20 18:23:15
From: Markus Elfring <redacted>
Date: Fri, 20 Jan 2017 11:00:08 +0100
Add a jump target so that a bit of exception handling can be better reused
at the end of this function.
Signed-off-by: Markus Elfring <redacted>
---
arch/powerpc/kvm/book3s_hv.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
@@ -3509,11 +3509,9 @@ static int kvmppc_clr_passthru_irq(struct kvm *kvm, int host_irq, int guest_gsi)return-EIO;mutex_lock(&kvm->lock);+if(!kvm->arch.pimap)+gotounlock;-if(kvm->arch.pimap==NULL){-mutex_unlock(&kvm->lock);-return0;-}pimap=kvm->arch.pimap;for(i=0;i<pimap->n_mapped;i++){
@@ -3535,7 +3533,7 @@ static int kvmppc_clr_passthru_irq(struct kvm *kvm, int host_irq, int guest_gsi)*Wedon'tfreethisstructureevenwhenthecountgoesto*zero.ThestructureisfreedwhenwedestroytheVM.*/-+unlock:mutex_unlock(&kvm->lock);return0;}
From: SF Markus Elfring <hidden> Date: 2017-01-20 18:26:07
From: Markus Elfring <redacted>
Date: Fri, 20 Jan 2017 16:20:43 +0100
* A multiplication for the size determination of a memory allocation
indicated that an array data structure should be processed.
Thus use the corresponding function "kcalloc".
This issue was detected by using the Coccinelle software.
* Delete the local variable "size" which became unnecessary
with this refactoring.
* Replace the specification of a data structure by a pointer dereference
to make the corresponding size determination a bit safer according to
the Linux coding style convention.
Signed-off-by: Markus Elfring <redacted>
---
arch/powerpc/kvm/book3s_hv.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
@@ -3199,7 +3199,6 @@ void kvmppc_alloc_host_rm_ops(void)structkvmppc_host_rm_ops*ops;unsignedlongl_ops;intcpu,core;-intsize;/* Not the first time here ? */if(kvmppc_host_rm_ops_hv)
From: SF Markus Elfring <hidden> Date: 2017-01-20 18:26:11
From: Markus Elfring <redacted>
Date: Fri, 20 Jan 2017 16:23:30 +0100
Replace the specification of data structures by pointer dereferences
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.
Signed-off-by: Markus Elfring <redacted>
---
arch/powerpc/kvm/book3s_hv.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
From: SF Markus Elfring <hidden> Date: 2017-01-20 18:29:30
From: Markus Elfring <redacted>
Date: Fri, 20 Jan 2017 16:28:43 +0100
Replace the specification of data structures by pointer dereferences
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.
Signed-off-by: Markus Elfring <redacted>
---
arch/powerpc/kvm/book3s_xics.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: SF Markus Elfring <hidden> Date: 2017-01-20 18:29:41
From: Markus Elfring <redacted>
Date: Fri, 20 Jan 2017 16:30:18 +0100
* A multiplication for the size determination of a memory allocation
indicated that an array data structure should be processed.
Thus use 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.
Signed-off-by: Markus Elfring <redacted>
---
arch/powerpc/kvm/e500_mmu_host.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
From: SF Markus Elfring <hidden> Date: 2017-01-20 18:30:47
From: Markus Elfring <redacted>
Date: Fri, 20 Jan 2017 18:00:35 +0100
* Return directly after a call of the function "copy_from_user"
(or two other checks) failed in a case block.
This issue was detected by using the Coccinelle software.
* Delete the jump label "out" which became unnecessary with
this refactoring.
Signed-off-by: Markus Elfring <redacted>
---
arch/powerpc/kvm/powerpc.c | 48 ++++++++++++++++++----------------------------
1 file changed, 19 insertions(+), 29 deletions(-)
From: SF Markus Elfring <hidden> Date: 2017-01-20 18:32:02
From: Markus Elfring <redacted>
Date: Fri, 20 Jan 2017 11:25:48 +0100
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The script "checkpatch.pl" pointed information out like the following.
Comparison to NULL could be written …
Thus fix affected source code places.
Signed-off-by: Markus Elfring <redacted>
---
arch/powerpc/kvm/book3s_hv.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
@@ -3203,7 +3202,7 @@ void kvmppc_alloc_host_rm_ops(void)intsize;/* Not the first time here ? */-if(kvmppc_host_rm_ops_hv!=NULL)+if(kvmppc_host_rm_ops_hv)return;ops=kzalloc(sizeof(structkvmppc_host_rm_ops),GFP_KERNEL);
@@ -3430,10 +3429,10 @@ static int kvmppc_set_passthru_irq(struct kvm *kvm, int host_irq, int guest_gsi)mutex_lock(&kvm->lock);pimap=kvm->arch.pimap;-if(pimap==NULL){+if(!pimap){/* First call, allocate structure to hold IRQ map */pimap=kvmppc_alloc_pimap();-if(pimap==NULL){+if(!pimap){mutex_unlock(&kvm->lock);return-ENOMEM;}
From: SF Markus Elfring <hidden> Date: 2017-01-20 18:34:51
From: Markus Elfring <redacted>
Date: Fri, 20 Jan 2017 16:26:51 +0100
A string which did not contain data format specifications should be put
into a sequence. Thus use the corresponding function "seq_puts".
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <redacted>
---
arch/powerpc/kvm/book3s_xics.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
arch/powerpc/kvm/book3s_xics.c:1035:16: error: incompatible type for argument 1 of 'kzalloc'
ics = kzalloc(*ics), GFP_KERNEL);
^
In file included from include/linux/kvm_host.h:21:0,
from arch/powerpc/kvm/book3s_xics.c:11:
include/linux/slab.h:634:21: note: expected 'size_t {aka long unsigned int}' but argument is of type 'struct kvmppc_ics'
static inline void *kzalloc(size_t size, gfp_t flags)
^~~~~~~
quoted
arch/powerpc/kvm/book3s_xics.c:1035:8: error: too few arguments to function 'kzalloc'
ics = kzalloc(*ics), GFP_KERNEL);
^~~~~~~
In file included from include/linux/kvm_host.h:21:0,
from arch/powerpc/kvm/book3s_xics.c:11:
include/linux/slab.h:634:21: note: declared here
static inline void *kzalloc(size_t size, gfp_t flags)
^~~~~~~
arch/powerpc/kvm/book3s_xics.c:1035:21: warning: left-hand operand of comma expression has no effect [-Wunused-value]
ics = kzalloc(*ics), GFP_KERNEL);
^
arch/powerpc/kvm/book3s_xics.c:1035:21: warning: statement with no effect [-Wunused-value]
quoted
arch/powerpc/kvm/book3s_xics.c:1035:33: error: expected ';' before ')' token
ics = kzalloc(*ics), GFP_KERNEL);
^
quoted
arch/powerpc/kvm/book3s_xics.c:1035:33: error: expected statement before ')' token
vim +/kzalloc +1035 arch/powerpc/kvm/book3s_xics.c
1029
1030 /* ICS already exists - somebody else got here first */
1031 if (xics->ics[icsid])
1032 goto out;
1033
1034 /* Create the ICS */
From: kbuild test robot <hidden> Date: 2017-01-21 06:37:24
Hi Markus,
[auto build test ERROR on next-20170119]
[also build test ERROR on v4.10-rc4]
[cannot apply to v4.9-rc8 v4.9-rc7 v4.9-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/SF-Markus-Elfring/PowerPC-KVM-Fine-tuning-for-some-function-implementations/20170121-121537
config: powerpc-defconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=powerpc
All errors (new ones prefixed by >>):
arch/powerpc/kvm/book3s_xics.c: In function 'kvmppc_xics_create_ics':
arch/powerpc/kvm/book3s_xics.c:1035:16: error: incompatible type for argument 1 of 'kzalloc'
ics = kzalloc(*ics), GFP_KERNEL);
^
In file included from include/linux/kvm_host.h:21:0,
from arch/powerpc/kvm/book3s_xics.c:11:
include/linux/slab.h:634:21: note: expected 'size_t {aka long unsigned int}' but argument is of type 'struct kvmppc_ics'
static inline void *kzalloc(size_t size, gfp_t flags)
^~~~~~~
arch/powerpc/kvm/book3s_xics.c:1035:8: error: too few arguments to function 'kzalloc'
ics = kzalloc(*ics), GFP_KERNEL);
^~~~~~~
In file included from include/linux/kvm_host.h:21:0,
from arch/powerpc/kvm/book3s_xics.c:11:
include/linux/slab.h:634:21: note: declared here
static inline void *kzalloc(size_t size, gfp_t flags)
^~~~~~~
quoted
arch/powerpc/kvm/book3s_xics.c:1035:21: error: left-hand operand of comma expression has no effect [-Werror=unused-value]
ics = kzalloc(*ics), GFP_KERNEL);
^
quoted
arch/powerpc/kvm/book3s_xics.c:1035:21: error: statement with no effect [-Werror=unused-value]
arch/powerpc/kvm/book3s_xics.c:1035:33: error: expected ';' before ')' token
ics = kzalloc(*ics), GFP_KERNEL);
^
arch/powerpc/kvm/book3s_xics.c:1035:33: error: expected statement before ')' token
cc1: all warnings being treated as errors
vim +1035 arch/powerpc/kvm/book3s_xics.c
1029
1030 /* ICS already exists - somebody else got here first */
1031 if (xics->ics[icsid])
1032 goto out;
1033
1034 /* Create the ICS */
From: Paolo Bonzini <pbonzini@redhat.com> Date: 2017-01-23 14:13:45
On 20/01/2017 19:19, SF Markus Elfring wrote:
From: Markus Elfring <redacted>
Date: Thu, 19 Jan 2017 22:45:56 +0100
* A local variable was set to an error code in five cases
before a concrete error situation was detected.
Thus move the corresponding assignments into
if branches to indicate a software failure there.
This issue was detected by using the Coccinelle software.
* Delete two zero assignments which became unnecessary with
this refactoring.
Signed-off-by: Markus Elfring <redacted>
From: Paolo Bonzini <pbonzini@redhat.com> Date: 2017-01-23 14:13:46
On 20/01/2017 19:21, SF Markus Elfring wrote:
From: Markus Elfring <redacted>
Date: Fri, 20 Jan 2017 10:30:26 +0100
A local variable was set to an error code in a few cases before a concrete
error situation was detected. Thus move the corresponding assignments into
if branches to indicate a software failure there.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <redacted>
---
arch/powerpc/kvm/book3s_hv.c | 25 +++++++++++++++----------
1 file changed, 15 insertions(+), 10 deletions(-)
@@ -2967,15 +2967,17 @@ static int kvm_vm_ioctl_get_dirty_log_hv(struct kvm *kvm,mutex_lock(&kvm->slots_lock);-r=-EINVAL;-if(log->slot>=KVM_USER_MEM_SLOTS)+if(log->slot>=KVM_USER_MEM_SLOTS){+r=-EINVAL;gotoout;+}slots=kvm_memslots(kvm);memslot=id_to_memslot(slots,log->slot);-r=-ENOENT;-if(!memslot->dirty_bitmap)+if(!memslot->dirty_bitmap){+r=-ENOENT;gotoout;+}n=kvm_dirty_bitmap_bytes(memslot);memset(memslot->dirty_bitmap,0,n);
@@ -2984,9 +2986,10 @@ static int kvm_vm_ioctl_get_dirty_log_hv(struct kvm *kvm,if(r)gotoout;-r=-EFAULT;-if(copy_to_user(log->dirty_bitmap,memslot->dirty_bitmap,n))+if(copy_to_user(log->dirty_bitmap,memslot->dirty_bitmap,n)){+r=-EFAULT;gotoout;+}r=0;out:
@@ -3127,9 +3130,10 @@ static int kvmppc_hv_setup_htab_rma(struct kvm_vcpu *vcpu)memslot=gfn_to_memslot(kvm,0);/* We must have some memory at 0 by now */-err=-EINVAL;-if(!memslot||(memslot->flags&KVM_MEMSLOT_INVALID))+if(!memslot||(memslot->flags&KVM_MEMSLOT_INVALID)){+err=-EINVAL;gotoout_srcu;+}/* Look up the VMA for the start of this memory slot */hva=memslot->userspace_addr;
@@ -3144,10 +3148,11 @@ static int kvmppc_hv_setup_htab_rma(struct kvm_vcpu *vcpu)up_read(¤t->mm->mmap_sem);/* We can handle 4k, 64k or 16M pages in the VRMA */-err=-EINVAL;if(!(psize==0x1000||psize==0x10000||-psize==0x1000000))+psize==0x1000000)){+err=-EINVAL;gotoout_srcu;+}senc=slb_pgsize_encoding(psize);kvm->arch.vrma_slb_v=senc|SLB_VSID_B_1T|
From: Thomas Huth <hidden> Date: 2017-01-23 19:22:19
On 20.01.2017 19:23, SF Markus Elfring wrote:
From: Markus Elfring <redacted>
Date: Fri, 20 Jan 2017 11:25:48 +0100
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The script "checkpatch.pl" pointed information out like the following.
Comparison to NULL could be written …
That's maybe ok for new code / if the code has to be touched anyway ...
but for existing code, this sounds very much like unnecessary code-churn
to me (e.g. it hides more important information with "git blame").
Thomas
@@ -3203,7 +3202,7 @@ void kvmppc_alloc_host_rm_ops(void)intsize;/* Not the first time here ? */-if(kvmppc_host_rm_ops_hv!=NULL)+if(kvmppc_host_rm_ops_hv)return;ops=kzalloc(sizeof(structkvmppc_host_rm_ops),GFP_KERNEL);
@@ -3430,10 +3429,10 @@ static int kvmppc_set_passthru_irq(struct kvm *kvm, int host_irq, int guest_gsi)mutex_lock(&kvm->lock);pimap=kvm->arch.pimap;-if(pimap==NULL){+if(!pimap){/* First call, allocate structure to hold IRQ map */pimap=kvmppc_alloc_pimap();-if(pimap==NULL){+if(!pimap){mutex_unlock(&kvm->lock);return-ENOMEM;}