Convert stderr message calling error_get_pretty() to error_report() to prepend timestamp

PATCH Superseded Seiji Aguchi 4 participants 9 messages Jul 22, 2013 – Jul 30, 2013

lore

Share & tools

lore

Seiji Aguchi

author

2013-07-22 21:04 UTC #0 lore raw

PATCH 1/1 Convert stderr message calling error_get_pretty() to error_report() to prepend timestamp

Convert stderr messages calling error_get_pretty() to error_report().

Timestamp is prepended by -msg timstamp option with it.

This is suggested by Luiz Capitulino.

http://marc.info/?l=qemu-devel&m=137331442628866&w=2

Signed-off-by Seiji Aguchi <[email protected]>

---
 arch_init.c                 |    4 ++--
 hw/char/serial.c            |    5 +++--
 hw/i386/pc.c                |    6 +++---
 qemu-char.c                 |    2 +-
 target-i386/cpu.c           |    2 +-
 target-ppc/translate_init.c |    3 ++-
 vl.c                        |    9 +++++----
 7 files changed, 17 insertions(+), 14 deletions(-)

arch_init.c +2 -2

index e9dd96f..da83560 100644
@@ -1085,8 +1085,8 @@ void do_acpitable_option(const QemuOpts *opts)
 
     acpi_table_add(opts, &err);
     if (err) {
-        fprintf(stderr, "Wrong acpi table provided: %s\n",
-                error_get_pretty(err));
+        error_report("Wrong acpi table provided: %s\n",
+                     error_get_pretty(err));
         error_free(err);
         exit(1);
     }

hw/char/serial.c +3 -2

index 6025592..de54a42 100644
@@ -27,6 +27,7 @@
 #include "sysemu/char.h"
 #include "qemu/timer.h"
 #include "exec/address-spaces.h"
+#include "qemu/error-report.h"
 
 //#define DEBUG_SERIAL

@@ -696,7 +697,7 @@ SerialState *serial_init(int base, qemu_irq irq, int baudbase,
     s->chr = chr;
     serial_realize_core(s, &err);
     if (err != NULL) {
-        fprintf(stderr, "%s\n", error_get_pretty(err));
+        error_report("%s\n", error_get_pretty(err));
         error_free(err);
         exit(1);
     }

@@ -760,7 +761,7 @@ SerialState *serial_mm_init(MemoryRegion *address_space,
 
     serial_realize_core(s, &err);
     if (err != NULL) {
-        fprintf(stderr, "%s\n", error_get_pretty(err));
+        error_report("%s\n", error_get_pretty(err));
         error_free(err);
         exit(1);
     }

hw/i386/pc.c +3 -3

index 2a87563..375fe7a 100644
@@ -980,7 +980,7 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
         cpu = pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i),
                          icc_bridge, &error);
         if (error) {
-            fprintf(stderr, "%s\n", error_get_pretty(error));
+            error_report("%s\n", error_get_pretty(error));
             error_free(error);
             exit(1);
         }

@@ -1087,8 +1087,8 @@ void pc_acpi_init(const char *default_dsdt)
 
         acpi_table_add(opts, &err);
         if (err) {
-            fprintf(stderr, "WARNING: failed to load %s: %s\n", filename,
-                    error_get_pretty(err));
+            error_report("WARNING: failed to load %s: %s\n", filename,
+                         error_get_pretty(err));
             error_free(err);
         }
         g_free(arg);

qemu-char.c +1 -1

