[Qemu-devel] [PATCH 00/11] Error handling cleanups for pseries machine type

[Qemu-devel] [PATCH 00/11] Error handling cleanups for pseries machine type

[David Gibson]

The code for the pseries machine type is a bit of a hodge podge in
terms of error handling.  Some parts use the modern error handling
APIs, others use old explicit fprintf()s and exit()s.

This patch makes a start on cleaning this up, modernizing the error
handling in a number of places to use the current APIs.

After this series there are still quite a lot of places using explicit
fprintf()s and exit()s, particularly in the device tree handling code.
I hope to get to those eventually, but in the meantime, something is
better than nothing, right?

RESEND: Sorry, forgot to CC the lists the first time around.
Apologies to those who get it twice.

David Gibson (11):
  ppc: Cleanup error handling in ppc_set_compat()
  pseries: Cleanup error handling of spapr_cpu_init()
  pseries: Clean up hash page table allocation error handling
  pseries: Clean up error handling in spapr_validate_node_memory()
  pseries: Cleanup error handling in spapr_vga_init()
  pseries: Improve error handling in find_unknown_sysbus_device()
  pseries: Cleanup error handling in spapr_kvm_type()
  pseries: Clean up error handling in spapr_rtas_register()
  pseries: Clean up error handling in xics_system_init()
  pseries: Clean up error handling in ppc_spapr_init()
  pseries: Clean up error reporting in htab migration functions

 hw/ppc/spapr.c              | 174 ++++++++++++++++++++++++--------------------
 hw/ppc/spapr_hcall.c        |  10 +--
 hw/ppc/spapr_rtas.c         |   9 +--
 target-ppc/cpu.h            |   2 +-
 target-ppc/translate_init.c |  13 ++--
 5 files changed, 111 insertions(+), 97 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH 01/11] 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.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 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 2d57ab0..1a5500f 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1633,9 +1633,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 e88dc7f..83e5332 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(errp, "Unable to set CPU compatibility mode in KVM: %s",
+                       strerror(-ret));
+        }
     }
-
-    return ret;
 }
 
 static gint ppc_cpu_compare_class_pvr(gconstpointer a, gconstpointer b)
-- 
2.5.0

Re: [Qemu-devel] [PATCH 01/11] ppc: Cleanup error handling in ppc_set_compat()

[Thomas Huth]

On 11/12/15 01:11, 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.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  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 2d57ab0..1a5500f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1633,9 +1633,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 e88dc7f..83e5332 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(errp, "Unable to set CPU compatibility mode in KVM: %s",
> +                       strerror(-ret));
> +        }
>      }
> -
> -    return ret;
>  }
>  
>  static gint ppc_cpu_compare_class_pvr(gconstpointer a, gconstpointer b)
> 

Looks good.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 01/11] ppc: Cleanup error handling in ppc_set_compat()

[Eric Blake]

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

On 12/10/2015 05:11 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.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

> @@ -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(errp, "Unable to set CPU compatibility mode in KVM: %s",
> +                       strerror(-ret));
> +        }

Could use error_setg_errno() here instead of manually calling strerror().

-- 
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 01/11] ppc: Cleanup error handling in ppc_set_compat()

[David Gibson]

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

On Fri, Dec 11, 2015 at 06:58:12AM -0700, Eric Blake wrote:
> On 12/10/2015 05:11 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.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> 
> > @@ -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(errp, "Unable to set CPU compatibility mode in KVM: %s",
> > +                       strerror(-ret));
> > +        }
> 
> Could use error_setg_errno() here instead of manually calling strerror().

Ah, thanks, I hadn't spotted that function before.

-- 
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 02/11] 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
the error infrastructure instead of an old-style explicit fprintf / exit.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 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 1a5500f..91396cc 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1615,7 +1615,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;
 
@@ -1633,7 +1634,7 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu)
     }
 
     if (cpu->max_compat) {
-        ppc_set_compat(cpu, cpu->max_compat, &error_fatal);
+        ppc_set_compat(cpu, cpu->max_compat, errp);
     }
 
     xics_cpu_setup(spapr->icp, cpu);
@@ -1802,10 +1803,9 @@ 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");
-            exit(1);
+            error_setg(&error_fatal, "Unable to find PowerPC CPU definition");
         }
-        spapr_cpu_init(spapr, cpu);
+        spapr_cpu_init(spapr, cpu, &error_fatal);
     }
 
     if (kvm_enabled()) {
-- 
2.5.0

Re: [Qemu-devel] [PATCH 02/11] pseries: Cleanup error handling of spapr_cpu_init()

[Bharata B Rao]

On Fri, Dec 11, 2015 at 5:41 AM, David Gibson
<david@gibson.dropbear.id.au> 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
> the error infrastructure instead of an old-style explicit fprintf / exit.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  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 1a5500f..91396cc 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1615,7 +1615,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;
>
> @@ -1633,7 +1634,7 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu)
>      }
>
>      if (cpu->max_compat) {
> -        ppc_set_compat(cpu, cpu->max_compat, &error_fatal);
> +        ppc_set_compat(cpu, cpu->max_compat, errp);
>      }
>
>      xics_cpu_setup(spapr->icp, cpu);
> @@ -1802,10 +1803,9 @@ 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");
> -            exit(1);
> +            error_setg(&error_fatal, "Unable to find PowerPC CPU definition");
>          }
> -        spapr_cpu_init(spapr, cpu);
> +        spapr_cpu_init(spapr, cpu, &error_fatal);

