Re: [PATCH] KVM: s390: Cleanup kvm_arch_init error path

David Hildenbrand
2019-10-02T18:21:21Z

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