[Qemu-devel] [RFC][PATCH v4 0/5] utils: Improve and document error reporting

[Qemu-devel] [RFC][PATCH v4 0/5] utils: Improve and document error reporting

[Lluís Vilanova]

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


Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---

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: qemu-devel@nongnu.org
Cc: Stefan Hajnoczi <stefanha@gmail.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Thomas Huth <thuth@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Eric Blake <eblake@redhat.com>

[Qemu-devel] [PATCH v4 1/5] util: Introduce error reporting functions with fatal/abort

[Lluís Vilanova]

Provide two lean functions to report error messages that fatal/abort
QEMU.

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 include/qemu/error-report.h |   19 +++++++++++++++++++
 util/qemu-error.c           |   33 +++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index 7ab2355..6c2f142 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -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
diff --git a/util/qemu-error.c b/util/qemu-error.c
index ecf5708..3de002b 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -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);
+}

[Qemu-devel] [PATCH v4 2/5] util: Use new error_report_fatal/abort instead of error_setg(&error_fatal/abort)

[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 <vilanova@ac.upc.edu>
---
 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(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index e3b0e1e..8f0c947 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -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]);
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 50e5a26..a5afea1 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -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 */
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index dccb908..0d8f5b4 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -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);
diff --git a/util/error.c b/util/error.c
index 57303fd..b8a9120 100644
--- a/util/error.c
+++ b/util/error.c
@@ -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);
     }
 }
 

[Qemu-devel] [PATCH v4 3/5] util: [ppc] Use new error_report_fatal() instead of exit()

