Re: [Qemu-trivial] [Qemu-devel] [PATCH for-1.4 1/6] error: Clean up error strings with embedded newlines

[Markus Armbruster]

Peter Maydell <address@hidden> writes:

> On 8 February 2013 12:15, Markus Armbruster <address@hidden> wrote:
>> The arguments of error_report() should yield a short error string
>> without newlines.
>>
>> A few places try to print additional help after the error message by
>> embedding newlines in the error string.  That's nice, but let's do it
>> the right way.
>>
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  hw/kvm/pci-assign.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/kvm/pci-assign.c b/hw/kvm/pci-assign.c
>> index 896cfe8..c31a7f2 100644
>> --- a/hw/kvm/pci-assign.c
>> +++ b/hw/kvm/pci-assign.c
>> @@ -936,8 +936,8 @@ retry:
>>              /* Retry with host-side MSI. There might be an IRQ conflict and
>>               * either the kernel or the device doesn't support sharing. */
>>              error_report("Host-side INTx sharing not supported, "
>> -                         "using MSI instead.\n"
>> -                         "Some devices do not to work properly in this 
>> mode.");
>> +                         "using MSI instead");
>> +            error_printf("Some devices do not to work properly in this 
>> mode.");
>
> "do not work properly"

Since I'm touching the line anyway...

>>              dev->features |= ASSIGNED_DEVICE_PREFER_MSI_MASK;
>>              goto retry;
>>          }
>> @@ -1903,10 +1903,10 @@ static void 
>> assigned_dev_load_option_rom(AssignedDevice *dev)
>>      memset(ptr, 0xff, st.st_size);
>>
>>      if (!fread(ptr, 1, st.st_size, fp)) {
>> -        error_report("pci-assign: Cannot read from host %s\n"
>> -                     "\tDevice option ROM contents are probably invalid "
>> +        error_report("pci-assign: Cannot read from host %s", rom_file);
>> +        error_printf("\tDevice option ROM contents are probably invalid "
>>                       "(check dmesg).\n\tSkip option ROM probe with 
>> rombar=0, "
>> -                     "or load from file with romfile=", rom_file);
>> +                     "or load from file with romfile=\n");
>
> So should error_printf() strings end with \n or not? This one does,
> the one in the hunk above doesn't...

The hunk above is a bug.  Will fix.

> (also we should probably either drop those \t indents or have
> error_printf() add them automatically or otherwise indicate that
> this is followon additional info about a previous error.)

error_printf() is just like printf(), except it prints to wherever error
messages go.  In fact, error_report() uses error_printf() to print.

We could have an API to for emitting helpful text after an error
message.  Its implementation could print the text indented, if we so
desire.  I doubt it's really worthwhile.

Since this is the only place that indents text following an error
message, let's just drop the tabs.

Thanks!