Re: [Qemu-devel] [PATCH v6 04/29] hw/arm: Replace fprintf(stderr, "*\n" with error_report()

[Markus Armbruster]

Second thoughts...

Alistair Francis <address@hidden> writes:

> Replace a large number of the fprintf(stderr, "*\n" calls with
> error_report(). The functions were renamed with these commands and then
> compiler issues where manually fixed.
>
> find ./* -type f -exec sed -i \
>     'N;N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr, 
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>     {} +
> find ./* -type f -exec sed -i \
>     'N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr, 
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>     {} +
> find ./* -type f -exec sed -i \
>     'N;N;N;N;N;N;N;N;N; {s|fprintf(stderr, 
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>     {} +
> find ./* -type f -exec sed -i \
>     'N;N;N;N;N;N;N;N; {s|fprintf(stderr, 
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>     {} +
> find ./* -type f -exec sed -i \
>     'N;N;N;N;N;N;N; {s|fprintf(stderr, 
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>     {} +
> find ./* -type f -exec sed -i \
>     'N;N;N;N;N;N; {s|fprintf(stderr, 
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>     {} +
> find ./* -type f -exec sed -i \
>     'N;N;N;N;N; {s|fprintf(stderr, 
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>     {} +
> find ./* -type f -exec sed -i \
>     'N;N;N;N; {s|fprintf(stderr, 
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>     {} +
> find ./* -type f -exec sed -i \
>     'N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' 
> \
>     {} +
> find ./* -type f -exec sed -i \
>     'N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>     {} +
> find ./* -type f -exec sed -i \
>     'N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>     {} +
>
> Some lines where then manually tweaked to pass checkpatch.
>
> The 'qemu: ' prefix was manually removed from the hw/arm/boot.c file.
>
> Signed-off-by: Alistair Francis <address@hidden>
> Cc: address@hidden
> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
> ---
> V6:
>  - Fix indendation
> V3:
>  - Remoave 'qemu: ' prefix
> V2:
>  - Split hw patch into individual directories
>
>  hw/arm/armv7m.c      |  2 +-
>  hw/arm/boot.c        | 37 ++++++++++++++++++-------------------
>  hw/arm/gumstix.c     | 13 +++++++------
>  hw/arm/mainstone.c   |  7 ++++---
>  hw/arm/musicpal.c    |  2 +-
>  hw/arm/omap1.c       |  5 +++--
>  hw/arm/omap2.c       | 23 ++++++++++++-----------
>  hw/arm/omap_sx1.c    |  8 +++-----
>  hw/arm/palm.c        | 10 +++++-----
>  hw/arm/pxa2xx.c      |  7 ++++---
>  hw/arm/stellaris.c   |  3 ++-
>  hw/arm/tosa.c        | 18 +++++++++---------
>  hw/arm/versatilepb.c |  2 +-
>  hw/arm/vexpress.c    |  8 ++++----
>  hw/arm/z2.c          |  6 +++---
>  15 files changed, 77 insertions(+), 74 deletions(-)
>
> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
> index bb2dfc942b..56770a7048 100644
> --- a/hw/arm/armv7m.c
> +++ b/hw/arm/armv7m.c
> @@ -278,7 +278,7 @@ void armv7m_load_kernel(ARMCPU *cpu, const char 
> *kernel_filename, int mem_size)
>  #endif
>  
>      if (!kernel_filename && !qtest_enabled()) {
> -        fprintf(stderr, "Guest image must be specified (using -kernel)\n");
> +        error_report("Guest image must be specified (using -kernel)");
>          exit(1);
>      }
>  

This is obviously a genuine (fatal) error.

> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index c2720c8046..6e6b8c0c6a 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -8,6 +8,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
>  #include "qapi/error.h"
>  #include <libfdt.h>
>  #include "hw/hw.h"
> @@ -418,13 +419,13 @@ static int load_dtb(hwaddr addr, const struct 
> arm_boot_info *binfo,
>          char *filename;
>          filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, binfo->dtb_filename);
>          if (!filename) {
> -            fprintf(stderr, "Couldn't open dtb file %s\n", 
> binfo->dtb_filename);
> +            error_report("Couldn't open dtb file %s", binfo->dtb_filename);
>              goto fail;
>          }

These is obviously a genuine (recoverable) error..

[More of the same...]
> diff --git a/hw/arm/omap2.c b/hw/arm/omap2.c
> index b53878b8b9..3a1d995d6a 100644
> --- a/hw/arm/omap2.c
> +++ b/hw/arm/omap2.c
> @@ -19,6 +19,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
>  #include "qapi/error.h"
>  #include "qemu-common.h"
>  #include "cpu.h"
> @@ -1311,8 +1312,8 @@ static void omap_prcm_apll_update(struct omap_prcm_s *s)
>      /* TODO: update clocks */
>  
>      if (mode[0] == 1 || mode[0] == 2 || mode[1] == 1 || mode[1] == 2)
> -        fprintf(stderr, "%s: bad EN_54M_PLL or bad EN_96M_PLL\n",
> -                        __func__);
> +        error_report("%s: bad EN_54M_PLL or bad EN_96M_PLL",
> +                     __func__);
>  }

This one's different: we neither exit() nor return a "failed" status to
the caller.

We get here when the guest writes something funny to a certain
memory-mapped I/O register.  In other words, it's guest misbehavior, not
a user error.  I doubt it should be reported with error_report().
Peter, do we have a canonical way to report or log  guest misbehavior?

[...]