[Qemu-devel] [PATCHv4 0/8] Cleanups to error reporting on ppc and spapr

[Qemu-devel] [PATCHv4 0/8] Cleanups to error reporting on ppc and spapr

[David Gibson]

Another spin of my patches to clean up a bunch of error reporting in
the pseries machine type and target-ppc code, to better use the error
API.

Once reviewed, I hope to merge this into ppc-for-2.6 shortly.

Changes in v4:
 * Corrected a place I'd accidentally messed up the indenting
 * Dropped the patch changing the htab allocation path and some hunks
   of the migration path cleanups, since they will be obsoleted by
   other things I'm working on
Changes in v3:
 * Adjusted a commit message for accuracy (suggest by Markus)
 * Dropped a patch which relied on a wrong guess about the behaviour
   of foreach_dynamic_sysbus_device().
Changes in v2:
 * Assorted minor tweaks based on review
      
David Gibson (8):
  ppc: Cleanup error handling in ppc_set_compat()
  pseries: Cleanup error handling of spapr_cpu_init()
  pseries: Clean up error handling in spapr_validate_node_memory()
  pseries: Cleanup error handling in spapr_vga_init()
  pseries: Clean up error handling in spapr_rtas_register()
  pseries: Clean up error handling in xics_system_init()
  pseries: Clean up error reporting in ppc_spapr_init()
  pseries: Clean up error reporting in htab migration functions

 hw/ppc/spapr.c              | 103 +++++++++++++++++++++++++-------------------
 hw/ppc/spapr_hcall.c        |  10 ++---
 hw/ppc/spapr_rtas.c         |  12 ++----
 target-ppc/cpu.h            |   2 +-
 target-ppc/translate_init.c |  13 +++---
 5 files changed, 74 insertions(+), 66 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCHv4 1/8] ppc: Cleanup error handling in ppc_set_compat()

[David Gibson]

Current ppc_set_compat() returns -1 for errors, and also (unconditionally)
reports an error message.  The caller in h_client_architecture_support()
may then report it again using an outdated fprintf().

Clean this up by using the modern error reporting mechanisms.  Also add
strerror(errno) to the error message.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 hw/ppc/spapr.c              |  4 +---
 hw/ppc/spapr_hcall.c        | 10 +++++-----
 target-ppc/cpu.h            |  2 +-
 target-ppc/translate_init.c | 13 +++++++------
 4 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 50e5a26..fa7a7f4 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1635,9 +1635,7 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu)
     }
 
     if (cpu->max_compat) {
-        if (ppc_set_compat(cpu, cpu->max_compat) < 0) {
-            exit(1);
-        }
+        ppc_set_compat(cpu, cpu->max_compat, &error_fatal);
     }
 
     xics_cpu_setup(spapr->icp, cpu);
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index cebceea..8b0fcb3 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -837,7 +837,7 @@ static target_ulong cas_get_option_vector(int vector, target_ulong table)
 typedef struct {
     PowerPCCPU *cpu;
     uint32_t cpu_version;
-    int ret;
+    Error *err;
 } SetCompatState;
 
 static void do_set_compat(void *arg)
@@ -845,7 +845,7 @@ static void do_set_compat(void *arg)
     SetCompatState *s = arg;
 
     cpu_synchronize_state(CPU(s->cpu));
-    s->ret = ppc_set_compat(s->cpu, s->cpu_version);
+    ppc_set_compat(s->cpu, s->cpu_version, &s->err);
 }
 
 #define get_compat_level(cpuver) ( \
@@ -929,13 +929,13 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu_,
             SetCompatState s = {
                 .cpu = POWERPC_CPU(cs),
                 .cpu_version = cpu_version,
-                .ret = 0
+                .err = NULL,
             };
 
             run_on_cpu(cs, do_set_compat, &s);
 
-            if (s.ret < 0) {
-                fprintf(stderr, "Unable to set compatibility mode\n");
+            if (s.err) {
+                error_report_err(s.err);
                 return H_HARDWARE;
             }
         }
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 9706000..b3b89e6 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -1210,7 +1210,7 @@ void ppc_store_msr (CPUPPCState *env, target_ulong value);
 
 void ppc_cpu_list (FILE *f, fprintf_function cpu_fprintf);
 int ppc_get_compat_smt_threads(PowerPCCPU *cpu);
-int ppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version);
+void ppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version, Error **errp);
 
 /* Time-base and decrementer management */
 #ifndef NO_CPU_IO_DEFS
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 4ab2d92..678957a 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -9186,7 +9186,7 @@ int ppc_get_compat_smt_threads(PowerPCCPU *cpu)
     return ret;
 }
 
-int ppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version)
+void ppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version, Error **errp)
 {
     int ret = 0;
     CPUPPCState *env = &cpu->env;
@@ -9208,12 +9208,13 @@ int ppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version)
         break;
     }
 
-    if (kvm_enabled() && kvmppc_set_compat(cpu, cpu->cpu_version) < 0) {
-        error_report("Unable to set compatibility mode in KVM");
-        ret = -1;
+    if (kvm_enabled()) {
+        ret = kvmppc_set_compat(cpu, cpu->cpu_version);
+        if (ret < 0) {
+            error_setg_errno(errp, -ret,
+                             "Unable to set CPU compatibility mode in KVM");
+        }
     }
-
-    return ret;
 }
 
 static gint ppc_cpu_compare_class_pvr(gconstpointer a, gconstpointer b)
-- 
2.5.0

[Qemu-devel] [PATCHv4 2/8] pseries: Cleanup error handling of spapr_cpu_init()

[David Gibson]

Currently spapr_cpu_init() is hardcoded to handle any errors as fatal.
That works for now, since it's only called from initial setup where an
error here means we really can't proceed.

However, we'll want to handle this more flexibly for cpu hotplug in future
so generalize this using the error reporting infrastructure.  While we're
at it make a small cleanup in a related part of ppc_spapr_init() to use
error_report() instead of an old-style explicit fprintf().

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index fa7a7f4..b7fd09a 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1617,7 +1617,8 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
     machine->boot_order = g_strdup(boot_device);
 }
 
