On Thu, Jul 06, 2017 at 04:49:44PM -0700, Alistair Francis wrote: > Convert all uses of error_report("[Ww]arning:"... to use warn_report() > instead. This helps standardise on a single method of printing warnings > to the user. > > All of the warnings were found using this regex expression: > error_report.*[Ww]arning: > and replaced with: > warn_report(" > > Signed-off-by: Alistair Francis <alistair.fran...@xilinx.com> > Suggested-by: Thomas Huth <th...@redhat.com>
I have reviewed the changes in: */i386/*, qdev-properties.c, vl.c, and tests/test-qdev-global-props.c. They look good, except for kvm_arch_init_vcpu() below. There are also a few places below where we could improve the error messages in a follow-up patch (or in a new version of this patch if you prefer). [...] > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index 5464977424..6b7bade183 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -2766,17 +2766,17 @@ void acpi_build(AcpiBuildTables *tables, MachineState > *machine) > ACPI_BUILD_ALIGN_SIZE); > if (tables_blob->len > legacy_table_size) { > /* Should happen only with PCI bridges and -M pc-i440fx-2.0. */ > - error_report("Warning: migration may not work."); > + warn_report("migration may not work."); I suggest removing the period. Adding a justification to why migration may not work is a good idea, too. We could copy the messages below (replacing "64k" with legacy_table_size). > } > g_array_set_size(tables_blob, legacy_table_size); > } else { > /* Make sure we have a buffer in case we need to resize the tables. > */ > if (tables_blob->len > ACPI_BUILD_TABLE_SIZE / 2) { > /* As of QEMU 2.1, this fires with 160 VCPUs and 255 memory > slots. */ > - error_report("Warning: ACPI tables are larger than 64k."); > - error_report("Warning: migration may not work."); > - error_report("Warning: please remove CPUs, NUMA nodes, " > - "memory slots or PCI bridges."); > + warn_report("ACPI tables are larger than 64k."); > + warn_report("migration may not work."); > + warn_report("please remove CPUs, NUMA nodes, " > + "memory slots or PCI bridges."); I was going to suggest removing the periods here, but I'm not sure because we have multiple full sentences. However, if we are going to keep full sentences, I believe we should use uppercase letters. > } > acpi_align_size(tables_blob, ACPI_BUILD_TABLE_SIZE); > } > diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c > index 9f2615cbe0..33e20cb3e8 100644 > --- a/hw/i386/kvm/pci-assign.c > +++ b/hw/i386/kvm/pci-assign.c > @@ -1353,9 +1353,9 @@ static int assigned_device_pci_cap_init(PCIDevice > *pci_dev, Error **errp) > PCI_CAP_ID_EXP); > return -EINVAL; > } else if (size != 0x3c) { > - error_report("WARNING, %s: PCIe cap-id 0x%x has " > - "non-standard size 0x%x; std size should be > 0x3c", > - __func__, PCI_CAP_ID_EXP, size); > + warn_report("%s: PCIe cap-id 0x%x has " > + "non-standard size 0x%x; std size should be > 0x3c", > + __func__, PCI_CAP_ID_EXP, size); > } > } else if (version == 0) { > uint16_t vid, did; > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 224fe58fe7..58f8a4f4a5 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -381,8 +381,8 @@ ISADevice *pc_find_fdc0(void) > } > > if (state.multiple) { > - error_report("warning: multiple floppy disk controllers with " > - "iobase=0x3f0 have been found"); > + warn_report("multiple floppy disk controllers with " > + "iobase=0x3f0 have been found"); > error_printf("the one being picked for CMOS setup might not reflect " > "your intent\n"); The error_printf() on the second line could use warn_report() for consistency? In either case, the "\n" needs to be removed. > } > @@ -1310,7 +1310,7 @@ void pc_acpi_init(const char *default_dsdt) > > filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, default_dsdt); > if (filename == NULL) { > - fprintf(stderr, "WARNING: failed to find %s\n", default_dsdt); > + warn_report("failed to find %s", default_dsdt); > } else { > QemuOpts *opts = qemu_opts_create(qemu_find_opts("acpi"), NULL, 0, > &error_abort); > @@ -2087,9 +2087,9 @@ static void pc_machine_set_max_ram_below_4g(Object > *obj, Visitor *v, > } > > if (value < (1ULL << 20)) { > - error_report("Warning: small max_ram_below_4g(%"PRIu64 > - ") less than 1M. BIOS may not work..", > - value); > + warn_report("small max_ram_below_4g(%"PRIu64 > + ") less than 1M. BIOS may not work..", > + value); I suggest removing the "..", adding a space before "(", and possibly removing the word "small". Maybe we could use the same "<fact>; <possible consequence>." pattern from the other messages below? e.g.: warn_report("max_ram_below_4g(%" PRIu64 ") is less than 1M; " "BIOS may not work.", value); > } > > pcms->max_ram_below_4g = value; > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index 22dbef64c6..11b4336a42 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -131,10 +131,10 @@ static void pc_init1(MachineState *machine, > lowmem = 0xc0000000; > } > if (lowmem & ((1ULL << 30) - 1)) { > - error_report("Warning: Large machine and > max_ram_below_4g " > - "(%" PRIu64 ") not a multiple of 1G; " > - "possible bad performance.", > - pcms->max_ram_below_4g); > + warn_report("Large machine and max_ram_below_4g " > + "(%" PRIu64 ") not a multiple of 1G; " > + "possible bad performance.", > + pcms->max_ram_below_4g); > } > } > } > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > index 8f696b7cb6..1653a47f0a 100644 > --- a/hw/i386/pc_q35.c > +++ b/hw/i386/pc_q35.c > @@ -101,9 +101,9 @@ static void pc_q35_init(MachineState *machine) > lowmem = pcms->max_ram_below_4g; > if (machine->ram_size - lowmem > lowmem && > lowmem & ((1ULL << 30) - 1)) { > - error_report("Warning: Large machine and > max_ram_below_4g(%"PRIu64 > - ") not a multiple of 1G; possible bad performance.", > - pcms->max_ram_below_4g); > + warn_report("Large machine and max_ram_below_4g(%"PRIu64 > + ") not a multiple of 1G; possible bad performance.", > + pcms->max_ram_below_4g); I suggest a space before "(". > } > } > [...] > diff --git a/target/i386/kvm.c b/target/i386/kvm.c > index f84a49d366..3b29f5a758 100644 > --- a/target/i386/kvm.c > +++ b/target/i386/kvm.c > @@ -600,10 +600,10 @@ static int kvm_arch_set_tsc_khz(CPUState *cs) > kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ) : > -ENOTSUP; > if (cur_freq <= 0 || cur_freq != env->tsc_khz) { > - error_report("warning: TSC frequency mismatch between " > - "VM (%" PRId64 " kHz) and host (%d kHz), " > - "and TSC scaling unavailable", > - env->tsc_khz, cur_freq); > + warn_report("TSC frequency mismatch between " > + "VM (%" PRId64 " kHz) and host (%d kHz), " > + "and TSC scaling unavailable", > + env->tsc_khz, cur_freq); > return r; > } > } > @@ -919,7 +919,7 @@ int kvm_arch_init_vcpu(CPUState *cs) > error_report("kvm: LMCE not supported"); > return -ENOTSUP; > } > - error_report("warning: Unsupported MCG_CAP bits: 0x%" PRIx64, > + warn_report(" Unsupported MCG_CAP bits: 0x%" PRIx64, > unsupported_caps); Extra whitespace here. > } > [...] -- Eduardo