| Message ID | 20191002075627.3582-1-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: s390: Cleanup kvm_arch_init error path | expand |
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>
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>
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> >
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> >> > >
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. ;)
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; > } > >
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; }
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(-)