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

[Luiz Capitulino]

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

> 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?

 2. What's the worse case here?