Re: [Qemu-devel] [PATCH v1 3/6] Convert error_report() to warn_report()

[Eduardo Habkost]

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 <address@hidden>
> Suggested-by: Thomas Huth <address@hidden>

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