-static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu)
+static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
+                           Error **errp)
 {
     CPUPPCState *env = &cpu->env;
 
@@ -1635,7 +1636,13 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu)
     }
 
     if (cpu->max_compat) {
-        ppc_set_compat(cpu, cpu->max_compat, &error_fatal);
+        Error *local_err = NULL;
+
+        ppc_set_compat(cpu, cpu->max_compat, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
     }
 
     xics_cpu_setup(spapr->icp, cpu);
@@ -1804,10 +1811,10 @@ static void ppc_spapr_init(MachineState *machine)
     for (i = 0; i < smp_cpus; i++) {
         cpu = cpu_ppc_init(machine->cpu_model);
         if (cpu == NULL) {
-            fprintf(stderr, "Unable to find PowerPC CPU definition\n");
+            error_report("Unable to find PowerPC CPU definition");
             exit(1);
         }
-        spapr_cpu_init(spapr, cpu);
+        spapr_cpu_init(spapr, cpu, &error_fatal);
     }
 
     if (kvm_enabled()) {
-- 
2.5.0

[Qemu-devel] [PATCHv4 3/8] pseries: Clean up error handling in spapr_validate_node_memory()

[David Gibson]

Use error_setg() and return an error, rather than using an explicit exit().

Also improve messages, and be more explicit about which constraint failed.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 hw/ppc/spapr.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index b7fd09a..fb0e254 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1691,27 +1691,34 @@ static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr)
  * to SPAPR_MEMORY_BLOCK_SIZE(256MB), then refuse to start the guest
  * since we can't support such unaligned sizes with DRCONF_MEMORY.
  */
-static void spapr_validate_node_memory(MachineState *machine)
+static void spapr_validate_node_memory(MachineState *machine, Error **errp)
 {
     int i;
 
-    if (machine->maxram_size % SPAPR_MEMORY_BLOCK_SIZE ||
-        machine->ram_size % SPAPR_MEMORY_BLOCK_SIZE) {
-        error_report("Can't support memory configuration where RAM size "
-                     "0x" RAM_ADDR_FMT " or maxmem size "
-                     "0x" RAM_ADDR_FMT " isn't aligned to %llu MB",
-                     machine->ram_size, machine->maxram_size,
-                     SPAPR_MEMORY_BLOCK_SIZE/M_BYTE);
-        exit(EXIT_FAILURE);
+    if (machine->ram_size % SPAPR_MEMORY_BLOCK_SIZE) {
+        error_setg(errp, "Memory size 0x" RAM_ADDR_FMT
+                   " is not aligned to %llu MiB",
+                   machine->ram_size,
+                   SPAPR_MEMORY_BLOCK_SIZE / M_BYTE);
+        return;
+    }
+
+    if (machine->maxram_size % SPAPR_MEMORY_BLOCK_SIZE) {
+        error_setg(errp, "Maximum memory size 0x" RAM_ADDR_FMT
+                   " is not aligned to %llu MiB",
+                   machine->ram_size,
+                   SPAPR_MEMORY_BLOCK_SIZE / M_BYTE);
+        return;
     }
 
     for (i = 0; i < nb_numa_nodes; i++) {
         if (numa_info[i].node_mem % SPAPR_MEMORY_BLOCK_SIZE) {
-            error_report("Can't support memory configuration where memory size"
-                         " %" PRIx64 " of node %d isn't aligned to %llu MB",
-                         numa_info[i].node_mem, i,
-                         SPAPR_MEMORY_BLOCK_SIZE/M_BYTE);
-            exit(EXIT_FAILURE);
+            error_setg(errp,
+                       "Node %d memory size 0x" RAM_ADDR_FMT
+                       " is not aligned to %llu MiB",
+                       i, numa_info[i].node_mem,
+                       SPAPR_MEMORY_BLOCK_SIZE / M_BYTE);
+            return;
         }
     }
 }
@@ -1801,7 +1808,7 @@ static void ppc_spapr_init(MachineState *machine)
                                   XICS_IRQS);
 
     if (smc->dr_lmb_enabled) {
-        spapr_validate_node_memory(machine);
+        spapr_validate_node_memory(machine, &error_fatal);
     }
 
     /* init CPUs */
-- 
2.5.0

[Qemu-devel] [PATCHv4 4/8] pseries: Cleanup error handling in spapr_vga_init()

[David Gibson]

Use error_setg() to return an error rather than an explicit exit().
Previously it was an exit(0) instead of a non-zero exit code, which was
simply a bug.  Also improve the error message.

While we're at it change the type of spapr_vga_init() to bool since that's
how we're using it anyway.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 hw/ppc/spapr.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index fb0e254..1ce9b27 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1238,7 +1238,7 @@ static void spapr_rtc_create(sPAPRMachineState *spapr)
 }
 
 /* Returns whether we want to use VGA or not */
-static int spapr_vga_init(PCIBus *pci_bus)
+static bool spapr_vga_init(PCIBus *pci_bus, Error **errp)
 {
     switch (vga_interface_type) {
     case VGA_NONE:
@@ -1249,9 +1249,9 @@ static int spapr_vga_init(PCIBus *pci_bus)
     case VGA_VIRTIO:
         return pci_vga_init(pci_bus) != NULL;
     default:
-        fprintf(stderr, "This vga model is not supported,"
-                "currently it only supports -vga std\n");
-        exit(0);
+        error_setg(errp,
+                   "Unsupported VGA mode, only -vga std or -vga virtio is supported");
+        return false;
     }
 }
 
@@ -1926,7 +1926,7 @@ static void ppc_spapr_init(MachineState *machine)
     }
 
     /* Graphics */
