qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 4/5] Convert multi-line fprintf() to warn_rep


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v2 4/5] Convert multi-line fprintf() to warn_report()
Date: Mon, 14 Aug 2017 15:30:59 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux)

Alistair Francis <address@hidden> writes:

> Convert all the multi-line uses of fprintf(stderr, "warning:"..."\n"...
> to use warn_report() instead. This helps standardise on a single
> method of printing warnings to the user.
>
> All of the warnings were changed using these commands:
>   find ./* -type f -exec sed -i \
>     'N; {s|fprintf(.*".*warning[,:] 
> \(.*\)\\n"\(.*\));|warn_report("\1"\2);|Ig}' \
>     {} +
>   find ./* -type f -exec sed -i \
>     'N;N; {s|fprintf(.*".*warning[,:] 
> \(.*\)\\n"\(.*\));|warn_report("\1"\2);|Ig}' \
>     {} +
>   find ./* -type f -exec sed -i \
>     'N;N;N; {s|fprintf(.*".*warning[,:] 
> \(.*\)\\n"\(.*\));|warn_report("\1"\2);|Ig}' \
>     {} +
>   find ./* -type f -exec sed -i \
>     'N;N;N;N {s|fprintf(.*".*warning[,:] 
> \(.*\)\\n"\(.*\));|warn_report("\1"\2);|Ig}' \
>     {} +
>   find ./* -type f -exec sed -i \
>     'N;N;N;N;N {s|fprintf(.*".*warning[,:] 
> \(.*\)\\n"\(.*\));|warn_report("\1"\2);|Ig}' \
>     {} +
>   find ./* -type f -exec sed -i \
>     'N;N;N;N;N;N {s|fprintf(.*".*warning[,:] 
> \(.*\)\\n"\(.*\));|warn_report("\1"\2);|Ig}' \
>     {} +
>   find ./* -type f -exec sed -i \
>     'N;N;N;N;N;N;N; {s|fprintf(.*".*warning[,:] 
> \(.*\)\\n"\(.*\));|warn_report("\1"\2);|Ig}' \
>     {} +
>
> Indentation fixed up manually afterwards.
>
> Some of the lines were manually edited to reduce the line length to below
> 80 charecters. Some of the lines with newlines in the middle of the
> string were also manually edit to avoid checkpatch errrors.
>
> The #include lines were manually updated to allow the code to compile.
>
> Signed-off-by: Alistair Francis <address@hidden>
> Cc: Paolo Bonzini <address@hidden>
> Cc: Kevin Wolf <address@hidden>
> Cc: Max Reitz <address@hidden>
> Cc: "Michael S. Tsirkin" <address@hidden>
> Cc: Igor Mammedov <address@hidden>
> Cc: Peter Maydell <address@hidden>
> Cc: Stefano Stabellini <address@hidden>
> Cc: Anthony Perard <address@hidden>
> Cc: Richard Henderson <address@hidden>
> Cc: Eduardo Habkost <address@hidden>
> Cc: Aurelien Jarno <address@hidden>
> Cc: Yongbok Kim <address@hidden>
> Cc: Cornelia Huck <address@hidden>
> Cc: Christian Borntraeger <address@hidden>
> Cc: Alexander Graf <address@hidden>
> Cc: Jason Wang <address@hidden>
> Cc: David Gibson <address@hidden>
> Cc: Gerd Hoffmann <address@hidden>
> Acked-by: Cornelia Huck <address@hidden>
> ---
> I couldn't figure out any nice way (it is possible with some more logic
> inside the sed apparently) to do this is one command, so I had to use
> all of the commands above.

There's coccinelle, but swinging that hammer successfully requires
crawling up its learning curve.

>  accel/kvm/kvm-all.c         |  7 +++----
>  block/vvfat.c               |  4 ++--
>  hw/acpi/core.c              |  7 +++----
>  hw/arm/vexpress.c           |  4 ++--
>  hw/i386/xen/xen-mapcache.c  |  4 ++--
>  hw/mips/mips_malta.c        |  4 ++--
>  hw/mips/mips_r4k.c          |  6 +++---
>  hw/s390x/s390-virtio.c      | 16 ++++++++--------
>  net/hub.c                   |  9 ++++-----
>  net/net.c                   | 14 +++++++-------
>  target/i386/cpu.c           | 12 ++++++------
>  target/i386/hax-mem.c       |  6 +++---
>  target/ppc/translate_init.c | 18 +++++++++---------
>  ui/keymaps.c                |  9 +++++----
>  util/main-loop.c            |  6 +++---
>  15 files changed, 62 insertions(+), 64 deletions(-)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 46ce479dc3..03e26e5a07 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -1629,10 +1629,9 @@ static int kvm_init(MachineState *ms)
>  
>      while (nc->name) {
>          if (nc->num > soft_vcpus_limit) {
> -            fprintf(stderr,
> -                    "Warning: Number of %s cpus requested (%d) exceeds "
> -                    "the recommended cpus supported by KVM (%d)\n",
> -                    nc->name, nc->num, soft_vcpus_limit);
> +            warn_report("Number of %s cpus requested (%d) exceeds "
> +                        "the recommended cpus supported by KVM (%d)",
> +                        nc->name, nc->num, soft_vcpus_limit);
>  
>              if (nc->num > hard_vcpus_limit) {
>                  fprintf(stderr, "Number of %s cpus requested (%d) exceeds "
> diff --git a/block/vvfat.c b/block/vvfat.c
> index d682f0a9dc..04801f3136 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -1227,8 +1227,8 @@ static int vvfat_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  
>      switch (s->fat_type) {
>      case 32:
> -            fprintf(stderr, "Big fat greek warning: FAT32 has not been 
> tested. "
> -                "You are welcome to do so!\n");
> +        warn_report("FAT32 has not been tested. "
> +                    "You are welcome to do so!");
>          break;
>      case 16:
>      case 12:
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index 2a1b79c838..cd0a1d357b 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -184,10 +184,9 @@ static void acpi_table_install(const char unsigned 
> *blob, size_t bloblen,
>      }
>  
>      if (has_header && le32_to_cpu(ext_hdr->length) != acpi_payload_size) {
> -        fprintf(stderr,
> -                "warning: ACPI table has wrong length, header says "
> -                "%" PRIu32 ", actual size %zu bytes\n",
> -                le32_to_cpu(ext_hdr->length), acpi_payload_size);
> +        warn_report("ACPI table has wrong length, header says "
> +                    "%" PRIu32 ", actual size %zu bytes",
> +                    le32_to_cpu(ext_hdr->length), acpi_payload_size);
>      }
>      ext_hdr->length = cpu_to_le32(acpi_payload_size);
>  
> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
> index 528c65ddb6..2445eb4408 100644
> --- a/hw/arm/vexpress.c
> +++ b/hw/arm/vexpress.c
> @@ -491,8 +491,8 @@ static void vexpress_modify_dtb(const struct 
> arm_boot_info *info, void *fdt)
>          /* Not fatal, we just won't provide virtio. This will
>           * happen with older device tree blobs.
>           */
> -        fprintf(stderr, "QEMU: warning: couldn't find interrupt controller 
> in "
> -                "dtb; will not include virtio-mmio devices in the dtb.\n");
> +        warn_report("couldn't find interrupt controller in "
> +                    "dtb; will not include virtio-mmio devices in the dtb.");

Drop the period, please.

>      } else {
>          int i;
>          const hwaddr *map = daughterboard->motherboard_map;
> diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
> index 369c3df8a0..3985a92f02 100644
> --- a/hw/i386/xen/xen-mapcache.c
> +++ b/hw/i386/xen/xen-mapcache.c
> @@ -125,8 +125,8 @@ void xen_map_cache_init(phys_offset_to_gaddr_t f, void 
> *opaque)
>          rlimit_as.rlim_cur = rlimit_as.rlim_max;
>  
>          if (rlimit_as.rlim_max != RLIM_INFINITY) {
> -            fprintf(stderr, "Warning: QEMU's maximum size of virtual"
> -                    " memory is not infinity.\n");
> +            warn_report("QEMU's maximum size of virtual"
> +                        " memory is not infinity.");

Likewise.

>          }
>          if (rlimit_as.rlim_max < MCACHE_MAX_SIZE + NON_MCACHE_MEMORY_SIZE) {
>              mapcache->max_mcache_size = rlimit_as.rlim_max -
> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> index 8ecd544baa..4fb6dfdf74 100644
> --- a/hw/mips/mips_malta.c
> +++ b/hw/mips/mips_malta.c
> @@ -216,8 +216,8 @@ static void generate_eeprom_spd(uint8_t *eeprom, 
> ram_addr_t ram_size)
>      }
>  
>      if (ram_size) {
> -        fprintf(stderr, "Warning: SPD cannot represent final %dMB"
> -                " of SDRAM\n", (int)ram_size);
> +        warn_report("SPD cannot represent final %dMB"
> +                    " of SDRAM", (int)ram_size);
>      }

Not your patch's fault, but here goes anyway: ram_addr_t ram_size should
be printed with "0x" RAM_ADDR_FMT.

If you consider RAM_ADDR_FMT too ugly to bear (I sympathize; including
the '%' in the macro shows lack of taste), you can perhaps get away with
%lld and (long long)ram_size.

>  
>      /* fill in SPD memory information */
> diff --git a/hw/mips/mips_r4k.c b/hw/mips/mips_r4k.c
> index 2f5ced7409..6ffb88fd70 100644
> --- a/hw/mips/mips_r4k.c
> +++ b/hw/mips/mips_r4k.c
> @@ -253,9 +253,9 @@ void mips_r4k_init(MachineState *machine)
>              fprintf(stderr, "qemu: Error registering flash memory.\n");
>       }
>      } else if (!qtest_enabled()) {
> -     /* not fatal */
> -        fprintf(stderr, "qemu: Warning, could not load MIPS bios '%s'\n",
> -             bios_name);
> +        /* not fatal */
> +        warn_report("could not load MIPS bios '%s'",
> +                    bios_name);

Could be one line.

>      }
>      g_free(filename);
>  
> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> index afa4148e6b..964df517b4 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -146,9 +146,9 @@ void gtod_save(QEMUFile *f, void *opaque)
>  
>      r = s390_get_clock(&tod_high, &tod_low);
>      if (r) {
> -        fprintf(stderr, "WARNING: Unable to get guest clock for migration. "
> -                        "Error code %d. Guest clock will not be migrated "
> -                        "which could cause the guest to hang.\n", r);
> +        warn_report("Unable to get guest clock for migration. "
> +                    "Error code %d. Guest clock will not be migrated "
> +                    "which could cause the guest to hang.", r);

Not your patch's fault, but here goes anyway: multiple sentences in an
error message are an anti-pattern.  Printing errno codes with %d is
another one.  Better:

           warn_report("Unable to get guest clock for migration: %s",
                       strerror(-r));
           error_printf("Guest clock will not be migrated "
                        "which could cause the guest to hang.");

More of the same below.

>          qemu_put_byte(f, S390_TOD_CLOCK_VALUE_MISSING);
>          return;
>      }
> @@ -165,8 +165,8 @@ int gtod_load(QEMUFile *f, void *opaque, int version_id)
>      int r;
>  
>      if (qemu_get_byte(f) == S390_TOD_CLOCK_VALUE_MISSING) {
> -        fprintf(stderr, "WARNING: Guest clock was not migrated. This could "
> -                        "cause the guest to hang.\n");
> +        warn_report("Guest clock was not migrated. This could "
> +                    "cause the guest to hang.");
>          return 0;
>      }
>  
> @@ -175,9 +175,9 @@ int gtod_load(QEMUFile *f, void *opaque, int version_id)
>  
>      r = s390_set_clock(&tod_high, &tod_low);
>      if (r) {
> -        fprintf(stderr, "WARNING: Unable to set guest clock value. "
> -                        "s390_get_clock returned error %d. This could cause "
> -                        "the guest to hang.\n", r);
> +        warn_report("Unable to set guest clock value. "
> +                    "s390_get_clock returned error %d. This could cause "
> +                    "the guest to hang.", r);
>      }
>  
>      return 0;
> diff --git a/net/hub.c b/net/hub.c
> index afe941ae7a..745a2168a1 100644
> --- a/net/hub.c
> +++ b/net/hub.c
> @@ -310,8 +310,8 @@ void net_hub_check_clients(void)
>          QLIST_FOREACH(port, &hub->ports, next) {
>              peer = port->nc.peer;
>              if (!peer) {
> -                fprintf(stderr, "Warning: hub port %s has no peer\n",
> -                        port->nc.name);
> +                warn_report("hub port %s has no peer",
> +                            port->nc.name);
>                  continue;
>              }
>  
> @@ -334,9 +334,8 @@ void net_hub_check_clients(void)
>              warn_report("vlan %d with no nics", hub->id);
>          }
>          if (has_nic && !has_host_dev) {
> -            fprintf(stderr,
> -                    "Warning: vlan %d is not connected to host network\n",
> -                    hub->id);
> +            warn_report("vlan %d is not connected to host network",
> +                        hub->id);
>          }
>      }
>  }
> diff --git a/net/net.c b/net/net.c
> index 0e28099554..45ab2a1a02 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1481,9 +1481,9 @@ void net_check_clients(void)
>  
>      QTAILQ_FOREACH(nc, &net_clients, next) {
>          if (!nc->peer) {
> -            fprintf(stderr, "Warning: %s %s has no peer\n",
> -                    nc->info->type == NET_CLIENT_DRIVER_NIC ?
> -                    "nic" : "netdev", nc->name);
> +            warn_report("%s %s has no peer",
> +                        nc->info->type == NET_CLIENT_DRIVER_NIC ?
> +                        "nic" : "netdev", nc->name);

Opportunity to clean up the tasteless line break in the middle of an
argument expression:

               warn_report("%s %s has no peer",
                           nc->info->type == NET_CLIENT_DRIVER_NIC
                           ? "nic" : "netdev",
                           nc->name);

>          }
>      }
>  
> @@ -1494,10 +1494,10 @@ void net_check_clients(void)
>      for (i = 0; i < MAX_NICS; i++) {
>          NICInfo *nd = &nd_table[i];
>          if (nd->used && !nd->instantiated) {
> -            fprintf(stderr, "Warning: requested NIC (%s, model %s) "
> -                    "was not created (not supported by this machine?)\n",
> -                    nd->name ? nd->name : "anonymous",
> -                    nd->model ? nd->model : "unspecified");
> +            warn_report("requested NIC (%s, model %s) "
> +                        "was not created (not supported by this machine?)",
> +                        nd->name ? nd->name : "anonymous",
> +                        nd->model ? nd->model : "unspecified");
>          }
>      }
>  }
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index ddc45abd70..8c9ec7da0f 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -1722,12 +1722,12 @@ static void report_unavailable_features(FeatureWord 
> w, uint32_t mask)
>          if ((1UL << i) & mask) {
>              const char *reg = get_register_name_32(f->cpuid_reg);
>              assert(reg);
> -            fprintf(stderr, "warning: %s doesn't support requested feature: "
> -                "CPUID.%02XH:%s%s%s [bit %d]\n",
> -                kvm_enabled() ? "host" : "TCG",
> -                f->cpuid_eax, reg,
> -                f->feat_names[i] ? "." : "",
> -                f->feat_names[i] ? f->feat_names[i] : "", i);
> +            warn_report("%s doesn't support requested feature: "
> +                        "CPUID.%02XH:%s%s%s [bit %d]",
> +                        kvm_enabled() ? "host" : "TCG",
> +                        f->cpuid_eax, reg,
> +                        f->feat_names[i] ? "." : "",
> +                        f->feat_names[i] ? f->feat_names[i] : "", i);
>          }
>      }
>  }
> diff --git a/target/i386/hax-mem.c b/target/i386/hax-mem.c
> index af090343f3..756f2dd268 100644
> --- a/target/i386/hax-mem.c
> +++ b/target/i386/hax-mem.c
> @@ -178,9 +178,9 @@ static void hax_process_section(MemoryRegionSection 
> *section, uint8_t flags)
>      if (!memory_region_is_ram(mr)) {
>          if (memory_region_is_romd(mr)) {
>              /* HAXM kernel module does not support ROMD yet  */
> -            fprintf(stderr, "%s: Warning: Ignoring ROMD region 0x%016" PRIx64
> -                    "->0x%016" PRIx64 "\n", __func__, start_pa,
> -                    start_pa + size);
> +            warn_report("Ignoring ROMD region 0x%016" PRIx64
> +                        "->0x%016" PRIx64 "", __func__, start_pa,
> +                        start_pa + size);

__func__ again.  Not your patch's fault.

>          }
>          return;
>      }
> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> index 01723bdfec..a6f02f3c45 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -9215,14 +9215,14 @@ static void init_ppc_proc(PowerPCCPU *cpu)
>          env->tlb_per_way = env->nb_tlb / env->nb_ways;
>      }
>      if (env->irq_inputs == NULL) {
> -        fprintf(stderr, "WARNING: no internal IRQ controller registered.\n"
> -                " Attempt QEMU to crash very soon !\n");
> +        warn_report("no internal IRQ controller registered."
> +                    " Attempt QEMU to crash very soon !");
>      }
>  #endif
>      if (env->check_pow == NULL) {
> -        fprintf(stderr, "WARNING: no power management check handler "
> -                "registered.\n"
> -                " Attempt QEMU to crash very soon !\n");
> +        warn_report("no power management check handler "
> +                    "registered."
> +                    " Attempt QEMU to crash very soon !");
>      }
>  }
>  
> @@ -9776,10 +9776,10 @@ static int ppc_fixup_cpu(PowerPCCPU *cpu)
>       * tree. */
>      if ((env->insns_flags & ~PPC_TCG_INSNS)
>          || (env->insns_flags2 & ~PPC_TCG_INSNS2)) {
> -        fprintf(stderr, "Warning: Disabling some instructions which are not "
> -                "emulated by TCG (0x%" PRIx64 ", 0x%" PRIx64 ")\n",
> -                env->insns_flags & ~PPC_TCG_INSNS,
> -                env->insns_flags2 & ~PPC_TCG_INSNS2);
> +        warn_report("Disabling some instructions which are not "
> +                    "emulated by TCG (0x%" PRIx64 ", 0x%" PRIx64 ")",
> +                    env->insns_flags & ~PPC_TCG_INSNS,
> +                    env->insns_flags2 & ~PPC_TCG_INSNS2);
>      }
>      env->insns_flags &= PPC_TCG_INSNS;
>      env->insns_flags2 &= PPC_TCG_INSNS2;
> diff --git a/ui/keymaps.c b/ui/keymaps.c
> index 7fa21f81b2..a6cefdaff9 100644
> --- a/ui/keymaps.c
> +++ b/ui/keymaps.c
> @@ -26,6 +26,7 @@
>  #include "keymaps.h"
>  #include "sysemu/sysemu.h"
>  #include "trace.h"
> +#include "qemu/error-report.h"
>  
>  static int get_keysym(const name2keysym_t *table,
>                        const char *name)
> @@ -76,8 +77,8 @@ static void add_keysym(char *line, int keysym, int keycode, 
> kbd_layout_t *k) {
>          k->keysym2keycode[keysym] = keycode;
>      } else {
>          if (k->extra_count >= MAX_EXTRA_COUNT) {
> -            fprintf(stderr, "Warning: Could not assign keysym %s (0x%x)"
> -                    " because of memory constraints.\n", line, keysym);
> +            warn_report("Could not assign keysym %s (0x%x)"
> +                        " because of memory constraints.", line, keysym);
>          } else {
>              trace_keymap_add("extra", keysym, keycode, line);
>              k->keysym2keycode_extra[k->extra_count].
> @@ -197,8 +198,8 @@ int keysym2scancode(void *kbd_layout, int keysym)
>      if (keysym < MAX_NORMAL_KEYCODE) {
>          if (k->keysym2keycode[keysym] == 0) {
>              trace_keymap_unmapped(keysym);
> -            fprintf(stderr, "Warning: no scancode found for keysym %d\n",
> -                    keysym);
> +            warn_report("no scancode found for keysym %d",
> +                        keysym);
>          }
>          return k->keysym2keycode[keysym];
>      } else {
> diff --git a/util/main-loop.c b/util/main-loop.c
> index 2f48f41e62..7558eb5f53 100644
> --- a/util/main-loop.c
> +++ b/util/main-loop.c
> @@ -32,6 +32,7 @@
>  #include "slirp/libslirp.h"
>  #include "qemu/main-loop.h"
>  #include "block/aio.h"
> +#include "qemu/error-report.h"
>  
>  #ifndef _WIN32
>  
> @@ -236,9 +237,8 @@ static int os_host_main_loop_wait(int64_t timeout)
>          static bool notified;
>  
>          if (!notified && !qtest_enabled() && !qtest_driver()) {
> -            fprintf(stderr,
> -                    "main-loop: WARNING: I/O thread spun for %d 
> iterations\n",
> -                    MAX_MAIN_LOOP_SPIN);
> +            warn_report("I/O thread spun for %d iterations",
> +                        MAX_MAIN_LOOP_SPIN);
>              notified = true;
>          }

Drop the periods from the warning messages, and you may add
Reviewed-by: Markus Armbruster <address@hidden>

I encourage you to also use the opportunity to improve line breaks.

I'm not asking you to fix the other issues with the messages.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]