[Qemu-devel] [PATCH 00/10] Cleanups to error reporting on ppc and spapr (v2)

[Qemu-devel] [PATCH 00/10] Cleanups to error reporting on ppc and spapr (v2)

[David Gibson]

Here's a new 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 v2:
 * Assorted minor tweaks based on review

David Gibson (10):
  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: 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              | 144 ++++++++++++++++++++++++++------------------
 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, 100 insertions(+), 81 deletions(-)

-- 
2.5.0

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

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

[Markus Armbruster]

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

> 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>
> 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 e88dc7f..d0feed0 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)

Error message now includes strerror() of the ioctl's errno.  Suggest to
mention that in the commit message.

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

[David Gibson]

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

On Fri, Jan 15, 2016 at 04:19:18PM +0100, Markus Armbruster wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > 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>
> > 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 e88dc7f..d0feed0 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)
> 
> Error message now includes strerror() of the ioctl's errno.  Suggest to
> mention that in the commit message.

Done.

-- 
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/10] 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] [PATCH 03/10] 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, ...).

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

In the callers we change to &error_fatal instead of &error_abort, since
this can be triggered by a bad configuration or kernel error rather than
indicating a programming error in qemu.

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 | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index b7fd09a..d28e349 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1016,7 +1016,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;
@@ -1031,7 +1031,8 @@ 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_errno(errp, -shift,
+                         "Error allocating KVM hash page table, try smaller maxmem");
     } else if (shift > 0) {
         /*
          * Kernel handles htab, we don't need to allocate one
@@ -1040,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;
@@ -1064,17 +1068,21 @@ 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_errno(errp, -shift,
+                   "Error resetting KVM hash page table, try smaller maxmem");
     } 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 */
@@ -1145,7 +1153,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_fatal);
 
     qemu_devices_reset();
 
@@ -1792,7 +1800,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/10] pseries: Clean up hash page table allocation error handling

[Alexey Kardashevskiy]