-    if (spapr_vga_init(phb->bus)) {
+    if (spapr_vga_init(phb->bus, &error_fatal)) {
         spapr->has_graphics = true;
         machine->usb |= defaults_enabled() && !machine->usb_disabled;
     }
-- 
2.5.0

[Qemu-devel] [PATCHv4 5/8] pseries: Clean up error handling in spapr_rtas_register()

[David Gibson]

The errors detected in this function necessarily indicate bugs in the rest
of the qemu code, rather than an external or configuration problem.

So, a simple assert() is more appropriate than any more complex error
reporting.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 hw/ppc/spapr_rtas.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 34b12a3..0be52ae 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -648,17 +648,11 @@ target_ulong spapr_rtas_call(PowerPCCPU *cpu, sPAPRMachineState *spapr,
 
 void spapr_rtas_register(int token, const char *name, spapr_rtas_fn fn)
 {
-    if (!((token >= RTAS_TOKEN_BASE) && (token < RTAS_TOKEN_MAX))) {
-        fprintf(stderr, "RTAS invalid token 0x%x\n", token);
-        exit(1);
-    }
+    assert((token >= RTAS_TOKEN_BASE) && (token < RTAS_TOKEN_MAX));
 
     token -= RTAS_TOKEN_BASE;
-    if (rtas_table[token].name) {
-        fprintf(stderr, "RTAS call \"%s\" is registered already as 0x%x\n",
-                rtas_table[token].name, token);
-        exit(1);
-    }
+
+    assert(!rtas_table[token].name);
 
     rtas_table[token].name = name;
     rtas_table[token].fn = fn;
-- 
2.5.0

[Qemu-devel] [PATCHv4 6/8] pseries: Clean up error handling in xics_system_init()

[David Gibson]

Use the error handling infrastructure to pass an error out from
try_create_xics() instead of assuming &error_abort - the caller is in a
better position to decide on error handling policy.

Also change the error handling from an &error_abort to &error_fatal, since
this occurs during the initial machine construction and could be triggered
by bad configuration rather than a program error.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 hw/ppc/spapr.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 1ce9b27..f45be7f 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -111,7 +111,7 @@ static XICSState *try_create_xics(const char *type, int nr_servers,
 }
 
 static XICSState *xics_system_init(MachineState *machine,
-                                   int nr_servers, int nr_irqs)
+                                   int nr_servers, int nr_irqs, Error **errp)
 {
     XICSState *icp = NULL;
 
@@ -130,7 +130,7 @@ static XICSState *xics_system_init(MachineState *machine,
     }
 
     if (!icp) {
-        icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs, &error_abort);
+        icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs, errp);
     }
 
     return icp;
@@ -1805,7 +1805,7 @@ static void ppc_spapr_init(MachineState *machine)
     spapr->icp = xics_system_init(machine,
                                   DIV_ROUND_UP(max_cpus * kvmppc_smt_threads(),
                                                smp_threads),
-                                  XICS_IRQS);
+                                  XICS_IRQS, &error_fatal);
 
     if (smc->dr_lmb_enabled) {
         spapr_validate_node_memory(machine, &error_fatal);
-- 
2.5.0

[Qemu-devel] [PATCHv4 7/8] pseries: Clean up error reporting in ppc_spapr_init()

[David Gibson]

This function includes a number of explicit fprintf()s for errors.
Change these to use error_report() instead.

Also replace the single exit(EXIT_FAILURE) with an explicit exit(1), since
the latter is the more usual idiom in qemu by a large margin.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f45be7f..3cfacb9 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1781,8 +1781,8 @@ static void ppc_spapr_init(MachineState *machine)
     }
 
     if (spapr->rma_size > node0_size) {
-        fprintf(stderr, "Error: Numa node 0 has to span the RMA (%#08"HWADDR_PRIx")\n",
-                spapr->rma_size);
+        error_report("Numa node 0 has to span the RMA (%#08"HWADDR_PRIx")",
+                     spapr->rma_size);
         exit(1);
     }
 
@@ -1848,10 +1848,10 @@ static void ppc_spapr_init(MachineState *machine)
         ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
 
         if (machine->ram_slots > SPAPR_MAX_RAM_SLOTS) {
-            error_report("Specified number of memory slots %" PRIu64
-                         " exceeds max supported %d",
+            error_report("Specified number of memory slots %"
+                         PRIu64" exceeds max supported %d",
                          machine->ram_slots, SPAPR_MAX_RAM_SLOTS);
-            exit(EXIT_FAILURE);
+            exit(1);
         }
 
         spapr->hotplug_memory.base = ROUND_UP(machine->ram_size,
@@ -1947,8 +1947,9 @@ static void ppc_spapr_init(MachineState *machine)
     }
 
     if (spapr->rma_size < (MIN_RMA_SLOF << 20)) {
-        fprintf(stderr, "qemu: pSeries SLOF firmware requires >= "
-                "%ldM guest RMA (Real Mode Area memory)\n", MIN_RMA_SLOF);
+        error_report(
+            "pSeries SLOF firmware requires >= %ldM guest RMA (Real Mode Area memory)",
+            MIN_RMA_SLOF);
         exit(1);
     }
 
@@ -1964,8 +1965,8 @@ static void ppc_spapr_init(MachineState *machine)
             kernel_le = kernel_size > 0;
         }
         if (kernel_size < 0) {
-            fprintf(stderr, "qemu: error loading %s: %s\n",
-                    kernel_filename, load_elf_strerror(kernel_size));
+            error_report("error loading %s: %s",
+                         kernel_filename, load_elf_strerror(kernel_size));
             exit(1);
         }
 
@@ -1978,8 +1979,8 @@ static void ppc_spapr_init(MachineState *machine)
             initrd_size = load_image_targphys(initrd_filename, initrd_base,
                                               load_limit - initrd_base);
             if (initrd_size < 0) {
-                fprintf(stderr, "qemu: could not load initial ram disk '%s'\n",
-                        initrd_filename);
+                error_report("could not load initial ram disk '%s'",
+                             initrd_filename);
                 exit(1);
             }
         } else {
-- 
2.5.0

[Qemu-devel] [PATCHv4 8/8] pseries: Clean up error reporting in htab migration functions

[David Gibson]

The functions for migrating the hash page table on pseries machine type
(htab_save_setup() and htab_load()) can report some errors with an
explicit fprintf() before returning an appropriate error code.  Change these
to use error_report() instead.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 hw/ppc/spapr.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 3cfacb9..1eb7d03 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1526,7 +1526,7 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
     int fd = -1;
 
     if (version_id < 1 || version_id > 1) {
-        fprintf(stderr, "htab_load() bad version\n");
+        error_report("htab_load() bad version");
         return -EINVAL;
     }
 
@@ -1547,8 +1547,8 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
 
         fd = kvmppc_get_htab_fd(true);
         if (fd < 0) {
-            fprintf(stderr, "Unable to open fd to restore KVM hash table: %s\n",
-                    strerror(errno));
+            error_report("Unable to open fd to restore KVM hash table: %s",
+                         strerror(errno));
         }
     }
 
