[PATCH 0/2] vfio: powerpc/spapr: Deletion of an unnecessary check

[PATCH 0/2] vfio: powerpc/spapr: Deletion of an unnecessary check

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>

Another update suggestion was taken into account after a patch was applied
from static source code analysis.

Markus Elfring (2):
  Delete an unnecessary check before the function call "kfree"
  One function call less in tce_iommu_attach_group() after kzalloc() failure

 drivers/vfio/vfio_iommu_spapr_tce.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

-- 
2.4.4

[PATCH 1/2] vfio: powerpc/spapr: Delete an unnecessary check before the function call "kfree"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 28 Jun 2015 17:43:48 +0200

The kfree() function tests whether its argument is NULL and then
returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

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

diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
index 0582b72..50ddfac 100644
--- a/drivers/vfio/vfio_iommu_spapr_tce.c
+++ b/drivers/vfio/vfio_iommu_spapr_tce.c
@@ -1215,7 +1215,7 @@ static int tce_iommu_attach_group(void *iommu_data,
 	}
 
 unlock_exit:
-	if (ret && tcegrp)
+	if (ret)
 		kfree(tcegrp);
 
 	mutex_unlock(&container->lock);
-- 
2.4.4

[PATCH 2/2] vfio: powerpc/spapr: One function call less in tce_iommu_attach_group() after kzalloc() failure

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 28 Jun 2015 17:58:42 +0200

The kfree() function was called even if a previous memory allocation
try failed.

This implementation detail could be improved by the introduction
of another jump label.

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

diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
index 50ddfac..2523075 100644
--- a/drivers/vfio/vfio_iommu_spapr_tce.c
+++ b/drivers/vfio/vfio_iommu_spapr_tce.c
@@ -1200,7 +1200,7 @@ static int tce_iommu_attach_group(void *iommu_data,
 	tcegrp = kzalloc(sizeof(*tcegrp), GFP_KERNEL);
 	if (!tcegrp) {
 		ret = -ENOMEM;
-		goto unlock_exit;
+		goto unlock_container;
 	}
 
 	if (!table_group->ops || !table_group->ops->take_ownership ||
@@ -1217,7 +1217,7 @@ static int tce_iommu_attach_group(void *iommu_data,
 unlock_exit:
 	if (ret)
 		kfree(tcegrp);
-
+unlock_container:
 	mutex_unlock(&container->lock);
 
 	return ret;
-- 
2.4.4

Re: [PATCH 2/2] vfio: powerpc/spapr: One function call less in tce_iommu_attach_group() after kzalloc() failure

[Alexey Kardashevskiy]

On 06/29/2015 02:24 AM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 28 Jun 2015 17:58:42 +0200
>
> The kfree() function was called even if a previous memory allocation
> try failed.

tcegrp will be NULL and kfree() can handle this just fine (is not it the 
whole point of this patchset - remove the check and just call kfree() even 
if the pointer is NULL?). And if you wanted another label, than the 
existing one should have been renamed to "free_exit" or "free_unlock_exit" 
and new one would be "unlock_exit".



> This implementation detail could be improved by the introduction
> of another jump label.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>   drivers/vfio/vfio_iommu_spapr_tce.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> index 50ddfac..2523075 100644
> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> @@ -1200,7 +1200,7 @@ static int tce_iommu_attach_group(void *iommu_data,
>   	tcegrp = kzalloc(sizeof(*tcegrp), GFP_KERNEL);
>   	if (!tcegrp) {
>   		ret = -ENOMEM;
> -		goto unlock_exit;
> +		goto unlock_container;
>   	}
>
>   	if (!table_group->ops || !table_group->ops->take_ownership ||
> @@ -1217,7 +1217,7 @@ static int tce_iommu_attach_group(void *iommu_data,
>   unlock_exit:
>   	if (ret)
>   		kfree(tcegrp);
> -
> +unlock_container:
>   	mutex_unlock(&container->lock);
>
>   	return ret;
>


-- 
Alexey

Re: vfio: powerpc/spapr: One function call less in tce_iommu_attach_group() after kzalloc() failure

[SF Markus Elfring]

> tcegrp will be NULL and kfree() can handle this just fine

The affected function did not show this API knowledge, did it?


> (is not it the whole point of this patchset
> - remove the check and just call kfree() even if the pointer is NULL?).

Partly, yes.


> And if you wanted another label,

I suggest this to improve corresponding exception handling.


> than the existing one should have been renamed to "free_exit" or "free_unlock_exit"
> and new one would be "unlock_exit".

I chose a smaller change at this place.

I am not familiar enough with other called functions there at the moment.
Are the remaining goto statements also update candidates?

Regards,
Markus

Re: vfio: powerpc/spapr: One function call less in tce_iommu_attach_group() after kzalloc() failure

[Alexey Kardashevskiy]

On 06/29/2015 04:02 PM, SF Markus Elfring wrote:
>> tcegrp will be NULL and kfree() can handle this just fine
>
> The affected function did not show this API knowledge, did it?


but you fixed this in 1/2 :)

>
>
>> (is not it the whole point of this patchset
>> - remove the check and just call kfree() even if the pointer is NULL?).
>
> Partly, yes.
>
>
>> And if you wanted another label,
>
> I suggest this to improve corresponding exception handling.
>
>
>> than the existing one should have been renamed to "free_exit" or "free_unlock_exit"
>> and new one would be "unlock_exit".
>
> I chose a smaller change at this place.


I'd just drop this patch.


> I am not familiar enough with other called functions there at the moment.
> Are the remaining goto statements also update candidates?




-- 
Alexey

Re: vfio: powerpc/spapr: One function call less in tce_iommu_attach_group() after kzalloc() failure

[SF Markus Elfring]

>>> than the existing one should have been renamed to "free_exit" or "free_unlock_exit"
>>> and new one would be "unlock_exit".
>>
>> I chose a smaller change at this place.
> 
> I'd just drop this patch.

How do you think about to improve the affected jump labels
a bit more there?

Regards,
Markus

Re: vfio: powerpc/spapr: One function call less in tce_iommu_attach_group() after kzalloc() failure

[Alexey Kardashevskiy]

On 06/30/2015 04:08 PM, SF Markus Elfring wrote:
>>>> than the existing one should have been renamed to "free_exit" or "free_unlock_exit"
>>>> and new one would be "unlock_exit".
>>>
>>> I chose a smaller change at this place.
>>
>> I'd just drop this patch.
>
> How do you think about to improve the affected jump labels
> a bit more there?

This branch is very unlikely to work ever so I cannot think of any 
improvement here.



-- 
Alexey

[PATCH] KVM-async_pf: Delete an unnecessary check before the function call "kmem_cache_destroy"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 15 Nov 2015 10:40:36 +0100

The kmem_cache_destroy() function tests whether its argument is NULL
and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

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

diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c
index 77d42be..3531599 100644
--- a/virt/kvm/async_pf.c
+++ b/virt/kvm/async_pf.c
@@ -57,8 +57,7 @@ int kvm_async_pf_init(void)
 
 void kvm_async_pf_deinit(void)
 {
-	if (async_pf_cache)
-		kmem_cache_destroy(async_pf_cache);
+	kmem_cache_destroy(async_pf_cache);
 	async_pf_cache = NULL;
 }
 
-- 
2.6.2

Re: [PATCH] KVM-async_pf: Delete an unnecessary check before the function call "kmem_cache_destroy"

[Paolo Bonzini]

On 15/11/2015 10:45, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 15 Nov 2015 10:40:36 +0100
> 
> The kmem_cache_destroy() function tests whether its argument is NULL
> and then returns immediately. Thus the test around the call is not needed.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  virt/kvm/async_pf.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c
> index 77d42be..3531599 100644
> --- a/virt/kvm/async_pf.c
> +++ b/virt/kvm/async_pf.c
> @@ -57,8 +57,7 @@ int kvm_async_pf_init(void)
>  
>  void kvm_async_pf_deinit(void)
>  {
> -	if (async_pf_cache)
> -		kmem_cache_destroy(async_pf_cache);
> +	kmem_cache_destroy(async_pf_cache);
>  	async_pf_cache = NULL;
>  }
>  
> 

Applied to kvm/queue, thanks.

Paolo