Using error_fatal is fine here for now, but just want to sound out
that with CPU hotplug, spapr_cpu_init() would move to ->plug() where
we can't be calling this unconditionally with error_fatal as we need a
graceful recovery for hotplugged CPUs and termination for boot CPUs.

Otherwise, Reviewed-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

Regards,
Bharata.

Re: [Qemu-devel] [PATCH 02/11] pseries: Cleanup error handling of spapr_cpu_init()

[David Gibson]

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

On Fri, Dec 11, 2015 at 02:15:38PM +0530, Bharata B Rao wrote:
> On Fri, Dec 11, 2015 at 5:41 AM, David Gibson
> <david@gibson.dropbear.id.au> 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
> > the error infrastructure instead of an old-style explicit fprintf / exit.
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  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 1a5500f..91396cc 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1615,7 +1615,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;
> >
> > @@ -1633,7 +1634,7 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu)
> >      }
> >
> >      if (cpu->max_compat) {
> > -        ppc_set_compat(cpu, cpu->max_compat, &error_fatal);
> > +        ppc_set_compat(cpu, cpu->max_compat, errp);
> >      }
> >
> >      xics_cpu_setup(spapr->icp, cpu);
> > @@ -1802,10 +1803,9 @@ 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");
> > -            exit(1);
> > +            error_setg(&error_fatal, "Unable to find PowerPC CPU definition");
> >          }
> > -        spapr_cpu_init(spapr, cpu);
> > +        spapr_cpu_init(spapr, cpu, &error_fatal);
> 
> Using error_fatal is fine here for now, but just want to sound out
> that with CPU hotplug, spapr_cpu_init() would move to ->plug() where
> we can't be calling this unconditionally with error_fatal as we need a
> graceful recovery for hotplugged CPUs and termination for boot CPUs.

Right... part of the point of the patch is that the &error_fatal is
moved out of spapr_cpu_init() into the caller.  Hotplug will add a new
caller which can use a different error handler.

> Otherwise, Reviewed-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> 
> Regards,
> Bharata.
> 

-- 
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 02/11] pseries: Cleanup error handling of spapr_cpu_init()

[Eric Blake]

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

On 12/10/2015 05:11 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
> the error infrastructure instead of an old-style explicit fprintf / exit.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

> @@ -1633,7 +1634,7 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu)
>      }
>  
>      if (cpu->max_compat) {
> -        ppc_set_compat(cpu, cpu->max_compat, &error_fatal);
> +        ppc_set_compat(cpu, cpu->max_compat, errp);
>      }
>  
>      xics_cpu_setup(spapr->icp, cpu);

Pre-patch: you can't reach the xics_cpu_setup() call on error.

Post-patch: depending on what the caller passed in, you can fall through
to xics_cpu_setup() with a potentially incomplete cpu.

I think a more robust solution is probably along the lines of:

Error *err = NULL;
if (cpu->max_compat) {
    ppc_set_compat(cpu, cpu->max_compat, &err);
    if (err) {
        error_propagate(errp, err);
        return;
    }
}
xics_cpu_setup(spapr_icp, cpu);

> @@ -1802,10 +1803,9 @@ 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");
> -            exit(1);
> +            error_setg(&error_fatal, "Unable to find PowerPC CPU definition");
>          }
> -        spapr_cpu_init(spapr, cpu);
> +        spapr_cpu_init(spapr, cpu, &error_fatal);

Of course, in _this_ patch, since the (only) caller is passing
&error_fatal, you don't hit the situation of fallthrough to
xics_cpu_setup().

-- 
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 02/11] pseries: Cleanup error handling of spapr_cpu_init()

[David Gibson]

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

On Fri, Dec 11, 2015 at 07:54:48AM -0700, Eric Blake wrote:
> On 12/10/2015 05:11 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
> > the error infrastructure instead of an old-style explicit fprintf / exit.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> > @@ -1633,7 +1634,7 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu)
> >      }
> >  
> >      if (cpu->max_compat) {
> > -        ppc_set_compat(cpu, cpu->max_compat, &error_fatal);
> > +        ppc_set_compat(cpu, cpu->max_compat, errp);
> >      }
> >  
> >      xics_cpu_setup(spapr->icp, cpu);
> 
> Pre-patch: you can't reach the xics_cpu_setup() call on error.
> 
> Post-patch: depending on what the caller passed in, you can fall through
> to xics_cpu_setup() with a potentially incomplete cpu.
> 
> I think a more robust solution is probably along the lines of:
> 
> Error *err = NULL;
> if (cpu->max_compat) {
>     ppc_set_compat(cpu, cpu->max_compat, &err);
>     if (err) {
>         error_propagate(errp, err);
>         return;
>     }
> }
> xics_cpu_setup(spapr_icp, cpu);

Yes, good point.  I _think_ xics_cpu_setup() would be safe to call
even if ppc_set_compat() fails, but checking for the error immediately
is safer indeed.  I'll adjust in the next spin.

-- 
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 03/11] pseries: Clean up hash page table allocation error handling

[David Gibson]

The spapr_alloc_htab() and spapr_reset_htab() functions currently handle
all errors with error_setg(&error_abort, ...).  That's correct for
spapr_reset_htab() - if anything goes wrong there, there's really nothing
we can do about it.  For spapr_alloc_htab() &error_fatal would make more
sense, since it occurs during initialization.