@@ -1568,9 +1568,9 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
         if ((index + n_valid + n_invalid) >
             (HTAB_SIZE(spapr) / HASH_PTE_SIZE_64)) {
             /* Bad index in stream */
-            fprintf(stderr, "htab_load() bad index %d (%hd+%hd entries) "
-                    "in htab stream (htab_shift=%d)\n", index, n_valid, n_invalid,
-                    spapr->htab_shift);
+            error_report(
+                "htab_load() bad index %d (%hd+%hd entries) in htab stream (htab_shift=%d)",
+                index, n_valid, n_invalid, spapr->htab_shift);
             return -EINVAL;
         }
 
-- 
2.5.0

Re: [Qemu-devel] [PATCHv4 1/8] ppc: Cleanup error handling in ppc_set_compat()

[Alexey Kardashevskiy]

On 01/19/2016 02:39 PM, David Gibson wrote:
> Current ppc_set_compat() returns -1 for errors, and also (unconditionally)
> reports an error message.  The caller in h_client_architecture_support()
> may then report it again using an outdated fprintf().
>
> Clean this up by using the modern error reporting mechanisms.  Also add
> strerror(errno) to the error message.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>

> ---
>   hw/ppc/spapr.c              |  4 +---
>   hw/ppc/spapr_hcall.c        | 10 +++++-----
>   target-ppc/cpu.h            |  2 +-
>   target-ppc/translate_init.c | 13 +++++++------
>   4 files changed, 14 insertions(+), 15 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 50e5a26..fa7a7f4 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1635,9 +1635,7 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu)
>       }
>
>       if (cpu->max_compat) {
> -        if (ppc_set_compat(cpu, cpu->max_compat) < 0) {
> -            exit(1);
> -        }
> +        ppc_set_compat(cpu, cpu->max_compat, &error_fatal);
>       }
>
>       xics_cpu_setup(spapr->icp, cpu);
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index cebceea..8b0fcb3 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -837,7 +837,7 @@ static target_ulong cas_get_option_vector(int vector, target_ulong table)
>   typedef struct {
>       PowerPCCPU *cpu;
>       uint32_t cpu_version;
> -    int ret;
> +    Error *err;
>   } SetCompatState;
>
>   static void do_set_compat(void *arg)
> @@ -845,7 +845,7 @@ static void do_set_compat(void *arg)
>       SetCompatState *s = arg;
>
>       cpu_synchronize_state(CPU(s->cpu));
> -    s->ret = ppc_set_compat(s->cpu, s->cpu_version);
> +    ppc_set_compat(s->cpu, s->cpu_version, &s->err);
>   }
>
>   #define get_compat_level(cpuver) ( \
> @@ -929,13 +929,13 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu_,
>               SetCompatState s = {
>                   .cpu = POWERPC_CPU(cs),
>                   .cpu_version = cpu_version,
> -                .ret = 0
> +                .err = NULL,
>               };
>
>               run_on_cpu(cs, do_set_compat, &s);
>
> -            if (s.ret < 0) {
> -                fprintf(stderr, "Unable to set compatibility mode\n");
> +            if (s.err) {
> +                error_report_err(s.err);
>                   return H_HARDWARE;
>               }
>           }
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 9706000..b3b89e6 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -1210,7 +1210,7 @@ void ppc_store_msr (CPUPPCState *env, target_ulong value);
>
>   void ppc_cpu_list (FILE *f, fprintf_function cpu_fprintf);
>   int ppc_get_compat_smt_threads(PowerPCCPU *cpu);
> -int ppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version);
> +void ppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version, Error **errp);
>
>   /* Time-base and decrementer management */
>   #ifndef NO_CPU_IO_DEFS
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 4ab2d92..678957a 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -9186,7 +9186,7 @@ int ppc_get_compat_smt_threads(PowerPCCPU *cpu)
>       return ret;
>   }
>
> -int ppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version)
> +void ppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version, Error **errp)
>   {
>       int ret = 0;
>       CPUPPCState *env = &cpu->env;
> @@ -9208,12 +9208,13 @@ int ppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version)
>           break;
>       }
>
> -    if (kvm_enabled() && kvmppc_set_compat(cpu, cpu->cpu_version) < 0) {
> -        error_report("Unable to set compatibility mode in KVM");
> -        ret = -1;
> +    if (kvm_enabled()) {
> +        ret = kvmppc_set_compat(cpu, cpu->cpu_version);
> +        if (ret < 0) {
> +            error_setg_errno(errp, -ret,
> +                             "Unable to set CPU compatibility mode in KVM");
> +        }
>       }
> -
> -    return ret;
>   }
>
>   static gint ppc_cpu_compare_class_pvr(gconstpointer a, gconstpointer b)
>


-- 
Alexey

Re: [Qemu-devel] [PATCHv4 2/8] pseries: Cleanup error handling of spapr_cpu_init()

[Alexey Kardashevskiy]

On 01/19/2016 02:39 PM, David Gibson wrote:
> Currently spapr_cpu_init() is hardcoded to handle any errors as fatal.
> That works for now, since it's only called from initial setup where an
> error here means we really can't proceed.
>
> However, we'll want to handle this more flexibly for cpu hotplug in future
> so generalize this using the error reporting infrastructure.  While we're
> at it make a small cleanup in a related part of ppc_spapr_init() to use
> error_report() instead of an old-style explicit fprintf().
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>


