KVM: s390: Cleanup kvm_arch_init error path
diff mbox series

Message ID 20191002075627.3582-1-frankja@linux.ibm.com
State New
Headers show
Series
  • KVM: s390: Cleanup kvm_arch_init error path
Related show

Commit Message

Janosch Frank Oct. 2, 2019, 7:56 a.m. UTC
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(-)

Comments

David Hildenbrand Oct. 2, 2019, 8:01 a.m. UTC | #1
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>
Thomas Huth Oct. 2, 2019, 8:07 a.m. UTC | #2
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>
David Hildenbrand Oct. 2, 2019, 8:20 a.m. UTC | #3
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>
>
Christian Borntraeger Oct. 2, 2019, 10:45 a.m. UTC | #4
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>
>>
> 
>
David Hildenbrand Oct. 2, 2019, 6:21 p.m. UTC | #5
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. ;)
Christian Borntraeger Oct. 2, 2019, 6:32 p.m. UTC | #6
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;
>  }
>  
>

Patch
diff mbox series

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;
 }