On Fri, 16 Sep 2016 15:56:07 +0200
Laurent Vivier <lviv...@redhat.com> wrote:
> This patch is the result of coccinelle script
> scripts/coccinelle/exit.cocci
As stated in my other reply, I'm not convinced that this conversion is
useful, but I did take a look at the exit()s we have in here:
>
> Signed-off-by: Laurent Vivier <lviv...@redhat.com>
> CC: Cornelia Huck <cornelia.h...@de.ibm.com>
> ---
> target-s390x/cpu.c | 2 +-
> target-s390x/kvm.c | 18 +++++++++---------
> target-s390x/mmu_helper.c | 2 +-
> 3 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 2f3c8e2..d4ca6af 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -385,7 +385,7 @@ unsigned int s390_cpu_set_state(uint8_t cpu_state,
> S390CPU *cpu)
> default:
> error_report("Requested CPU state is not a valid S390 CPU state: %u",
> cpu_state);
> - exit(1);
Can only be hit via programming error in qemu and exiting is the only
thing that really makes sense.
> + exit(EXIT_FAILURE);
> }
> if (kvm_enabled() && cpu->env.cpu_state != cpu_state) {
> kvm_s390_set_cpu_state(cpu, cpu_state);
> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> index dfaf1ca..2b1b908 100644
> --- a/target-s390x/kvm.c
> +++ b/target-s390x/kvm.c
> @@ -920,13 +920,13 @@ static void inject_vcpu_irq_legacy(CPUState *cs, struct
> kvm_s390_irq *irq)
> r = s390_kvm_irq_to_interrupt(irq, &kvmint);
> if (r < 0) {
> fprintf(stderr, "%s called with bogus interrupt\n", __func__);
> - exit(1);
Dito, but we probably should convert the fprintf to error_report().
> + exit(EXIT_FAILURE);
> }
>
> r = kvm_vcpu_ioctl(cs, KVM_S390_INTERRUPT, &kvmint);
> if (r < 0) {
> fprintf(stderr, "KVM failed to inject interrupt\n");
> - exit(1);
There's several reasons why we could hit this:
- qemu programming error
- the kvm module can't get memory... we're probably already dead
- we've hit some internal limit in the kvm module... should not happen
and is probably a follow-on of a programming error in qemu as well
So I think the exit is fine, but we should still convert to
error_report().
> + exit(EXIT_FAILURE);
> }
> }
>
> @@ -941,7 +941,7 @@ void kvm_s390_vcpu_interrupt(S390CPU *cpu, struct
> kvm_s390_irq *irq)
> return;
> }
> error_report("KVM failed to inject interrupt %llx", irq->type);
> - exit(1);
Dito as for the old injection interface.
> + exit(EXIT_FAILURE);
> }
>
> inject_vcpu_irq_legacy(cs, irq);
> @@ -955,13 +955,13 @@ static void __kvm_s390_floating_interrupt(struct
> kvm_s390_irq *irq)
> r = s390_kvm_irq_to_interrupt(irq, &kvmint);
> if (r < 0) {
> fprintf(stderr, "%s called with bogus interrupt\n", __func__);
> - exit(1);
Same as for vcpu interrupts.
> + exit(EXIT_FAILURE);
> }
>
> r = kvm_vm_ioctl(kvm_state, KVM_S390_INTERRUPT, &kvmint);
> if (r < 0) {
> fprintf(stderr, "KVM failed to inject interrupt\n");
> - exit(1);
And here as well.
> + exit(EXIT_FAILURE);
> }
> }
>
> @@ -1905,15 +1905,15 @@ static int handle_intercept(S390CPU *cpu)
> break;
> case ICPT_SOFT_INTERCEPT:
> fprintf(stderr, "KVM unimplemented icpt SOFT\n");
> - exit(1);
> + exit(EXIT_FAILURE);
> break;
> case ICPT_IO:
> fprintf(stderr, "KVM unimplemented icpt IO\n");
> - exit(1);
> + exit(EXIT_FAILURE);
> break;
> default:
> fprintf(stderr, "Unknown intercept code: %d\n", icpt_code);
> - exit(1);
> + exit(EXIT_FAILURE);
What happened here is that we intercepted with an intercept that we
either don't expect to get or we don't know about. In either case, this
is something the kvm module should have avoided/handled and exit()ing
is probably the best thing to do here as well (or implement support for
an intercept). But this should probably use error_report() as well.
> break;
> }
>
> @@ -2222,7 +2222,7 @@ int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t
> cpu_state)
> default:
> error_report("Requested CPU state is not a valid S390 CPU state: %u",
> cpu_state);
> - exit(1);
qemu programming error
> + exit(EXIT_FAILURE);
> }
>
> ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MP_STATE, &mp_state);
> diff --git a/target-s390x/mmu_helper.c b/target-s390x/mmu_helper.c
> index b11a027..dc9960a 100644
> --- a/target-s390x/mmu_helper.c
> +++ b/target-s390x/mmu_helper.c
> @@ -414,7 +414,7 @@ static bool lowprot_enabled(const CPUS390XState *env)
> default:
> /* We don't support access register mode */
> error_report("unsupported addressing mode");
> - exit(1);
Not implemented - until someone does implement it, exit() looks like
the best way out.
> + exit(EXIT_FAILURE);
> }
> }
>
All in all, I think our usage of exit() is fine, but some more
error_report() conversions should be done. On the todo list.
