utils: Improve and document error reporting
v5 cover utils: Improve and document error reporting
Adds leaner error-reporting functions for simple cases, and documents the purpose of the different facilities available in QEMU.
Although not all printf+exit/abort are replaced with the proper functions, a few are ported as an example.
Changes in v5
=============
* Fix typo in documentation [Eric Blake].
Changes in v4
=============
* Introduce 'error_report_fatal()' and 'error_report_abort()' functions [suggested by Thomas Huth].
* Repalce all existing uses of 'error_setg(error_fatal)' and 'error_setg(error_abort)' with 'error_report_fatal()' and 'error_report_abort()'.
* Replace all uses of 'exit()' with 'error_report_fatal()' in 'target-ppc'.
* Replace all uses of 'abort()' with 'error_report_abort()' in 'target-ppc'.
Changes in v3
=============
* Drop special object 'error_warn' in favour of raw 'error_report()' [suggested by Markus Armbruster].
Changes in v2
=============
* Split in two patches.
* Explicitly add a warning error object.
---
Lluís Vilanova (5):
util: Introduce error reporting functions with fatal/abort
util: Use new error_report_fatal/abort instead of error_setg(&error_fatal/abort)
util: [ppc] Use new error_report_fatal() instead of exit()
util: [ppc] Use new error_report_abort() instead of abort()
doc: Introduce coding style for errors
HACKING | 33 ++++++++++++++++++ hw/block/fdc.c | 6 ++- hw/ppc/spapr.c | 8 ++-- hw/ppc/spapr_drc.c | 2 + include/qemu/error-report.h | 19 ++++++++++ target-ppc/kvm.c | 9 ++--- target-ppc/kvm_ppc.h | 15 +++++--- target-ppc/mmu-hash32.c | 5 ++- target-ppc/mmu_helper.c | 3 +- target-ppc/translate.c | 7 ++-- target-ppc/translate_init.c | 80 +++++++++++++++++++++---------------------- util/error.c | 9 ++--- util/qemu-error.c | 33 ++++++++++++++++++ 13 files changed, 155 insertions(+), 74 deletions(-)
To: [email protected]
Cc: Stefan Hajnoczi <[email protected]>
Cc: Dr. David Alan Gilbert <[email protected]>
Cc: Thomas Huth <[email protected]>
Cc: Markus Armbruster <[email protected]>
Cc: Eric Blake <[email protected]>
PATCH v5 1/5 util: Introduce error reporting functions with fatal/abort
Provide two lean functions to report error messages that fatal/abort QEMU.
--- include/qemu/error-report.h | 19 +++++++++++++++++++ util/qemu-error.c | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+)
include/qemu/error-report.h +19 -0
@@ -43,4 +43,23 @@ void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);const char *error_get_progname (void );extern bool enable_timestamp_msg; +/* Report message and exit with error */ +void QEMU_NORETURN error_vreport_fatal(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0); +void QEMU_NORETURN error_report_fatal(const char *fmt, ...) GCC_FMT_ATTR(1, 2); +/* Report message with caller location and abort */ +#define error_vreport_abort (fmt, ap) \ + do { \ + error_report_abort_caller_internal(__FILE__, __LINE__, __func__); \ + error_vreport_abort_internal(fmt, ap); \ + } while (0) +#define error_report_abort (fmt, ...) \ + do { \ + error_report_abort_caller_internal(__FILE__, __LINE__, __func__); \ + error_report_abort_internal(fmt, ##__VA_ARGS__); \ + } while (0) + +void error_report_abort_caller_internal (const char *file,int line,const char *func); +void QEMU_NORETURN error_vreport_abort_internal(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0); +void QEMU_NORETURN error_report_abort_internal(const char *fmt, ...) GCC_FMT_ATTR(1, 2); +#endif
util/qemu-error.c +33 -0
@@ -237,3 +237,36 @@ void error_report(const char *fmt, ...)error_vreport (fmt, ap);va_end (ap); } + +void error_vreport_fatal (const char *fmt,va_list ap) +{ +error_vreport (fmt, ap); +exit (1 ); +} + +void error_report_fatal (const char *fmt, ...) +{ +va_list ap; +va_start (ap, fmt); +error_vreport_fatal (fmt, ap); +va_end (ap); +} + +void error_report_abort_caller_internal (const char *file,int line,const char *func) +{ +error_report ("Unexpected error in %s() at %s:%d:" , func, file, line); +} + +void error_vreport_abort_internal (const char *fmt,va_list ap) +{ +error_vreport (fmt, ap); +abort (); +} + +void error_report_abort_internal (const char *fmt, ...) +{ +va_list ap; +va_start (ap, fmt); +error_vreport_abort_internal (fmt, ap); +va_end (ap); +}
PATCH v5 2/5 util: Use new error_report_fatal/abort instead of error_setg(&error_fatal/abort)
In reply to Lluís VilanovaReplaces all direct uses of 'error_setg(&error_fatal/abort)' with 'error_report_fatal/abort'. Also reimplements the former on top of the latter.
--- hw/block/fdc.c | 6 +++--- hw/ppc/spapr.c | 8 ++++---- hw/ppc/spapr_drc.c | 2 +- util/error.c | 9 +++------ 4 files changed, 11 insertions(+), 14 deletions(-)
hw/block/fdc.c +3 -3
@@ -347,9 +347,9 @@ static int pick_geometry(FDrive *drv)/* No match of any kind found -- fd_format is misconfigured, abort. */ if (match ==-1 ) { -error_setg (&error_abort,"No candidate geometries present in table " -" for floppy drive type '%s'" , - FloppyDriveType_lookup[drv->drive]); +error_report_abort ("No candidate geometries present in table " +" for floppy drive type '%s'" , + FloppyDriveType_lookup[drv->drive]); } parse = &(fd_formats[match]);
hw/ppc/spapr.c +4 -4
@@ -1031,7 +1031,7 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr)* For HV KVM, host kernel will return -ENOMEM when requested * HTAB size can't be allocated. */ -error_setg (&error_abort,"Failed to allocate HTAB of requested size, try with smaller maxmem" ); +error_report_abort ("Failed to allocate HTAB of requested size, try with smaller maxmem" ); }else if (shift >0 ) {/* * Kernel handles htab, we don't need to allocate one
@@ -1040,7 +1040,7 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr)* but we don't allow booting of such guests. */ if (shift != spapr->htab_shift) { -error_setg (&error_abort,"Failed to allocate HTAB of requested size, try with smaller maxmem" ); +error_report_abort ("Failed to allocate HTAB of requested size, try with smaller maxmem" ); } spapr->htab_shift = shift;
@@ -1071,10 +1071,10 @@ static void spapr_reset_htab(sPAPRMachineState *spapr) shift =kvmppc_reset_htab (spapr->htab_shift);if (shift <0 ) { -error_setg (&error_abort,"Failed to reset HTAB" ); +error_report_abort ("Failed to reset HTAB" ); }else if (shift >0 ) {if (shift != spapr->htab_shift) { -error_setg (&error_abort,"Requested HTAB allocation failed during reset" ); +error_report_abort ("Requested HTAB allocation failed during reset" ); }/* Tell readers to update their file descriptor */
hw/ppc/spapr_drc.c +1 -1
@@ -322,7 +322,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, void *opaque,break ; }default : -error_setg (&error_abort,"device FDT in unexpected state: %d" , tag); +error_report_abort ("device FDT in unexpected state: %d" , tag); } fdt_offset = fdt_offset_next; }while (fdt_depth !=0 );
util/error.c +3 -6
@@ -30,15 +30,12 @@ Error *error_fatal;static void error_handle_fatal (Error **errp,Error *err) { +/* None of them has a hint, so error_report_err() is not necessary here */ if (errp == &error_abort) { -fprintf (stderr,"Unexpected error in %s() at %s:%d:\n" , - err->func, err->src, err->line); -error_report_err (err); -abort (); +error_report_abort_internal ("%s" , err->msg); }if (errp == &error_fatal) { -error_report_err (err); -exit (1 ); +error_report_fatal ("%s" , err->msg); } }
Re: [Qemu-devel] [PATCH v5 2/5] util: Use new error_report_fatal/abort instead of error_setg(&error_fatal/abort)
Quoted 5 lines · Lluís Vilanova
Replaces all direct uses of '
error_setg(&error_fatal/abort)' with 'error_report_fatal/abort'. Also reimplements the former on top of the latter.Signed-off-by Lluís Vilanova <[email protected]>
I think the spapr parts of this will be obsoleted by the cleanups to error handling included in the pull request I sent today.
Signature · 4 lines
David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
Re: [Qemu-devel] [PATCH v5 2/5] util: Use new error_report_fatal/abort instead of error_setg(&error_fatal/abort)
Quoted 9 lines · David Gibson
Quoted 5 lines · Lluís Vilanova
Replaces all direct uses of '
error_setg(&error_fatal/abort)' with 'error_report_fatal/abort'. Also reimplements the former on top of the latter.Signed-off-by Lluís Vilanova <[email protected]>I think the spapr parts of this will be obsoleted by the cleanups to error handling included in the pull request I sent today.
Ok, then I'll rebase once merged. Is there an ETA for the merge?
Thanks,
Lluis
Re: [Qemu-devel] [PATCH v5 2/5] util: Use new error_report_fatal/abort instead of error_setg(&error_fatal/abort)
Quoted 7 lines · Lluís Vilanova
Quoted 5 lines · David Gibson
Quoted 4 lines · Lluís Vilanova
Replaces all direct uses of '
error_setg(&error_fatal/abort)' with 'error_report_fatal/abort'. Also reimplements the former on top of the latter.
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Lluís Vilanova <[email protected]>I think the spapr parts of this will be obsoleted by the cleanups to error handling included in the pull request I sent today.
Ok, then I'll rebase once merged. Is there an ETA for the merge?
Well, I'd been hoping a couple of days ago, but there were some mistakes in the last pull request, so.. sometime this week, I hope?
Signature · 4 lines
David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
PATCH v5 3/5 util: [ppc] Use new error_report_fatal() instead of exit()
In reply to Lluís Vilanova--- target-ppc/kvm.c | 5 +-- target-ppc/translate.c | 7 ++-- target-ppc/translate_init.c | 80 +++++++++++++++++++++---------------------- 3 files changed, 44 insertions(+), 48 deletions(-)
target-ppc/kvm.c +2 -3
@@ -316,9 +316,8 @@ static long gethugepagesize(const char *mem_path) }while (ret !=0 && errno ==EINTR );if (ret !=0 ) { -fprintf (stderr,"Couldn't statfs() memory path: %s\n" , -strerror (errno)); -exit (1 ); +error_report_fatal ("Couldn't statfs() memory path: %s" , +strerror (errno)); }#define HUGETLBFS_MAGIC 0x958458f6
target-ppc/translate.c +3 -4
@@ -11574,10 +11574,9 @@ void gen_intermediate_code(CPUPPCState *env, struct TranslationBlock *tb)break ; }if (tcg_check_temp_count ()) { -fprintf (stderr,"Opcode %02x %02x %02x (%08x) leaked temporaries\n" , -opc1 (ctx.opcode),opc2 (ctx.opcode),opc3 (ctx.opcode), - ctx.opcode); -exit (1 ); +error_report_fatal ("Opcode %02x %02x %02x (%08x) leaked temporaries" , +opc1 (ctx.opcode),opc2 (ctx.opcode),opc3 (ctx.opcode), + ctx.opcode); } }if (tb->cflags &CF_LAST_IO )
target-ppc/translate_init.c +39 -41
141 lines not on this page · raw message
PATCH v5 4/5 util: [ppc] Use new error_report_abort() instead of abort()
In reply to Lluís Vilanova--- target-ppc/kvm.c | 4 ++-- target-ppc/kvm_ppc.h | 15 +++++++++------ target-ppc/mmu-hash32.c | 5 +++-- target-ppc/mmu_helper.c | 3 +-- 4 files changed, 15 insertions(+), 12 deletions(-)
target-ppc/kvm.c +2 -2
@@ -587,7 +587,7 @@ static void kvm_get_one_spr(CPUState *cs, uint64_t id, int spr)default :/* Don't handle this size yet */ -abort (); +error_report_abort ("Unhandled size: %d" , id &KVM_REG_SIZE_MASK ); } } }
@@ -617,7 +617,7 @@ static void kvm_put_one_spr(CPUState *cs, uint64_t id, int spr)default :/* Don't handle this size yet */ -abort (); +error_report_abort ("Unhandled size: %d" , id &KVM_REG_SIZE_MASK ); } ret =kvm_vcpu_ioctl (cs,KVM_SET_ONE_REG , ®);
target-ppc/kvm_ppc.h +9 -6
@@ -9,6 +9,9 @@#ifndef __KVM_PPC_H__#define __KVM_PPC_H__ +#include "qemu/error-report.h" + +#define TYPE_HOST_POWERPC_CPU "host-" TYPE_POWERPC_CPU#ifdef CONFIG_KVM
@@ -220,36 +223,36 @@ static inline int kvmppc_get_htab_fd(bool write)static inline int kvmppc_save_htab (QEMUFile *f,int fd,size_t bufsize,int64_t max_ns) { -abort (); +error_report_abort (" " ); }static inline int kvmppc_load_htab_chunk (QEMUFile *f,int fd,uint32_t index,uint16_t n_valid,uint16_t n_invalid) { -abort (); +error_report_abort (" " ); }static inline uint64_t kvmppc_hash64_read_pteg (PowerPCCPU *cpu,target_ulong pte_index) { -abort (); +error_report_abort (" " ); }static inline void kvmppc_hash64_free_pteg (uint64_t token) { -abort (); +error_report_abort (" " ); }static inline void kvmppc_hash64_write_pte (CPUPPCState *env,target_ulong pte_index,target_ulong pte0,target_ulong pte1) { -abort (); +error_report_abort (" " ); }static inline bool kvmppc_has_cap_fixup_hcalls (void ) { -abort (); +error_report_abort (" " ); }static inline int kvmppc_enable_hwrng (void )
target-ppc/mmu-hash32.c +3 -2
@@ -20,6 +20,7 @@#include "cpu.h" #include "exec/helper-proto.h" +#include "qemu/error-report.h" #include "sysemu/kvm.h" #include "kvm_ppc.h" #include "mmu-hash32.h"
@@ -55,7 +56,7 @@ static int ppc_hash32_pp_prot(int key, int pp, int nx)break ;default : -abort (); +error_report_abort ("Unhandled pp: %d" , pp); } }else {switch (pp) {
@@ -73,7 +74,7 @@ static int ppc_hash32_pp_prot(int key, int pp, int nx)break ;default : -abort (); +error_report_abort ("Unhandled pp: %d" , pp); } }if (nx ==0 ) {
target-ppc/mmu_helper.c +1 -2
@@ -1349,8 +1349,7 @@ static inline int check_physical(CPUPPCState *env, mmu_ctx_t *ctx,default :/* Caller's checks mean we should never get here for other models */ -abort (); -return -1 ; +error_report_abort ("Unhandled MMU model: %d" , env->mmu_model); }return ret;
PATCH v5 5/5 doc: Introduce coding style for errors
In reply to Lluís VilanovaGives some general guidelines for reporting errors in QEMU.
--- HACKING | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)
HACKING +33 -0
@@ -157,3 +157,36 @@ painful. These are: * you may assume that integers are 2s complement representation * you may assume that right shift of a signed integer duplicates the sign bit (ie it is an arithmetic shift, not a logical shift) + +7. Error reporting + +QEMU provides various mechanisms for reporting errors using a uniform format, +ensuring the user will receive them (e.g., shown in QMP when necessary). You +should use one of these mechanisms instead of manually reporting them (i.e., do +not use 'printf()', 'exit()' or 'abort()'). + +7.1. Simple error messages + +The 'error_report*()' functions in "include/qemu/error-report.h" will +immediately report error messages to the user. + +WARNING: Do *not* use 'error_report_fatal()' or 'error_report_abort()' for +errors that are (or can be) triggered by guest code (e.g., some unimplemented +corner case in guest code translation or device code). Otherwise that can be +abused by guest code to terminate QEMU. Instead, you should use +'error_report()'. + +7.2. Errors in user inputs + +The 'loc_*()' functions in "include/qemu/error-report.h" will extend the +messages from 'error_report*()' with references to locations in inputs provided +by the user (e.g., command line arguments or configuration files). + +7.3. More complex error management + +The functions in "include/qapi/error.h" can be used to accumulate error messages +in an 'Error' object, which can be propagated up the call chain where it is +finally reported. + +WARNING: The special 'error_fatal' and 'error_abort' objects follow the same +constrains as the 'error_report_fatal' and 'error_report_abort' functions.
Re: [Qemu-devel] [PATCH v5 5/5] doc: Introduce coding style for errors
Quoted 6 lines · Lluís Vilanova
Gives some general guidelines for reporting errors in QEMU.
Signed-off-by Lluís Vilanova <[email protected]>--- HACKING | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)
I'm not sure if my v4 review crossed paths with this, but I still see typos:
HACKING painful. These are: · line 192
Quoted 3 lines
HACKING +3
-0+ +WARNING: The special 'error_fatal' and 'error_abort' objects follow the same +constrains as the 'error_report_fatal' and 'error_report_abort' functions.
s/constrains/constraints/
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH v5 5/5] doc: Introduce coding style for errors
Quoted 8 lines · Eric Blake
Quoted 6 lines · Lluís Vilanova
Gives some general guidelines for reporting errors in QEMU.
Signed-off-by Lluís Vilanova <[email protected]>---
HACKING | 33 +++++++++++++++++++++++++++++++++1 file changed, 33 insertions(+)
Quoted 7 lines
I'm not sure if my v4 review crossed paths with this, but I still see typos:
HACKING painful. These are: · line 192
Quoted 3 lines
HACKING +3
-0+ +WARNING: The special 'error_fatal' and 'error_abort' objects follow the same +constrains as the 'error_report_fatal' and 'error_report_abort' functions.s/constrains/constraints/
We did cross paths, sorry. It's fixed on upcoming v6.
Lluis