From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36837) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1blwQG-0002Dy-Se for qemu-devel@nongnu.org; Mon, 19 Sep 2016 07:07:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1blwQC-00044y-B2 for qemu-devel@nongnu.org; Mon, 19 Sep 2016 07:07:39 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:39582) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1blwQC-00044g-3T for qemu-devel@nongnu.org; Mon, 19 Sep 2016 07:07:36 -0400 Received: from pps.filterd (m0098410.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.17/8.16.0.17) with SMTP id u8JB4IfK068191 for ; Mon, 19 Sep 2016 07:07:35 -0400 Received: from e06smtp13.uk.ibm.com (e06smtp13.uk.ibm.com [195.75.94.109]) by mx0a-001b2d01.pphosted.com with ESMTP id 25h2cfcwse-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Mon, 19 Sep 2016 07:07:34 -0400 Received: from localhost by e06smtp13.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 19 Sep 2016 12:07:32 +0100 Date: Mon, 19 Sep 2016 13:07:26 +0200 From: Cornelia Huck In-Reply-To: <1474034177-17663-17-git-send-email-lvivier@redhat.com> References: <1474034177-17663-1-git-send-email-lvivier@redhat.com> <1474034177-17663-17-git-send-email-lvivier@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-Id: <20160919130726.63b5cb24.cornelia.huck@de.ibm.com> Subject: Re: [Qemu-devel] [PATCH 16/26] s390: use exit(EXIT_SUCCESS) and exit(EXIT_FAILURE) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laurent Vivier Cc: qemu-trivial@nongnu.org, qemu-devel@nongnu.org On Fri, 16 Sep 2016 15:56:07 +0200 Laurent Vivier 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 > CC: Cornelia Huck > --- > 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.