On 01/15/2016 11:00 PM, David Gibson wrote:
> The spapr_alloc_htab() and spapr_reset_htab() functions currently handle
> all errors with error_setg(&error_abort, ...).
>
> But really, the callers are really better placed to decide on the error
> handling.  So, instead make the functions use the error propagation
> infrastructure.
>
> In the callers we change to &error_fatal instead of &error_abort, since
> this can be triggered by a bad configuration or kernel error rather than
> indicating a programming error in qemu.
>
> 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 | 24 ++++++++++++++++--------
>   1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b7fd09a..d28e349 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1016,7 +1016,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;
> @@ -1031,7 +1031,8 @@ 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_errno(errp, -shift,
> +                         "Error allocating KVM hash page table, try smaller maxmem");
>       } else if (shift > 0) {
>           /*
>            * Kernel handles htab, we don't need to allocate one
> @@ -1040,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",



Even though it is not in the CODING_STYLE, I have not seen anyone objecting 
the very good kernel's "never break user-visible strings" rule or rejecting 
patches with user-visible strings failing to fit 80 chars limit.




-- 
Alexey

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

[David Gibson]

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

On Mon, Jan 18, 2016 at 01:44:00PM +1100, Alexey Kardashevskiy wrote:
> On 01/15/2016 11:00 PM, David Gibson wrote:
> >The spapr_alloc_htab() and spapr_reset_htab() functions currently handle
> >all errors with error_setg(&error_abort, ...).
> >
> >But really, the callers are really better placed to decide on the error
> >handling.  So, instead make the functions use the error propagation
> >infrastructure.
> >
> >In the callers we change to &error_fatal instead of &error_abort, since
> >this can be triggered by a bad configuration or kernel error rather than
> >indicating a programming error in qemu.
> >
> >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 | 24 ++++++++++++++++--------
> >  1 file changed, 16 insertions(+), 8 deletions(-)
> >
> >diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >index b7fd09a..d28e349 100644
> >--- a/hw/ppc/spapr.c
> >+++ b/hw/ppc/spapr.c
> >@@ -1016,7 +1016,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;
> >@@ -1031,7 +1031,8 @@ 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_errno(errp, -shift,
> >+                         "Error allocating KVM hash page table, try smaller maxmem");
> >      } else if (shift > 0) {
> >          /*
> >           * Kernel handles htab, we don't need to allocate one
> >@@ -1040,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",
> 
> 
> 
> Even though it is not in the CODING_STYLE, I have not seen anyone objecting
> the very good kernel's "never break user-visible strings" rule or rejecting
> patches with user-visible strings failing to fit 80 chars limit.

I'm not.  Or rather, the string is already broken by the PRIu32, so
the newline doesn't make it any less greppable.

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

[Alexey Kardashevskiy]

On 01/18/2016 03:42 PM, David Gibson wrote:
> On Mon, Jan 18, 2016 at 01:44:00PM +1100, Alexey Kardashevskiy wrote:
>> On 01/15/2016 11:00 PM, David Gibson wrote:
>>> The spapr_alloc_htab() and spapr_reset_htab() functions currently handle
>>> all errors with error_setg(&error_abort, ...).
>>>
>>> But really, the callers are really better placed to decide on the error
>>> handling.  So, instead make the functions use the error propagation
>>> infrastructure.
>>>
>>> In the callers we change to &error_fatal instead of &error_abort, since
>>> this can be triggered by a bad configuration or kernel error rather than
>>> indicating a programming error in qemu.
>>>
>>> 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 | 24 ++++++++++++++++--------
>>>   1 file changed, 16 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>> index b7fd09a..d28e349 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -1016,7 +1016,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;
>>> @@ -1031,7 +1031,8 @@ 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_errno(errp, -shift,
>>> +                         "Error allocating KVM hash page table, try smaller maxmem");
>>>       } else if (shift > 0) {
>>>           /*
>>>            * Kernel handles htab, we don't need to allocate one
>>> @@ -1040,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",
>>
>>
>>
>> Even though it is not in the CODING_STYLE, I have not seen anyone objecting
>> the very good kernel's "never break user-visible strings" rule or rejecting
>> patches with user-visible strings failing to fit 80 chars limit.
>
> I'm not.  Or rather, the string is already broken by the PRIu32, so
> the newline doesn't make it any less greppable.


"KVM hash page table.*smaller maxmem" stopped working. Not a big deal but I 
do not see any win in breaking strings anyway.

btw the chunk above (and other patches in the patchset) uses incorrect indent.



-- 
Alexey

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

[David Gibson]

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

On Mon, Jan 18, 2016 at 04:17:08PM +1100, Alexey Kardashevskiy wrote:
> On 01/18/2016 03:42 PM, David Gibson wrote:
> >On Mon, Jan 18, 2016 at 01:44:00PM +1100, Alexey Kardashevskiy wrote:
> >>On 01/15/2016 11:00 PM, David Gibson wrote:
> >>>The spapr_alloc_htab() and spapr_reset_htab() functions currently handle
> >>>all errors with error_setg(&error_abort, ...).
> >>>
> >>>But really, the callers are really better placed to decide on the error
> >>>handling.  So, instead make the functions use the error propagation
> >>>infrastructure.
> >>>
> >>>In the callers we change to &error_fatal instead of &error_abort, since
> >>>this can be triggered by a bad configuration or kernel error rather than
> >>>indicating a programming error in qemu.
> >>>
> >>>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 | 24 ++++++++++++++++--------
> >>>  1 file changed, 16 insertions(+), 8 deletions(-)
> >>>
> >>>diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >>>index b7fd09a..d28e349 100644
> >>>--- a/hw/ppc/spapr.c
> >>>+++ b/hw/ppc/spapr.c
> >>>@@ -1016,7 +1016,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;
> >>>@@ -1031,7 +1031,8 @@ 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_errno(errp, -shift,
> >>>+                         "Error allocating KVM hash page table, try smaller maxmem");
> >>>      } else if (shift > 0) {
> >>>          /*
> >>>           * Kernel handles htab, we don't need to allocate one
> >>>@@ -1040,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",
> >>
> >>
> >>
> >>Even though it is not in the CODING_STYLE, I have not seen anyone objecting
> >>the very good kernel's "never break user-visible strings" rule or rejecting
> >>patches with user-visible strings failing to fit 80 chars limit.
> >
> >I'm not.  Or rather, the string is already broken by the PRIu32, so
> >the newline doesn't make it any less greppable.
> 
> 
> "KVM hash page table.*smaller maxmem" stopped working. Not a big deal but I
> do not see any win in breaking strings anyway.

The problem is that the current pre-commit hooks don't agree with
you.  They seem to allow long unbroken strings, but if there's a break
like the PRIu32, they won't permit the commit.

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

[Alexey Kardashevskiy]

On 01/18/2016 04:35 PM, David Gibson wrote:
> On Mon, Jan 18, 2016 at 04:17:08PM +1100, Alexey Kardashevskiy wrote:
>> On 01/18/2016 03:42 PM, David Gibson wrote:
>>> On Mon, Jan 18, 2016 at 01:44:00PM +1100, Alexey Kardashevskiy wrote:
>>>> On 01/15/2016 11:00 PM, David Gibson wrote:
>>>>> The spapr_alloc_htab() and spapr_reset_htab() functions currently handle
>>>>> all errors with error_setg(&error_abort, ...).
>>>>>
>>>>> But really, the callers are really better placed to decide on the error
>>>>> handling.  So, instead make the functions use the error propagation
>>>>> infrastructure.
>>>>>
>>>>> In the callers we change to &error_fatal instead of &error_abort, since
>>>>> this can be triggered by a bad configuration or kernel error rather than
>>>>> indicating a programming error in qemu.
>>>>>
>>>>> 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 | 24 ++++++++++++++++--------
>>>>>   1 file changed, 16 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>>> index b7fd09a..d28e349 100644
>>>>> --- a/hw/ppc/spapr.c
>>>>> +++ b/hw/ppc/spapr.c
>>>>> @@ -1016,7 +1016,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;
>>>>> @@ -1031,7 +1031,8 @@ 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_errno(errp, -shift,
>>>>> +                         "Error allocating KVM hash page table, try smaller maxmem");
>>>>>       } else if (shift > 0) {
>>>>>           /*
>>>>>            * Kernel handles htab, we don't need to allocate one
>>>>> @@ -1040,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",
>>>>
>>>>
>>>>
>>>> Even though it is not in the CODING_STYLE, I have not seen anyone objecting
>>>> the very good kernel's "never break user-visible strings" rule or rejecting
>>>> patches with user-visible strings failing to fit 80 chars limit.
>>>
>>> I'm not.  Or rather, the string is already broken by the PRIu32, so
>>> the newline doesn't make it any less greppable.
>>
>>
>> "KVM hash page table.*smaller maxmem" stopped working. Not a big deal but I
>> do not see any win in breaking strings anyway.
>
> The problem is that the current pre-commit hooks don't agree with
> you.  They seem to allow long unbroken strings, but if there's a break
> like the PRIu32, they won't permit the commit.


checkpatch.pl reports it as "WARNING: line over 80 characters", not an 
ERROR, so I'd say the hook has a problem.



-- 
Alexey

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

[David Gibson]

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

On Mon, Jan 18, 2016 at 05:04:55PM +1100, Alexey Kardashevskiy wrote:
> On 01/18/2016 04:35 PM, David Gibson wrote:
> >On Mon, Jan 18, 2016 at 04:17:08PM +1100, Alexey Kardashevskiy wrote:
> >>On 01/18/2016 03:42 PM, David Gibson wrote:
> >>>On Mon, Jan 18, 2016 at 01:44:00PM +1100, Alexey Kardashevskiy wrote:
> >>>>On 01/15/2016 11:00 PM, David Gibson wrote:
> >>>>>The spapr_alloc_htab() and spapr_reset_htab() functions currently handle
> >>>>>all errors with error_setg(&error_abort, ...).
> >>>>>
> >>>>>But really, the callers are really better placed to decide on the error
> >>>>>handling.  So, instead make the functions use the error propagation
> >>>>>infrastructure.
> >>>>>
> >>>>>In the callers we change to &error_fatal instead of &error_abort, since
> >>>>>this can be triggered by a bad configuration or kernel error rather than
> >>>>>indicating a programming error in qemu.
> >>>>>
> >>>>>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 | 24 ++++++++++++++++--------
> >>>>>  1 file changed, 16 insertions(+), 8 deletions(-)
> >>>>>
> >>>>>diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >>>>>index b7fd09a..d28e349 100644
> >>>>>--- a/hw/ppc/spapr.c
> >>>>>+++ b/hw/ppc/spapr.c
> >>>>>@@ -1016,7 +1016,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;
> >>>>>@@ -1031,7 +1031,8 @@ 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_errno(errp, -shift,
> >>>>>+                         "Error allocating KVM hash page table, try smaller maxmem");
> >>>>>      } else if (shift > 0) {
> >>>>>          /*
> >>>>>           * Kernel handles htab, we don't need to allocate one
> >>>>>@@ -1040,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",
> >>>>
> >>>>
> >>>>
> >>>>Even though it is not in the CODING_STYLE, I have not seen anyone objecting
> >>>>the very good kernel's "never break user-visible strings" rule or rejecting
> >>>>patches with user-visible strings failing to fit 80 chars limit.
> >>>
> >>>I'm not.  Or rather, the string is already broken by the PRIu32, so
> >>>the newline doesn't make it any less greppable.
> >>
> >>
> >>"KVM hash page table.*smaller maxmem" stopped working. Not a big deal but I
> >>do not see any win in breaking strings anyway.
> >
> >The problem is that the current pre-commit hooks don't agree with
> >you.  They seem to allow long unbroken strings, but if there's a break
> >like the PRIu32, they won't permit the commit.
> 
> 
> checkpatch.pl reports it as "WARNING: line over 80 characters", not an
> ERROR, so I'd say the hook has a problem.

Well, maybe, but it's not a problem I'm really inclined to tackle at
this time.

-- 
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/10] 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>
---
 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 d28e349..87097bc 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1699,27 +1699,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;
         }
     }
 }
@@ -1809,7 +1816,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] [PATCH 05/10] 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>
---
 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 87097bc..bb5eaa5 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 bool 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;
     }
 }
 
@@ -1934,7 +1934,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] [PATCH 06/10] 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 | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index bb5eaa5..ddca6e6 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1106,6 +1106,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)) {
@@ -1113,9 +1114,10 @@ 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 1; /* Don't continue scanning devices */
     }
 
     return 0;
