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

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> On Fri, 08 Feb 2013 19:56:17 +0100
> Markus Armbruster <address@hidden> wrote:
>
>> Luiz Capitulino <address@hidden> writes:
>> 
>> > On Fri,  8 Feb 2013 17:17:07 +0100
>> > 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.
>> >> 
>> >> Since I'm touching these lines anyway, drop a stray preposition and
>> >> some tabs.  We don't use tabs for similar messages elsewhere.
>> >> 
>> >> Signed-off-by: Markus Armbruster <address@hidden>
>> >> ---
>> >>  hw/kvm/pci-assign.c | 12 ++++++------
>> >>  1 file changed, 6 insertions(+), 6 deletions(-)
>> >> 
>> >> diff --git a/hw/kvm/pci-assign.c b/hw/kvm/pci-assign.c
>> >> index 896cfe8..da64b5b 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 work properly in this 
>> >> mode.\n");
>> >
>> > This is fixing command-line, right?
>> 
>> Whatever made assign_intx() run.  I'm not familiar with this code, and I
>> don't know how to trigger the error.
>> 
>> Hmm, one call chain is via PCIDeviceClass init method assigned_initfn().
>> So it could also be device_add.
>> 
>> > I honestly don't know which is less worse, the current code or having
>> > to call two different functions in the correct order to report an
>> > error :(
>> 
>> You call one function to report the error.  In the rare case that you
>> want to add some explanation or hint to the error message, you use
>> another function to print to the error destination.  Big deal :)
>
> I think the important point is not whether or not this is a big deal,
> but that this is a bad API which will break from time to time (as it's
> more or less the case now).

See the discussion we're having under PATCH 4/6.

>> Explanations and hints are *not* error messages.  Sticking them into the
>> error string like the code does before my patch happens to work due to
>> the way error_report() formats the error message.  Relying on that is
>> unclean.  Besides, error_report()'s function comment clearly stipulates
>> "no newlines".
>
> I agree.
>
> But regarding this patch, we first have to decide whether or not this is
> good for 1.4 and then we have to come with a better solution for this
> (post 1.4).
>
> Regarding the first point, I have to questions:
>
>  1. Were the additional newlines added in 1.4?

Both offending calls were added in commit c3ebd3ba during 1.3
development.

>  2. What's the worse case here?

"worse"?

It's a cleanup.  It's only user-visible effect is getting rid of an
extra newline on stderr.  I'm fixing those globally.  Tiny improvement
in user experience, but next to no risk, thus proposed for 1.4.  Since I
need to touch this call anyway for that, I can just as well clean up the
API abuse.  The alternative is to leave the abuse alone and just strip
the final newline.  That would make me sad.

If you hate the API, fix it.  Don't make me not fix abuses of it :)