> ---
>   hw/ppc/spapr.c | 15 +++++++++++----
>   1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index fa7a7f4..b7fd09a 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1617,7 +1617,8 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
>       machine->boot_order = g_strdup(boot_device);
>   }
>
> -static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu)
> +static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> +                           Error **errp)
>   {
>       CPUPPCState *env = &cpu->env;
>
> @@ -1635,7 +1636,13 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu)
>       }
>
>       if (cpu->max_compat) {
> -        ppc_set_compat(cpu, cpu->max_compat, &error_fatal);
> +        Error *local_err = NULL;
> +
> +        ppc_set_compat(cpu, cpu->max_compat, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
>       }
>
>       xics_cpu_setup(spapr->icp, cpu);
> @@ -1804,10 +1811,10 @@ static void ppc_spapr_init(MachineState *machine)
>       for (i = 0; i < smp_cpus; i++) {
>           cpu = cpu_ppc_init(machine->cpu_model);
>           if (cpu == NULL) {
> -            fprintf(stderr, "Unable to find PowerPC CPU definition\n");
> +            error_report("Unable to find PowerPC CPU definition");
>               exit(1);
>           }
> -        spapr_cpu_init(spapr, cpu);
> +        spapr_cpu_init(spapr, cpu, &error_fatal);
>       }
>
>       if (kvm_enabled()) {
>


-- 
Alexey

Re: [Qemu-devel] [PATCHv4 8/8] pseries: Clean up error reporting in htab migration functions

[Alexey Kardashevskiy]

On 01/19/2016 02:39 PM, David Gibson wrote:
> The functions for migrating the hash page table on pseries machine type
> (htab_save_setup() and htab_load()) can report some errors with an
> explicit fprintf() before returning an appropriate error code.  Change these
> to use error_report() instead.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: Thomas Huth <thuth@redhat.com>


Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>	

> ---
>   hw/ppc/spapr.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 3cfacb9..1eb7d03 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1526,7 +1526,7 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
>       int fd = -1;
>
>       if (version_id < 1 || version_id > 1) {
> -        fprintf(stderr, "htab_load() bad version\n");
> +        error_report("htab_load() bad version");
>           return -EINVAL;
>       }
>
> @@ -1547,8 +1547,8 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
>
>           fd = kvmppc_get_htab_fd(true);
>           if (fd < 0) {
> -            fprintf(stderr, "Unable to open fd to restore KVM hash table: %s\n",
> -                    strerror(errno));
> +            error_report("Unable to open fd to restore KVM hash table: %s",
> +                         strerror(errno));
>           }
>       }
>
> @@ -1568,9 +1568,9 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
>           if ((index + n_valid + n_invalid) >
>               (HTAB_SIZE(spapr) / HASH_PTE_SIZE_64)) {
>               /* Bad index in stream */
> -            fprintf(stderr, "htab_load() bad index %d (%hd+%hd entries) "
> -                    "in htab stream (htab_shift=%d)\n", index, n_valid, n_invalid,
> -                    spapr->htab_shift);
> +            error_report(
> +                "htab_load() bad index %d (%hd+%hd entries) in htab stream (htab_shift=%d)",
> +                index, n_valid, n_invalid, spapr->htab_shift);
>               return -EINVAL;
>           }
>
>


-- 
Alexey

Re: [Qemu-devel] [PATCHv4 7/8] pseries: Clean up error reporting in ppc_spapr_init()

[Alexey Kardashevskiy]

On 01/19/2016 02:39 PM, David Gibson wrote:
> This function includes a number of explicit fprintf()s for errors.
> Change these to use error_report() instead.
>
> Also replace the single exit(EXIT_FAILURE) with an explicit exit(1), since
> the latter is the more usual idiom in qemu by a large margin.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>


> ---
>   hw/ppc/spapr.c | 23 ++++++++++++-----------
>   1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f45be7f..3cfacb9 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1781,8 +1781,8 @@ static void ppc_spapr_init(MachineState *machine)
>       }
>
>       if (spapr->rma_size > node0_size) {
> -        fprintf(stderr, "Error: Numa node 0 has to span the RMA (%#08"HWADDR_PRIx")\n",
> -                spapr->rma_size);
> +        error_report("Numa node 0 has to span the RMA (%#08"HWADDR_PRIx")",
> +                     spapr->rma_size);
>           exit(1);
>       }
>
> @@ -1848,10 +1848,10 @@ static void ppc_spapr_init(MachineState *machine)
>           ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
>
>           if (machine->ram_slots > SPAPR_MAX_RAM_SLOTS) {
> -            error_report("Specified number of memory slots %" PRIu64
> -                         " exceeds max supported %d",
> +            error_report("Specified number of memory slots %"
> +                         PRIu64" exceeds max supported %d",
>                            machine->ram_slots, SPAPR_MAX_RAM_SLOTS);
> -            exit(EXIT_FAILURE);
> +            exit(1);
>           }
>
>           spapr->hotplug_memory.base = ROUND_UP(machine->ram_size,
> @@ -1947,8 +1947,9 @@ static void ppc_spapr_init(MachineState *machine)
>       }
>
>       if (spapr->rma_size < (MIN_RMA_SLOF << 20)) {
> -        fprintf(stderr, "qemu: pSeries SLOF firmware requires >= "
> -                "%ldM guest RMA (Real Mode Area memory)\n", MIN_RMA_SLOF);
> +        error_report(
> +            "pSeries SLOF firmware requires >= %ldM guest RMA (Real Mode Area memory)",
> +            MIN_RMA_SLOF);
>           exit(1);
>       }
>
> @@ -1964,8 +1965,8 @@ static void ppc_spapr_init(MachineState *machine)
>               kernel_le = kernel_size > 0;
>           }
>           if (kernel_size < 0) {
> -            fprintf(stderr, "qemu: error loading %s: %s\n",
> -                    kernel_filename, load_elf_strerror(kernel_size));
> +            error_report("error loading %s: %s",
> +                         kernel_filename, load_elf_strerror(kernel_size));
>               exit(1);
>           }
>
> @@ -1978,8 +1979,8 @@ static void ppc_spapr_init(MachineState *machine)
>               initrd_size = load_image_targphys(initrd_filename, initrd_base,
>                                                 load_limit - initrd_base);
>               if (initrd_size < 0) {
> -                fprintf(stderr, "qemu: could not load initial ram disk '%s'\n",
> -                        initrd_filename);
> +                error_report("could not load initial ram disk '%s'",
> +                             initrd_filename);
>                   exit(1);
>               }
>           } else {
>


-- 
Alexey

Re: [Qemu-devel] [PATCHv4 6/8] pseries: Clean up error handling in xics_system_init()

[Alexey Kardashevskiy]

On 01/19/2016 02:39 PM, David Gibson wrote:
> Use the error handling infrastructure to pass an error out from
> try_create_xics() instead of assuming &error_abort - the caller is in a
> better position to decide on error handling policy.
>
> Also change the error handling from an &error_abort to &error_fatal, since
> this occurs during the initial machine construction and could be triggered
> by bad configuration rather than a program error.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>


> ---
>   hw/ppc/spapr.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 1ce9b27..f45be7f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -111,7 +111,7 @@ static XICSState *try_create_xics(const char *type, int nr_servers,
>   }
>
>   static XICSState *xics_system_init(MachineState *machine,
> -                                   int nr_servers, int nr_irqs)
> +                                   int nr_servers, int nr_irqs, Error **errp)
>   {
>       XICSState *icp = NULL;
>
> @@ -130,7 +130,7 @@ static XICSState *xics_system_init(MachineState *machine,
>       }
>
>       if (!icp) {
> -        icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs, &error_abort);
> +        icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs, errp);
>       }
>
>       return icp;
> @@ -1805,7 +1805,7 @@ static void ppc_spapr_init(MachineState *machine)
>       spapr->icp = xics_system_init(machine,
>                                     DIV_ROUND_UP(max_cpus * kvmppc_smt_threads(),
>                                                  smp_threads),
> -                                  XICS_IRQS);
> +                                  XICS_IRQS, &error_fatal);
>
>       if (smc->dr_lmb_enabled) {
>           spapr_validate_node_memory(machine, &error_fatal);
>


-- 
Alexey

Re: [Qemu-devel] [PATCHv4 4/8] pseries: Cleanup error handling in spapr_vga_init()

[Alexey Kardashevskiy]

On 01/19/2016 02:39 PM, David Gibson wrote:
> Use error_setg() to return an error rather than an explicit exit().
> Previously it was an exit(0) instead of a non-zero exit code, which was
> simply a bug.  Also improve the error message.
>
> While we're at it change the type of spapr_vga_init() to bool since that's
> how we're using it anyway.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>


> ---
>   hw/ppc/spapr.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index fb0e254..1ce9b27 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1238,7 +1238,7 @@ static void spapr_rtc_create(sPAPRMachineState *spapr)
>   }
>
>   /* Returns whether we want to use VGA or not */
> -static int spapr_vga_init(PCIBus *pci_bus)
> +static bool spapr_vga_init(PCIBus *pci_bus, Error **errp)
>   {
>       switch (vga_interface_type) {
>       case VGA_NONE:
> @@ -1249,9 +1249,9 @@ static int spapr_vga_init(PCIBus *pci_bus)
>       case VGA_VIRTIO:
>           return pci_vga_init(pci_bus) != NULL;
>       default:
> -        fprintf(stderr, "This vga model is not supported,"
> -                "currently it only supports -vga std\n");
> -        exit(0);
> +        error_setg(errp,
> +                   "Unsupported VGA mode, only -vga std or -vga virtio is supported");
> +        return false;
>       }
>   }
>
> @@ -1926,7 +1926,7 @@ static void ppc_spapr_init(MachineState *machine)
>       }
>
>       /* Graphics */
> -    if (spapr_vga_init(phb->bus)) {
> +    if (spapr_vga_init(phb->bus, &error_fatal)) {
>           spapr->has_graphics = true;
>           machine->usb |= defaults_enabled() && !machine->usb_disabled;
>       }
>


-- 
Alexey

Re: [Qemu-devel] [PATCHv4 5/8] pseries: Clean up error handling in spapr_rtas_register()

[Alexey Kardashevskiy]

On 01/19/2016 02:39 PM, David Gibson wrote:
> The errors detected in this function necessarily indicate bugs in the rest
> of the qemu code, rather than an external or configuration problem.
>
> So, a simple assert() is more appropriate than any more complex error
> reporting.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: Thomas Huth <thuth@redhat.com>


Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>

> ---
>   hw/ppc/spapr_rtas.c | 12 +++---------
>   1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 34b12a3..0be52ae 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -648,17 +648,11 @@ target_ulong spapr_rtas_call(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>
>   void spapr_rtas_register(int token, const char *name, spapr_rtas_fn fn)
>   {
> -    if (!((token >= RTAS_TOKEN_BASE) && (token < RTAS_TOKEN_MAX))) {
> -        fprintf(stderr, "RTAS invalid token 0x%x\n", token);
> -        exit(1);
> -    }
> +    assert((token >= RTAS_TOKEN_BASE) && (token < RTAS_TOKEN_MAX));
>
>       token -= RTAS_TOKEN_BASE;
> -    if (rtas_table[token].name) {
> -        fprintf(stderr, "RTAS call \"%s\" is registered already as 0x%x\n",
> -                rtas_table[token].name, token);
> -        exit(1);
> -    }
> +
> +    assert(!rtas_table[token].name);
>
>       rtas_table[token].name = name;
>       rtas_table[token].fn = fn;
>


-- 
Alexey

Re: [Qemu-devel] [PATCHv4 3/8] pseries: Clean up error handling in spapr_validate_node_memory()

[Alexey Kardashevskiy]

On 01/19/2016 02:39 PM, David Gibson wrote:
> Use error_setg() and return an error, rather than using an explicit exit().
>
> Also improve messages, and be more explicit about which constraint failed.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>


Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>

> ---
>   hw/ppc/spapr.c | 37 ++++++++++++++++++++++---------------
>   1 file changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b7fd09a..fb0e254 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1691,27 +1691,34 @@ static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr)
>    * to SPAPR_MEMORY_BLOCK_SIZE(256MB), then refuse to start the guest
>    * since we can't support such unaligned sizes with DRCONF_MEMORY.
>    */
> -static void spapr_validate_node_memory(MachineState *machine)
> +static void spapr_validate_node_memory(MachineState *machine, Error **errp)
>   {
>       int i;
>
> -    if (machine->maxram_size % SPAPR_MEMORY_BLOCK_SIZE ||
> -        machine->ram_size % SPAPR_MEMORY_BLOCK_SIZE) {
> -        error_report("Can't support memory configuration where RAM size "
> -                     "0x" RAM_ADDR_FMT " or maxmem size "
> -                     "0x" RAM_ADDR_FMT " isn't aligned to %llu MB",
> -                     machine->ram_size, machine->maxram_size,
> -                     SPAPR_MEMORY_BLOCK_SIZE/M_BYTE);
> -        exit(EXIT_FAILURE);
> +    if (machine->ram_size % SPAPR_MEMORY_BLOCK_SIZE) {
> +        error_setg(errp, "Memory size 0x" RAM_ADDR_FMT
> +                   " is not aligned to %llu MiB",
> +                   machine->ram_size,
> +                   SPAPR_MEMORY_BLOCK_SIZE / M_BYTE);
> +        return;
> +    }
> +
> +    if (machine->maxram_size % SPAPR_MEMORY_BLOCK_SIZE) {
> +        error_setg(errp, "Maximum memory size 0x" RAM_ADDR_FMT
> +                   " is not aligned to %llu MiB",
> +                   machine->ram_size,
> +                   SPAPR_MEMORY_BLOCK_SIZE / M_BYTE);
> +        return;
>       }
>
>       for (i = 0; i < nb_numa_nodes; i++) {
>           if (numa_info[i].node_mem % SPAPR_MEMORY_BLOCK_SIZE) {
> -            error_report("Can't support memory configuration where memory size"
> -                         " %" PRIx64 " of node %d isn't aligned to %llu MB",
> -                         numa_info[i].node_mem, i,
> -                         SPAPR_MEMORY_BLOCK_SIZE/M_BYTE);
> -            exit(EXIT_FAILURE);
> +            error_setg(errp,
> +                       "Node %d memory size 0x" RAM_ADDR_FMT
> +                       " is not aligned to %llu MiB",
> +                       i, numa_info[i].node_mem,
> +                       SPAPR_MEMORY_BLOCK_SIZE / M_BYTE);
> +            return;
>           }
>       }
>   }
> @@ -1801,7 +1808,7 @@ static void ppc_spapr_init(MachineState *machine)
>                                     XICS_IRQS);
>
>       if (smc->dr_lmb_enabled) {
> -        spapr_validate_node_memory(machine);
> +        spapr_validate_node_memory(machine, &error_fatal);
>       }
>
>       /* init CPUs */
>


-- 
Alexey

Re: [Qemu-devel] [PATCHv4 8/8] pseries: Clean up error reporting in htab migration functions

[Markus Armbruster]

David Gibson <david@gibson.dropbear.id.au> writes:

> The functions for migrating the hash page table on pseries machine type
> (htab_save_setup() and htab_load()) can report some errors with an
> explicit fprintf() before returning an appropriate error code.  Change these
> to use error_report() instead.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/ppc/spapr.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 3cfacb9..1eb7d03 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c

Lost this hunk:

  @@ -1309,8 +1309,9 @@ static int htab_save_setup(QEMUFile *f, void *opaque)
           spapr->htab_fd = kvmppc_get_htab_fd(false);
           spapr->htab_fd_stale = false;
           if (spapr->htab_fd < 0) {
  -            fprintf(stderr, "Unable to open fd for reading hash table from KVM: %s\n",
  -                    strerror(errno));
  +            error_report(
  +                "Unable to open fd for reading hash table from KVM: %s",
  +                strerror(errno));
               return -1;
           }
       }

Intentional?

> @@ -1526,7 +1526,7 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
>      int fd = -1;
>  
>      if (version_id < 1 || version_id > 1) {
> -        fprintf(stderr, "htab_load() bad version\n");
> +        error_report("htab_load() bad version");
>          return -EINVAL;
>      }
>  
> @@ -1547,8 +1547,8 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
>  
>          fd = kvmppc_get_htab_fd(true);
>          if (fd < 0) {
> -            fprintf(stderr, "Unable to open fd to restore KVM hash table: %s\n",
> -                    strerror(errno));
> +            error_report("Unable to open fd to restore KVM hash table: %s",
> +                         strerror(errno));
>          }
>      }
>  
> @@ -1568,9 +1568,9 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
>          if ((index + n_valid + n_invalid) >
>              (HTAB_SIZE(spapr) / HASH_PTE_SIZE_64)) {
>              /* Bad index in stream */
> -            fprintf(stderr, "htab_load() bad index %d (%hd+%hd entries) "
> -                    "in htab stream (htab_shift=%d)\n", index, n_valid, n_invalid,
> -                    spapr->htab_shift);
> +            error_report(
> +                "htab_load() bad index %d (%hd+%hd entries) in htab stream (htab_shift=%d)",
> +                index, n_valid, n_invalid, spapr->htab_shift);
>              return -EINVAL;
>          }