@@ -1150,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_fatal);
 
     /* Reset the hash table & recalc the RMA */
     spapr_reset_htab(spapr, &error_fatal);
-- 
2.5.0

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

[Markus Armbruster]

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

> 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 | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index bb5eaa5..ddca6e6 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1106,6 +1106,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)) {
> @@ -1113,9 +1114,10 @@ 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 1; /* Don't continue scanning devices */

Re the comment: really?

find_unknown_sysbus_device() gets passed to
foreach_dynamic_sysbus_device(), which passes it on to
find_sysbus_device().

find_sysbus_device() calls it directly for non-containers, ignoring the
function value.

For containers, it iterates over the container's contents with
object_child_foreach().  That function indeed stops when the callback
returns non-zero.  However, the callback is find_sysbus_device(), not
find_unknown_sysbus_device().

Am I confused?

>      }
>  
>      return 0;
> @@ -1150,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_fatal);
>  
>      /* Reset the hash table & recalc the RMA */
>      spapr_reset_htab(spapr, &error_fatal);

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

[David Gibson]

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

On Fri, Jan 15, 2016 at 04:40:24PM +0100, Markus Armbruster wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > 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 | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index bb5eaa5..ddca6e6 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1106,6 +1106,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)) {
> > @@ -1113,9 +1114,10 @@ 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 1; /* Don't continue scanning devices */
> 
> Re the comment: really?
> 
> find_unknown_sysbus_device() gets passed to
> foreach_dynamic_sysbus_device(), which passes it on to
> find_sysbus_device().
> 
> find_sysbus_device() calls it directly for non-containers, ignoring the
> function value.
> 
> For containers, it iterates over the container's contents with
> object_child_foreach().  That function indeed stops when the callback
> returns non-zero.  However, the callback is find_sysbus_device(), not
> find_unknown_sysbus_device().
> 
> Am I confused?

No, I am.

I can't see a reasonable way to change this without assuming the error
is fatal, so I think I'll just drop this patch from the series.

> 
> >      }
> >  
> >      return 0;
> > @@ -1150,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_fatal);
> >  
> >      /* Reset the hash table & recalc the RMA */
> >      spapr_reset_htab(spapr, &error_fatal);
> 

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

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

