| Message ID | 1446909925-12201-1-git-send-email-drjones@redhat.com |
|---|---|
| State | Accepted |
| Commit | faa811f6de44d58180f5d235787678dcdd4b2e9d |
| Headers | show |
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
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
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
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
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
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
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); } }
Signed-off-by: Andrew Jones <drjones@redhat.com> --- hw/arm/virt.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) -- 2.4.3