Re: [Qemu-devel] [PATCHv4 8/8] pseries: Clean up error reporting in htab migration functions

[David Gibson]

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

On Tue, Jan 19, 2016 at 08:44:51AM +0100, Markus Armbruster wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > The functions for migrating the hash page table on pseries machine type
> > (htab_save_setup() and htab_load()) can report some errors with an
> > explicit fprintf() before returning an appropriate error code.  Change these
> > to use error_report() instead.
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > Reviewed-by: Thomas Huth <thuth@redhat.com>
> > ---
> >  hw/ppc/spapr.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 3cfacb9..1eb7d03 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> 
> Lost this hunk:
> 
>   @@ -1309,8 +1309,9 @@ static int htab_save_setup(QEMUFile *f, void *opaque)
>            spapr->htab_fd = kvmppc_get_htab_fd(false);
>            spapr->htab_fd_stale = false;
>            if (spapr->htab_fd < 0) {
>   -            fprintf(stderr, "Unable to open fd for reading hash table from KVM: %s\n",
>   -                    strerror(errno));
>   +            error_report(
>   +                "Unable to open fd for reading hash table from KVM: %s",
>   +                strerror(errno));
>                return -1;
>            }
>        }
> 
> Intentional?

Yes.  As noted in the cover letter, this conflicts with another series
I'm working on, which will obsolete this change anyway.