[Eric Blake]

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

On 01/15/2016 05:00 AM, 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>
> ---
>  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));

You could drop the redundant () while touching this, as in:

assert(token >= RTAS_TOKEN_BASE && token < RTAS_TOKEN_MAX);

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

[Alexey Kardashevskiy]

On 01/20/2016 09:58 AM, Eric Blake wrote:
> On 01/15/2016 05:00 AM, 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>
>> ---
>>   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));
>
> You could drop the redundant () while touching this, as in:


Seriously? Why? I personally find it really annoying (but I stay silent) 
when people omit braces in cases like this.


> assert(token >= RTAS_TOKEN_BASE && token < RTAS_TOKEN_MAX);
>


-- 
Alexey

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

[Eric Blake]

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

On 01/19/2016 05:21 PM, Alexey Kardashevskiy wrote:

>> You could drop the redundant () while touching this, as in:
> 
> 
> Seriously? Why? I personally find it really annoying (but I stay silent)
> when people omit braces in cases like this.
> 
> 
>> assert(token >= RTAS_TOKEN_BASE && token < RTAS_TOKEN_MAX);

Because it's the prevailing style. I estimate that less than 10% of qemu
over-parenthesizes, mostly because && and || are well-known C operator
precedence:

$ git grep ' && ' | wc
   6462   57034  482477
