diff mbox

[PATCHv3,8/9] pseries: Clean up error reporting in ppc_spapr_init()

Message ID 1453091083-13931-9-git-send-email-david@gibson.dropbear.id.au
State New, archived
Headers show

Commit Message

David Gibson Jan. 18, 2016, 4:24 a.m. UTC
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(-)

Comments

Thomas Huth Jan. 18, 2016, 9:31 a.m. UTC | #1
On 18.01.2016 05:24, David Gibson wrote:
> This function includes a number of explicit fprintf()s for errors.
> Change these to use error_report() instead.
> 
> Also replace the single exit(EXIT_FAILURE) with an explicit exit(1), since
> the latter is the more usual idiom in qemu by a large margin.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  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 148ca5a..58f26cd 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1789,8 +1789,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);
>      }
>  
> @@ -1856,10 +1856,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);

Why did you change the indentation of the "machine->ram_slots, ..." line
here? The original looked better to me.

> +            exit(1);

EXIT_FAILURE still seems to be used quite often in the QEMU sources...
All in all, this hunk does not really change anything from a functional
point of view, so I'd like to suggest to omit this hunk completely
instead to avoid code churn here.

 Thomas
Markus Armbruster Jan. 18, 2016, 10:06 a.m. UTC | #2
Thomas Huth <thuth@redhat.com> writes:

> On 18.01.2016 05:24, David Gibson wrote:
>> This function includes a number of explicit fprintf()s for errors.
>> Change these to use error_report() instead.
>> 
>> Also replace the single exit(EXIT_FAILURE) with an explicit exit(1), since
>> the latter is the more usual idiom in qemu by a large margin.
>> 
>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>> ---
>>  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 148ca5a..58f26cd 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -1789,8 +1789,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);
>>      }
>>  
>> @@ -1856,10 +1856,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);
>
> Why did you change the indentation of the "machine->ram_slots, ..." line
> here? The original looked better to me.

Agreed.

>> +            exit(1);
>
> EXIT_FAILURE still seems to be used quite often in the QEMU sources...
> All in all, this hunk does not really change anything from a functional
> point of view, so I'd like to suggest to omit this hunk completely
> instead to avoid code churn here.

It makes the code locally consistent, so I'd keep it.
David Gibson Jan. 19, 2016, 1:23 a.m. UTC | #3
On Mon, Jan 18, 2016 at 10:31:42AM +0100, Thomas Huth wrote:
> On 18.01.2016 05:24, David Gibson wrote:
> > This function includes a number of explicit fprintf()s for errors.
> > Change these to use error_report() instead.
> > 
> > Also replace the single exit(EXIT_FAILURE) with an explicit exit(1), since
> > the latter is the more usual idiom in qemu by a large margin.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  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 148ca5a..58f26cd 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1789,8 +1789,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);
> >      }
> >  
> > @@ -1856,10 +1856,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);
> 
> Why did you change the indentation of the "machine->ram_slots, ..." line
> here? The original looked better to me.

I don't know.  Probably just a mistake on my part, I'll fix it.

> > +            exit(1);
> 
> EXIT_FAILURE still seems to be used quite often in the QEMU sources...
> All in all, this hunk does not really change anything from a functional
> point of view, so I'd like to suggest to omit this hunk completely
> instead to avoid code churn here.

This I'll keep, as Markus says for local consistency.
diff mbox

Patch

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 148ca5a..58f26cd 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1789,8 +1789,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);
     }
 
@@ -1856,10 +1856,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,
@@ -1955,8 +1955,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);
     }
 
@@ -1972,8 +1973,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);
         }
 
@@ -1986,8 +1987,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 {