On Fri, 08 Feb 2013 19:56:17 +0100
Markus Armbruster <arm...@redhat.com> wrote:
> Luiz Capitulino <lcapitul...@redhat.com> writes:
>
> > On Fri, 8 Feb 2013 17:17:07 +0100
> > Markus Armbruster <arm...@redhat.com> 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 <arm...@redhat.com>
> >> ---
> >> 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?