$ git grep ') && (' | wc
    578    6151   48655

Of course, that's a rough estimate, as it has false positives on 'if
(foo() && (b || c))', and false negatives on conditionals where there is
a unary rather than binary operator on either side of &&; but I'm sure
you could write a Coccinelle script if you wanted more accurate counting.

But you are equally right that as long as HACKING doesn't document it,
and checkpatch.pl doesn't flag it, then you can over-parenthesize binary
arguments to the short-circuiting operators to your aesthetic tastes.

And for other operators, like '&' and '|', I definitely recommend the
parenthesis, particularly if you manage to trigger a gcc or clang
warning (in spite of the precedence being unambiguous) if you omit the
parenthesis.

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

[Alexey Kardashevskiy]

On 01/20/2016 03:53 PM, Eric Blake wrote:
> On 01/19/2016 05:21 PM, Alexey Kardashevskiy wrote:
>
>>> You could drop the redundant () while touching this, as in:
>>
>>
>> Seriously? Why? I personally find it really annoying (but I stay silent)
>> when people omit braces in cases like this.
>>
>>
>>> assert(token >= RTAS_TOKEN_BASE && token < RTAS_TOKEN_MAX);
>
> Because it's the prevailing style. I estimate that less than 10% of qemu
> over-parenthesizes, mostly because && and || are well-known C operator
> precedence:
 >
