| Message ID | 51ED9E3F.3080504@hds.com |
|---|---|
| State | New |
| Headers | show |
Am 22.07.2013 23:03, schrieb Seiji Aguchi: > 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 <seiji.aguchi@hds.com> > --- > 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
Andreas Färber <afaerber@suse.de> writes: > Am 22.07.2013 23:03, schrieb Seiji Aguchi: >> 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 <seiji.aguchi@hds.com> >> --- >> 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. 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 [...]
On Mon, 22 Jul 2013 23:23:29 +0200 Andreas Färber <afaerber@suse.de> wrote: > Am 22.07.2013 23:03, schrieb Seiji Aguchi: > > 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 <seiji.aguchi@hds.com> > > --- > > 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()? Yeah, we're converting fprintf(stderr,) calls to error_report() so that error messages get a timestamp. > 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.
Am 29.07.2013 23:20, schrieb Luiz Capitulino: > On Mon, 22 Jul 2013 23:23:29 +0200 > Andreas Färber <afaerber@suse.de> wrote: >> Am 22.07.2013 23:03, schrieb Seiji Aguchi: >>> 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
On Mon, 29 Jul 2013 23:23:32 +0200 Andreas Färber <afaerber@suse.de> wrote: > Am 29.07.2013 23:20, schrieb Luiz Capitulino: > > On Mon, 22 Jul 2013 23:23:29 +0200 > > Andreas Färber <afaerber@suse.de> wrote: > >> Am 22.07.2013 23:03, schrieb Seiji Aguchi: > >>> 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? :) 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.
Luiz Capitulino <lcapitulino@redhat.com> writes: > On Mon, 29 Jul 2013 23:23:32 +0200 > Andreas Färber <afaerber@suse.de> wrote: > >> Am 29.07.2013 23:20, schrieb Luiz Capitulino: >> > On Mon, 22 Jul 2013 23:23:29 +0200 >> > Andreas Färber <afaerber@suse.de> wrote: >> >> Am 22.07.2013 23:03, schrieb Seiji Aguchi: >> >>> 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? :) > > 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. > But it's widespread and it's where the timestamp lives, so calling fprintf() > instead of error_report() will probably start hurting soon.
On Tue, 30 Jul 2013 02:00:40 +0200 Markus Armbruster <armbru@redhat.com> wrote: > Luiz Capitulino <lcapitulino@redhat.com> writes: > > > On Mon, 29 Jul 2013 23:23:32 +0200 > > Andreas Färber <afaerber@suse.de> wrote: > > > >> Am 29.07.2013 23:20, schrieb Luiz Capitulino: > >> > On Mon, 22 Jul 2013 23:23:29 +0200 > >> > Andreas Färber <afaerber@suse.de> wrote: > >> >> Am 22.07.2013 23:03, schrieb Seiji Aguchi: > >> >>> 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? :) > > > > 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.
Markus, Luiz > > 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 > -----Original Message----- > From: Luiz Capitulino [mailto:lcapitulino@redhat.com] > Sent: Monday, July 29, 2013 8:35 PM > To: Markus Armbruster > Cc: Andreas Färber; Seiji Aguchi; lersek@redhat.com; qemu-devel@nongnu.org; Tomoki Sekiyama > Subject: Re: [Qemu-devel] [PATCH] Convert stderr message calling error_get_pretty() to error_report() to prepend timestamp > > On Tue, 30 Jul 2013 02:00:40 +0200 > Markus Armbruster <armbru@redhat.com> wrote: > > > Luiz Capitulino <lcapitulino@redhat.com> writes: > > > > > On Mon, 29 Jul 2013 23:23:32 +0200 > > > Andreas Färber <afaerber@suse.de> wrote: > > > > > >> Am 29.07.2013 23:20, schrieb Luiz Capitulino: > > >> > On Mon, 22 Jul 2013 23:23:29 +0200 > > >> > Andreas Färber <afaerber@suse.de> wrote: > > >> >> Am 22.07.2013 23:03, schrieb Seiji Aguchi: > > >> >>> 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? :) > > > > > > 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.
diff --git a/arch_init.c b/arch_init.c index e9dd96f..da83560 100644 --- a/arch_init.c +++ b/arch_init.c @@ -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); } diff --git a/hw/char/serial.c b/hw/char/serial.c index 6025592..de54a42 100644 --- a/hw/char/serial.c +++ b/hw/char/serial.c @@ -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); } diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 2a87563..375fe7a 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -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); diff --git a/qemu-char.c b/qemu-char.c index c86ce4b..b1f4e95 100644 --- a/qemu-char.c +++ b/qemu-char.c @@ -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)) { diff --git a/target-i386/cpu.c b/target-i386/cpu.c index e3f75a8..5991c9d 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -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)); diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c index 79bfcd8..865b6b1 100644 --- a/target-ppc/translate_init.c +++ b/target-ppc/translate_init.c @@ -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; diff --git a/vl.c b/vl.c index 25b8f2f..c2d0a1c 100644 --- a/vl.c +++ b/vl.c @@ -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); }
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 <seiji.aguchi@hds.com> --- 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(-)