index c86ce4b..b1f4e95 100644
@@ -3299,7 +3299,7 @@ CharDriverState *qemu_chr_new(const char *label, const char *filename, void (*in
 
     chr = qemu_chr_new_from_opts(opts, init, &err);
     if (error_is_set(&err)) {
-        fprintf(stderr, "%s\n", error_get_pretty(err));
+        error_report("%s\n", error_get_pretty(err));
         error_free(err);
     }
     if (chr && qemu_opt_get_bool(opts, "mux", 0)) {

target-i386/cpu.c +1 -1

index e3f75a8..5991c9d 100644
@@ -1839,7 +1839,7 @@ X86CPU *cpu_x86_init(const char *cpu_model)
 
 out:
     if (error) {
-        fprintf(stderr, "%s\n", error_get_pretty(error));
+        error_report("%s\n", error_get_pretty(error));
         error_free(error);
         if (cpu != NULL) {
             object_unref(OBJECT(cpu));

target-ppc/translate_init.c +2 -1

index 79bfcd8..865b6b1 100644
@@ -27,6 +27,7 @@
 #include "cpu-models.h"
 #include "mmu-hash32.h"
 #include "mmu-hash64.h"
+#include "qemu/error-report.h"
 
 //#define PPC_DUMP_CPU
 //#define PPC_DEBUG_SPR

@@ -8174,7 +8175,7 @@ PowerPCCPU *cpu_ppc_init(const char *cpu_model)
 
     object_property_set_bool(OBJECT(cpu), true, "realized", &err);
     if (err != NULL) {
-        fprintf(stderr, "%s\n", error_get_pretty(err));
+        error_report("%s\n", error_get_pretty(err));
         error_free(err);
         object_unref(OBJECT(cpu));
         return NULL;

vl.c +5 -4

index 25b8f2f..c2d0a1c 100644
@@ -2393,7 +2393,7 @@ static int chardev_init_func(QemuOpts *opts, void *opaque)
 
     qemu_chr_new_from_opts(opts, NULL, &local_err);
     if (error_is_set(&local_err)) {
-        fprintf(stderr, "%s\n", error_get_pretty(local_err));
+        error_report("%s\n", error_get_pretty(local_err));
         error_free(local_err);
         return -1;
     }

@@ -4375,8 +4375,8 @@ int main(int argc, char **argv, char **envp)
         vnc_display_init(ds);
         vnc_display_open(ds, vnc_display, &local_err);
         if (local_err != NULL) {
-            fprintf(stderr, "Failed to start VNC server on `%s': %s\n",
-                    vnc_display, error_get_pretty(local_err));
+            error_report("Failed to start VNC server on `%s': %s\n",
+                         vnc_display, error_get_pretty(local_err));
             error_free(local_err);
             exit(1);
         }

@@ -4419,7 +4419,8 @@ int main(int argc, char **argv, char **envp)
         Error *local_err = NULL;
         qemu_start_incoming_migration(incoming, &local_err);
         if (local_err) {
-            fprintf(stderr, "-incoming %s: %s\n", incoming, error_get_pretty(local_err));
+            error_report("-incoming %s: %s\n", incoming,
+                         error_get_pretty(local_err));
             error_free(local_err);
             exit(1);
         }

Signature · 1 line

1.7.1

Andreas Färber

2013-07-22 21:23 UTC #1 lore raw

Am 22.07.2013 23:03, schrieb Seiji Aguchi:

Quoted 10 lines

Convert stderr messages calling error_get_pretty() to error_report().
Timestamp is prepended by -msg timstamp option with it.
This is suggested by Luiz Capitulino.
http://marc.info/?l=qemu-devel&m=137331442628866&w=2
Signed-off-by Seiji Aguchi <[email protected]>

Quoted 9 lines

---
 arch_init.c                 |    4 ++--
 hw/char/serial.c            |    5 +++--
 hw/i386/pc.c                |    6 +++---
 qemu-char.c                 |    2 +-
 target-i386/cpu.c           |    2 +-
 target-ppc/translate_init.c |    3 ++-
 vl.c                        |    9 +++++----
 7 files changed, 17 insertions(+), 14 deletions(-)

How is this related to error_get_pretty()?

And does error_report() expect a trailing \n or not? I would assume no, but spotted some in this patch.

If you feel strongly about having error_report() consistently, please let me know if there are any in my qom-next queue that need fixing.
https://github.com/afaerber/qemu-cpu/commits/qom-next

Thanks,
Andreas

Signature · 2 lines

SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Markus Armbruster

2013-07-23 06:47 UTC #2 lore raw

Quoted 21 lines · Andreas Färber

Am 22.07.2013 23:03, schrieb Seiji Aguchi:
Quoted 10 lines
Convert stderr messages calling error_get_pretty() to error_report().
Timestamp is prepended by -msg timstamp option with it.
This is suggested by Luiz Capitulino.
http://marc.info/?l=qemu-devel&m=137331442628866&w=2
Signed-off-by Seiji Aguchi <[email protected]>
Quoted 9 lines
---
 arch_init.c                 |    4 ++--
 hw/char/serial.c            |    5 +++--
 hw/i386/pc.c                |    6 +++---
 qemu-char.c                 |    2 +-
 target-i386/cpu.c           |    2 +-
 target-ppc/translate_init.c |    3 ++-
 vl.c                        |    9 +++++----
 7 files changed, 17 insertions(+), 14 deletions(-)

Quoted 4 lines

How is this related to error_get_pretty()?
And does error_report() expect a trailing \n or not? I would assume no, but spotted some in this patch.

You're correct, and the patch needs fixing. See commits

312fd5f error: Strip trailing '\n' from error string arguments (again) be62a2e Strip trailing '\n' from error_report()'s first argument (again) 6daf194 Strip trailing '\n' from error_report()'s first argument

[...]

Luiz Capitulino

2013-07-29 21:20 UTC #3 lore raw

In reply to Andreas Färber

Quoted 21 lines · On Mon, 22 Jul 2013 23:23:29 +0200 Andreas Färber <[email protected]> wrote:

Am 22.07.2013 23:03, schrieb Seiji Aguchi:
Quoted 10 lines
Convert stderr messages calling error_get_pretty() to error_report().
Timestamp is prepended by -msg timstamp option with it.
This is suggested by Luiz Capitulino.
http://marc.info/?l=qemu-devel&m=137331442628866&w=2
Signed-off-by Seiji Aguchi <[email protected]>
Quoted 9 lines
---
 arch_init.c                 |    4 ++--
 hw/char/serial.c            |    5 +++--
 hw/i386/pc.c                |    6 +++---
 qemu-char.c                 |    2 +-
 target-i386/cpu.c           |    2 +-
 target-ppc/translate_init.c |    3 ++-
 vl.c                        |    9 +++++----
 7 files changed, 17 insertions(+), 14 deletions(-)

Quoted 1 line

How is this related to error_get_pretty()?

Yeah, we're converting fprintf(stderr,) calls to error_report() so that error messages get a timestamp.

Quoted 2 lines

And does error_report() expect a trailing \n or not? I would assume no, but spotted some in this patch.

Yes, already confirmed by Markus.

Andreas Färber

2013-07-29 21:23 UTC #4 lore raw

Am 29.07.2013 23:20, schrieb Luiz Capitulino:

Quoted 10 lines

Quoted 5 lines · On Mon, 22 Jul 2013 23:23:29 +0200 Andreas Färber <[email protected]> wrote:
Am 22.07.2013 23:03, schrieb Seiji Aguchi:
Quoted 2 lines
Convert stderr messages calling error_get_pretty() to error_report().
How is this related to error_get_pretty()?
Yeah, we're converting fprintf(stderr,) calls to error_report() so that error messages get a timestamp.

Want to add that to my http://wiki.qemu.org/DeveloperNews so that people reading it stop adding new ones? :)

Andreas

Signature · 2 lines

SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Luiz Capitulino

2013-07-29 21:46 UTC #5 lore raw

Quoted 7 lines · On Mon, 29 Jul 2013 23:23:32 +0200 Andreas Färber <[email protected]> wrote:

Am 29.07.2013 23:20, schrieb Luiz Capitulino:
Quoted 6 lines
Quoted 4 lines · On Mon, 22 Jul 2013 23:23:29 +0200 Andreas Färber <[email protected]> wrote:
Am 22.07.2013 23:03, schrieb Seiji Aguchi:
Quoted 2 lines
Convert stderr messages calling error_get_pretty() to error_report().

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
How is this related to error_get_pretty()?
Yeah, we're converting fprintf(stderr,) calls to error_report() so that error messages get a timestamp.
Want to add that to my http://wiki.qemu.org/DeveloperNews so that people reading it stop adding new ones? :)

Big IMHO here, but honestly speaking I'm not a huge fan of error_report() because I think that random code shouldn't be allowed to print to the monitor (only HMP code should).

But it's widespread and it's where the timestamp lives, so calling fprintf() instead of error_report() will probably start hurting soon.

Markus Armbruster

2013-07-30 00:00 UTC #6 lore raw

Quoted 15 lines · Luiz Capitulino

Quoted 12 lines · On Mon, 29 Jul 2013 23:23:32 +0200 Andreas Färber <[email protected]> wrote:
Am 29.07.2013 23:20, schrieb Luiz Capitulino:
Quoted 10 lines
Quoted 5 lines · On Mon, 22 Jul 2013 23:23:29 +0200 Andreas Färber <[email protected]> wrote:
Am 22.07.2013 23:03, schrieb Seiji Aguchi:
Quoted 2 lines
Convert stderr messages calling error_get_pretty() to error_report().
How is this related to error_get_pretty()?
Yeah, we're converting fprintf(stderr,) calls to error_report() so that error messages get a timestamp.

Quoted 6 lines

Quoted 2 lines
Want to add that to my http://wiki.qemu.org/DeveloperNews so that people reading it stop adding new ones? :)
Big IMHO here, but honestly speaking I'm not a huge fan of error_report() because I think that random code shouldn't be allowed to print to the monitor (only HMP code should).

A conversion from fprintf() to error_report() is always an improvement, because

1. It makes the nature of the message explicit in the code.

2. It prints the error message in the proper form, with error location when available.

3. When called in monitor context, it prints to the monitor rather than stderr. Printing directly to the monitor may not be clean, but it sure beats printing to stderr, where the monitor user can't see it unless the monitor happens to be on stdio. Besides, there's plenty of code that doesn't ever run in monitor context, thus needn't worry about QMP vs. HMP.

Quoted 2 lines

But it's widespread and it's where the timestamp lives, so calling fprintf() instead of error_report() will probably start hurting soon.

Luiz Capitulino

2013-07-30 00:34 UTC #7 lore raw

Quoted 20 lines · On Tue, 30 Jul 2013 02:00:40 +0200 Markus Armbruster <[email protected]> wrote:

Quoted 18 lines · Luiz Capitulino
Quoted 14 lines · On Mon, 29 Jul 2013 23:23:32 +0200 Andreas Färber <[email protected]> wrote:
Am 29.07.2013 23:20, schrieb Luiz Capitulino:
Quoted 10 lines
Quoted 5 lines · On Mon, 22 Jul 2013 23:23:29 +0200 Andreas Färber <[email protected]> wrote:
Am 22.07.2013 23:03, schrieb Seiji Aguchi:
Quoted 2 lines
Convert stderr messages calling error_get_pretty() to error_report().
How is this related to error_get_pretty()?
Yeah, we're converting fprintf(stderr,) calls to error_report() so that error messages get a timestamp.
Want to add that to my http://wiki.qemu.org/DeveloperNews so that people reading it stop adding new ones? :)

Quoted 6 lines

Quoted 3 lines
Big IMHO here, but honestly speaking I'm not a huge fan of error_report() because I think that random code shouldn't be allowed to print to the monitor (only HMP code should).
A conversion from fprintf() to error_report() is always an improvement, because

No disagreement here.

Seiji Aguchi

author

2013-07-30 01:22 UTC #8 lore raw

Markus, Luiz

Quoted 4 lines

Quoted 2 lines
A conversion from fprintf() to error_report() is always an improvement, because
No disagreement here.

Thank you for discussing this.
I will update my patch by converting from fprintf(stderr, ..) to error_report().

Seiji

Quoted 38 lines

-----Original Message-----
From: Luiz Capitulino [mailto:[email protected]]
Sent: Monday, July 29, 2013 8:35 PM
To: Markus Armbruster
Cc: Andreas Färber; Seiji Aguchi; [email protected]; [email protected]; Tomoki Sekiyama
Subject: Re: [Qemu-devel] [PATCH] Convert stderr message calling error_get_pretty() to error_report() to prepend timestamp
Quoted 20 lines · On Tue, 30 Jul 2013 02:00:40 +0200 Markus Armbruster <[email protected]> wrote:
Quoted 18 lines · Luiz Capitulino
Quoted 14 lines · On Mon, 29 Jul 2013 23:23:32 +0200 Andreas Färber <[email protected]> wrote:
Am 29.07.2013 23:20, schrieb Luiz Capitulino:
Quoted 10 lines
Quoted 5 lines · On Mon, 22 Jul 2013 23:23:29 +0200 Andreas Färber <[email protected]> wrote:
Am 22.07.2013 23:03, schrieb Seiji Aguchi:
Quoted 2 lines
Convert stderr messages calling error_get_pretty() to error_report().
How is this related to error_get_pretty()?
Yeah, we're converting fprintf(stderr,) calls to error_report() so that error messages get a timestamp.
Want to add that to my http://wiki.qemu.org/DeveloperNews so that people reading it stop adding new ones? :)
Quoted 6 lines
Quoted 3 lines
Big IMHO here, but honestly speaking I'm not a huge fan of error_report() because I think that random code shouldn't be allowed to print to the monitor (only HMP code should).
A conversion from fprintf() to error_report() is always an improvement, because
No disagreement here.