[Lluís Vilanova]

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 target-ppc/kvm.c            |    5 +--
 target-ppc/translate.c      |    7 ++--
 target-ppc/translate_init.c |   80 +++++++++++++++++++++----------------------
 3 files changed, 44 insertions(+), 48 deletions(-)

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 9940a90..098a40d 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -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
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 4be7eaa..2dfbbc2 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -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)
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index d7e1a4e..dc9bbd6 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -619,8 +619,8 @@ static inline void _spr_register(CPUPPCState *env, int num,
         spr->oea_read != NULL || spr->oea_write != NULL ||
 #endif
         spr->uea_read != NULL || spr->uea_write != NULL) {
-        printf("Error: Trying to register SPR %d (%03x) twice !\n", num, num);
-        exit(1);
+        error_report_fatal("Error: Trying to register SPR %d (%03x) twice !",
+                           num, num);
     }
 #if defined(PPC_DEBUG_SPR)
     printf("*** register spr %d (%03x) %s val " TARGET_FMT_lx "\n", num, num,
@@ -1608,8 +1608,7 @@ static void gen_spr_BookE (CPUPPCState *env, uint64_t ivor_mask)
     for (i = 0; i < 64; i++) {
         if (ivor_mask & (1ULL << i)) {
             if (ivor_sprn[i] == SPR_BOOKE_IVORxx) {
-                fprintf(stderr, "ERROR: IVOR %d SPR is not defined\n", i);
-                exit(1);
+                error_report_fatal("ERROR: IVOR %d SPR is not defined", i);
             }
             spr_register(env, ivor_sprn[i], ivor_names[i],
                          SPR_NOACCESS, SPR_NOACCESS,
@@ -8319,14 +8318,14 @@ static void init_ppc_proc(PowerPCCPU *cpu)
         case POWERPC_FLAG_VRE:
             break;
         default:
-            fprintf(stderr, "PowerPC MSR definition inconsistency\n"
-                    "Should define POWERPC_FLAG_SPE or POWERPC_FLAG_VRE\n");
-            exit(1);
+            error_report("PowerPC MSR definition inconsistency");
+            error_report_fatal(
+                "Should define POWERPC_FLAG_SPE or POWERPC_FLAG_VRE");
         }
     } else if (env->flags & (POWERPC_FLAG_SPE | POWERPC_FLAG_VRE)) {
-        fprintf(stderr, "PowerPC MSR definition inconsistency\n"
-                "Should not define POWERPC_FLAG_SPE nor POWERPC_FLAG_VRE\n");
-        exit(1);
+        error_report("PowerPC MSR definition inconsistency");
+        error_report_fatal(
+            "Should not define POWERPC_FLAG_SPE nor POWERPC_FLAG_VRE");
     }
     if (env->msr_mask & (1 << 17)) {
         switch (env->flags & (POWERPC_FLAG_TGPR | POWERPC_FLAG_CE)) {
@@ -8334,14 +8333,14 @@ static void init_ppc_proc(PowerPCCPU *cpu)
         case POWERPC_FLAG_CE:
             break;
         default:
-            fprintf(stderr, "PowerPC MSR definition inconsistency\n"
-                    "Should define POWERPC_FLAG_TGPR or POWERPC_FLAG_CE\n");
-            exit(1);
+            error_report("PowerPC MSR definition inconsistency");
+            error_report_fatal(
+                "Should define POWERPC_FLAG_TGPR or POWERPC_FLAG_CE");
         }
     } else if (env->flags & (POWERPC_FLAG_TGPR | POWERPC_FLAG_CE)) {
-        fprintf(stderr, "PowerPC MSR definition inconsistency\n"
-                "Should not define POWERPC_FLAG_TGPR nor POWERPC_FLAG_CE\n");
-        exit(1);
+        error_report("PowerPC MSR definition inconsistency");
+        error_report_fatal(
+            "Should not define POWERPC_FLAG_TGPR nor POWERPC_FLAG_CE");
     }
     if (env->msr_mask & (1 << 10)) {
         switch (env->flags & (POWERPC_FLAG_SE | POWERPC_FLAG_DWE |
@@ -8351,17 +8350,17 @@ static void init_ppc_proc(PowerPCCPU *cpu)
         case POWERPC_FLAG_UBLE:
             break;
         default:
-            fprintf(stderr, "PowerPC MSR definition inconsistency\n"
-                    "Should define POWERPC_FLAG_SE or POWERPC_FLAG_DWE or "
-                    "POWERPC_FLAG_UBLE\n");
-            exit(1);
+            error_report("PowerPC MSR definition inconsistency");
+            error_report_fatal(
+                "Should define POWERPC_FLAG_SE or POWERPC_FLAG_DWE or "
+                "POWERPC_FLAG_UBLE");
         }
     } else if (env->flags & (POWERPC_FLAG_SE | POWERPC_FLAG_DWE |
                              POWERPC_FLAG_UBLE)) {
-        fprintf(stderr, "PowerPC MSR definition inconsistency\n"
-                "Should not define POWERPC_FLAG_SE nor POWERPC_FLAG_DWE nor "
-                "POWERPC_FLAG_UBLE\n");
-            exit(1);
+        error_report("PowerPC MSR definition inconsistency");
+        error_report_fatal(
+            "Should not define POWERPC_FLAG_SE nor POWERPC_FLAG_DWE nor "
+            "POWERPC_FLAG_UBLE");
     }
     if (env->msr_mask & (1 << 9)) {
         switch (env->flags & (POWERPC_FLAG_BE | POWERPC_FLAG_DE)) {
@@ -8369,14 +8368,14 @@ static void init_ppc_proc(PowerPCCPU *cpu)
         case POWERPC_FLAG_DE:
             break;
         default:
-            fprintf(stderr, "PowerPC MSR definition inconsistency\n"
-                    "Should define POWERPC_FLAG_BE or POWERPC_FLAG_DE\n");
-            exit(1);
+            error_report("PowerPC MSR definition inconsistency");
+            error_report_fatal(
+                "Should define POWERPC_FLAG_BE or POWERPC_FLAG_DE");
         }
     } else if (env->flags & (POWERPC_FLAG_BE | POWERPC_FLAG_DE)) {
-        fprintf(stderr, "PowerPC MSR definition inconsistency\n"
-                "Should not define POWERPC_FLAG_BE nor POWERPC_FLAG_DE\n");
-        exit(1);
+        error_report("PowerPC MSR definition inconsistency");
+        error_report_fatal(
+            "Should not define POWERPC_FLAG_BE nor POWERPC_FLAG_DE");
     }
     if (env->msr_mask & (1 << 2)) {
         switch (env->flags & (POWERPC_FLAG_PX | POWERPC_FLAG_PMM)) {
@@ -8384,19 +8383,19 @@ static void init_ppc_proc(PowerPCCPU *cpu)
         case POWERPC_FLAG_PMM:
             break;
         default:
-            fprintf(stderr, "PowerPC MSR definition inconsistency\n"
-                    "Should define POWERPC_FLAG_PX or POWERPC_FLAG_PMM\n");
-            exit(1);
+            error_report("PowerPC MSR definition inconsistency");
+            error_report_fatal(
+                "Should define POWERPC_FLAG_PX or POWERPC_FLAG_PMM");
         }
     } else if (env->flags & (POWERPC_FLAG_PX | POWERPC_FLAG_PMM)) {
-        fprintf(stderr, "PowerPC MSR definition inconsistency\n"
-                "Should not define POWERPC_FLAG_PX nor POWERPC_FLAG_PMM\n");
-        exit(1);
+        error_report("PowerPC MSR definition inconsistency");
+        error_report_fatal(
+            "Should not define POWERPC_FLAG_PX nor POWERPC_FLAG_PMM");
     }
     if ((env->flags & (POWERPC_FLAG_RTC_CLK | POWERPC_FLAG_BUS_CLK)) == 0) {
-        fprintf(stderr, "PowerPC flags inconsistency\n"
-                "Should define the time-base and decrementer clock source\n");
-        exit(1);
+        error_report("PowerPC flags inconsistency");
+        error_report_fatal(
+            "Should define the time-base and decrementer clock source");
     }
     /* Allocate TLBs buffer when needed */
 #if !defined(CONFIG_USER_ONLY)
@@ -9570,8 +9569,7 @@ static void ppc_cpu_reset(CPUState *s)
 #if !defined(TARGET_WORDS_BIGENDIAN)
     msr |= (target_ulong)1 << MSR_LE; /* Little-endian user mode */
     if (!((env->msr_mask >> MSR_LE) & 1)) {
-        fprintf(stderr, "Selected CPU does not support little-endian.\n");
-        exit(1);
+        error_report_fatal("Selected CPU does not support little-endian.");
     }
 #endif
 #endif

[Qemu-devel] [PATCH v4 4/5] util: [ppc] Use new error_report_abort() instead of abort()

[Lluís Vilanova]

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 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(-)

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 098a40d..e7596a2 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -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, &reg);
diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
index 5e1333d..07ff3fc 100644
--- a/target-ppc/kvm_ppc.h
+++ b/target-ppc/kvm_ppc.h
@@ -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)
diff --git a/target-ppc/mmu-hash32.c b/target-ppc/mmu-hash32.c
index a00ae3c..9d1cc33 100644
--- a/target-ppc/mmu-hash32.c
+++ b/target-ppc/mmu-hash32.c
@@ -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) {
diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
index 5217691..7ded975 100644
--- a/target-ppc/mmu_helper.c
+++ b/target-ppc/mmu_helper.c
@@ -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;

[Qemu-devel] [PATCH v4 5/5] doc: Introduce coding style for errors

[Lluís Vilanova]

Gives some general guidelines for reporting errors in QEMU.

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 HACKING |   33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/HACKING b/HACKING
index 12fbc8a..f5783d4 100644
--- a/HACKING
+++ b/HACKING
@@ -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 unimplimented
+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 v4 4/5] util: [ppc] Use new error_report_abort() instead of abort()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1086 bytes --]