-- 
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] [PATCHv4 8/8] pseries: Clean up error reporting in htab migration functions

[Markus Armbruster]

David Gibson <david@gibson.dropbear.id.au> writes:

> On Tue, Jan 19, 2016 at 08:44:51AM +0100, Markus Armbruster wrote:
>> David Gibson <david@gibson.dropbear.id.au> writes:
>> 
>> > The functions for migrating the hash page table on pseries machine type
>> > (htab_save_setup() and htab_load()) can report some errors with an
>> > explicit fprintf() before returning an appropriate error code.  Change these
>> > to use error_report() instead.
>> >
>> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>> > Reviewed-by: Thomas Huth <thuth@redhat.com>
>> > ---
>> >  hw/ppc/spapr.c | 12 ++++++------
>> >  1 file changed, 6 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> > index 3cfacb9..1eb7d03 100644
>> > --- a/hw/ppc/spapr.c
>> > +++ b/hw/ppc/spapr.c
>> 
>> Lost this hunk:
>> 
>>   @@ -1309,8 +1309,9 @@ static int htab_save_setup(QEMUFile *f, void *opaque)
>>            spapr->htab_fd = kvmppc_get_htab_fd(false);
>>            spapr->htab_fd_stale = false;
>>            if (spapr->htab_fd < 0) {
>>   -            fprintf(stderr, "Unable to open fd for reading hash table from KVM: %s\n",
>>   -                    strerror(errno));
>>   +            error_report(
>>   +                "Unable to open fd for reading hash table from KVM: %s",
>>   +                strerror(errno));
>>                return -1;
>>            }
>>        }
>> 
>> Intentional?
>
> Yes.  As noted in the cover letter, this conflicts with another series
> I'm working on, which will obsolete this change anyway.