> $ git grep ' && ' | wc
>     6462   57034  482477
> $ git grep ') && (' | wc
>      578    6151   48655
>
> Of course, that's a rough estimate, as it has false positives on 'if
> (foo() && (b || c))', and false negatives on conditionals where there is
> a unary rather than binary operator on either side of &&; but I'm sure
> you could write a Coccinelle script if you wanted more accurate counting.
>
> But you are equally right that as long as HACKING doesn't document it,
> and checkpatch.pl doesn't flag it, then you can over-parenthesize binary
> arguments to the short-circuiting operators to your aesthetic tastes.

C Operator Precedence is well-known and still confusing, I cannot get used 
to the fact that </>/==/etc have higher priority than &/&&/etc so not 
seeing braces in the cases like above makes me nervous. Yes, I am sort of 
retarded :(

So, we can keep doing this over-parenthesizing, good, thanks :)

> And for other operators, like '&' and '|', I definitely recommend the
> parenthesis, particularly if you manage to trigger a gcc or clang
> warning (in spite of the precedence being unambiguous) if you omit the
> parenthesis.

Goood.


-- 
Alexey

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

[Thomas Huth]

On 20.01.2016 06:53, Alexey Kardashevskiy wrote:
> On 01/20/2016 03:53 PM, Eric Blake wrote:
>> On 01/19/2016 05:21 PM, Alexey Kardashevskiy wrote:
>>
>>>> You could drop the redundant () while touching this, as in:
>>>
>>>
>>> Seriously? Why? I personally find it really annoying (but I stay silent)
>>> when people omit braces in cases like this.
>>>
>>>
>>>> assert(token >= RTAS_TOKEN_BASE && token < RTAS_TOKEN_MAX);
>>
>> Because it's the prevailing style. I estimate that less than 10% of qemu
>> over-parenthesizes, mostly because && and || are well-known C operator
>> precedence:
>>
>> $ git grep ' && ' | wc
>>     6462   57034  482477
>> $ git grep ') && (' | wc
>>      578    6151   48655
>>
>> Of course, that's a rough estimate, as it has false positives on 'if
>> (foo() && (b || c))', and false negatives on conditionals where there is
>> a unary rather than binary operator on either side of &&; but I'm sure
>> you could write a Coccinelle script if you wanted more accurate counting.
>>
>> But you are equally right that as long as HACKING doesn't document it,
>> and checkpatch.pl doesn't flag it, then you can over-parenthesize binary
>> arguments to the short-circuiting operators to your aesthetic tastes.
> 
> C Operator Precedence is well-known and still confusing, I cannot get
> used to the fact that </>/==/etc have higher priority than &/&&/etc so
> not seeing braces in the cases like above makes me nervous. Yes, I am
> sort of retarded :(
> 
> So, we can keep doing this over-parenthesizing, good, thanks :)

For me, it's the other way round: If I notice too many parentheses while
reading source code, I have to start thinking - because I then assume
that there is something special with the statement so that the
parentheses are needed. If I then discover that it was just unnecessary
waste of time, I start complaining... So please try to get rid of your
parenthesitis, or you've got to live with my complaints ;-)

 Thomas

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 01/19/2016 05:21 PM, Alexey Kardashevskiy wrote:
>
>>> You could drop the redundant () while touching this, as in:
>> 
>> 
>> Seriously? Why? I personally find it really annoying (but I stay silent)
>> when people omit braces in cases like this.
>> 
>> 
>>> assert(token >= RTAS_TOKEN_BASE && token < RTAS_TOKEN_MAX);
>
> Because it's the prevailing style. I estimate that less than 10% of qemu
> over-parenthesizes, mostly because && and || are well-known C operator
> precedence:
>
> $ git grep ' && ' | wc
>    6462   57034  482477
> $ git grep ') && (' | wc
>     578    6151   48655
>
> Of course, that's a rough estimate, as it has false positives on 'if
> (foo() && (b || c))', and false negatives on conditionals where there is
> a unary rather than binary operator on either side of &&; but I'm sure
> you could write a Coccinelle script if you wanted more accurate counting.
>
> But you are equally right that as long as HACKING doesn't document it,
> and checkpatch.pl doesn't flag it, then you can over-parenthesize binary
> arguments to the short-circuiting operators to your aesthetic tastes.