On 01/28/2016 02:41 PM, Lluís Vilanova wrote:
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
>  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(-)
> 

> +++ b/target-ppc/kvm_ppc.h
> @@ -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(" ");

Aborting with an empty string with trailing spaces feels awkward.
Either this should be a real message, or abort() was just fine.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 5/5] doc: Introduce coding style for errors

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2720 bytes --]

On 01/28/2016 02:41 PM, Lluís Vilanova wrote:
> Gives some general guidelines for reporting errors in QEMU.
> 
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
>  HACKING |   33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/HACKING b/HACKING
> index 12fbc8a..f5783d4 100644
> --- a/HACKING
> +++ b/HACKING
> @@ -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()').

abort() for unreachable code may be okay, but I'm not sure how to word
that.  Maybe "avoid use 'printf()' or 'exit()', and minimize use of
'abort()' situations that should be unreachable code".

May be worth mentioning that if the user can trigger it (command line,
hotplug, etc) then we want fatal; if it represents a programming bug
that the user should not be able to trigger, then abort is okay.

> +
> +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 unimplimented

s/unimplimented/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.

s/constrains/constraints/

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [RFC][PATCH v4 0/5] utils: Improve and document error reporting

[Lluís Vilanova]

Please ignore this version; I forgot to fix a typo (fixed in v5).

