Re: [Qemu-arm] [Qemu-devel] [PATCH v3 46/46] target: Replace fprintf(stderr, "*\n" with error_report()

[Thomas Huth]

On 19.10.2017 18:18, Alistair Francis wrote:
> 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.
[...]
>  target/arm/arm-powerctl.c    |  5 +++--
>  target/arm/arm-semi.c        |  3 ++-
>  target/arm/helper.c          |  4 ++--
>  target/arm/kvm.c             | 16 ++++++-------
>  target/arm/kvm32.c           |  2 +-
>  target/arm/kvm64.c           |  2 +-
>  target/arm/translate-a64.c   |  4 ++--
>  target/arm/translate.c       |  2 +-
>  target/cris/helper.c         |  2 +-
>  target/i386/hax-all.c        | 53 
> ++++++++++++++++++++++----------------------
>  target/i386/hax-darwin.c     | 26 +++++++++++-----------
>  target/i386/hax-mem.c        |  4 ++--
>  target/i386/hax-windows.c    | 43 +++++++++++++++++------------------
>  target/i386/kvm.c            | 38 +++++++++++++++----------------
>  target/i386/misc_helper.c    | 12 +++++-----
>  target/lm32/op_helper.c      |  4 ++--
>  target/mips/mips-semi.c      |  3 ++-
>  target/mips/translate.c      |  2 +-
>  target/ppc/excp_helper.c     |  4 ++--
>  target/ppc/kvm.c             | 34 ++++++++++++++--------------
>  target/ppc/mmu-hash64.c      |  2 +-
>  target/ppc/mmu_helper.c      |  2 +-
>  target/ppc/translate_init.c  | 53 
> ++++++++++++++++++++++----------------------
>  target/s390x/kvm.c           | 20 ++++++++---------
>  target/s390x/misc_helper.c   |  2 +-
>  target/unicore32/translate.c |  2 +-

If you want to get this committed, it's likely better to split it up
into the individual subsystems and send the patches to the according
maintainers, I think.

>  26 files changed, 175 insertions(+), 169 deletions(-)
> 
> diff --git a/target/arm/arm-powerctl.c b/target/arm/arm-powerctl.c
> index 25207cb850..2d56d5d579 100644
> --- a/target/arm/arm-powerctl.c
> +++ b/target/arm/arm-powerctl.c
> @@ -9,6 +9,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
>  #include "cpu.h"
>  #include "cpu-qom.h"
>  #include "internals.h"
> @@ -24,7 +25,7 @@
>  #define DPRINTF(fmt, args...) \
>      do { \
>          if (DEBUG_ARM_POWERCTL) { \
> -            fprintf(stderr, "[ARM]%s: " fmt , __func__, ##args); \
> +            error_report("[ARM]%s: " fmt , __func__, ##args); \

Using error_report for debug printing sounds wrong to me. These strings
are not errors. Maybe info_report would be a better choice? ... or maybe
just leave it as fprintf, since this is just a debug macro.

>          } \
>      } while (0)
>  
> @@ -32,7 +33,7 @@ CPUState *arm_get_cpu_by_id(uint64_t id)
>  {
>      CPUState *cpu;
>  
> -    DPRINTF("cpu %" PRId64 "\n", id);
> +    DPRINTF("cpu %" PRId64 "", id);
>  
>      CPU_FOREACH(cpu) {
>          ARMCPU *armcpu = ARM_CPU(cpu);

This looks incomplete. There are more DPRINTF statements in this file,
so if you really want to convert this, you've got to touch all of them.

> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index a39b9d3633..9c736481f6 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -505,8 +505,8 @@ static inline void assert_fp_access_checked(DisasContext 
> *s)
>  {
>  #ifdef CONFIG_DEBUG_TCG
>      if (unlikely(!s->fp_access_checked || s->fp_excp_el)) {
> -        fprintf(stderr, "target-arm: FP access check missing for "
> -                "instruction 0x%08x\n", s->insn);
> +        error_report("target-arm: FP access check missing for "
> +                "instruction 0x%08x", s->insn);

Bad indentation.

>          abort();
>      }
>  #endif
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 4da1a4cbc6..55bdcad97e 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -861,7 +861,7 @@ void arm_test_cc(DisasCompare *cmp, int cc)
>          goto no_invert;
>  
>      default:
> -        fprintf(stderr, "Bad condition code 0x%x\n", cc);
> +        error_report("Bad condition code 0x%x", cc);
>          abort();
>      }
>  
> diff --git a/target/cris/helper.c b/target/cris/helper.c
> index af78cca8b9..ba9ce538c3 100644
> --- a/target/cris/helper.c
> +++ b/target/cris/helper.c
> @@ -282,7 +282,7 @@ hwaddr cris_cpu_get_phys_page_debug(CPUState *cs, vaddr 
> addr)
>      if (!miss) {
>          phy = res.phy;
>      }
> -    D(fprintf(stderr, "%s %x -> %x\n", __func__, addr, phy));
> +    D(error_report("%s %x -> %x", __func__, addr, phy));

I think it would be better to remove the D() macro from this file and
turn this statement into a qemu_log_mask(CPU_LOG_MMU, ...).

> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 9d57debf0e..b87f3d6f84 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -159,8 +159,8 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>      cap_ppc_pvr_compat = false;
>  
>      if (!cap_interrupt_level) {
> -        fprintf(stderr, "KVM: Couldn't find level irq capability. Expect the 
> "
> -                        "VM to stall at times!\n");
> +        error_report("KVM: Couldn't find level irq capability. Expect the "
> +                        "VM to stall at times!");
>      }
>  
>      kvm_ppc_register_host_cpu_type(ms);
> @@ -188,7 +188,7 @@ static int kvm_arch_sync_sregs(PowerPCCPU *cpu)
>          return 0;
>      } else {
>          if (!cap_segstate) {
> -            fprintf(stderr, "kvm error: missing PVR setting capability\n");
> +            error_report("kvm error: missing PVR setting capability");
>              return -ENOSYS;
>          }
>      }
> @@ -237,7 +237,7 @@ static int kvm_booke206_tlb_init(PowerPCCPU *cpu)
>  
>      ret = kvm_vcpu_enable_cap(cs, KVM_CAP_SW_TLB, 0, (uintptr_t)&cfg);
>      if (ret < 0) {
> -        fprintf(stderr, "%s: couldn't enable KVM_CAP_SW_TLB: %s\n",
> +        error_report("%s: couldn't enable KVM_CAP_SW_TLB: %s",
>                  __func__, strerror(-ret));
>          return ret;
>      }
> @@ -552,7 +552,7 @@ static void kvmppc_hw_debug_points_init(CPUPPCState *cenv)
>      }
>  
>      if ((max_hw_breakpoint + max_hw_watchpoint) > MAX_HW_BKPTS) {
> -        fprintf(stderr, "Error initializing h/w breakpoints\n");
> +        error_report("Error initializing h/w breakpoints");
>          return;
>      }
>  }
> @@ -623,7 +623,7 @@ static void kvm_sw_tlb_put(PowerPCCPU *cpu)
>  
>      ret = kvm_vcpu_ioctl(cs, KVM_DIRTY_TLB, &dirty_tlb);
>      if (ret) {
> -        fprintf(stderr, "%s: KVM_DIRTY_TLB: %s\n",
> +        error_report("%s: KVM_DIRTY_TLB: %s",
>                  __func__, strerror(-ret));
>      }
>  
> @@ -1480,7 +1480,7 @@ static int kvmppc_handle_halt(PowerPCCPU *cpu)
>  static int kvmppc_handle_dcr_read(CPUPPCState *env, uint32_t dcrn, uint32_t 
> *data)
>  {
>      if (ppc_dcr_read(env->dcr_env, dcrn, data) < 0)
> -        fprintf(stderr, "Read to unhandled DCR (0x%x)\n", dcrn);
> +        error_report("Read to unhandled DCR (0x%x)", dcrn);
>  
>      return 0;
>  }
> @@ -1488,7 +1488,7 @@ static int kvmppc_handle_dcr_read(CPUPPCState *env, 
> uint32_t dcrn, uint32_t *dat
>  static int kvmppc_handle_dcr_write(CPUPPCState *env, uint32_t dcrn, uint32_t 
> data)
>  {
>      if (ppc_dcr_write(env->dcr_env, dcrn, data) < 0)
> -        fprintf(stderr, "Write to unhandled DCR (0x%x)\n", dcrn);
> +        error_report("Write to unhandled DCR (0x%x)", dcrn);
>  
>      return 0;
>  }
> @@ -1799,7 +1799,7 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run 
> *run)
>          break;
>  
>      default:
> -        fprintf(stderr, "KVM: unknown exit reason %d\n", run->exit_reason);
> +        error_report("KVM: unknown exit reason %d", run->exit_reason);
>          ret = -1;
>          break;
>      }
> @@ -1863,7 +1863,7 @@ int kvmppc_booke_watchdog_enable(PowerPCCPU *cpu)
>  
>      ret = kvm_vcpu_enable_cap(cs, KVM_CAP_PPC_BOOKE_WATCHDOG, 0);
>      if (ret < 0) {
> -        fprintf(stderr, "%s: couldn't enable KVM_CAP_PPC_BOOKE_WATCHDOG: 
> %s\n",
> +        error_report("%s: couldn't enable KVM_CAP_PPC_BOOKE_WATCHDOG: %s",
>                  __func__, strerror(-ret));
>          return ret;
>      }
> @@ -2198,7 +2198,7 @@ off_t kvmppc_alloc_rma(void **rma)
>  
>      fd = kvm_vm_ioctl(kvm_state, KVM_ALLOCATE_RMA, &ret);
>      if (fd < 0) {
> -        fprintf(stderr, "KVM: Error on KVM_ALLOCATE_RMA: %s\n",
> +        error_report("KVM: Error on KVM_ALLOCATE_RMA: %s",
>                  strerror(errno));
>          return -1;
>      }
> @@ -2207,7 +2207,7 @@ off_t kvmppc_alloc_rma(void **rma)
>  
>      *rma = mmap(NULL, size, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
>      if (*rma == MAP_FAILED) {
> -        fprintf(stderr, "KVM: Error mapping RMA: %s\n", strerror(errno));
> +        error_report("KVM: Error mapping RMA: %s", strerror(errno));
>          return -1;
>      };
>  
> @@ -2309,7 +2309,7 @@ void *kvmppc_create_spapr_tce(uint32_t liobn, uint32_t 
> page_shift,
>          }
>          fd = kvm_vm_ioctl(kvm_state, KVM_CREATE_SPAPR_TCE, &args);
>          if (fd < 0) {
> -            fprintf(stderr, "KVM: Failed to create TCE table for liobn 
> 0x%x\n",
> +            error_report("KVM: Failed to create TCE table for liobn 0x%x",
>                      liobn);
>              return NULL;
>          }
> @@ -2322,7 +2322,7 @@ void *kvmppc_create_spapr_tce(uint32_t liobn, uint32_t 
> page_shift,
>  
>      table = mmap(NULL, len, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
>      if (table == MAP_FAILED) {
> -        fprintf(stderr, "KVM: Failed to map TCE table for liobn 0x%x\n",
> +        error_report("KVM: Failed to map TCE table for liobn 0x%x",
>                  liobn);
>          close(fd);
>          return NULL;
> @@ -2584,7 +2584,7 @@ int kvmppc_save_htab(QEMUFile *f, int fd, size_t 
> bufsize, int64_t max_ns)
>      do {
>          rc = read(fd, buf, bufsize);
>          if (rc < 0) {
> -            fprintf(stderr, "Error reading data from KVM HTAB fd: %s\n",
> +            error_report("Error reading data from KVM HTAB fd: %s",
>                      strerror(errno));
>              return rc;
>          } else if (rc) {
> @@ -2629,13 +2629,13 @@ int kvmppc_load_htab_chunk(QEMUFile *f, int fd, 
> uint32_t index,
>  
>      rc = write(fd, buf, chunksize);
>      if (rc < 0) {
> -        fprintf(stderr, "Error writing KVM hash table: %s\n",
> +        error_report("Error writing KVM hash table: %s",
>                  strerror(errno));
>          return rc;
>      }

Wrong indentation in the above hunks.

> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index 14d34e512f..8713ed6682 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -377,7 +377,7 @@ static int ppc_hash64_amr_prot(PowerPCCPU *cpu, 
> ppc_hash_pte64_t pte)
>      key = HPTE64_R_KEY(pte.pte1);
>      amrbits = (env->spr[SPR_AMR] >> 2*(31 - key)) & 0x3;
>  
> -    /* fprintf(stderr, "AMR protection: key=%d AMR=0x%" PRIx64 "\n", key, */
> +    /* error_report("AMR protection: key=%d AMR=0x%" PRIx64 "", key, */
>      /*         env->spr[SPR_AMR]); */

It's just a comment but still - indentation in the second line is wrong
here, too.

 Thomas