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: Alistair Francis
Subject: Re: [Qemu-devel] [PATCH v2 4/5] Convert multi-line fprintf() to warn_report()
Date: Mon, 14 Aug 2017 11:48:20 -0700

On Mon, Aug 14, 2017 at 6:30 AM, Markus Armbruster <address@hidden> wrote:
> 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.

I'm happy to fix them. Do you want them fixed in this commit or split
into a seperate commit?

Thanks,
Alistair



reply via email to

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