But in any case the callers are really better placed to decide on the error
handling.  So, instead make the functions use the error propagation
infrastructure.

While we're at it improve the messages themselves a bit, and clean up the
indentation a little.

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

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 91396cc..85474ee 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1015,7 +1015,7 @@ static void emulate_spapr_hypercall(PowerPCCPU *cpu)
 #define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= tswap64(~HPTE64_V_HPTE_DIRTY))
 #define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= tswap64(HPTE64_V_HPTE_DIRTY))
 
-static void spapr_alloc_htab(sPAPRMachineState *spapr)
+static void spapr_alloc_htab(sPAPRMachineState *spapr, Error **errp)
 {
     long shift;
     int index;
@@ -1030,7 +1030,9 @@ 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_setg(errp,
+            "Error allocating KVM hash page table, try smaller maxmem: %s",
+            strerror(-shift));
     } else if (shift > 0) {
         /*
          * Kernel handles htab, we don't need to allocate one
@@ -1039,7 +1041,10 @@ 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_setg(errp,
+                "Small allocation for KVM hash page table (%ld < %"
+                PRIu32 "), try smaller maxmem",
+                shift, spapr->htab_shift);
         }
 
         spapr->htab_shift = shift;
@@ -1063,17 +1068,22 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr)
  * If host kernel has allocated HTAB, KVM_PPC_ALLOCATE_HTAB ioctl is
  * used to clear HTAB. Otherwise QEMU-allocated HTAB is cleared manually.
  */
-static void spapr_reset_htab(sPAPRMachineState *spapr)
+static void spapr_reset_htab(sPAPRMachineState *spapr, Error **errp)
 {
     long shift;
     int index;
 
     shift = kvmppc_reset_htab(spapr->htab_shift);
     if (shift < 0) {
-        error_setg(&error_abort, "Failed to reset HTAB");
+        error_setg(errp,
+            "Error resetting KVM hash page table, try smaller maxmem: %s",
+            strerror(-shift));
     } else if (shift > 0) {
         if (shift != spapr->htab_shift) {
-            error_setg(&error_abort, "Requested HTAB allocation failed during reset");
+            error_setg(errp,
+                "Reduced size on reset of KVM hash page table (%ld < %"
+                PRIu32 "), try smaller maxmem",
+                shift, spapr->htab_shift);
         }
 
         /* Tell readers to update their file descriptor */
@@ -1144,7 +1154,7 @@ static void ppc_spapr_reset(void)
     foreach_dynamic_sysbus_device(find_unknown_sysbus_device, NULL);
 
     /* Reset the hash table & recalc the RMA */
-    spapr_reset_htab(spapr);
+    spapr_reset_htab(spapr, &error_abort);
 
     qemu_devices_reset();
 
@@ -1784,7 +1794,7 @@ static void ppc_spapr_init(MachineState *machine)
         }
         spapr->htab_shift++;
     }
-    spapr_alloc_htab(spapr);
+    spapr_alloc_htab(spapr, &error_fatal);
 
     /* Set up Interrupt Controller before we create the VCPUs */
     spapr->icp = xics_system_init(machine,
-- 
2.5.0

Re: [Qemu-devel] [PATCH 03/11] pseries: Clean up hash page table allocation error handling

[Bharata B Rao]

On Fri, Dec 11, 2015 at 5:41 AM, David Gibson
<david@gibson.dropbear.id.au> wrote:
> The spapr_alloc_htab() and spapr_reset_htab() functions currently handle
> all errors with error_setg(&error_abort, ...).  That's correct for
> spapr_reset_htab() - if anything goes wrong there, there's really nothing
> we can do about it.  For spapr_alloc_htab() &error_fatal would make more
> sense, since it occurs during initialization.
>
> But in any case the callers are really better placed to decide on the error
> handling.  So, instead make the functions use the error propagation
> infrastructure.
>
> While we're at it improve the messages themselves a bit, and clean up the
> indentation a little.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 91396cc..85474ee 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1015,7 +1015,7 @@ static void emulate_spapr_hypercall(PowerPCCPU *cpu)
>  #define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= tswap64(~HPTE64_V_HPTE_DIRTY))
>  #define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= tswap64(HPTE64_V_HPTE_DIRTY))
>
> -static void spapr_alloc_htab(sPAPRMachineState *spapr)
> +static void spapr_alloc_htab(sPAPRMachineState *spapr, Error **errp)
>  {
>      long shift;
>      int index;
> @@ -1030,7 +1030,9 @@ 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_setg(errp,
> +            "Error allocating KVM hash page table, try smaller maxmem: %s",
> +            strerror(-shift));

Do you want to explicitly "return" from here and other places in this
patch like you do in 4/11 to improve readability ?

Regards,
Bharata.

Re: [Qemu-devel] [PATCH 03/11] pseries: Clean up hash page table allocation error handling

[David Gibson]

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

On Fri, Dec 11, 2015 at 02:10:40PM +0530, Bharata B Rao wrote:
> On Fri, Dec 11, 2015 at 5:41 AM, David Gibson
> <david@gibson.dropbear.id.au> wrote:
> > The spapr_alloc_htab() and spapr_reset_htab() functions currently handle
> > all errors with error_setg(&error_abort, ...).  That's correct for
> > spapr_reset_htab() - if anything goes wrong there, there's really nothing
> > we can do about it.  For spapr_alloc_htab() &error_fatal would make more
> > sense, since it occurs during initialization.
> >
> > But in any case the callers are really better placed to decide on the error
> > handling.  So, instead make the functions use the error propagation
> > infrastructure.
> >
> > While we're at it improve the messages themselves a bit, and clean up the
> > indentation a little.
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr.c | 26 ++++++++++++++++++--------
> >  1 file changed, 18 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 91396cc..85474ee 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1015,7 +1015,7 @@ static void emulate_spapr_hypercall(PowerPCCPU *cpu)
> >  #define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= tswap64(~HPTE64_V_HPTE_DIRTY))
> >  #define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= tswap64(HPTE64_V_HPTE_DIRTY))
> >
> > -static void spapr_alloc_htab(sPAPRMachineState *spapr)
> > +static void spapr_alloc_htab(sPAPRMachineState *spapr, Error **errp)
> >  {
> >      long shift;
> >      int index;
> > @@ -1030,7 +1030,9 @@ 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_setg(errp,
> > +            "Error allocating KVM hash page table, try smaller maxmem: %s",
> > +            strerror(-shift));
> 
> Do you want to explicitly "return" from here and other places in this
> patch like you do in 4/11 to improve readability ?

Given we still need the if (shift > 0), I don't think it really makes
any difference to readability.

-- 
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 03/11] pseries: Clean up hash page table allocation error handling

[Eric Blake]

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

On 12/10/2015 05:11 PM, David Gibson wrote:
> The spapr_alloc_htab() and spapr_reset_htab() functions currently handle
> all errors with error_setg(&error_abort, ...).  That's correct for
> spapr_reset_htab() - if anything goes wrong there, there's really nothing
> we can do about it.  For spapr_alloc_htab() &error_fatal would make more
> sense, since it occurs during initialization.
> 
> But in any case the callers are really better placed to decide on the error
> handling.  So, instead make the functions use the error propagation
> infrastructure.
> 
> While we're at it improve the messages themselves a bit, and clean up the
> indentation a little.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)

> @@ -1030,7 +1030,9 @@ 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_setg(errp,
> +            "Error allocating KVM hash page table, try smaller maxmem: %s",
> +            strerror(-shift));

error_setg_errno() is nicer than calling strerror().

-- 
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 03/11] pseries: Clean up hash page table allocation error handling

[David Gibson]

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

On Fri, Dec 11, 2015 at 08:01:39AM -0700, Eric Blake wrote:
> On 12/10/2015 05:11 PM, David Gibson wrote:
> > The spapr_alloc_htab() and spapr_reset_htab() functions currently handle
> > all errors with error_setg(&error_abort, ...).  That's correct for
> > spapr_reset_htab() - if anything goes wrong there, there's really nothing
> > we can do about it.  For spapr_alloc_htab() &error_fatal would make more
> > sense, since it occurs during initialization.
> > 
> > But in any case the callers are really better placed to decide on the error
> > handling.  So, instead make the functions use the error propagation
> > infrastructure.
> > 
> > While we're at it improve the messages themselves a bit, and clean up the
> > indentation a little.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr.c | 26 ++++++++++++++++++--------
> >  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> > @@ -1030,7 +1030,9 @@ 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_setg(errp,
> > +            "Error allocating KVM hash page table, try smaller maxmem: %s",
> > +            strerror(-shift));
> 
> error_setg_errno() is nicer than calling strerror().

Will adjust in next spin, thanks.

-- 
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 04/11] 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>
---
 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 85474ee..b478ca6 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1693,27 +1693,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;
         }
     }
 }
