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

[Qemu-devel] [RFC][PATCH v5 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 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.


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 v5 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 v5 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);
     }
 }
 

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

[David Gibson]

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

On Thu, Jan 28, 2016 at 10:53:43PM +0100, Lluís Vilanova wrote:
> 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>

I think the spapr parts of this will be obsoleted by the cleanups to
error handling included in the pull request I sent today.
-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Lluís Vilanova]

David Gibson writes:

> On Thu, Jan 28, 2016 at 10:53:43PM +0100, Lluís Vilanova wrote:
>> 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>

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

[David Gibson]

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

On Fri, Jan 29, 2016 at 02:33:08PM +0100, Lluís Vilanova wrote:
> David Gibson writes:
> 
> > On Thu, Jan 28, 2016 at 10:53:43PM +0100, Lluís Vilanova wrote:
> >> 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>
> 
> > 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?

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[Qemu-devel] [PATCH v5 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 v5 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 v5 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..aecc77c 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 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

[Eric Blake]

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

On 01/28/2016 02:53 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(+)

I'm not sure if my v4 review crossed paths with this, but I still see typos:

> +
> +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] [PATCH v5 5/5] doc: Introduce coding style for errors

[Lluís Vilanova]

Eric Blake writes:

> On 01/28/2016 02:53 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(+)

> I'm not sure if my v4 review crossed paths with this, but I still see typos:

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