| Message ID | 1360340232-4670-2-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 8 Feb 2013 17:17:07 +0100 Markus Armbruster <armbru@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 <armbru@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? 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 :( I'd say the best solution for this would be to propagate errors, but this will loose LOC support. > 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 " > - "(check dmesg).\n\tSkip option ROM probe with rombar=0, " > - "or load from file with romfile=", rom_file); > + error_report("pci-assign: Cannot read from host %s", rom_file); > + error_printf("Device option ROM contents are probably invalid " > + "(check dmesg).\nSkip option ROM probe with rombar=0, " > + "or load from file with romfile=\n"); > memory_region_destroy(&dev->dev.rom); > goto close_rom; > }
Luiz Capitulino <lcapitulino@redhat.com> writes: > On Fri, 8 Feb 2013 17:17:07 +0100 > Markus Armbruster <armbru@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 <armbru@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 :) 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'd say the best solution for this would be to propagate errors, but this > will loose LOC support. I'm unwilling to degrade error message quality even further.
On Fri, 08 Feb 2013 19:56:17 +0100 Markus Armbruster <armbru@redhat.com> wrote: > Luiz Capitulino <lcapitulino@redhat.com> writes: > > > On Fri, 8 Feb 2013 17:17:07 +0100 > > Markus Armbruster <armbru@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 <armbru@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?
Luiz Capitulino <lcapitulino@redhat.com> writes: > On Fri, 08 Feb 2013 19:56:17 +0100 > Markus Armbruster <armbru@redhat.com> wrote: > >> Luiz Capitulino <lcapitulino@redhat.com> writes: >> >> > On Fri, 8 Feb 2013 17:17:07 +0100 >> > Markus Armbruster <armbru@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 <armbru@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). 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 :)
On Fri, 08 Feb 2013 20:48:18 +0100 Markus Armbruster <armbru@redhat.com> wrote: > 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 :) I honestly would prefer to defer this and fix the API once and for all. But it's not worth to discuss this anymore, as this patch has already been merged.
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"); 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 " - "(check dmesg).\n\tSkip option ROM probe with rombar=0, " - "or load from file with romfile=", rom_file); + error_report("pci-assign: Cannot read from host %s", rom_file); + error_printf("Device option ROM contents are probably invalid " + "(check dmesg).\nSkip option ROM probe with rombar=0, " + "or load from file with romfile=\n"); memory_region_destroy(&dev->dev.rom); goto close_rom; }
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 <armbru@redhat.com> --- hw/kvm/pci-assign.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)