* [PATCH] KVM: s390: Cleanup kvm_arch_init error path
@ 2019-10-02 7:56 Janosch Frank
2019-10-02 8:01 ` David Hildenbrand
2019-10-02 18:32 ` Christian Borntraeger
0 siblings, 2 replies; 7+ messages in thread
From: Janosch Frank @ 2019-10-02 7:56 UTC (permalink / raw)
To: kvm; +Cc: linux-s390, borntraeger, david
Both kvm_s390_gib_destroy and debug_unregister test if the needed
pointers are not NULL and hence can be called unconditionally.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
arch/s390/kvm/kvm-s390.c | 18 +++++++-----------
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 895fb2006c0d..66720d69cd24 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -458,16 +458,14 @@ static void kvm_s390_cpu_feat_init(void)
int kvm_arch_init(void *opaque)
{
- int rc;
+ int rc = -ENOMEM;
kvm_s390_dbf = debug_register("kvm-trace", 32, 1, 7 * sizeof(long));
if (!kvm_s390_dbf)
return -ENOMEM;
- if (debug_register_view(kvm_s390_dbf, &debug_sprintf_view)) {
- rc = -ENOMEM;
- goto out_debug_unreg;
- }
+ if (debug_register_view(kvm_s390_dbf, &debug_sprintf_view))
+ goto out;
kvm_s390_cpu_feat_init();
@@ -475,19 +473,17 @@ int kvm_arch_init(void *opaque)
rc = kvm_register_device_ops(&kvm_flic_ops, KVM_DEV_TYPE_FLIC);
if (rc) {
pr_err("A FLIC registration call failed with rc=%d\n", rc);
- goto out_debug_unreg;
+ goto out;
}
rc = kvm_s390_gib_init(GAL_ISC);
if (rc)
- goto out_gib_destroy;
+ goto out;
return 0;
-out_gib_destroy:
- kvm_s390_gib_destroy();
-out_debug_unreg:
- debug_unregister(kvm_s390_dbf);
+out:
+ kvm_arch_exit();
return rc;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] KVM: s390: Cleanup kvm_arch_init error path
2019-10-02 7:56 [PATCH] KVM: s390: Cleanup kvm_arch_init error path Janosch Frank
@ 2019-10-02 8:01 ` David Hildenbrand
2019-10-02 8:07 ` Thomas Huth
2019-10-02 18:32 ` Christian Borntraeger
1 sibling, 1 reply; 7+ messages in thread
From: David Hildenbrand @ 2019-10-02 8:01 UTC (permalink / raw)
To: Janosch Frank, kvm; +Cc: linux-s390, borntraeger
On 02.10.19 09:56, Janosch Frank wrote:
> Both kvm_s390_gib_destroy and debug_unregister test if the needed
> pointers are not NULL and hence can be called unconditionally.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> arch/s390/kvm/kvm-s390.c | 18 +++++++-----------
> 1 file changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 895fb2006c0d..66720d69cd24 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -458,16 +458,14 @@ static void kvm_s390_cpu_feat_init(void)
>
> int kvm_arch_init(void *opaque)
> {
> - int rc;
> + int rc = -ENOMEM;
>
> kvm_s390_dbf = debug_register("kvm-trace", 32, 1, 7 * sizeof(long));
> if (!kvm_s390_dbf)
> return -ENOMEM;
>
> - if (debug_register_view(kvm_s390_dbf, &debug_sprintf_view)) {
> - rc = -ENOMEM;
> - goto out_debug_unreg;
> - }
> + if (debug_register_view(kvm_s390_dbf, &debug_sprintf_view))
> + goto out;
>
> kvm_s390_cpu_feat_init();
>
> @@ -475,19 +473,17 @@ int kvm_arch_init(void *opaque)
> rc = kvm_register_device_ops(&kvm_flic_ops, KVM_DEV_TYPE_FLIC);
> if (rc) {
> pr_err("A FLIC registration call failed with rc=%d\n", rc);
> - goto out_debug_unreg;
> + goto out;
> }
>
> rc = kvm_s390_gib_init(GAL_ISC);
> if (rc)
> - goto out_gib_destroy;
> + goto out;
>
> return 0;
>
> -out_gib_destroy:
> - kvm_s390_gib_destroy();
> -out_debug_unreg:
> - debug_unregister(kvm_s390_dbf);
> +out:
> + kvm_arch_exit();
> return rc;
> }
Wonder why "debug_info_t *kvm_s390_dbf" is not declared as static.
Instead of the two manual calls we could also call kvm_arch_exit().
Reviewed-by: David Hildenbrand <david@redhat.com>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] KVM: s390: Cleanup kvm_arch_init error path
2019-10-02 8:01 ` David Hildenbrand
@ 2019-10-02 8:07 ` Thomas Huth
2019-10-02 8:20 ` David Hildenbrand
0 siblings, 1 reply; 7+ messages in thread
From: Thomas Huth @ 2019-10-02 8:07 UTC (permalink / raw)
To: David Hildenbrand, Janosch Frank, kvm; +Cc: linux-s390, borntraeger
On 02/10/2019 10.01, David Hildenbrand wrote:
> On 02.10.19 09:56, Janosch Frank wrote:
>> Both kvm_s390_gib_destroy and debug_unregister test if the needed
>> pointers are not NULL and hence can be called unconditionally.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>> arch/s390/kvm/kvm-s390.c | 18 +++++++-----------
>> 1 file changed, 7 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index 895fb2006c0d..66720d69cd24 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -458,16 +458,14 @@ static void kvm_s390_cpu_feat_init(void)
>>
>> int kvm_arch_init(void *opaque)
>> {
>> - int rc;
>> + int rc = -ENOMEM;
>>
>> kvm_s390_dbf = debug_register("kvm-trace", 32, 1, 7 * sizeof(long));
>> if (!kvm_s390_dbf)
>> return -ENOMEM;
>>
>> - if (debug_register_view(kvm_s390_dbf, &debug_sprintf_view)) {
>> - rc = -ENOMEM;
>> - goto out_debug_unreg;
>> - }
>> + if (debug_register_view(kvm_s390_dbf, &debug_sprintf_view))
>> + goto out;
>>
>> kvm_s390_cpu_feat_init();
>>
>> @@ -475,19 +473,17 @@ int kvm_arch_init(void *opaque)
>> rc = kvm_register_device_ops(&kvm_flic_ops, KVM_DEV_TYPE_FLIC);
>> if (rc) {
>> pr_err("A FLIC registration call failed with rc=%d\n", rc);
>> - goto out_debug_unreg;
>> + goto out;
>> }
>>
>> rc = kvm_s390_gib_init(GAL_ISC);
>> if (rc)
>> - goto out_gib_destroy;
>> + goto out;
>>
>> return 0;
>>
>> -out_gib_destroy:
>> - kvm_s390_gib_destroy();
>> -out_debug_unreg:
>> - debug_unregister(kvm_s390_dbf);
>> +out:
>> + kvm_arch_exit();
>> return rc;
>> }
>
> Wonder why "debug_info_t *kvm_s390_dbf" is not declared as static.
Because it is used in the KVM_EVENT macro?
> Instead of the two manual calls we could also call kvm_arch_exit().
Huh, isn't that what this patch is doing here?
To me, the patch is looking fine, so
Reviewed-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] KVM: s390: Cleanup kvm_arch_init error path
2019-10-02 8:07 ` Thomas Huth
@ 2019-10-02 8:20 ` David Hildenbrand
2019-10-02 10:45 ` Christian Borntraeger
0 siblings, 1 reply; 7+ messages in thread
From: David Hildenbrand @ 2019-10-02 8:20 UTC (permalink / raw)
To: Thomas Huth, Janosch Frank, kvm; +Cc: linux-s390, borntraeger
On 02.10.19 10:07, Thomas Huth wrote:
> On 02/10/2019 10.01, David Hildenbrand wrote:
>> On 02.10.19 09:56, Janosch Frank wrote:
>>> Both kvm_s390_gib_destroy and debug_unregister test if the needed
>>> pointers are not NULL and hence can be called unconditionally.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>> arch/s390/kvm/kvm-s390.c | 18 +++++++-----------
>>> 1 file changed, 7 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>> index 895fb2006c0d..66720d69cd24 100644
>>> --- a/arch/s390/kvm/kvm-s390.c
>>> +++ b/arch/s390/kvm/kvm-s390.c
>>> @@ -458,16 +458,14 @@ static void kvm_s390_cpu_feat_init(void)
>>>
>>> int kvm_arch_init(void *opaque)
>>> {
>>> - int rc;
>>> + int rc = -ENOMEM;
>>>
>>> kvm_s390_dbf = debug_register("kvm-trace", 32, 1, 7 * sizeof(long));
>>> if (!kvm_s390_dbf)
>>> return -ENOMEM;
>>>
>>> - if (debug_register_view(kvm_s390_dbf, &debug_sprintf_view)) {
>>> - rc = -ENOMEM;
>>> - goto out_debug_unreg;
>>> - }
>>> + if (debug_register_view(kvm_s390_dbf, &debug_sprintf_view))
>>> + goto out;
>>>
>>> kvm_s390_cpu_feat_init();
>>>
>>> @@ -475,19 +473,17 @@ int kvm_arch_init(void *opaque)
>>> rc = kvm_register_device_ops(&kvm_flic_ops, KVM_DEV_TYPE_FLIC);
>>> if (rc) {
>>> pr_err("A FLIC registration call failed with rc=%d\n", rc);
>>> - goto out_debug_unreg;
>>> + goto out;
>>> }
>>>
>>> rc = kvm_s390_gib_init(GAL_ISC);
>>> if (rc)
>>> - goto out_gib_destroy;
>>> + goto out;
>>>
>>> return 0;
>>>
>>> -out_gib_destroy:
>>> - kvm_s390_gib_destroy();
>>> -out_debug_unreg:
>>> - debug_unregister(kvm_s390_dbf);
>>> +out:
>>> + kvm_arch_exit();
>>> return rc;
>>> }
>>
>> Wonder why "debug_info_t *kvm_s390_dbf" is not declared as static.
>
> Because it is used in the KVM_EVENT macro?
Ah, makes sense.
>
>> Instead of the two manual calls we could also call kvm_arch_exit().
>
> Huh, isn't that what this patch is doing here?
Lol, still tired and thought only the two labels would get removed. Even
better :)
>
> To me, the patch is looking fine, so
> Reviewed-by: Thomas Huth <thuth@redhat.com>
>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] KVM: s390: Cleanup kvm_arch_init error path
2019-10-02 8:20 ` David Hildenbrand
@ 2019-10-02 10:45 ` Christian Borntraeger
2019-10-02 18:21 ` David Hildenbrand
0 siblings, 1 reply; 7+ messages in thread
From: Christian Borntraeger @ 2019-10-02 10:45 UTC (permalink / raw)
To: David Hildenbrand, Thomas Huth, Janosch Frank, kvm; +Cc: linux-s390
On 02.10.19 10:20, David Hildenbrand wrote:
> On 02.10.19 10:07, Thomas Huth wrote:
>> On 02/10/2019 10.01, David Hildenbrand wrote:
>>> On 02.10.19 09:56, Janosch Frank wrote:
>>>> Both kvm_s390_gib_destroy and debug_unregister test if the needed
>>>> pointers are not NULL and hence can be called unconditionally.
>>>>
>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>> ---
>>>> arch/s390/kvm/kvm-s390.c | 18 +++++++-----------
>>>> 1 file changed, 7 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>>> index 895fb2006c0d..66720d69cd24 100644
>>>> --- a/arch/s390/kvm/kvm-s390.c
>>>> +++ b/arch/s390/kvm/kvm-s390.c
>>>> @@ -458,16 +458,14 @@ static void kvm_s390_cpu_feat_init(void)
>>>>
>>>> int kvm_arch_init(void *opaque)
>>>> {
>>>> - int rc;
>>>> + int rc = -ENOMEM;
>>>>
>>>> kvm_s390_dbf = debug_register("kvm-trace", 32, 1, 7 * sizeof(long));
>>>> if (!kvm_s390_dbf)
>>>> return -ENOMEM;
>>>>
>>>> - if (debug_register_view(kvm_s390_dbf, &debug_sprintf_view)) {
>>>> - rc = -ENOMEM;
>>>> - goto out_debug_unreg;
>>>> - }
>>>> + if (debug_register_view(kvm_s390_dbf, &debug_sprintf_view))
>>>> + goto out;
>>>>
>>>> kvm_s390_cpu_feat_init();
>>>>
>>>> @@ -475,19 +473,17 @@ int kvm_arch_init(void *opaque)
>>>> rc = kvm_register_device_ops(&kvm_flic_ops, KVM_DEV_TYPE_FLIC);
>>>> if (rc) {
>>>> pr_err("A FLIC registration call failed with rc=%d\n", rc);
>>>> - goto out_debug_unreg;
>>>> + goto out;
>>>> }
>>>>
>>>> rc = kvm_s390_gib_init(GAL_ISC);
>>>> if (rc)
>>>> - goto out_gib_destroy;
>>>> + goto out;
>>>>
>>>> return 0;
>>>>
>>>> -out_gib_destroy:
>>>> - kvm_s390_gib_destroy();
>>>> -out_debug_unreg:
>>>> - debug_unregister(kvm_s390_dbf);
>>>> +out:
>>>> + kvm_arch_exit();
>>>> return rc;
>>>> }
>>>
>>> Wonder why "debug_info_t *kvm_s390_dbf" is not declared as static.
>>
>> Because it is used in the KVM_EVENT macro?
>
> Ah, makes sense.
>
>>
>>> Instead of the two manual calls we could also call kvm_arch_exit().
>>
>> Huh, isn't that what this patch is doing here?
>
> Lol, still tired and thought only the two labels would get removed. Even
> better :)
So I guess we should not take your Reviewed-by: then? ;-)
>
>>
>> To me, the patch is looking fine, so
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] KVM: s390: Cleanup kvm_arch_init error path
2019-10-02 10:45 ` Christian Borntraeger
@ 2019-10-02 18:21 ` David Hildenbrand
0 siblings, 0 replies; 7+ messages in thread
From: David Hildenbrand @ 2019-10-02 18:21 UTC (permalink / raw)
To: Christian Borntraeger, Thomas Huth, Janosch Frank, kvm; +Cc: linux-s390
On 02.10.19 12:45, Christian Borntraeger wrote:
>
>
> On 02.10.19 10:20, David Hildenbrand wrote:
>> On 02.10.19 10:07, Thomas Huth wrote:
>>> On 02/10/2019 10.01, David Hildenbrand wrote:
>>>> On 02.10.19 09:56, Janosch Frank wrote:
>>>>> Both kvm_s390_gib_destroy and debug_unregister test if the needed
>>>>> pointers are not NULL and hence can be called unconditionally.
>>>>>
>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>>> ---
>>>>> arch/s390/kvm/kvm-s390.c | 18 +++++++-----------
>>>>> 1 file changed, 7 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>>>> index 895fb2006c0d..66720d69cd24 100644
>>>>> --- a/arch/s390/kvm/kvm-s390.c
>>>>> +++ b/arch/s390/kvm/kvm-s390.c
>>>>> @@ -458,16 +458,14 @@ static void kvm_s390_cpu_feat_init(void)
>>>>>
>>>>> int kvm_arch_init(void *opaque)
>>>>> {
>>>>> - int rc;
>>>>> + int rc = -ENOMEM;
>>>>>
>>>>> kvm_s390_dbf = debug_register("kvm-trace", 32, 1, 7 * sizeof(long));
>>>>> if (!kvm_s390_dbf)
>>>>> return -ENOMEM;
>>>>>
>>>>> - if (debug_register_view(kvm_s390_dbf, &debug_sprintf_view)) {
>>>>> - rc = -ENOMEM;
>>>>> - goto out_debug_unreg;
>>>>> - }
>>>>> + if (debug_register_view(kvm_s390_dbf, &debug_sprintf_view))
>>>>> + goto out;
>>>>>
>>>>> kvm_s390_cpu_feat_init();
>>>>>
>>>>> @@ -475,19 +473,17 @@ int kvm_arch_init(void *opaque)
>>>>> rc = kvm_register_device_ops(&kvm_flic_ops, KVM_DEV_TYPE_FLIC);
>>>>> if (rc) {
>>>>> pr_err("A FLIC registration call failed with rc=%d\n", rc);
>>>>> - goto out_debug_unreg;
>>>>> + goto out;
>>>>> }
>>>>>
>>>>> rc = kvm_s390_gib_init(GAL_ISC);
>>>>> if (rc)
>>>>> - goto out_gib_destroy;
>>>>> + goto out;
>>>>>
>>>>> return 0;
>>>>>
>>>>> -out_gib_destroy:
>>>>> - kvm_s390_gib_destroy();
>>>>> -out_debug_unreg:
>>>>> - debug_unregister(kvm_s390_dbf);
>>>>> +out:
>>>>> + kvm_arch_exit();
>>>>> return rc;
>>>>> }
>>>>
>>>> Wonder why "debug_info_t *kvm_s390_dbf" is not declared as static.
>>>
>>> Because it is used in the KVM_EVENT macro?
>>
>> Ah, makes sense.
>>
>>>
>>>> Instead of the two manual calls we could also call kvm_arch_exit().
>>>
>>> Huh, isn't that what this patch is doing here?
>>
>> Lol, still tired and thought only the two labels would get removed. Even
>> better :)
>
> So I guess we should not take your Reviewed-by: then? ;-)
No, please take it. ;)
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] KVM: s390: Cleanup kvm_arch_init error path
2019-10-02 7:56 [PATCH] KVM: s390: Cleanup kvm_arch_init error path Janosch Frank
2019-10-02 8:01 ` David Hildenbrand
@ 2019-10-02 18:32 ` Christian Borntraeger
1 sibling, 0 replies; 7+ messages in thread
From: Christian Borntraeger @ 2019-10-02 18:32 UTC (permalink / raw)
To: Janosch Frank, kvm; +Cc: linux-s390, david
On 02.10.19 09:56, Janosch Frank wrote:
> Both kvm_s390_gib_destroy and debug_unregister test if the needed
> pointers are not NULL and hence can be called unconditionally.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Thanks applied.
> ---
> arch/s390/kvm/kvm-s390.c | 18 +++++++-----------
> 1 file changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 895fb2006c0d..66720d69cd24 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -458,16 +458,14 @@ static void kvm_s390_cpu_feat_init(void)
>
> int kvm_arch_init(void *opaque)
> {
> - int rc;
> + int rc = -ENOMEM;
>
> kvm_s390_dbf = debug_register("kvm-trace", 32, 1, 7 * sizeof(long));
> if (!kvm_s390_dbf)
> return -ENOMEM;
>
> - if (debug_register_view(kvm_s390_dbf, &debug_sprintf_view)) {
> - rc = -ENOMEM;
> - goto out_debug_unreg;
> - }
> + if (debug_register_view(kvm_s390_dbf, &debug_sprintf_view))
> + goto out;
>
> kvm_s390_cpu_feat_init();
>
> @@ -475,19 +473,17 @@ int kvm_arch_init(void *opaque)
> rc = kvm_register_device_ops(&kvm_flic_ops, KVM_DEV_TYPE_FLIC);
> if (rc) {
> pr_err("A FLIC registration call failed with rc=%d\n", rc);
> - goto out_debug_unreg;
> + goto out;
> }
>
> rc = kvm_s390_gib_init(GAL_ISC);
> if (rc)
> - goto out_gib_destroy;
> + goto out;
>
> return 0;
>
> -out_gib_destroy:
> - kvm_s390_gib_destroy();
> -out_debug_unreg:
> - debug_unregister(kvm_s390_dbf);
> +out:
> + kvm_arch_exit();
> return rc;
> }
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-10-02 18:33 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-10-02 7:56 [PATCH] KVM: s390: Cleanup kvm_arch_init error path Janosch Frank
2019-10-02 8:01 ` David Hildenbrand
2019-10-02 8:07 ` Thomas Huth
2019-10-02 8:20 ` David Hildenbrand
2019-10-02 10:45 ` Christian Borntraeger
2019-10-02 18:21 ` David Hildenbrand
2019-10-02 18:32 ` Christian Borntraeger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox