[Qemu-devel] [PATCH] hw/arm/virt: error_report cleanups

[Qemu-devel] [PATCH] hw/arm/virt: error_report cleanups

[Andrew Jones]

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 hw/arm/virt.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 77d9267599b7e..9c6792cea16f6 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -941,8 +941,8 @@ static void machvirt_init(MachineState *machine)
     if (!gic_version) {
         gic_version = kvm_arm_vgic_probe();
         if (!gic_version) {
-            error_report("Unable to determine GIC version supported by host\n"
-                         "Probably KVM acceleration is not supported\n");
+            error_report("Unable to determine GIC version supported by host");
+            error_printf("KVM acceleration is probably not supported\n");
             exit(1);
         }
     }
@@ -990,7 +990,7 @@ static void machvirt_init(MachineState *machine)
         char *cpuopts = g_strdup(cpustr[1]);
 
         if (!oc) {
-            fprintf(stderr, "Unable to find CPU definition\n");
+            error_report("Unable to find CPU definition");
             exit(1);
         }
         cpuobj = object_new(object_class_get_name(oc));
@@ -1126,8 +1126,8 @@ static void virt_set_gic_version(Object *obj, const char *value, Error **errp)
     } else if (!strcmp(value, "host")) {
         vms->gic_version = 0; /* Will probe later */
     } else {
-        error_report("Invalid gic-version option value\n"
-                     "Allowed values are: 3, 2, host\n");
+        error_report("Invalid gic-version option value");
+        error_printf("Allowed gic-version values are: 3, 2, host\n");
         exit(1);
     }
 }
-- 
2.4.3

Re: [Qemu-devel] [PATCH] hw/arm/virt: error_report cleanups

[Peter Maydell]

On 7 November 2015 at 15:25, Andrew Jones <drjones@redhat.com> wrote:
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  hw/arm/virt.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 77d9267599b7e..9c6792cea16f6 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -941,8 +941,8 @@ static void machvirt_init(MachineState *machine)
>      if (!gic_version) {
>          gic_version = kvm_arm_vgic_probe();
>          if (!gic_version) {
> -            error_report("Unable to determine GIC version supported by host\n"
> -                         "Probably KVM acceleration is not supported\n");
> +            error_report("Unable to determine GIC version supported by host");
> +            error_printf("KVM acceleration is probably not supported\n");
>              exit(1);
>          }
>      }
> @@ -990,7 +990,7 @@ static void machvirt_init(MachineState *machine)
>          char *cpuopts = g_strdup(cpustr[1]);
>
>          if (!oc) {
> -            fprintf(stderr, "Unable to find CPU definition\n");
> +            error_report("Unable to find CPU definition");
>              exit(1);
>          }
>          cpuobj = object_new(object_class_get_name(oc));
> @@ -1126,8 +1126,8 @@ static void virt_set_gic_version(Object *obj, const char *value, Error **errp)
>      } else if (!strcmp(value, "host")) {
>          vms->gic_version = 0; /* Will probe later */
>      } else {
> -        error_report("Invalid gic-version option value\n"
> -                     "Allowed values are: 3, 2, host\n");
> +        error_report("Invalid gic-version option value");
> +        error_printf("Allowed gic-version values are: 3, 2, host\n");
>          exit(1);
>      }

Would it be better just to have a single error_report
for these without the newlines, eg

          error_report("Unable to determine GIC version supported by host. "
                       "KVM acceleration is probably not supported.");