@@ -1803,7 +1810,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

Re: [Qemu-devel] [PATCH 04/11] pseries: Clean up error handling in spapr_validate_node_memory()

[Bharata B Rao]

On Fri, Dec 11, 2015 at 5:41 AM, David Gibson
<david@gibson.dropbear.id.au> 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>

> ---
>  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 85474ee..b478ca6 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1693,27 +1693,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;
>          }
>      }
>  }
> @@ -1803,7 +1810,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
>
>



-- 
http://raobharata.wordpress.com/

[Qemu-devel] [PATCH 05/11] 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.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 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 b478ca6..0ff09b9 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1246,7 +1246,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 int spapr_vga_init(PCIBus *pci_bus, Error **errp)
 {
     switch (vga_interface_type) {
     case VGA_NONE:
@@ -1257,9 +1257,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

Re: [Qemu-devel] [PATCH 05/11] pseries: Cleanup error handling in spapr_vga_init()

[Thomas Huth]

On 11/12/15 01:11, 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.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  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 b478ca6..0ff09b9 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1246,7 +1246,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 int spapr_vga_init(PCIBus *pci_bus, Error **errp)

While you're at it, you could also change the return type from "int" to
"bool".

>  {
>      switch (vga_interface_type) {
>      case VGA_NONE:
> @@ -1257,9 +1257,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;
>      }
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 05/11] pseries: Cleanup error handling in spapr_vga_init()

[David Gibson]

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

On Fri, Dec 11, 2015 at 10:35:58AM +0100, Thomas Huth wrote:
> On 11/12/15 01:11, 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.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  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 b478ca6..0ff09b9 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1246,7 +1246,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 int spapr_vga_init(PCIBus *pci_bus, Error **errp)
> 
> While you're at it, you could also change the return type from "int" to
> "bool".

Good point, done.

> 
> >  {
> >      switch (vga_interface_type) {
> >      case VGA_NONE:
> > @@ -1257,9 +1257,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;
> >      }
> > 
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

-- 
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 06/11] pseries: Improve error handling in find_unknown_sysbus_device()

[David Gibson]

Use error_setg() to return an error instead of using an explicit exit().

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

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 0ff09b9..fd16db4 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1107,6 +1107,7 @@ static void spapr_reset_htab(sPAPRMachineState *spapr, Error **errp)
 
 static int find_unknown_sysbus_device(SysBusDevice *sbdev, void *opaque)
 {
+    Error **errp = opaque;
     bool matched = false;
 
     if (object_dynamic_cast(OBJECT(sbdev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
@@ -1114,9 +1115,9 @@ static int find_unknown_sysbus_device(SysBusDevice *sbdev, void *opaque)
     }
 
     if (!matched) {
-        error_report("Device %s is not supported by this machine yet.",
-                     qdev_fw_name(DEVICE(sbdev)));
-        exit(1);
+        error_setg(errp,
+                   "Device %s is not supported by this machine yet",
+                   qdev_fw_name(DEVICE(sbdev)));
     }
 
     return 0;
@@ -1151,7 +1152,7 @@ static void ppc_spapr_reset(void)
     uint32_t rtas_limit;
 
     /* Check for unknown sysbus devices */
-    foreach_dynamic_sysbus_device(find_unknown_sysbus_device, NULL);
+    foreach_dynamic_sysbus_device(find_unknown_sysbus_device, &error_abort);
 
     /* Reset the hash table & recalc the RMA */
     spapr_reset_htab(spapr, &error_abort);
-- 
2.5.0

Re: [Qemu-devel] [PATCH 06/11] pseries: Improve error handling in find_unknown_sysbus_device()

[Thomas Huth]

On 11/12/15 01:11, David Gibson wrote:
> Use error_setg() to return an error instead of using an explicit exit().
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 0ff09b9..fd16db4 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1107,6 +1107,7 @@ static void spapr_reset_htab(sPAPRMachineState *spapr, Error **errp)
>  
>  static int find_unknown_sysbus_device(SysBusDevice *sbdev, void *opaque)
>  {
> +    Error **errp = opaque;
>      bool matched = false;
>  
>      if (object_dynamic_cast(OBJECT(sbdev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
> @@ -1114,9 +1115,9 @@ static int find_unknown_sysbus_device(SysBusDevice *sbdev, void *opaque)
>      }
>  
>      if (!matched) {
> -        error_report("Device %s is not supported by this machine yet.",
> -                     qdev_fw_name(DEVICE(sbdev)));
> -        exit(1);
> +        error_setg(errp,
> +                   "Device %s is not supported by this machine yet",
> +                   qdev_fw_name(DEVICE(sbdev)));
>      }
>  
>      return 0;
> @@ -1151,7 +1152,7 @@ static void ppc_spapr_reset(void)
>      uint32_t rtas_limit;
>  
>      /* Check for unknown sysbus devices */
> -    foreach_dynamic_sysbus_device(find_unknown_sysbus_device, NULL);
> +    foreach_dynamic_sysbus_device(find_unknown_sysbus_device, &error_abort);
>  
>      /* Reset the hash table & recalc the RMA */
>      spapr_reset_htab(spapr, &error_abort);

Can this error be triggered by the user (by plugging certain devices or
so)? If so, I'm not sure whether error_abort is the right thing to use
here ... it's always ugly if the user can trigger an abort directly.
Maybe error_fatal would be better instead?

 Thomas

Re: [Qemu-devel] [PATCH 06/11] pseries: Improve error handling in find_unknown_sysbus_device()

[David Gibson]

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

On Fri, Dec 11, 2015 at 10:49:40AM +0100, Thomas Huth wrote:
> On 11/12/15 01:11, David Gibson wrote:
> > Use error_setg() to return an error instead of using an explicit exit().
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 0ff09b9..fd16db4 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1107,6 +1107,7 @@ static void spapr_reset_htab(sPAPRMachineState *spapr, Error **errp)
> >  
> >  static int find_unknown_sysbus_device(SysBusDevice *sbdev, void *opaque)
> >  {
> > +    Error **errp = opaque;
> >      bool matched = false;
> >  
> >      if (object_dynamic_cast(OBJECT(sbdev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
> > @@ -1114,9 +1115,9 @@ static int find_unknown_sysbus_device(SysBusDevice *sbdev, void *opaque)
> >      }
> >  
> >      if (!matched) {
> > -        error_report("Device %s is not supported by this machine yet.",
> > -                     qdev_fw_name(DEVICE(sbdev)));
> > -        exit(1);
> > +        error_setg(errp,
> > +                   "Device %s is not supported by this machine yet",
> > +                   qdev_fw_name(DEVICE(sbdev)));
> >      }
> >  
> >      return 0;
> > @@ -1151,7 +1152,7 @@ static void ppc_spapr_reset(void)
> >      uint32_t rtas_limit;
> >  
> >      /* Check for unknown sysbus devices */
> > -    foreach_dynamic_sysbus_device(find_unknown_sysbus_device, NULL);
> > +    foreach_dynamic_sysbus_device(find_unknown_sysbus_device, &error_abort);
> >  
> >      /* Reset the hash table & recalc the RMA */
> >      spapr_reset_htab(spapr, &error_abort);
> 
> Can this error be triggered by the user (by plugging certain devices or
> so)? If so, I'm not sure whether error_abort is the right thing to use
> here ... it's always ugly if the user can trigger an abort directly.
> Maybe error_fatal would be better instead?

Hm.  So, yes, I think it can be triggered by the user.  I used
&error_abort on the grounds that this is during reset - after the
initial setup phase.

Markus, do you have an opinion on what the right handling here is?

-- 
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 06/11] pseries: Improve error handling in find_unknown_sysbus_device()

[Eric Blake]

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

On 12/10/2015 05:11 PM, David Gibson wrote:
> Use error_setg() to return an error instead of using an explicit exit().
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 

> +++ b/hw/ppc/spapr.c
> @@ -1107,6 +1107,7 @@ static void spapr_reset_htab(sPAPRMachineState *spapr, Error **errp)
>  
>  static int find_unknown_sysbus_device(SysBusDevice *sbdev, void *opaque)
>  {

>      if (!matched) {
> -        error_report("Device %s is not supported by this machine yet.",
> -                     qdev_fw_name(DEVICE(sbdev)));
> -        exit(1);
> +        error_setg(errp,
> +                   "Device %s is not supported by this machine yet",
> +                   qdev_fw_name(DEVICE(sbdev)));
>      }
>  
>      return 0;

It looks like find_unknown_sysbus_device is designed to be called in a
loop, and that returning 0 lets the loop continue.

> @@ -1151,7 +1152,7 @@ static void ppc_spapr_reset(void)
>      uint32_t rtas_limit;
>  
>      /* Check for unknown sysbus devices */
> -    foreach_dynamic_sysbus_device(find_unknown_sysbus_device, NULL);
> +    foreach_dynamic_sysbus_device(find_unknown_sysbus_device, &error_abort);

If a caller passes something other than &error_abort as the opaque, AND
the error condition occurs more than once in the loop iteration, then
you really need to change find_unknown_sysbus_device() to return
non-zero after raising error on the first failure, so that the loop
doesn't continue on to a second pass and attempt an error_setg() onto an
already-set error.

Of course, since _this_ patch uses &error_abort as the only client, the
loop will never continue after the first failure, but it's more robust
to be clean without having to audit the callers.

-- 
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 06/11] pseries: Improve error handling in find_unknown_sysbus_device()

[David Gibson]

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

On Fri, Dec 11, 2015 at 08:15:39AM -0700, Eric Blake wrote:
> On 12/10/2015 05:11 PM, David Gibson wrote:
> > Use error_setg() to return an error instead of using an explicit exit().
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> 
> > +++ b/hw/ppc/spapr.c
> > @@ -1107,6 +1107,7 @@ static void spapr_reset_htab(sPAPRMachineState *spapr, Error **errp)
> >  
> >  static int find_unknown_sysbus_device(SysBusDevice *sbdev, void *opaque)
> >  {
> 
> >      if (!matched) {
> > -        error_report("Device %s is not supported by this machine yet.",
> > -                     qdev_fw_name(DEVICE(sbdev)));
> > -        exit(1);
> > +        error_setg(errp,
> > +                   "Device %s is not supported by this machine yet",
> > +                   qdev_fw_name(DEVICE(sbdev)));
> >      }
> >  
> >      return 0;
> 
> It looks like find_unknown_sysbus_device is designed to be called in a
> loop, and that returning 0 lets the loop continue.
> 
> > @@ -1151,7 +1152,7 @@ static void ppc_spapr_reset(void)
> >      uint32_t rtas_limit;
> >  
> >      /* Check for unknown sysbus devices */
> > -    foreach_dynamic_sysbus_device(find_unknown_sysbus_device, NULL);
> > +    foreach_dynamic_sysbus_device(find_unknown_sysbus_device, &error_abort);
> 
> If a caller passes something other than &error_abort as the opaque, AND
> the error condition occurs more than once in the loop iteration, then
> you really need to change find_unknown_sysbus_device() to return
> non-zero after raising error on the first failure, so that the loop
> doesn't continue on to a second pass and attempt an error_setg() onto an
> already-set error.

Ah, good catch.  I'll correct in the next spin.

> Of course, since _this_ patch uses &error_abort as the only client, the
> loop will never continue after the first failure, but it's more robust
> to be clean without having to audit the callers.
> 



-- 
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 07/11] pseries: Cleanup error handling in spapr_kvm_type()

[David Gibson]

Use error_setg() and &error_fatal instead of an explicit exit().

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

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index fd16db4..546d2f5 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2035,8 +2035,8 @@ static int spapr_kvm_type(const char *vm_type)
         return 2;
     }
 
-    error_report("Unknown kvm-type specified '%s'", vm_type);
-    exit(1);
+    error_setg(&error_fatal, "Unknown kvm-type specified '%s'", vm_type);
+    return 0;
 }
 
 /*
-- 
2.5.0

Re: [Qemu-devel] [PATCH 07/11] pseries: Cleanup error handling in spapr_kvm_type()

[Thomas Huth]

On 11/12/15 01:11, David Gibson wrote:
> Use error_setg() and &error_fatal instead of an explicit exit().
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index fd16db4..546d2f5 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2035,8 +2035,8 @@ static int spapr_kvm_type(const char *vm_type)
>          return 2;
>      }
>  
> -    error_report("Unknown kvm-type specified '%s'", vm_type);
> -    exit(1);
> +    error_setg(&error_fatal, "Unknown kvm-type specified '%s'", vm_type);
> +    return 0;
>  }

Honestly, I'd rather prefer the original code here. error_setg() should
IMHO be used to set an error in an "flexible" error variable. Using it
with an "hard-coded" error_fatal sounds ugly to me. And as far as I can
see, no other code in QEMU uses error_setg(&error_fatal, ...) - so we
should maybe not start with this in the spapr code as well.

If you still would like to get rid of the exit() here ... maybe you
could introduce some kind of error_report_fatal() function instead that
exits after reporting the error with error_report() ?

 Thomas

Re: [Qemu-devel] [PATCH 07/11] pseries: Cleanup error handling in spapr_kvm_type()

[David Gibson]

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

On Fri, Dec 11, 2015 at 11:01:37AM +0100, Thomas Huth wrote:
> On 11/12/15 01:11, David Gibson wrote:
> > Use error_setg() and &error_fatal instead of an explicit exit().
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index fd16db4..546d2f5 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2035,8 +2035,8 @@ static int spapr_kvm_type(const char *vm_type)
> >          return 2;
> >      }
> >  
> > -    error_report("Unknown kvm-type specified '%s'", vm_type);
> > -    exit(1);
> > +    error_setg(&error_fatal, "Unknown kvm-type specified '%s'", vm_type);
> > +    return 0;
> >  }
> 
> Honestly, I'd rather prefer the original code here. error_setg() should
> IMHO be used to set an error in an "flexible" error variable. Using it
> with an "hard-coded" error_fatal sounds ugly to me. And as far as I can
> see, no other code in QEMU uses error_setg(&error_fatal, ...) - so we
> should maybe not start with this in the spapr code as well.

Whereas I was thinking that if we have a standard way of tripping a
fatal error, we should use that, rather than using an explicit exit().


> If you still would like to get rid of the exit() here ... maybe you
> could introduce some kind of error_report_fatal() function instead that
> exits after reporting the error with error_report() ?

I don't see that there's any point adding an error_report_fatal() that
would be semantically equivalent to error_setg(&error_fatal, ...).

Markus, an opnion on the right way to do this?

-- 
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 08/11] pseries: Clean up error handling in spapr_rtas_register()

[David Gibson]

The errors detected in this function indicate problems with the rest of
the machine type code, rather than configuration or runtime problems.

Use error_setg() and &error_abort instead of explicit fprintf() and exit().

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_rtas.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 34b12a3..db4a675 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -649,15 +649,14 @@ 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);
+        error_setg(&error_abort, "RTAS invalid token 0x%x", token);
     }
 
     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);
+        error_setg(&error_abort,
+                   "RTAS call \"%s\" is registered already as 0x%x",
+                   rtas_table[token].name, token);
     }
 
     rtas_table[token].name = name;
-- 
2.5.0

Re: [Qemu-devel] [PATCH 08/11] pseries: Clean up error handling in spapr_rtas_register()

[Thomas Huth]

On 11/12/15 01:11, David Gibson wrote:
> The errors detected in this function indicate problems with the rest of
> the machine type code, rather than configuration or runtime problems.
> 
> Use error_setg() and &error_abort instead of explicit fprintf() and exit().
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_rtas.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 34b12a3..db4a675 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -649,15 +649,14 @@ 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);
> +        error_setg(&error_abort, "RTAS invalid token 0x%x", token);
>      }
>  
>      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);
> +        error_setg(&error_abort,
> +                   "RTAS call \"%s\" is registered already as 0x%x",
> +                   rtas_table[token].name, token);
>      }

As with the last patch, I also think these error_setg(&error_abort, ...)
are rather ugly. Maybe use error_report() + abort() instead?

 Thomas

[Qemu-devel] [PATCH 09/11] 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.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 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 546d2f5..c376748 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;
 
@@ -129,7 +129,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;
@@ -1808,7 +1808,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

Re: [Qemu-devel] [PATCH 09/11] pseries: Clean up error handling in xics_system_init()

[Thomas Huth]

On 11/12/15 01:11, 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.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  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 546d2f5..c376748 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;
>  
> @@ -129,7 +129,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;
> @@ -1808,7 +1808,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);
> 

Could you maybe explain in the patch description why you changed the
behavior in case of errors from "error_abort" into "error_fatal" ?

 Thomas

Re: [Qemu-devel] [PATCH 09/11] pseries: Clean up error handling in xics_system_init()

[David Gibson]

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

On Fri, Dec 11, 2015 at 11:08:59AM +0100, Thomas Huth wrote:
> On 11/12/15 01:11, 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.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  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 546d2f5..c376748 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;
> >  
> > @@ -129,7 +129,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;
> > @@ -1808,7 +1808,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);
> > 
> 
> Could you maybe explain in the patch description why you changed the
> behavior in case of errors from "error_abort" into "error_fatal" ?

Ah, yes, meant to do that but forgot.

-- 
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 10/11] pseries: Clean up error handling in ppc_spapr_init()

[David Gibson]

This function includes a number of explicit fprintf()s and exit()s for
error conditions.  Change these to use error_setg() and &error_fatal
instead.

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

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index c376748..deaf5c0 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1759,8 +1759,7 @@ static void ppc_spapr_init(MachineState *machine)
     rma_alloc_size = kvmppc_alloc_rma(&rma);
 
     if (rma_alloc_size == -1) {
-        error_report("Unable to create RMA");
-        exit(1);
+        error_setg(&error_fatal, "Unable to create RMA");
     }
 
     if (rma_alloc_size && (rma_alloc_size < node0_size)) {
@@ -1784,9 +1783,9 @@ 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);
-        exit(1);
+        error_setg(&error_fatal,
+                   "Numa node 0 has to span the RMA (%#08"HWADDR_PRIx")",
+                   spapr->rma_size);
     }
 
     /* Setup a load limit for the ramdisk leaving room for SLOF and FDT */
@@ -1850,9 +1849,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\n",
-                         machine->ram_slots, SPAPR_MAX_RAM_SLOTS);
-            exit(EXIT_FAILURE);
+            error_setg(&error_fatal,
+                       "Specified number of memory slots %" PRIu64
+                       " exceeds max supported %d",
+                       machine->ram_slots, SPAPR_MAX_RAM_SLOTS);
         }
 
         spapr->hotplug_memory.base = ROUND_UP(machine->ram_size,
@@ -1869,19 +1869,17 @@ static void ppc_spapr_init(MachineState *machine)
 
     filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, "spapr-rtas.bin");
     if (!filename) {
-        error_report("Could not find LPAR rtas '%s'", "spapr-rtas.bin");
-        exit(1);
+        error_setg(&error_fatal,
+                   "Could not find LPAR rtas '%s'", "spapr-rtas.bin");
     }
     spapr->rtas_size = get_image_size(filename);
     spapr->rtas_blob = g_malloc(spapr->rtas_size);
     if (load_image_size(filename, spapr->rtas_blob, spapr->rtas_size) < 0) {
-        error_report("Could not load LPAR rtas '%s'", filename);
-        exit(1);
+        error_setg(&error_fatal, "Could not load LPAR rtas '%s'", filename);
     }
     if (spapr->rtas_size > RTAS_MAX_SIZE) {
-        error_report("RTAS too big ! 0x%zx bytes (max is 0x%x)",
-                     (size_t)spapr->rtas_size, RTAS_MAX_SIZE);
-        exit(1);
+        error_setg(&error_fatal, "RTAS too big ! 0x%zx bytes (max is 0x%x)",
+                   (size_t)spapr->rtas_size, RTAS_MAX_SIZE);
     }
     g_free(filename);
 
@@ -1944,9 +1942,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);
-        exit(1);
+        error_setg(&error_fatal,
+                   "pSeries SLOF firmware requires >= %ldM guest RMA (Real Mode Area memory)",
+                   MIN_RMA_SLOF);
     }
 
     if (kernel_filename) {
@@ -1961,9 +1959,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));
-            exit(1);
+            error_setg(&error_fatal, "error loading %s: %s",
+                       kernel_filename, load_elf_strerror(kernel_size));
         }
 
         /* load initrd */
@@ -1975,9 +1972,9 @@ 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);
-                exit(1);
+                error_setg(&error_fatal,
+                           "could not load initial ram disk '%s'",
+                           initrd_filename);
             }
         } else {
             initrd_base = 0;
@@ -1990,13 +1987,13 @@ static void ppc_spapr_init(MachineState *machine)
     }
     filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
     if (!filename) {
-        error_report("Could not find LPAR firmware '%s'", bios_name);
-        exit(1);
+        error_setg(&error_fatal,
+                   "Could not find LPAR firmware '%s'", bios_name);
     }
     fw_size = load_image_targphys(filename, 0, FW_MAX_SIZE);
     if (fw_size <= 0) {
-        error_report("Could not load LPAR firmware '%s'", filename);
-        exit(1);
+        error_setg(&error_fatal,
+                   "Could not load LPAR firmware '%s'", filename);
     }
     g_free(filename);
 