HACKING doesn't document everything.  Trying to document everything
would drown the interesting parts in a sea of platitudes, and still
leave innumerable loopholes.

checkpatch.pl doesn't flag everything.  It checks for *common* unwanted
patterns.

When HACKING and checkpatch.pl are silent, make your change blend in
with the existing code.  Since the existing code overwhelmingly eschews
this kind of superfluous parenthesis, the general rule is to knock them
off unless *local* code overwhelmingly uses them.

Just because HACKING doesn't explicitly prohibit your personal
preferences doesn't mean you get to do leave your stylistic mark on the
code.  Show some taste and make yourself invisible.

[...]

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

[Alexey Kardashevskiy]

On 01/20/2016 06:18 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> On 01/19/2016 05:21 PM, Alexey Kardashevskiy wrote:
>>
>>>> You could drop the redundant () while touching this, as in:
>>>
>>>
>>> Seriously? Why? I personally find it really annoying (but I stay silent)
>>> when people omit braces in cases like this.
>>>
>>>
>>>> assert(token >= RTAS_TOKEN_BASE && token < RTAS_TOKEN_MAX);
>>
>> Because it's the prevailing style. I estimate that less than 10% of qemu
>> over-parenthesizes, mostly because && and || are well-known C operator
>> precedence:
>>
>> $ git grep ' && ' | wc
>>     6462   57034  482477
>> $ git grep ') && (' | wc
>>      578    6151   48655
>>
>> Of course, that's a rough estimate, as it has false positives on 'if
>> (foo() && (b || c))', and false negatives on conditionals where there is
>> a unary rather than binary operator on either side of &&; but I'm sure
>> you could write a Coccinelle script if you wanted more accurate counting.
>>
>> But you are equally right that as long as HACKING doesn't document it,
>> and checkpatch.pl doesn't flag it, then you can over-parenthesize binary
>> arguments to the short-circuiting operators to your aesthetic tastes.
>
> HACKING doesn't document everything.  Trying to document everything
> would drown the interesting parts in a sea of platitudes, and still
> leave innumerable loopholes.
>
> checkpatch.pl doesn't flag everything.  It checks for *common* unwanted
> patterns.
>
> When HACKING and checkpatch.pl are silent, make your change blend in
> with the existing code.  Since the existing code overwhelmingly eschews
> this kind of superfluous parenthesis, the general rule is to knock them
> off unless *local* code overwhelmingly uses them.


In order to educate myself - where/when was this wonderful rule 
established? What are the other rules then?


> Just because HACKING doesn't explicitly prohibit your personal
> preferences doesn't mean you get to do leave your stylistic mark on the
> code.  Show some taste and make yourself invisible.

Nice, now we are discussing taste :-/


-- 
Alexey

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

[Markus Armbruster]

Alexey Kardashevskiy <aik@ozlabs.ru> writes:

> On 01/20/2016 06:18 PM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> On 01/19/2016 05:21 PM, Alexey Kardashevskiy wrote:
>>>
>>>>> You could drop the redundant () while touching this, as in:
>>>>
>>>>
>>>> Seriously? Why? I personally find it really annoying (but I stay silent)
>>>> when people omit braces in cases like this.
>>>>
>>>>
>>>>> assert(token >= RTAS_TOKEN_BASE && token < RTAS_TOKEN_MAX);
>>>
>>> Because it's the prevailing style. I estimate that less than 10% of qemu
>>> over-parenthesizes, mostly because && and || are well-known C operator
>>> precedence:
>>>
>>> $ git grep ' && ' | wc
>>>     6462   57034  482477
>>> $ git grep ') && (' | wc
>>>      578    6151   48655
>>>
>>> Of course, that's a rough estimate, as it has false positives on 'if
>>> (foo() && (b || c))', and false negatives on conditionals where there is
>>> a unary rather than binary operator on either side of &&; but I'm sure
>>> you could write a Coccinelle script if you wanted more accurate counting.
>>>
>>> But you are equally right that as long as HACKING doesn't document it,
>>> and checkpatch.pl doesn't flag it, then you can over-parenthesize binary
>>> arguments to the short-circuiting operators to your aesthetic tastes.
>>
>> HACKING doesn't document everything.  Trying to document everything
>> would drown the interesting parts in a sea of platitudes, and still
>> leave innumerable loopholes.
>>
>> checkpatch.pl doesn't flag everything.  It checks for *common* unwanted
>> patterns.
>>
>> When HACKING and checkpatch.pl are silent, make your change blend in
>> with the existing code.  Since the existing code overwhelmingly eschews
>> this kind of superfluous parenthesis, the general rule is to knock them
>> off unless *local* code overwhelmingly uses them.
>
>
> In order to educate myself - where/when was this wonderful rule
> established? What are the other rules then?

