Skip to contentSkip to conversation

KVM: s390: Cleanup kvm_arch_init error path

PATCH Applied Janosch Frank 4 participants 7 messages Oct 2, 2019

Subsystems (3)
KVM s390 KVM s390
lore
Share & tools
lore

Janosch Frank

author maintainer
#0 lore raw

PATCH 1/1 KVM: s390: Cleanup kvm_arch_init error path

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 <[email protected]>
---
 arch/s390/kvm/kvm-s390.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)
arch/s390/kvm/kvm-s390.c +7 -11
index 895fb2006c0d..66720d69cd24 100644
@@ -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;
 }
Signature · 1 line
2.20.1
Reviewed-by on v1 1/1
#1 lore raw
Quoted 45 lines · Janosch Frank

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 <[email protected]>
---
 arch/s390/kvm/kvm-s390.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)
arch/s390/kvm/kvm-s390.c +5 -7
index 895fb2006c0d..66720d69cd24 100644
@@ -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;

arch/s390/kvm/kvm-s390.c int kvm_arch_init(void *opaque) · line 488

Quoted 10 lines
arch/s390/kvm/kvm-s390.c +2 -4
 	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 <[email protected]>
Signature · 3 lines
Thanks,

David / dhildenb
Reviewed-by
#2 lore raw
Quoted 48 lines · David Hildenbrand
Quoted 47 lines · Janosch Frank

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 <[email protected]>
---
 arch/s390/kvm/kvm-s390.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)
arch/s390/kvm/kvm-s390.c +5 -7
index 895fb2006c0d..66720d69cd24 100644
@@ -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;
Quoted 10 lines

arch/s390/kvm/kvm-s390.c int kvm_arch_init(void *opaque) · line 488

Quoted 8 lines
arch/s390/kvm/kvm-s390.c +2 -4
-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?

Quoted 1 line

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 <[email protected]>
#3 lore raw
Quoted 60 lines · Thomas Huth
Quoted 48 lines · David Hildenbrand
Quoted 47 lines · Janosch Frank

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 <[email protected]>
---
 arch/s390/kvm/kvm-s390.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)
arch/s390/kvm/kvm-s390.c +5 -7
index 895fb2006c0d..66720d69cd24 100644
@@ -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;
Quoted 10 lines

arch/s390/kvm/kvm-s390.c int kvm_arch_init(void *opaque) · line 488

Quoted 8 lines
arch/s390/kvm/kvm-s390.c +2 -4
-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.

Quoted 1 line

Because it is used in the KVM_EVENT macro?

Ah, makes sense.

Quoted 4 lines
Quoted 2 lines

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 :)

Quoted 4 lines

To me, the patch is looking fine, so

Reviewed-by Thomas Huth <[email protected]>
Signature · 3 lines
Thanks,

David / dhildenb
#4 lore raw
Quoted 66 lines · David Hildenbrand
Quoted 60 lines · Thomas Huth
Quoted 48 lines · David Hildenbrand
Quoted 47 lines · Janosch Frank

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 <[email protected]>
---
 arch/s390/kvm/kvm-s390.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)
arch/s390/kvm/kvm-s390.c +5 -7
index 895fb2006c0d..66720d69cd24 100644
@@ -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;
Quoted 10 lines

arch/s390/kvm/kvm-s390.c int kvm_arch_init(void *opaque) · line 488

Quoted 8 lines
arch/s390/kvm/kvm-s390.c +2 -4
-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.

Quoted 1 line

Because it is used in the KVM_EVENT macro?

Ah, makes sense.

Quoted 1 line
Quoted 6 lines
Quoted 3 lines
Quoted 1 line

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? ;-)

Quoted 7 lines
Quoted 5 lines

To me, the patch is looking fine, so

Reviewed-by Thomas Huth <[email protected]>
#5 lore raw
Quoted 69 lines · Christian Borntraeger
Quoted 66 lines · David Hildenbrand
Quoted 60 lines · Thomas Huth
Quoted 48 lines · David Hildenbrand
Quoted 47 lines · Janosch Frank

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 <[email protected]>
---
 arch/s390/kvm/kvm-s390.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)
arch/s390/kvm/kvm-s390.c +5 -7
index 895fb2006c0d..66720d69cd24 100644
@@ -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;
Quoted 10 lines

arch/s390/kvm/kvm-s390.c int kvm_arch_init(void *opaque) · line 488

Quoted 8 lines
arch/s390/kvm/kvm-s390.c +2 -4
-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.

Quoted 1 line

Because it is used in the KVM_EVENT macro?

Ah, makes sense.

Quoted 1 line
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Quoted 1 line

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. ;)

Signature · 3 lines
Thanks,

David / dhildenb
Applied
#6 lore raw
In reply to Janosch Frank
Quoted 4 lines · Janosch Frank

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 <[email protected]>

Thanks applied.

Quoted 53 lines
---
 arch/s390/kvm/kvm-s390.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)
arch/s390/kvm/kvm-s390.c +7 -11
index 895fb2006c0d..66720d69cd24 100644
@@ -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;
 }