-- 
2.5.0

[Qemu-devel] [PATCH 11/11] 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 eror code.  Change these
to use error_report() instead.

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

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index deaf5c0..c93ce09 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1318,8 +1318,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;
         }
     }
@@ -1535,7 +1536,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;
     }
 
@@ -1556,8 +1557,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));
         }
     }
 
@@ -1577,9 +1578,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)\n",
+                index, n_valid, n_invalid, spapr->htab_shift);
             return -EINVAL;
         }
 
-- 
2.5.0

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

[Thomas Huth]

On 11/12/15 01:11, 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 eror code.  Change these

s/eror/error/

> to use error_report() instead.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index deaf5c0..c93ce09 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1318,8 +1318,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;
>          }
>      }
> @@ -1535,7 +1536,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;
>      }
>  
> @@ -1556,8 +1557,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));
>          }
>      }
>  
> @@ -1577,9 +1578,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)\n",
> +                index, n_valid, n_invalid, spapr->htab_shift);
>              return -EINVAL;
>          }

With the typo fixed in the patch description:

Reviewed-by: Thomas Huth <thuth@redhat.com>

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

[Eric Blake]

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

On 12/10/2015 05:11 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 eror code.  Change these
> to use error_report() instead.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 

>  
> @@ -1577,9 +1578,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)\n",
> +                index, n_valid, n_invalid, spapr->htab_shift);

No trailing newline to error_report().

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

[David Gibson]

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

On Fri, Dec 11, 2015 at 08:22:59AM -0700, Eric Blake wrote:
> On 12/10/2015 05:11 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 eror code.  Change these
> > to use error_report() instead.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr.c | 17 +++++++++--------
> >  1 file changed, 9 insertions(+), 8 deletions(-)
> > 
> 
> >  
> > @@ -1577,9 +1578,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)\n",
> > +                index, n_valid, n_invalid, spapr->htab_shift);
> 
> No trailing newline to error_report().

Thanks.  Changed that in most of the places I removed fprintf(), but
missed this one.

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