The patch doesn't quite hold what the commit message's promises, but I
guess that's okay.  An "except for htab_save_setup()" in the commit
message wouldn't hurt, though.

Re: [Qemu-devel] [PATCHv4 0/8] Cleanups to error reporting on ppc and spapr

[Markus Armbruster]

David Gibson <david@gibson.dropbear.id.au> writes:

> Another spin of my patches to clean up a bunch of error reporting in
> the pseries machine type and target-ppc code, to better use the error
> API.
>
> Once reviewed, I hope to merge this into ppc-for-2.6 shortly.

Thanks for helping with error cleanup.

> Changes in v4:
>  * Corrected a place I'd accidentally messed up the indenting
>  * Dropped the patch changing the htab allocation path and some hunks
>    of the migration path cleanups, since they will be obsoleted by
>    other things I'm working on
> Changes in v3:
>  * Adjusted a commit message for accuracy (suggest by Markus)
>  * Dropped a patch which relied on a wrong guess about the behaviour
>    of foreach_dynamic_sysbus_device().
> Changes in v2:
>  * Assorted minor tweaks based on review
>       
> David Gibson (8):
>   ppc: Cleanup error handling in ppc_set_compat()
>   pseries: Cleanup error handling of spapr_cpu_init()
>   pseries: Clean up error handling in spapr_validate_node_memory()
>   pseries: Cleanup error handling in spapr_vga_init()
>   pseries: Clean up error handling in spapr_rtas_register()
>   pseries: Clean up error handling in xics_system_init()
>   pseries: Clean up error reporting in ppc_spapr_init()
>   pseries: Clean up error reporting in htab migration functions

I think the two "Cleanup" should be spelled "Clean up".

Series
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCHv4 8/8] pseries: Clean up error reporting in htab migration functions

[David Gibson]

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

On Tue, Jan 19, 2016 at 01:21:34PM +0100, Markus Armbruster wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > On Tue, Jan 19, 2016 at 08:44:51AM +0100, Markus Armbruster wrote:
> >> David Gibson <david@gibson.dropbear.id.au> writes:
> >> 
> >> > The functions for migrating the hash page table on pseries machine type
> >> > (htab_save_setup() and htab_load()) can report some errors with an
> >> > explicit fprintf() before returning an appropriate error code.  Change these
> >> > to use error_report() instead.
> >> >
> >> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >> > Reviewed-by: Thomas Huth <thuth@redhat.com>
> >> > ---
> >> >  hw/ppc/spapr.c | 12 ++++++------
> >> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >> >
> >> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> > index 3cfacb9..1eb7d03 100644
> >> > --- a/hw/ppc/spapr.c
> >> > +++ b/hw/ppc/spapr.c
> >> 
> >> Lost this hunk:
> >> 
> >>   @@ -1309,8 +1309,9 @@ static int htab_save_setup(QEMUFile *f, void *opaque)
> >>            spapr->htab_fd = kvmppc_get_htab_fd(false);
> >>            spapr->htab_fd_stale = false;
> >>            if (spapr->htab_fd < 0) {
> >>   -            fprintf(stderr, "Unable to open fd for reading hash table from KVM: %s\n",
> >>   -                    strerror(errno));
> >>   +            error_report(
> >>   +                "Unable to open fd for reading hash table from KVM: %s",
> >>   +                strerror(errno));
> >>                return -1;
> >>            }
> >>        }
> >> 
> >> Intentional?
> >
> > Yes.  As noted in the cover letter, this conflicts with another series
> > I'm working on, which will obsolete this change anyway.
> 
> The patch doesn't quite hold what the commit message's promises, but I
> guess that's okay.  An "except for htab_save_setup()" in the commit
> message wouldn't hurt, though.

Ok.

-- 
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] [PATCHv4 0/8] Cleanups to error reporting on ppc and spapr

[David Gibson]

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

On Tue, Jan 19, 2016 at 01:23:13PM +0100, Markus Armbruster wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > Another spin of my patches to clean up a bunch of error reporting in
> > the pseries machine type and target-ppc code, to better use the error
> > API.
> >
> > Once reviewed, I hope to merge this into ppc-for-2.6 shortly.
> 
> Thanks for helping with error cleanup.
> 
> > Changes in v4:
> >  * Corrected a place I'd accidentally messed up the indenting
> >  * Dropped the patch changing the htab allocation path and some hunks
> >    of the migration path cleanups, since they will be obsoleted by
> >    other things I'm working on
> > Changes in v3:
> >  * Adjusted a commit message for accuracy (suggest by Markus)
> >  * Dropped a patch which relied on a wrong guess about the behaviour
> >    of foreach_dynamic_sysbus_device().
> > Changes in v2:
> >  * Assorted minor tweaks based on review
> >       
> > David Gibson (8):
> >   ppc: Cleanup error handling in ppc_set_compat()
> >   pseries: Cleanup error handling of spapr_cpu_init()
> >   pseries: Clean up error handling in spapr_validate_node_memory()
> >   pseries: Cleanup error handling in spapr_vga_init()
> >   pseries: Clean up error handling in spapr_rtas_register()
> >   pseries: Clean up error handling in xics_system_init()
> >   pseries: Clean up error reporting in ppc_spapr_init()
> >   pseries: Clean up error reporting in htab migration functions
> 
> I think the two "Cleanup" should be spelled "Clean up".
> 
> Series
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks I'll make that trivial change and fold into ppc-for-2.6.


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