Cheers,
  Lluis

Lluís Vilanova writes:

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


> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---

> 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: qemu-devel@nongnu.org
> Cc: Stefan Hajnoczi <stefanha@gmail.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>

Re: [Qemu-devel] [PATCH v4 5/5] doc: Introduce coding style for errors

[Lluís Vilanova]

Eric Blake writes:

> On 01/28/2016 02:41 PM, Lluís Vilanova wrote:
>> Gives some general guidelines for reporting errors in QEMU.
>> 
>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>> ---
>> HACKING |   33 +++++++++++++++++++++++++++++++++
>> 1 file changed, 33 insertions(+)
>> 
>> diff --git a/HACKING b/HACKING
>> index 12fbc8a..f5783d4 100644
>> --- a/HACKING
>> +++ b/HACKING
>> @@ -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()').

> abort() for unreachable code may be okay, but I'm not sure how to word
> that.  Maybe "avoid use 'printf()' or 'exit()', and minimize use of
> 'abort()' situations that should be unreachable code".

Hmmm. I was thinking it'd be more informative to always use error_report_abort()
instead of abort:

* The program name is shown (great for multi-app logs)
* The aborting location is shown (a bit useful to quickly see where it comes
  from)
* A message can be provided

I think the message should be optional, since error_report_abort() already
provides more information than plain abort().

But if that seems unreasonable, I can reword it as:

  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()' nor 'exit()', and minimize the use of 'abort()' to
  situations where code should be unreachable and an error message does not make
  sense.


> May be worth mentioning that if the user can trigger it (command line,
> hotplug, etc) then we want fatal; if it represents a programming bug
> that the user should not be able to trigger, then abort is okay.

So true. I'll add these.


>> +
>> +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 unimplimented

> s/unimplimented/unimplemented/

Fixed in v5 (sorry I forgot about it).


>> +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.

> s/constrains/constraints/

Will add.


Thanks,
  Lluis

Re: [Qemu-devel] [PATCH v4 4/5] util: [ppc] Use new error_report_abort() instead of abort()

[Lluís Vilanova]

Eric Blake writes:

> On 01/28/2016 02:41 PM, Lluís Vilanova wrote:
>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>> ---
>> 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(-)
>> 

>> +++ b/target-ppc/kvm_ppc.h
>> @@ -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(" ");

> Aborting with an empty string with trailing spaces feels awkward.
> Either this should be a real message, or abort() was just fine.

See my other mail for why (I think) it makes sense to abort without an
additional message. Also, an empty string makes gcc grumpy with a warning.


Cheers,
  Lluis