The rule to make your new code blend in with the surrounding existing
code is common sense, and as such predates HACKING, or even QEMU.

If everybody did his own thing unless told otherwise in writing, we'd
end up with an incoherent mess[*].

I figure few people agree with every aspect of the prevailing QEMU
coding style.  I certainly don't.  But the development community largely
agrees that a reasonable level of stylistic consistency is wanted, and
worth some adjustment of habits and sacrifice of personal preferences.

Making your code blend in with the existing code requires a bit of care
and taste.  It doesn't require memorizing a long list of arbitrary
rules.  Give us your honest best effort, and respond constructively to
review comments, and you'll be fine.

[...]

[Qemu-devel] [PATCH 08/10] 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>
---
 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 ddca6e6..8e02ae8 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;
@@ -1815,7 +1815,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] [PATCH 09/10] 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 | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 8e02ae8..9f3d590 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1791,8 +1791,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);
     }
 
@@ -1858,10 +1858,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",
-                         machine->ram_slots, SPAPR_MAX_RAM_SLOTS);
-            exit(EXIT_FAILURE);
+            error_report("Specified number of memory slots %"
+                         PRIu64" exceeds max supported %d",
+                machine->ram_slots, SPAPR_MAX_RAM_SLOTS);
+            exit(1);
         }
 
         spapr->hotplug_memory.base = ROUND_UP(machine->ram_size,
@@ -1957,8 +1957,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);
     }
 
@@ -1974,8 +1975,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);
         }
 
@@ -1988,8 +1989,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] [PATCH 10/10] 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 | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 9f3d590..51f15cb 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1319,8 +1319,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;
         }
     }
@@ -1536,7 +1537,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;
     }
 
@@ -1557,8 +1558,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));
         }
     }
 
@@ -1578,9 +1579,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] [PATCH 00/10] Cleanups to error reporting on ppc and spapr (v2)

[Markus Armbruster]

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

> Here's a new 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.

There's an error_setg(&error_abort, ...) left in spapr_drc.c.  Should
that be converted to a straight abort()?

Re: [Qemu-devel] [PATCH 00/10] Cleanups to error reporting on ppc and spapr (v2)

[David Gibson]

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

On Fri, Jan 15, 2016 at 04:47:53PM +0100, Markus Armbruster wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > Here's a new 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.
> 
> There's an error_setg(&error_abort, ...) left in spapr_drc.c.  Should
> that be converted to a straight abort()?

Maybe.  I basically ignored for now all functions which work with the
device tree.  I have some other substantial rework I hope to get
around to there, which may make local changes like this error rework
moot.

-- 
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 00/10] Cleanups to error reporting on ppc and spapr (v2)

[Alexey Kardashevskiy]

On 01/16/2016 02:47 AM, Markus Armbruster wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
>
>> Here's a new 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.
>
> There's an error_setg(&error_abort, ...) left in spapr_drc.c.  Should
> that be converted to a straight abort()?
>

I was under impression that all abort()/exit() instances are aimed to be 
converted to error_setg(&error_abort) eventually (as all fprintf(error) to 
perror() and tracepoints, etc), is there any howto what to use when? :)


-- 
Alexey