?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] hw/arm/virt: error_report cleanups

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 7 November 2015 at 15:25, Andrew Jones <drjones@redhat.com> wrote:
>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>> ---
>>  hw/arm/virt.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 77d9267599b7e..9c6792cea16f6 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -941,8 +941,8 @@ static void machvirt_init(MachineState *machine)
>>      if (!gic_version) {
>>          gic_version = kvm_arm_vgic_probe();
>>          if (!gic_version) {
>> -            error_report("Unable to determine GIC version supported by host\n"
>> -                         "Probably KVM acceleration is not supported\n");
>> +            error_report("Unable to determine GIC version supported by host");
>> +            error_printf("KVM acceleration is probably not supported\n");
>>              exit(1);
>>          }
>>      }
>> @@ -990,7 +990,7 @@ static void machvirt_init(MachineState *machine)
>>          char *cpuopts = g_strdup(cpustr[1]);
>>
>>          if (!oc) {
>> -            fprintf(stderr, "Unable to find CPU definition\n");
>> +            error_report("Unable to find CPU definition");
>>              exit(1);
>>          }
>>          cpuobj = object_new(object_class_get_name(oc));
>> @@ -1126,8 +1126,8 @@ static void virt_set_gic_version(Object *obj, const char *value, Error **errp)
>>      } else if (!strcmp(value, "host")) {
>>          vms->gic_version = 0; /* Will probe later */
>>      } else {
>> -        error_report("Invalid gic-version option value\n"
>> -                     "Allowed values are: 3, 2, host\n");
>> +        error_report("Invalid gic-version option value");
>> +        error_printf("Allowed gic-version values are: 3, 2, host\n");
>>          exit(1);
>>      }
>
> Would it be better just to have a single error_report
> for these without the newlines, eg
>
>           error_report("Unable to determine GIC version supported by host. "
>                        "KVM acceleration is probably not supported.");
>
> ?

For consistency, error messages should be a phrase, not a full sentence,
let alone a paraphraph.

You can of course turn any parapgraph into a phrase by stringing
together its parts with semicolons, but that's cheating :)

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH] hw/arm/virt: error_report cleanups

[Peter Maydell]

On 9 November 2015 at 07:44, Markus Armbruster <armbru@redhat.com> wrote:
> For consistency, error messages should be a phrase, not a full sentence,
> let alone a paraphraph.

