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

[Alistair Francis]

On Fri, Jul 7, 2017 at 5:06 AM, Eduardo Habkost <address@hidden> wrote:
> 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).

I split your comments into a new patch as I wasn't trying to fix the
messages in this patch.

>
> [...]
>> 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).

Done.

>
>
>>          }
>>          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.

They aren't really sentences when every message has a prefix, so I
just removed the periods.

>
>>          }
>>          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.

Fixed.

>
>
>>      }
>> @@ -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);

Fixed.

>
>
>>      }
>>
>>      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 "(".

Fixed.

>
>>          }
>>      }
>>
> [...]
>> 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.

Fixed.

Thanks,
Alistair

>
>
>>          }
>>
> [...]
>
> --
> Eduardo