From: Markus Elfring <[email protected]>
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
From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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
From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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
On 06/29/2015 02:24 AM, SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> 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 <[email protected]>
> ---
> 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
> 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
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
>>> 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
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