This is in direct conflict with wanting them to be actually useful
to users :-(

thanks
-- PMM

Re: [Qemu-devel] [PATCH] hw/arm/virt: error_report cleanups

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 9 November 2015 at 07:44, Markus Armbruster <armbru@redhat.com> wrote:
>> For consistency, error messages should be a phrase, not a full sentence,
>> let alone a paraphraph.
>
> This is in direct conflict with wanting them to be actually useful
> to users :-(

I appreciate your drive for useful error messages.  Judging from the
error messages we got, it's a rare thing.

Let me rephrase.  The error message proper (the thing emitted by
error_report()) should be a phrase, and it should be short and to the
point.  It can be followed by hints.  Compare:

    qemu-system-arm: Unable to determine GIC version supported by host. KVM acceleration is probably not supported.

and

    qemu-system-arm: Unable to determine GIC version supported by host
    KVM acceleration is probably not supported

I prefer the latter.  The error message proper is short and to the
point.  The hint points to the most probable cause.  Sensible line
lengths.

By the way, the error.h API supports this message + hints convention
since commit 50b7b00.

Re: [Qemu-devel] [PATCH] hw/arm/virt: error_report cleanups

[Peter Maydell]

On 9 November 2015 at 10:21, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On 9 November 2015 at 07:44, Markus Armbruster <armbru@redhat.com> wrote:
>>> For consistency, error messages should be a phrase, not a full sentence,
>>> let alone a paraphraph.
>>
>> This is in direct conflict with wanting them to be actually useful
>> to users :-(
>
> I appreciate your drive for useful error messages.  Judging from the
> error messages we got, it's a rare thing.
>
> Let me rephrase.  The error message proper (the thing emitted by
> error_report()) should be a phrase, and it should be short and to the
> point.  It can be followed by hints.  Compare:
>
>     qemu-system-arm: Unable to determine GIC version supported by host. KVM acceleration is probably not supported.
>
> and
>
>     qemu-system-arm: Unable to determine GIC version supported by host
>     KVM acceleration is probably not supported
>
> I prefer the latter.  The error message proper is short and to the
> point.  The hint points to the most probable cause.  Sensible line
> lengths.

I agree that the latter is preferable; I had been under the
impression that we weren't allowed to use newlines in error
messages, though...

> By the way, the error.h API supports this message + hints convention
> since commit 50b7b00.

Thanks, I had missed this useful improvement to the API.
How does it work in cases like this where we don't have
an Error* to fill in?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] hw/arm/virt: error_report cleanups

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 9 November 2015 at 10:21, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> On 9 November 2015 at 07:44, Markus Armbruster <armbru@redhat.com> wrote:
>>>> For consistency, error messages should be a phrase, not a full sentence,
>>>> let alone a paraphraph.
>>>
>>> This is in direct conflict with wanting them to be actually useful
>>> to users :-(
>>
>> I appreciate your drive for useful error messages.  Judging from the
>> error messages we got, it's a rare thing.
>>
>> Let me rephrase.  The error message proper (the thing emitted by
>> error_report()) should be a phrase, and it should be short and to the
>> point.  It can be followed by hints.  Compare:
>>
>>     qemu-system-arm: Unable to determine GIC version supported by
>> host. KVM acceleration is probably not supported.
>>
>> and
>>
>>     qemu-system-arm: Unable to determine GIC version supported by host
>>     KVM acceleration is probably not supported
>>
>> I prefer the latter.  The error message proper is short and to the
>> point.  The hint points to the most probable cause.  Sensible line
>> lengths.
>
> I agree that the latter is preferable; I had been under the
> impression that we weren't allowed to use newlines in error
> messages, though...

error_report()'s contract:

/*
 * Print an error message to current monitor if we have one, else to stderr.
 * Format arguments like sprintf().  The result should not contain
 * newlines.
 * Prepend the current location and append a newline.
 * It's wrong to call this in a QMP monitor.  Use error_setg() there.
 */

If you want to print additional information, that's totally fine!  Use
error-printf().

>> By the way, the error.h API supports this message + hints convention
>> since commit 50b7b00.
>
> Thanks, I had missed this useful improvement to the API.
> How does it work in cases like this where we don't have
> an Error* to fill in?

You do what error_report_err() would do had you had an Error *err to
fill in:

void error_report_err(Error *err)
{
    error_report("%s", error_get_pretty(err));
    if (err->hint) {
        error_printf_unless_qmp("%s\n", err->hint->str);
    }
    error_free(err);
}

In other words, you print the error message proper with error_report(),
and the additional information with error_printf().

error_report_err() uses error_printf_unless_qmp() instead because it
wants to tolerate misuse in QMP context.  It isn't an assertion failure
error mostly for historical reasons.

Re: [Qemu-devel] [PATCH] hw/arm/virt: error_report cleanups

[Peter Maydell]

On 9 November 2015 at 18:52, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> Thanks, I had missed this useful improvement to the API.
>> How does it work in cases like this where we don't have
>> an Error* to fill in?
>
> You do what error_report_err() would do had you had an Error *err to
> fill in:

> In other words, you print the error message proper with error_report(),
> and the additional information with error_printf().

...so in conclusion Andrew's patch is correct as it stands
and I should just apply it? :-)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] hw/arm/virt: error_report cleanups

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 9 November 2015 at 18:52, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>> Thanks, I had missed this useful improvement to the API.
>>> How does it work in cases like this where we don't have
>>> an Error* to fill in?
>>
>> You do what error_report_err() would do had you had an Error *err to
>> fill in:
>
>> In other words, you print the error message proper with error_report(),
>> and the additional information with error_printf().
>
> ...so in conclusion Andrew's patch is correct as it stands
> and I should just apply it? :-)

Yes.  It got my R-by :)

Re: [Qemu-devel] [PATCH] hw/arm/virt: error_report cleanups

[Peter Maydell]

On 10 November 2015 at 09:39, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> ...so in conclusion Andrew's patch is correct as it stands
>> and I should just apply it? :-)
>
> Yes.  It got my R-by :)

OK, applied to target-arm.next. Thanks for walking me through this.

-- PMM

Re: [Qemu-devel] [PATCH] hw/arm/virt: error_report cleanups

[Andrew Jones]

On Tue, Nov 10, 2015 at 11:55:55AM +0000, Peter Maydell wrote:
> On 10 November 2015 at 09:39, Markus Armbruster <armbru@redhat.com> wrote:
> > Peter Maydell <peter.maydell@linaro.org> writes:
> >> ...so in conclusion Andrew's patch is correct as it stands
> >> and I should just apply it? :-)
> >
> > Yes.  It got my R-by :)
> 
> OK, applied to target-arm.next. Thanks for walking me through this.
>

Thanks guys! And I'm glad you saw this patch Markus! I'll definitely
remember to CC you on all error_* patches in the future :-)

drew