Re: [Qemu-devel] [PATCH v2] Convert stderr message calling error_get_pretty() to error_report()

2013-08-13 Thread Seiji Aguchi 
Luiz,

Can you pick up to your tree?

Seiji

 -Original Message-
 From: Laszlo Ersek [mailto:ler...@redhat.com]
 Sent: Wednesday, August 07, 2013 3:10 PM
 To: Seiji Aguchi
 Cc: qemu-devel@nongnu.org; arm...@redhat.com; Tomoki Sekiyama; 
 lcapitul...@redhat.com
 Subject: Re: [Qemu-devel] [PATCH v2] Convert stderr message calling 
 error_get_pretty() to error_report()
 
 On 08/05/13 21:40, Seiji Aguchi wrote:
  Convert stderr messages calling error_get_pretty()
  to error_report().
 
  Timestamp is prepended by -msg timstamp option with it.
 
  Per Markus's comment below, A conversion from fprintf() to
  error_report() is always an improvement, regardless of
  error_get_pretty().
 
  http://marc.info/?l=qemu-develm=137513283408601w=2
 
  But, it is not reasonable to convert them at one time
  because fprintf() is used everwhere in qemu.
 
  So, it should be done step by step with avoiding regression.
 
  Signed-off-by: Seiji Aguchi seiji.agu...@hds.com
  ---
  Change from v2
   - Remove \n from strings because error_report() appends it.
 
   Patch v1
   http://marc.info/?l=qemu-develm=137452706024842w=2
  ---
   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(-)
 
  diff --git a/arch_init.c b/arch_init.c
  index 68a7ab7..94d45e1 100644
  --- a/arch_init.c
  +++ b/arch_init.c
  @@ -1125,8 +1125,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,
  + error_get_pretty(err));
   error_free(err);
   exit(1);
   }
  diff --git a/hw/char/serial.c b/hw/char/serial.c
  index 6025592..a31eb57 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, 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, error_get_pretty(err));
   error_free(err);
   exit(1);
   }
  diff --git a/hw/i386/pc.c b/hw/i386/pc.c
  index a2b9d88..5bce10f 100644
  --- a/hw/i386/pc.c
  +++ b/hw/i386/pc.c
  @@ -979,7 +979,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, error_get_pretty(error));
   error_free(error);
   exit(1);
   }
  @@ -1097,8 +1097,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, filename,
  + error_get_pretty(err));
   error_free(err);
   }
   g_free(arg);
  diff --git a/qemu-char.c b/qemu-char.c
  index 3f606c9..2e9a11d 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, 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 71ab915..43061e2 100644
  --- a/target-i386/cpu.c
  +++ b/target-i386/cpu.c
  @@ -1843,7 +1843,7 @@ X86CPU *cpu_x86_init(const char *cpu_model)
 
   out:
   if (error) {
  -fprintf(stderr, %s\n, error_get_pretty(error));
  +error_report(%s, 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 0724226..3cf74a2 100644

[Qemu-devel] [PATCH v2] Convert stderr message calling error_get_pretty() to error_report()

2013-08-05 Thread Seiji Aguchi 
Convert stderr messages calling error_get_pretty()
to error_report().

Timestamp is prepended by -msg timstamp option with it.

Per Markus's comment below, A conversion from fprintf() to
error_report() is always an improvement, regardless of 
error_get_pretty().

http://marc.info/?l=qemu-develm=137513283408601w=2

But, it is not reasonable to convert them at one time
because fprintf() is used everwhere in qemu.

So, it should be done step by step with avoiding regression.

Signed-off-by: Seiji Aguchi seiji.agu...@hds.com
---
Change from v2
 - Remove \n from strings because error_report() appends it.

 Patch v1
 http://marc.info/?l=qemu-develm=137452706024842w=2
---
 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(-)

diff --git a/arch_init.c b/arch_init.c
index 68a7ab7..94d45e1 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -1125,8 +1125,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,
+ error_get_pretty(err));
 error_free(err);
 exit(1);
 }
diff --git a/hw/char/serial.c b/hw/char/serial.c
index 6025592..a31eb57 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, 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, error_get_pretty(err));
 error_free(err);
 exit(1);
 }
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index a2b9d88..5bce10f 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -979,7 +979,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, error_get_pretty(error));
 error_free(error);
 exit(1);
 }
@@ -1097,8 +1097,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, filename,
+ error_get_pretty(err));
 error_free(err);
 }
 g_free(arg);
diff --git a/qemu-char.c b/qemu-char.c
index 3f606c9..2e9a11d 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, 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 71ab915..43061e2 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1843,7 +1843,7 @@ X86CPU *cpu_x86_init(const char *cpu_model)
 
 out:
 if (error) {
-fprintf(stderr, %s\n, error_get_pretty(error));
+error_report(%s, 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 0724226..3cf74a2 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
@@ -8177,7 +8178,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, error_get_pretty(err));
 error_free(err);
 object_unref(OBJECT(cpu));
 return NULL;
diff --git a/vl.c b/vl.c
index 25b8f2f..2e427d8 100644
--- a/vl.c
+++ b/vl.c
@@ -2393,7

Re: [Qemu-devel] [PATCH] Convert stderr message calling error_get_pretty() to error_report() to prepend timestamp

2013-07-29 Thread Seiji Aguchi 
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:lcapitul...@redhat.com]
 Sent: Monday, July 29, 2013 8:35 PM
 To: Markus Armbruster
 Cc: Andreas Färber; Seiji Aguchi; ler...@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 arm...@redhat.com wrote:
 
  Luiz Capitulino lcapitul...@redhat.com writes:
 
   On Mon, 29 Jul 2013 23:23:32 +0200
   Andreas Färber afaer...@suse.de wrote:
  
   Am 29.07.2013 23:20, schrieb Luiz Capitulino:
On Mon, 22 Jul 2013 23:23:29 +0200
Andreas Färber afaer...@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.

[Qemu-devel] [PATCH] Convert stderr message calling error_get_pretty() to error_report() to prepend timestamp

2013-07-22 Thread 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-develm=137331442628866w=2

Signed-off-by: Seiji Aguchi seiji.agu...@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(-)

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

Re: [Qemu-devel] [PULL 6/6] add timestamp to error_report()

2013-07-12 Thread Seiji Aguchi 


 -Original Message-
 From: qemu-devel-bounces+seiji.aguchi=hds@nongnu.org 
 [mailto:qemu-devel-bounces+seiji.aguchi=hds@nongnu.org]
 On Behalf Of Luiz Capitulino
 Sent: Friday, July 12, 2013 8:47 AM
 To: Paolo Bonzini
 Cc: aligu...@us.ibm.com; qemu-devel@nongnu.org
 Subject: Re: [Qemu-devel] [PULL 6/6] add timestamp to error_report()
 
 On Fri, 12 Jul 2013 08:58:03 +0200
 Paolo Bonzini pbonz...@redhat.com wrote:
 
  Il 10/07/2013 19:52, Luiz Capitulino ha scritto:
   From: Seiji Aguchi seiji.agu...@hds.com
  
   [Issue]
   When we offer a customer support service and a problem happens
   in a customer's system, we try to understand the problem by
   comparing what the customer reports with message logs of the
   customer's system.
  
   In this case, we often need to know when the problem happens.
  
   But, currently, there is no timestamp in qemu's error messages.
   Therefore, we may not be able to understand the problem based on
   error messages.
  
   [Solution]
   Add a timestamp to qemu's error message logged by
   error_report() with g_time_val_to_iso8601().
  
   Signed-off-by: Seiji Aguchi seiji.agu...@hds.com
   Reviewed-by: Stefan Hajnoczi stefa...@redhat.com
   Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
 
  What about removing the option, and instead:
 
  - logging no timestamp until the virtual machine has started
 
 You mean, the guest has started? Why? What about if an error
 happens before the guest has started?

I agree with Luiz.
We need the timestamp to investigate an error before the guest has started.

 
  - always logging timestamps after the virtual machine has started
 
 I don't like having this enabled by default, because it makes
 error messages pretty verbose and also because it's not human
 readable at a first glance. So, IMO, only people who really
 want this should enable it.

We have already discussed this.
There is an use case that timestamp is not needed.

http://marc.info/?l=qemu-develm=135997005926563w=2

Seiji

Re: [Qemu-devel] [PATCH v6] add timestamp to error_report()

2013-07-08 Thread Seiji Aguchi 
  +DEF(msg, HAS_ARG, QEMU_OPTION_msg,
  +-msg timestamp[=on|off]\n
  +change the format of messages\n
  +on|off controls leading timestamps (default:on)\n,
 
 Did you really intend to say it's on by default? Because it's actually
 disabled by default. I prefer having this disabled btw, and I can fix it
 myself before applying if that's what you intended.


My default  is -msg timestamp.
Why do you think it is actually disabled by default?


   /*
* Print an error message to current monitor if we have one, else to 
  stderr.
* Format arguments like sprintf().  The result should not contain
  @@ -206,6 +207,15 @@ void error_print_loc(void)
   void error_report(const char *fmt, ...)
   {
 
 There are some errors being printed to stderr that call error_get_pretty(),
 you might want to convert them to error_report() to increase coverage. But
 you can do this as a follow up patch.

I will make a follow up patch after this patch is merged to your tree.

Seiji

Re: [Qemu-devel] [PATCH v5] Add timestamp to error_report()

2013-07-03 Thread Seiji Aguchi 
 Patches need to make sense today, please do not add extra code with
 potential future use in mind:
 
 1. Other developers must be able to read and modify the current codebase
on its own.  They do not know what potential future changes you were
thinking about.
 
 2. You may never end up submitting or getting the future stuff upstream.
Then we'd be left with extra layers that are never used.

I understand what you are concerned.
I will remove utils/qemu-time.c and its header file.

 Daniel's statement was about the code you copied from libvirt.  It was
 not about using the glib function, which simplifies things greatly and
 avoids the need for a test suite.

Honestly, I still think it is reasonable to introduce the common time-handling 
functionality.
You seem to say that using the glib function is simple, so we don't need to 
introduce 
the common functionality. 

However, commonly useful is not related whether it is simple or not.
 I'm concerned that we may duplicate same code by focusing on today  too much.

But, I don't stick to my opinion on this patch.

Seiji

Re: [Qemu-devel] [PATCH v5] Add timestamp to error_report()

2013-07-03 Thread Seiji Aguchi 


 -Original Message-
 From: Stefan Hajnoczi [mailto:stefa...@gmail.com]
 Sent: Wednesday, July 03, 2013 5:14 AM
 To: Seiji Aguchi
 Cc: qemu-devel@nongnu.org; aligu...@us.ibm.com; berra...@redhat.com; 
 kw...@redhat.com; mtosa...@redhat.com;
 arm...@redhat.com; Tomoki Sekiyama; pbonz...@redhat.com; 
 lcapitul...@redhat.com; ler...@redhat.com; ebl...@redhat.com;
 dle-deve...@lists.sourceforge.net
 Subject: Re: [PATCH v5] Add timestamp to error_report()
 
 On Tue, Jul 02, 2013 at 02:09:24PM +, Seiji Aguchi wrote:
 
 
+DEF(msg, HAS_ARG, QEMU_OPTION_msg,
+-msg [timestamp=on|off]\n
+  change the format of messages\n
+  timestamp=on|off enables leading timestamps (default:on)\n,
+QEMU_ARCH_ALL)
+STEXI
+@item -msg timestamp=on|off
+@findex -msg
+prepend a timestamp to each log message.
+(disabled by default)
+ETEXI
  
   I am confused.  If the user specifies -msg then enable_timestamp_msg is
   on by default.  If the user does not specify -msg then
   enable_timestmap_msg is off.  Did I get that right?
 
  Yes.
 
  
   This means that the default behavior of QEMU does not change but you can
   add -msg to enable timestamps.
  
   I'm happy with this but find the documentation confusing.
 
  I can remove (disabled by default) if needed.
 
 Perhaps the simplest solution is timestamp=off by default.  Then there
 can be no confusion and users must do -msg timestamp=on to enable
 timestamps.
 
 If you really want to keep -msg as a shortcut for -msg timestamp=on,
 then please document explicitly that:
 1. Without -msg timestamps are off.
 1. With -msg timestamps are on.
 2. -msg timestamp=off can be used to turn timestamps off again.

My apologies for the confusion.

The syntax, -msg [timestamp=on|off], was wrong.
-msg timestamp[=on|off] is correct.

And there is no way to make timestamp optional, as far as I looked into a 
source code.
Therefore, the explanation should be as below.
(I think it is reasonable to keep -msg timestamp as a shortcut for -msg 
timestamp=on.)

snip
+DEF(msg, HAS_ARG, QEMU_OPTION_msg,
+-msg timestamp[=on|off]\n
+change the format of messages\n
+on|off controls leading timestamps (default:on)\n,
+QEMU_ARCH_ALL)
+STEXI
+@item -msg timestamp[=on|off]
+@findex -msg
+prepend a timestamp to each log message.(default:on)
+ETEXI
snip

To be simpler, we may be able to introduce just a single -msg-timestamp.
But I think current -msg timestamp[=on|off] is reasonable
because other options may be introduced to msg, like log_level or debug.

Seiji

[Qemu-devel] [PATCH v6] add timestamp to error_report()

2013-07-03 Thread Seiji Aguchi 
[Issue]
When we offer a customer support service and a problem happens
in a customer's system, we try to understand the problem by
comparing what the customer reports with message logs of the
customer's system.

In this case, we often need to know when the problem happens.

But, currently, there is no timestamp in qemu's error messages.
Therefore, we may not be able to understand the problem based on
error messages.

[Solution]
Add a timestamp to qemu's error message logged by
error_report() with g_time_val_to_iso8601().

Signed-off-by: Seiji Aguchi seiji.agu...@hds.com
---
Changelog
 v5 - v6
 - Remove include/qemu/time.h and utils/qemu-time.c.
 - Fix a syntax and indent of messages in msg option's DEF().
 - Change explanation of the msg option.

 v4 - v5
 - Fix descriptions of msg option.
 - Rename TIME_H to QEMU_TIME_H. (avoiding double inclusion of qemu/time.h)
 - Change argument of qemu_get_timestamp_str to char * and size_t.
 - Confirmed msg option is displayed by query-command-line-options.

 v3 - v4
 - Correct email address of Signed-off-by.

 v2 - v3
 - Use g_time_val_to_iso8601() to get timestamp instead of
   copying libvirt's time-handling functions.

   According to discussion below, qemu doesn't need to take care
   if timestamp functions are async-signal safe or not.

   http://marc.info/?l=qemu-develm=136741841921265w=2

   Also, In the review of v2 patch, strftime() are recommended to
   format string. But it is not a suitable function to handle msec.

   Then, simply call g_time_val_to_iso8601().

 - Intoroduce a common time-handling function to util/qemu-time.c.
   (Suggested by Daniel P. Berrange)

 - Add testing for g_time_val_to_iso8601() to tests/test-time.c.
   The test cases are copied from libvirt's virtimetest.
   (Suggested by Daniel P. Berrange)

 v1 - v2

 - add an option, -msg timestamp={on|off}, to enable output message with 
timestamp
---
 include/qemu/error-report.h |2 ++
 qemu-options.hx |   11 +++
 util/qemu-error.c   |   10 ++
 vl.c|   26 ++
 4 files changed, 49 insertions(+), 0 deletions(-)

diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index 14c1719..3b098a9 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -14,6 +14,7 @@
 #define QEMU_ERROR_H
 
 #include stdarg.h
+#include stdbool.h
 #include qemu/compiler.h
 
 typedef struct Location {
@@ -40,5 +41,6 @@ void error_print_loc(void);
 void error_set_progname(const char *argv0);
 void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 const char *error_get_progname(void);
+extern bool enable_timestamp_msg;
 
 #endif
diff --git a/qemu-options.hx b/qemu-options.hx
index ca6fdf6..f241379 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3098,6 +3098,17 @@ property must be set.  These objects are placed in the
 '/objects' path.
 ETEXI
 
+DEF(msg, HAS_ARG, QEMU_OPTION_msg,
+-msg timestamp[=on|off]\n
+change the format of messages\n
+on|off controls leading timestamps (default:on)\n,
+QEMU_ARCH_ALL)
+STEXI
+@item -msg timestamp[=on|off]
+@findex -msg
+prepend a timestamp to each log message.(default:on)
+ETEXI
+
 HXCOMM This is the last statement. Insert new options before this line!
 STEXI
 @end table
diff --git a/util/qemu-error.c b/util/qemu-error.c
index 08a36f4..fec02c6 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -196,6 +196,7 @@ void error_print_loc(void)
 }
 }
 
+bool enable_timestamp_msg;
 /*
  * Print an error message to current monitor if we have one, else to stderr.
  * Format arguments like sprintf().  The result should not contain
@@ -206,6 +207,15 @@ void error_print_loc(void)
 void error_report(const char *fmt, ...)
 {
 va_list ap;
+GTimeVal tv;
+gchar *timestr;
+
+if (enable_timestamp_msg) {
+g_get_current_time(tv);
+timestr = g_time_val_to_iso8601(tv);
+error_printf(%s , timestr);
+g_free(timestr);
+}
 
 error_print_loc();
 va_start(ap, fmt);
diff --git a/vl.c b/vl.c
index 0a8f056..589d264 100644
--- a/vl.c
+++ b/vl.c
@@ -516,6 +516,18 @@ static QemuOptsList qemu_realtime_opts = {
 },
 };
 
+static QemuOptsList qemu_msg_opts = {
+.name = msg,
+.head = QTAILQ_HEAD_INITIALIZER(qemu_msg_opts.head),
+.desc = {
+{
+.name = timestamp,
+.type = QEMU_OPT_BOOL,
+},
+{ /* end of list */ }
+},
+};
+
 const char *qemu_get_vm_name(void)
 {
 return qemu_name;
@@ -1459,6 +1471,12 @@ static void configure_realtime(QemuOpts *opts)
 }
 }
 
+
+static void configure_msg(QemuOpts *opts)
+{
+enable_timestamp_msg = qemu_opt_get_bool(opts, timestamp, true);
+}
+
 /***/
 /* USB devices */
 
@@ -2901,6 +2919,7 @@ int main(int argc, char **argv, char **envp)
 qemu_add_opts(qemu_object_opts);
 qemu_add_opts(qemu_tpmdev_opts

Re: [Qemu-devel] [PATCH v5] Add timestamp to error_report()

2013-07-02 Thread Seiji Aguchi 


  +DEF(msg, HAS_ARG, QEMU_OPTION_msg,
  +-msg [timestamp=on|off]\n
  +  change the format of messages\n
  +  timestamp=on|off enables leading timestamps (default:on)\n,
  +QEMU_ARCH_ALL)
  +STEXI
  +@item -msg timestamp=on|off
  +@findex -msg
  +prepend a timestamp to each log message.
  +(disabled by default)
  +ETEXI
 
 I am confused.  If the user specifies -msg then enable_timestamp_msg is
 on by default.  If the user does not specify -msg then
 enable_timestmap_msg is off.  Did I get that right?

Yes.

 
 This means that the default behavior of QEMU does not change but you can
 add -msg to enable timestamps.
 
 I'm happy with this but find the documentation confusing.

I can remove (disabled by default) if needed.

 
  diff --git a/util/qemu-time.c b/util/qemu-time.c
  new file mode 100644
  index 000..3862788
  --- /dev/null
  +++ b/util/qemu-time.c
  @@ -0,0 +1,26 @@
  +/*
  + * Time handling
  + *
  + * Copyright (C) 2013 Hitachi Data Systems Corp.
  + *
  + * Authors:
  + *  Seiji Aguchi seiji.agu...@hds.com
  + *
  + * This work is licensed under the terms of the GNU GPL, version 2 or 
  later.
  + * See the COPYING file in the top-level directory.
  + */
  +#include qemu/time.h
  +
  +void qemu_get_timestamp_str(char *timestr, size_t len)
  +{
  +GTimeVal tv;
  +gchar *tmp_str = NULL;
  +
  +/* Size of len should be at least TIMESTAMP_LEN to avoid truncation */
  +assert(len = TIMESTAMP_LEN);
  +
  +g_get_current_time(tv);
  +tmp_str = g_time_val_to_iso8601(tv);
  +g_strlcpy((gchar *)timestr, tmp_str, len);
  +g_free(tmp_str);
  +}
 
 Why strcpy into a fixed-size buffer when glib gives us a heap-allocated
 string?
 
 This patch would be simpler by dropping qemu-time.c/time.h 

I plan to introduce timestamp to monitor_printf() and fprintf()
near future.

In this case, it is better from a maintenance POV to keep time-handling
 functionality  in a file that's separate from the qemu error file, so it can 
be reused
elsewhere in QEMU if needed.

It is suggested by Daniel.
http://marc.info/?l=qemu-develm=136741113218944w=2

 and simply
 doing:
 
 if (enable_timestamp_msg) {
 GTimeVal tv;
 gchar *timestamp;
 
 g_get_current_time(tv);
 timestamp = g_time_val_to_iso8601(tv);
 error_printf(%s , timestamp);
 g_free(timestamp);

I'm concerned that we may have potential memory leak
If someone neglect to call the g_free().
That is why I use the fixed-size buffer.

Seiji

[Qemu-devel] [PATCH v5] Add timestamp to error_report()

2013-07-01 Thread Seiji Aguchi 
[Issue]
When we offer a customer support service and a problem happens
in a customer's system, we try to understand the problem by
comparing what the customer reports with message logs of the
customer's system.

In this case, we often need to know when the problem happens.

But, currently, there is no timestamp in qemu's error messages.
Therefore, we may not be able to understand the problem based on
error messages.

[Solution]
Add a timestamp to qemu's error message logged by
error_report() with g_time_val_to_iso8601().

Signed-off-by: Seiji Aguchi seiji.agu...@hds.com
---
Changelog
 v4 - v5
 - Use sizeof() to define TIMESTAMP_LEN.
 - Fix descriptions of msg option.
 - Rename TIME_H to QEMU_TIME_H. (avoiding double inclusion of qemu/time.h)
 - Change argument of qemu_get_timestamp_str to char * and size_t.
 - Confirmed msg option is displayed by query-command-line-options.

 v3 - v4
 - Correct email address of Signed-off-by.

 v2 - v3
 - Use g_time_val_to_iso8601() to get timestamp instead of
   copying libvirt's time-handling functions.

   According to discussion below, qemu doesn't need to take care
   if timestamp functions are async-signal safe or not.

   http://marc.info/?l=qemu-develm=136741841921265w=2

   Also, In the review of v2 patch, strftime() are recommended to
   format string. But it is not a suitable function to handle msec.

   Then, simply call g_time_val_to_iso8601().

 - Intoroduce a common time-handling function to util/qemu-time.c.
   (Suggested by Daniel P. Berrange)

 - Add testing for g_time_val_to_iso8601() to tests/test-time.c.
   The test cases are copied from libvirt's virtimetest.
   (Suggested by Daniel P. Berrange)

 v1 - v2

 - add an option, -msg timestamp={on|off}, to enable output message with 
timestamp
---
 include/qemu/time.h |   10 ++
 qemu-options.hx |   12 
 util/Makefile.objs  |1 +
 util/qemu-error.c   |8 
 util/qemu-time.c|   26 ++
 vl.c|   28 
 6 files changed, 85 insertions(+), 0 deletions(-)
 create mode 100644 include/qemu/time.h
 create mode 100644 util/qemu-time.c

diff --git a/include/qemu/time.h b/include/qemu/time.h
new file mode 100644
index 000..ce3903e
--- /dev/null
+++ b/include/qemu/time.h
@@ -0,0 +1,10 @@
+#ifndef QEMU_TIME_H
+#define QEMU_TIME_H
+
+#include qemu-common.h
+
+#define TIMESTAMP_LEN sizeof(1970-01-01T00:00:00.99Z)
+extern void qemu_get_timestamp_str(char *, size_t);
+extern bool enable_timestamp_msg;
+
+#endif /* QEMU_TIME_H */
diff --git a/qemu-options.hx b/qemu-options.hx
index ca6fdf6..a6dac1a 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3102,3 +3102,15 @@ HXCOMM This is the last statement. Insert new options 
before this line!
 STEXI
 @end table
 ETEXI
+
+DEF(msg, HAS_ARG, QEMU_OPTION_msg,
+-msg [timestamp=on|off]\n
+  change the format of messages\n
+  timestamp=on|off enables leading timestamps (default:on)\n,
+QEMU_ARCH_ALL)
+STEXI
+@item -msg timestamp=on|off
+@findex -msg
+prepend a timestamp to each log message.
+(disabled by default)
+ETEXI
diff --git a/util/Makefile.objs b/util/Makefile.objs
index dc72ab0..063db56 100644
--- a/util/Makefile.objs
+++ b/util/Makefile.objs
@@ -11,3 +11,4 @@ util-obj-y += iov.o aes.o qemu-config.o qemu-sockets.o uri.o 
notify.o
 util-obj-y += qemu-option.o qemu-progress.o
 util-obj-y += hexdump.o
 util-obj-y += crc32c.o
+util-obj-y += qemu-time.o
diff --git a/util/qemu-error.c b/util/qemu-error.c
index 08a36f4..c65fdfd 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -12,6 +12,7 @@
 
 #include stdio.h
 #include monitor/monitor.h
+#include qemu/time.h
 
 /*
  * Print to current monitor if we have one, else to stderr.
@@ -196,6 +197,7 @@ void error_print_loc(void)
 }
 }
 
+bool enable_timestamp_msg;
 /*
  * Print an error message to current monitor if we have one, else to stderr.
  * Format arguments like sprintf().  The result should not contain
@@ -206,6 +208,12 @@ void error_print_loc(void)
 void error_report(const char *fmt, ...)
 {
 va_list ap;
+char timestr[TIMESTAMP_LEN];
+
+if (enable_timestamp_msg) {
+qemu_get_timestamp_str(timestr, sizeof(timestr));
+error_printf(%s , timestr);
+}
 
 error_print_loc();
 va_start(ap, fmt);
diff --git a/util/qemu-time.c b/util/qemu-time.c
new file mode 100644
index 000..3862788
--- /dev/null
+++ b/util/qemu-time.c
@@ -0,0 +1,26 @@
+/*
+ * Time handling
+ *
+ * Copyright (C) 2013 Hitachi Data Systems Corp.
+ *
+ * Authors:
+ *  Seiji Aguchi seiji.agu...@hds.com
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include qemu/time.h
+
+void qemu_get_timestamp_str(char *timestr, size_t len)
+{
+GTimeVal tv;
+gchar *tmp_str = NULL;
+
+/* Size of len should be at least TIMESTAMP_LEN to avoid truncation */
+assert(len = TIMESTAMP_LEN

Re: [Qemu-devel] [PATCH v4] Add timestamp to error message

2013-07-01 Thread Seiji Aguchi 
void error_report(const char *fmt, ...)
{
va_list ap;
   +char timestr[TIMESTAMP_LEN];
   +
   +if (enable_timestamp_msg) {
   +qemu_get_timestamp_str(timestr);
   +error_printf(%s , timestr);
   +}
  
error_print_loc();
va_start(ap, fmt);
 
  Does this print the timestamp to all kinds of monitors too? On stderr,
  the timestamp is great. When printed to an interactive monitor, it
  could also help post-mortem debugging. But would it not confuse libvirt
  eg.? (Apologies if this has been discussed before.)
 
 I will try to add timestamp to monitor_printf().
 (In the long term, I would like to add it to all fprintf() in qemu.)

I tried to add timestamp to monitor_printf().
But, it wasn't easier than I expected.
(At least, we should not add it to monitor_printf() in readline.c.)
We need to discuss which message of monitor_printf() has to be
added timestamp.

Therefore, I would like to forcus on error_report() in this thread.
(I posted patch v5 just now.)

Seiji

Re: [Qemu-devel] [PATCH v4] Add timestamp to error message

2013-06-28 Thread Seiji Aguchi 

  diff --git a/include/qemu/time.h b/include/qemu/time.h
  new file mode 100644
  index 000..f70739b
  --- /dev/null
  +++ b/include/qemu/time.h
  @@ -0,0 +1,11 @@
  +#ifndef TIME_H
  +#define TIME_H
 
 I wonder if TIME_H is maybe a bit too nondescript and could conflict
 with other guards.
 
 The guards currently used in include/qemu/*.h files are inconsistent
 -- some use a QEMU_ prefix, some a __QEMU_ one, and others use none.
 
 Don't respin just because of this, but maybe it's something to consider.

This should be discussed in other thread...

 
 
  +
  +#include qemu-common.h
  +
  +/*  1970-01-01T00:00:00.99Z + '\0' */
  +#define TIMESTAMP_LEN 28
  +extern void qemu_get_timestamp_str(char (*timestr)[]);

I will change to extern void qemu_get_timestamp_str(char *timestr, size_t len)
as Eric pointed out.

 
 
  +extern bool enable_timestamp_msg;
  +
  +#endif /* !TIME_H */
  diff --git a/qemu-options.hx b/qemu-options.hx
  index ca6fdf6..7890921 100644
  --- a/qemu-options.hx
  +++ b/qemu-options.hx
  @@ -3102,3 +3102,15 @@ HXCOMM This is the last statement. Insert new 
  options before this line!
   STEXI
   @end table
   ETEXI
  +
  +DEF(msg, HAS_ARG, QEMU_OPTION_msg,
  +-msg [timestamp=on|off]\n
  +output message with timestamp (default: off)\n,
  +QEMU_ARCH_ALL)
  +STEXI
  +@item -msg timestamp=on|off
  +@findex - msg
 
 A space has snuck in between - and msg. Perhaps this is the only
 note that would warrant a respin (or rather a maintainer fixup at commit).

I will fix it.

 
 
  +Output message with timestamp.
 
 As a non-native speaker, I propose rewording this as prepend a
 timestamp to each log message (in the prior occurrence as well) if you
 decided to repost.

Will fix as well.

   void error_report(const char *fmt, ...)
   {
   va_list ap;
  +char timestr[TIMESTAMP_LEN];
  +
  +if (enable_timestamp_msg) {
  +qemu_get_timestamp_str(timestr);
  +error_printf(%s , timestr);
  +}
 
   error_print_loc();
   va_start(ap, fmt);
 
 Does this print the timestamp to all kinds of monitors too? On stderr,
 the timestamp is great. When printed to an interactive monitor, it
 could also help post-mortem debugging. But would it not confuse libvirt
 eg.? (Apologies if this has been discussed before.)

I will try to add timestamp to monitor_printf().
(In the long term, I would like to add it to all fprintf() in qemu.)

 
 
  diff --git a/util/qemu-time.c b/util/qemu-time.c
  new file mode 100644
  index 000..37f7b9e
  --- /dev/null
  +++ b/util/qemu-time.c
  @@ -0,0 +1,24 @@
  +/*
  + * Time handling
  + *
  + * Copyright (C) 2013 Hitachi Data Systems Corp.
  + *
  + * Authors:
  + *  Seiji Aguchi seiji.agu...@hds.com
  + *
  + * This work is licensed under the terms of the GNU GPL, version 2 or 
  later.
  + * See the COPYING file in the top-level directory.
  + */
  +#include qemu/time.h
  +
  +void qemu_get_timestamp_str(char (*timestr)[])
  +{
  +GTimeVal tv;
  +gchar *tmp_str = NULL;
  +
  +g_get_current_time(tv);
 
 Hm. There's also g_get_monotonic_time(), but (a) that's less portable
 (available since 2.28), (b) this is simply good enough IMO, in practice. OK.
 
 
  +tmp_str = g_time_val_to_iso8601(tv);
  +g_strlcpy((gchar *)*timestr, tmp_str, TIMESTAMP_LEN);
  +g_free(tmp_str);
  +return;
 
 You're not returning a value so the last statement is superfluous.

I will remove the unnecessary return.

  +
  +static void configure_msg(QemuOpts *opts)
  +{
  +enable_timestamp_msg = qemu_opt_get_bool(opts, timestamp, true);
  +}
 
 I think the default value doesn't match the docs, which say deafult:
 off. As far as I recall (from Tomoki's -realtime [mlock=on|off]
 patch), statements about defaults in the option docs don't describe how
 qemu works by default (ie. when you omit the option altogether), they
 describe what happens if you specify the option but omit its arguments
 (ie. with -msg only.)
 
 In that case, the docs should state something like:
 
 DEF(msg, HAS_ARG, QEMU_OPTION_msg,
 -msg [timestamp=on|off]\n
  change the format of error messages\n
  timestamp=on|off enables leading timestamps (default: on)\n,
 QEMU_ARCH_ALL)

Currently, this patch add timestamp to just error_report().
But, we may need it for both error and normal messages.
So, I will change the sentence change the format of error messages
to change the format of messages.

I appreciate your review.

Seiji

Re: [Qemu-devel] [PATCH v3] Add timestamp to error message

2013-06-28 Thread Seiji Aguchi 
Thank you for the review.

  +#include qemu-common.h
  +
  +/*  1970-01-01T00:00:00.99Z + '\0' */
  +#define TIMESTAMP_LEN 28
 
 Self-documenting constants are nicer:
 
 #define TIMESTAMP_LEN (sizeof(1970-01-01T00:00:00.99Z)+1)

I will fix it.

 
 extern void qemu_get_timestamp_str(char *timestr, size_t len)
 
 where len is the length of timestr, and where the comments document that
 len should be at least TIMESTAMP_LEN to avoid truncation (or even
 assert() if it is not)?

I will fix as above.

 
  +++ b/qemu-options.hx
  @@ -3102,3 +3102,15 @@ HXCOMM This is the last statement. Insert new 
  options before this line!
   STEXI
   @end table
   ETEXI
  +
  +DEF(msg, HAS_ARG, QEMU_OPTION_msg,
  +-msg [timestamp=on|off]\n
  +output message with timestamp (default: off)\n,
  +QEMU_ARCH_ALL)
 
 Did you test that the existing query-command-line-options QMP command
 will list this option (just making sure that libvirt will be able to
 know when to use this option).

I will test it in the next patch.

Seiji

 
 --
 Eric Blake   eblake redhat com+1-919-301-3266
 Libvirt virtualization library http://libvirt.org

Re: [Qemu-devel] [PATCH v4] Add timestamp to error message

2013-06-28 Thread Seiji Aguchi 
 
  I wonder if TIME_H is maybe a bit too nondescript and could conflict
  with other guards.

OK.
I will use QEMU_TIME_H.
Apologies for my curt reply.

Seiji

[Qemu-devel] [PATCH v3] Add timestamp to error message

2013-06-26 Thread Seiji Aguchi 
[Issue]
When we offer a customer support service and a problem happens
in a customer's system, we try to understand the problem by
comparing what the customer reports with message logs of the
customer's system.

In this case, we often need to know when the problem happens.

But, currently, there is no timestamp in qemu's error messages.
Therefore, we may not be able to understand the problem based on
error messages.

[Solution]
Add a timestamp to qemu's error message logged by
error_report() with g_time_val_to_iso8601().

[TODO]
Add timestamp to monitor_printf() and fprintf().

Signed-off-by: Seiji Aguchi sagu...@redhat.com
---
Changelog
 v2 - v3

 - Use g_time_val_to_iso8601() to get timestamp instead of
   copying libvirt's time-handling functions.

   According to discussion below, qemu doesn't need to take care
   if timestamp functions are async-signal safe or not.

   http://marc.info/?l=qemu-develm=136741841921265w=2

   Also, In the review of v2 patch, strftime() are recommended to
   format string. But it is not a suitable function to handle msec.

   Then, simply call g_time_val_to_iso8601().

 - Intoroduce a common time-handling function to util/qemu-time.c.
   (Suggested by Daniel P. Berrange)

 v1 - v2

 - add an option, -msg timestamp={on|off}, to enable output message with 
timestamp
---
 include/qemu/time.h |   11 +++
 qemu-options.hx |   12 
 util/Makefile.objs  |1 +
 util/qemu-error.c   |8 
 util/qemu-time.c|   24 
 vl.c|   28 
 6 files changed, 84 insertions(+), 0 deletions(-)
 create mode 100644 include/qemu/time.h
 create mode 100644 util/qemu-time.c

diff --git a/include/qemu/time.h b/include/qemu/time.h
new file mode 100644
index 000..f70739b
--- /dev/null
+++ b/include/qemu/time.h
@@ -0,0 +1,11 @@
+#ifndef TIME_H
+#define TIME_H
+
+#include qemu-common.h
+
+/*  1970-01-01T00:00:00.99Z + '\0' */
+#define TIMESTAMP_LEN 28
+extern void qemu_get_timestamp_str(char (*timestr)[]);
+extern bool enable_timestamp_msg;
+
+#endif /* !TIME_H */
diff --git a/qemu-options.hx b/qemu-options.hx
index ca6fdf6..7890921 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3102,3 +3102,15 @@ HXCOMM This is the last statement. Insert new options 
before this line!
 STEXI
 @end table
 ETEXI
+
+DEF(msg, HAS_ARG, QEMU_OPTION_msg,
+-msg [timestamp=on|off]\n
+output message with timestamp (default: off)\n,
+QEMU_ARCH_ALL)
+STEXI
+@item -msg timestamp=on|off
+@findex - msg
+Output message with timestamp.
+Adding timestamp to messages with @option{timestamp=on}
+(disabled by default).
+ETEXI
diff --git a/util/Makefile.objs b/util/Makefile.objs
index dc72ab0..063db56 100644
--- a/util/Makefile.objs
+++ b/util/Makefile.objs
@@ -11,3 +11,4 @@ util-obj-y += iov.o aes.o qemu-config.o qemu-sockets.o uri.o 
notify.o
 util-obj-y += qemu-option.o qemu-progress.o
 util-obj-y += hexdump.o
 util-obj-y += crc32c.o
+util-obj-y += qemu-time.o
diff --git a/util/qemu-error.c b/util/qemu-error.c
index 08a36f4..33fa9d3 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -12,6 +12,7 @@
 
 #include stdio.h
 #include monitor/monitor.h
+#include qemu/time.h
 
 /*
  * Print to current monitor if we have one, else to stderr.
@@ -196,6 +197,7 @@ void error_print_loc(void)
 }
 }
 
+bool enable_timestamp_msg;
 /*
  * Print an error message to current monitor if we have one, else to stderr.
  * Format arguments like sprintf().  The result should not contain
@@ -206,6 +208,12 @@ void error_print_loc(void)
 void error_report(const char *fmt, ...)
 {
 va_list ap;
+char timestr[TIMESTAMP_LEN];
+
+if (enable_timestamp_msg) {
+qemu_get_timestamp_str(timestr);
+error_printf(%s , timestr);
+}
 
 error_print_loc();
 va_start(ap, fmt);
diff --git a/util/qemu-time.c b/util/qemu-time.c
new file mode 100644
index 000..37f7b9e
--- /dev/null
+++ b/util/qemu-time.c
@@ -0,0 +1,24 @@
+/*
+ * Time handling
+ *
+ * Copyright (C) 2013 Hitachi Data Systems Corp.
+ *
+ * Authors:
+ *  Seiji Aguchi seiji.agu...@hds.com
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include qemu/time.h
+
+void qemu_get_timestamp_str(char (*timestr)[])
+{
+GTimeVal tv;
+gchar *tmp_str = NULL;
+
+g_get_current_time(tv);
+tmp_str = g_time_val_to_iso8601(tv);
+g_strlcpy((gchar *)*timestr, tmp_str, TIMESTAMP_LEN);
+g_free(tmp_str);
+return;
+}
diff --git a/vl.c b/vl.c
index 0a8f056..aee7350 100644
--- a/vl.c
+++ b/vl.c
@@ -171,6 +171,8 @@ int main(int argc, char **argv)
 #include ui/qemu-spice.h
 #include qapi/string-input-visitor.h
 
+#include qemu/time.h
+
 //#define DEBUG_NET
 //#define DEBUG_SLIRP
 
@@ -516,6 +518,18 @@ static QemuOptsList qemu_realtime_opts = {
 },
 };
 
+static QemuOptsList qemu_msg_opts = {
+.name = msg,
+.head

Re: [Qemu-devel] [PATCH v3] Add timestamp to error message

2013-06-26 Thread Seiji Aguchi 


 Signed-off-by: Seiji Aguchi sagu...@redhat.com

I used wrong email address to Signed-off-by.
Please ignore this patch.

Seiji

 -Original Message-
 From: qemu-devel-bounces+seiji.aguchi=hds@nongnu.org 
 [mailto:qemu-devel-bounces+seiji.aguchi=hds@nongnu.org]
 On Behalf Of Seiji Aguchi
 Sent: Wednesday, June 26, 2013 6:21 PM
 To: qemu-devel@nongnu.org
 Cc: kw...@redhat.com; aligu...@us.ibm.com; Tomoki Sekiyama; m...@redhat.com; 
 stefa...@gmail.com; mtosa...@redhat.com;
 arm...@redhat.com; lcapitul...@redhat.com; dle-deve...@lists.sourceforge.net; 
 av1...@comtv.ru; stefa...@redhat.com;
 pbonz...@redhat.com
 Subject: [Qemu-devel] [PATCH v3] Add timestamp to error message
 
 [Issue]
 When we offer a customer support service and a problem happens
 in a customer's system, we try to understand the problem by
 comparing what the customer reports with message logs of the
 customer's system.
 
 In this case, we often need to know when the problem happens.
 
 But, currently, there is no timestamp in qemu's error messages.
 Therefore, we may not be able to understand the problem based on
 error messages.
 
 [Solution]
 Add a timestamp to qemu's error message logged by
 error_report() with g_time_val_to_iso8601().
 
 [TODO]
 Add timestamp to monitor_printf() and fprintf().
 
 Signed-off-by: Seiji Aguchi sagu...@redhat.com
 ---
 Changelog
  v2 - v3
 
  - Use g_time_val_to_iso8601() to get timestamp instead of
copying libvirt's time-handling functions.
 
According to discussion below, qemu doesn't need to take care
if timestamp functions are async-signal safe or not.
 
http://marc.info/?l=qemu-develm=136741841921265w=2
 
Also, In the review of v2 patch, strftime() are recommended to
format string. But it is not a suitable function to handle msec.
 
Then, simply call g_time_val_to_iso8601().
 
  - Intoroduce a common time-handling function to util/qemu-time.c.
(Suggested by Daniel P. Berrange)
 
  v1 - v2
 
  - add an option, -msg timestamp={on|off}, to enable output message with 
 timestamp
 ---
  include/qemu/time.h |   11 +++
  qemu-options.hx |   12 
  util/Makefile.objs  |1 +
  util/qemu-error.c   |8 
  util/qemu-time.c|   24 
  vl.c|   28 
  6 files changed, 84 insertions(+), 0 deletions(-)
  create mode 100644 include/qemu/time.h
  create mode 100644 util/qemu-time.c
 
 diff --git a/include/qemu/time.h b/include/qemu/time.h
 new file mode 100644
 index 000..f70739b
 --- /dev/null
 +++ b/include/qemu/time.h
 @@ -0,0 +1,11 @@
 +#ifndef TIME_H
 +#define TIME_H
 +
 +#include qemu-common.h
 +
 +/*  1970-01-01T00:00:00.99Z + '\0' */
 +#define TIMESTAMP_LEN 28
 +extern void qemu_get_timestamp_str(char (*timestr)[]);
 +extern bool enable_timestamp_msg;
 +
 +#endif /* !TIME_H */
 diff --git a/qemu-options.hx b/qemu-options.hx
 index ca6fdf6..7890921 100644
 --- a/qemu-options.hx
 +++ b/qemu-options.hx
 @@ -3102,3 +3102,15 @@ HXCOMM This is the last statement. Insert new options 
 before this line!
  STEXI
  @end table
  ETEXI
 +
 +DEF(msg, HAS_ARG, QEMU_OPTION_msg,
 +-msg [timestamp=on|off]\n
 +output message with timestamp (default: off)\n,
 +QEMU_ARCH_ALL)
 +STEXI
 +@item -msg timestamp=on|off
 +@findex - msg
 +Output message with timestamp.
 +Adding timestamp to messages with @option{timestamp=on}
 +(disabled by default).
 +ETEXI
 diff --git a/util/Makefile.objs b/util/Makefile.objs
 index dc72ab0..063db56 100644
 --- a/util/Makefile.objs
 +++ b/util/Makefile.objs
 @@ -11,3 +11,4 @@ util-obj-y += iov.o aes.o qemu-config.o qemu-sockets.o 
 uri.o notify.o
  util-obj-y += qemu-option.o qemu-progress.o
  util-obj-y += hexdump.o
  util-obj-y += crc32c.o
 +util-obj-y += qemu-time.o
 diff --git a/util/qemu-error.c b/util/qemu-error.c
 index 08a36f4..33fa9d3 100644
 --- a/util/qemu-error.c
 +++ b/util/qemu-error.c
 @@ -12,6 +12,7 @@
 
  #include stdio.h
  #include monitor/monitor.h
 +#include qemu/time.h
 
  /*
   * Print to current monitor if we have one, else to stderr.
 @@ -196,6 +197,7 @@ void error_print_loc(void)
  }
  }
 
 +bool enable_timestamp_msg;
  /*
   * Print an error message to current monitor if we have one, else to stderr.
   * Format arguments like sprintf().  The result should not contain
 @@ -206,6 +208,12 @@ void error_print_loc(void)
  void error_report(const char *fmt, ...)
  {
  va_list ap;
 +char timestr[TIMESTAMP_LEN];
 +
 +if (enable_timestamp_msg) {
 +qemu_get_timestamp_str(timestr);
 +error_printf(%s , timestr);
 +}
 
  error_print_loc();
  va_start(ap, fmt);
 diff --git a/util/qemu-time.c b/util/qemu-time.c
 new file mode 100644
 index 000..37f7b9e
 --- /dev/null
 +++ b/util/qemu-time.c
 @@ -0,0 +1,24 @@
 +/*
 + * Time handling
 + *
 + * Copyright (C) 2013 Hitachi Data Systems Corp.
 + *
 + * Authors:
 + *  Seiji Aguchi seiji.agu...@hds.com

[Qemu-devel] [PATCH v4] Add timestamp to error message

2013-06-26 Thread Seiji Aguchi 
[Issue]
When we offer a customer support service and a problem happens
in a customer's system, we try to understand the problem by
comparing what the customer reports with message logs of the
customer's system.

In this case, we often need to know when the problem happens.

But, currently, there is no timestamp in qemu's error messages.
Therefore, we may not be able to understand the problem based on
error messages.

[Solution]
Add a timestamp to qemu's error message logged by
error_report() with g_time_val_to_iso8601().

[TODO]
Add timestamp to monitor_printf() and fprintf().

Signed-off-by: Seiji Aguchi seiji.agu...@hds.com
---
Changelog
 v3 - v4
 - Correct email address of Signed-off-by.

 v2 - v3

 - Use g_time_val_to_iso8601() to get timestamp instead of
   copying libvirt's time-handling functions.

   According to discussion below, qemu doesn't need to take care
   if timestamp functions are async-signal safe or not.

   http://marc.info/?l=qemu-develm=136741841921265w=2

   Also, In the review of v2 patch, strftime() are recommended to
   format string. But it is not a suitable function to handle msec.

   Then, simply call g_time_val_to_iso8601().

 - Intoroduce a common time-handling function to util/qemu-time.c.
   (Suggested by Daniel P. Berrange)

 v1 - v2

 - add an option, -msg timestamp={on|off}, to enable output message with 
timestamp
---
 include/qemu/time.h |   11 +++
 qemu-options.hx |   12 
 util/Makefile.objs  |1 +
 util/qemu-error.c   |8 
 util/qemu-time.c|   24 
 vl.c|   28 
 6 files changed, 84 insertions(+), 0 deletions(-)
 create mode 100644 include/qemu/time.h
 create mode 100644 util/qemu-time.c

diff --git a/include/qemu/time.h b/include/qemu/time.h
new file mode 100644
index 000..f70739b
--- /dev/null
+++ b/include/qemu/time.h
@@ -0,0 +1,11 @@
+#ifndef TIME_H
+#define TIME_H
+
+#include qemu-common.h
+
+/*  1970-01-01T00:00:00.99Z + '\0' */
+#define TIMESTAMP_LEN 28
+extern void qemu_get_timestamp_str(char (*timestr)[]);
+extern bool enable_timestamp_msg;
+
+#endif /* !TIME_H */
diff --git a/qemu-options.hx b/qemu-options.hx
index ca6fdf6..7890921 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3102,3 +3102,15 @@ HXCOMM This is the last statement. Insert new options 
before this line!
 STEXI
 @end table
 ETEXI
+
+DEF(msg, HAS_ARG, QEMU_OPTION_msg,
+-msg [timestamp=on|off]\n
+output message with timestamp (default: off)\n,
+QEMU_ARCH_ALL)
+STEXI
+@item -msg timestamp=on|off
+@findex - msg
+Output message with timestamp.
+Adding timestamp to messages with @option{timestamp=on}
+(disabled by default).
+ETEXI
diff --git a/util/Makefile.objs b/util/Makefile.objs
index dc72ab0..063db56 100644
--- a/util/Makefile.objs
+++ b/util/Makefile.objs
@@ -11,3 +11,4 @@ util-obj-y += iov.o aes.o qemu-config.o qemu-sockets.o uri.o 
notify.o
 util-obj-y += qemu-option.o qemu-progress.o
 util-obj-y += hexdump.o
 util-obj-y += crc32c.o
+util-obj-y += qemu-time.o
diff --git a/util/qemu-error.c b/util/qemu-error.c
index 08a36f4..33fa9d3 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -12,6 +12,7 @@
 
 #include stdio.h
 #include monitor/monitor.h
+#include qemu/time.h
 
 /*
  * Print to current monitor if we have one, else to stderr.
@@ -196,6 +197,7 @@ void error_print_loc(void)
 }
 }
 
+bool enable_timestamp_msg;
 /*
  * Print an error message to current monitor if we have one, else to stderr.
  * Format arguments like sprintf().  The result should not contain
@@ -206,6 +208,12 @@ void error_print_loc(void)
 void error_report(const char *fmt, ...)
 {
 va_list ap;
+char timestr[TIMESTAMP_LEN];
+
+if (enable_timestamp_msg) {
+qemu_get_timestamp_str(timestr);
+error_printf(%s , timestr);
+}
 
 error_print_loc();
 va_start(ap, fmt);
diff --git a/util/qemu-time.c b/util/qemu-time.c
new file mode 100644
index 000..37f7b9e
--- /dev/null
+++ b/util/qemu-time.c
@@ -0,0 +1,24 @@
+/*
+ * Time handling
+ *
+ * Copyright (C) 2013 Hitachi Data Systems Corp.
+ *
+ * Authors:
+ *  Seiji Aguchi seiji.agu...@hds.com
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include qemu/time.h
+
+void qemu_get_timestamp_str(char (*timestr)[])
+{
+GTimeVal tv;
+gchar *tmp_str = NULL;
+
+g_get_current_time(tv);
+tmp_str = g_time_val_to_iso8601(tv);
+g_strlcpy((gchar *)*timestr, tmp_str, TIMESTAMP_LEN);
+g_free(tmp_str);
+return;
+}
diff --git a/vl.c b/vl.c
index 0a8f056..aee7350 100644
--- a/vl.c
+++ b/vl.c
@@ -171,6 +171,8 @@ int main(int argc, char **argv)
 #include ui/qemu-spice.h
 #include qapi/string-input-visitor.h
 
+#include qemu/time.h
+
 //#define DEBUG_NET
 //#define DEBUG_SLIRP
 
@@ -516,6 +518,18 @@ static QemuOptsList qemu_realtime_opts = {
 },
 };
 
+static QemuOptsList

Re: [Qemu-devel] [RFC][PATCH v2]Add timestamp to error message

2013-05-02 Thread Seiji Aguchi 
Thank you for giving the comments.

I will separate this functionality from the qemu error file.

 I also don't buy that we can't use strftime.  There are very few places
 where we use fork() in QEMU (it doesn't exist on Windows so it can't be
 used without protection).  None of those places use the error reporting
 infrastructure.
 
 This code is also extremely naive.  It doesn't take into account leap
 seconds and makes bad assumptions about leap years.

OK.
I will use strftime().

Seiji

 -Original Message-
 From: qemu-devel-bounces+seiji.aguchi=hds@nongnu.org 
 [mailto:qemu-devel-bounces+seiji.aguchi=hds@nongnu.org]
 On Behalf Of Anthony Liguori
 Sent: Wednesday, May 01, 2013 10:27 AM
 To: Daniel P. Berrange; Eric Blake
 Cc: kw...@redhat.com; dle-deve...@lists.sourceforge.net; Seiji Aguchi; 
 stefa...@redhat.com; m...@redhat.com; Stefan Hajnoczi;
 mtosa...@redhat.com; qemu-devel@nongnu.org; arm...@redhat.com; 
 av1...@comtv.ru; Tomoki Sekiyama; pbonz...@redhat.com;
 lcapitul...@redhat.com; Seiji Aguchi
 Subject: Re: [Qemu-devel] [RFC][PATCH v2]Add timestamp to error message
 
 Daniel P. Berrange berra...@redhat.com writes:
 
  On Wed, May 01, 2013 at 06:16:33AM -0600, Eric Blake wrote:
  On 05/01/2013 06:05 AM, Stefan Hajnoczi wrote:
 
   +error_printf(
   +   %4d-%02d-%02d %02d:%02d:%02d.%03lld+ ,
   +   fields.tm_year + 1900, fields.tm_mon + 1, fields.tm_mday,
   +   fields.tm_hour, fields.tm_min, fields.tm_sec,
   +   now % 1000);
  
   Can strftime(3) be used instead of copying code from glibc?
 
  No, because glibc's strftime() is not async-signal safe, and therefore
  is not safe to call between fork() and exec() (libvirt hit actual
  deadlocks[1] where a child was blocked on a mutex associated with
  time-related functions that happened to be held by another parent
  thread, but where the fork nuked that other thread so the mutex would
  never clear).  This code is copied from libvirt, which copied the
  no-lock-needed safe portions of glibc for a reason.
 
  [1] https://www.redhat.com/archives/libvir-list/2011-November/msg01609.html
 
  NB the original libvirt code had this  other related functions in
  a standalone file, along with a significant test suite. I think it
  is better from a maintenence POV to keep this functionality in a
  file that's separate from the qemu error file, so it can be reused
  elsewhere in QEMU if needed. It should also copy the test suite,
  since it is bad practice to throw away tests which already exist.
 
 I also don't buy that we can't use strftime.  There are very few places
 where we use fork() in QEMU (it doesn't exist on Windows so it can't be
 used without protection).  None of those places use the error reporting
 infrastructure.
 
 This code is also extremely naive.  It doesn't take into account leap
 seconds and makes bad assumptions about leap years.
 
 Regards,
 
 Anthony Liguori
 
 
  Daniel
  --
  |: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ 
  :|
  |: http://libvirt.org  -o- http://virt-manager.org 
  :|
  |: http://autobuild.org   -o- http://search.cpan.org/~danberr/ 
  :|
  |: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc 
  :|

[Qemu-devel] [RFC][PATCH v2]Add timestamp to error message

2013-04-29 Thread Seiji Aguchi 
From: Seiji Aguchi seiji.agu...@hds.com

[Issue]
When we offer a customer support service and a problem happens 
in a customer's system, we try to understand the problem by 
comparing what the customer reports with message logs of the 
customer's system.

In this case, we often need to know when the problem happens.

But, currently, there is no timestamp in qemu's error messages.
Therefore, we may not be able to understand the problem based on
error messages.

[Solution]
This patch adds a timestamp to qemu's error message logged by
error_report().

A logic calculating a time is copied from libvirt, src/util/virtime.c.
* 
http://libvirt.org/git/?p=libvirt.git;a=commit;h=3ec128989606278635a7c5dfbeee959692d12e15

[TODO]
Add timestamp Messages with monitor_printf() and fprintf().

Signed-off-by: Seiji Aguchi seiji.agu...@hds.com
---
Changelog v1 - v2

 - add an option, -msg timestamp={on|off}, to enable output message with
   timestamp
---
 include/monitor/monitor.h |2 +
 qemu-options.hx   |   12 ++
 util/qemu-error.c |   93 +
 vl.c  |   26 
 4 files changed, 133 insertions(+), 0 deletions(-)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index b868760..6cbb0ff 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -99,4 +99,6 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd);
 int monitor_fdset_dup_fd_remove(int dup_fd);
 int monitor_fdset_dup_fd_find(int dup_fd);
 
+extern bool enable_timestamp_msg;
+
 #endif /* !MONITOR_H */
diff --git a/qemu-options.hx b/qemu-options.hx
index e86cc24..a421261 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3097,3 +3097,15 @@ HXCOMM This is the last statement. Insert new options 
before this line!
 STEXI
 @end table
 ETEXI
+
+DEF(msg, HAS_ARG, QEMU_OPTION_msg,
+-msg [timestamp=on|off]\n
+output message with timestamp (default: off)\n,
+QEMU_ARCH_ALL)
+STEXI
+@item -msg timestamp=on|off
+@findex - msg
+Output message with timestamp.
+Adding timestamp to messages with @option{timestamp=on}
+(disabled by default).
+ETEXI
diff --git a/util/qemu-error.c b/util/qemu-error.c
index 08a36f4..35ef9ab 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -196,6 +196,96 @@ void error_print_loc(void)
 }
 }
 
+
+#define SECS_PER_HOUR   (60 * 60)
+#define SECS_PER_DAY(SECS_PER_HOUR * 24)
+#define DIV(a, b) ((a) / (b) - ((a) % (b)  0))
+#define LEAPS_THRU_END_OF(y) (DIV(y, 4) - DIV(y, 100) + DIV(y, 400))
+
+const unsigned short int __mon_yday[2][13] = {
+/* Normal years.  */
+{ 0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334, 365 },
+/* Leap years.  */
+{ 0, 31, 60, 91, 121, 152, 182, 213, 244, 274, 305, 335, 366 }
+};
+
+#define is_leap_year(y) \
+((y) % 4 == 0  ((y) % 100 != 0 || (y) % 400 == 0))
+
+static void error_print_timestamp(void)
+{
+struct timespec ts;
+unsigned long long now;
+
+struct tm fields;
+long int days, rem, y;
+const unsigned short int *ip;
+unsigned long long whenSecs;
+unsigned int offset = 0; /* We hardcoded GMT */
+
+if (clock_gettime(CLOCK_REALTIME, ts)  0) {
+return;
+}
+
+now = (ts.tv_sec * 1000ull) + (ts.tv_nsec / (1000ull * 1000ull));
+/* This code is taken from GLibC under terms of LGPLv2+ */
+
+whenSecs = now / 1000ull;
+
+days = whenSecs / SECS_PER_DAY;
+rem = whenSecs % SECS_PER_DAY;
+rem += offset;
+while (rem  0) {
+rem += SECS_PER_DAY;
+--days;
+}
+while (rem = SECS_PER_DAY) {
+rem -= SECS_PER_DAY;
+++days;
+}
+fields.tm_hour = rem / SECS_PER_HOUR;
+rem %= SECS_PER_HOUR;
+fields.tm_min = rem / 60;
+fields.tm_sec = rem % 60;
+/* January 1, 1970 was a Thursday.  */
+fields.tm_wday = (4 + days) % 7;
+if (fields.tm_wday  0) {
+fields.tm_wday += 7;
+}
+y = 1970;
+
+while (days  0 || days = (is_leap_year(y) ? 366 : 365)) {
+/* Guess a corrected year, assuming 365 days per year.  */
+long int yg = y + days / 365 - (days % 365  0);
+
+  /* Adjust DAYS and Y to match the guessed year.  */
+  days -= ((yg - y) * 365
+   + LEAPS_THRU_END_OF(yg - 1)
+   - LEAPS_THRU_END_OF(y - 1));
+  y = yg;
+}
+fields.tm_year = y - 1900;
+
+fields.tm_yday = days;
+ip = __mon_yday[is_leap_year(y)];
+for (y = 11; days  (long int) ip[y]; --y) {
+continue;
+}
+
+days -= ip[y];
+fields.tm_mon = y;
+fields.tm_mday = days + 1;
+
+error_printf(
+   %4d-%02d-%02d %02d:%02d:%02d.%03lld+ ,
+   fields.tm_year + 1900, fields.tm_mon + 1, fields.tm_mday,
+   fields.tm_hour, fields.tm_min, fields.tm_sec,
+   now % 1000);
+
+return;
+}
+
+bool enable_timestamp_msg;
 /*
  * Print an error message to current monitor if we have one, else to stderr.
  * Format arguments like

Re: [Qemu-devel] [PATCH v4] Add option to mlock qemu and guest memory

2013-04-02 Thread Seiji Aguchi 
Anthony,

 Currently my company doesn't allow me to use git-send-email to send a email.
 So now I'm trying to find work around with IT.

Satoru and I still talk if we can use git-send-email with my company IT.
But, it seems to  take a long time...

So, I attached Satoru's patch.
Could you please apply it to your tree?

Seiji

 -Original Message-
 From: Satoru Moriya
 Sent: Thursday, March 28, 2013 12:43 AM
 To: Anthony Liguori; qemu-devel@nongnu.org
 Cc: Jan Kiszka; mtosa...@redhat.com; Paolo Bonzini; Seiji Aguchi; Tomoki 
 Sekiyama; dle-deve...@lists.sourceforge.net;
 satoru.moriya...@hitachi.com
 Subject: RE: [Qemu-devel] [PATCH v4] Add option to mlock qemu and guest memory
 
  -Original Message-
  From: Anthony Liguori [mailto:aligu...@us.ibm.com]
  Sent: Wednesday, March 27, 2013 11:18 AM
  To: Satoru Moriya; qemu-devel@nongnu.org
  Cc: Jan Kiszka; mtosa...@redhat.com; Paolo Bonzini; Seiji Aguchi;
  Tomoki Sekiyama; dle- deve...@lists.sourceforge.net;
  satoru.moriya...@hitachi.com
  Subject: Re: [Qemu-devel] [PATCH v4] Add option to mlock qemu and
  guest memory
 
  Satoru Moriya satoru.mor...@hds.com writes:
 
   In certain scenario, latency induced by paging is significant and
   memory locking is needed. Also, in the scenario with untrusted
   guests, latency improvement due to mlock is desired.
  
   This patch introduces a following new option to mlock guest and qemu
   memory:
  
   -realtime mlock=on|off
  
   Signed-off-by: Satoru Moriya satoru.mor...@hds.com
   Reviewed-by: Paolo Bonzini pbonz...@redhat.com
   Reviewed-by: Marcelo Tosatti mtosa...@redhat.com
 
  This patch doesn't apply because you're sending it MIME encoded and
  the patch has carriage returns in it.  I think your mailer is badly munging 
  the patch.
 
  Please send it via git-send-email directly from the repository.
 
 I'm sorry for bothering you...
 
 Currently my company doesn't allow me to use git-send-email to send a email.
 So now I'm trying to find work around with IT.
 
 Once it is solved, I re-post the patch.
 
 Regards,
 Satoru


0001-Add-option-to-mlock-qemu-and-guest-memory.patch
Description: 0001-Add-option-to-mlock-qemu-and-guest-memory.patch

[Qemu-devel] [PATCH] Make guest OS bootable when hardware failure happens in log disk

2013-03-28 Thread Seiji Aguchi 
[Problem]
Currently, guest OS's messages can be logged to a local disk of host OS by 
creating chadevs with options below.
  -chardev file,id=charserial0,path=log file's path -device 
isa-serial,chardev=chardevserial0,id=serial0

When a hardware failure happens in the disk, qemu-kvm can't create the chardevs.
In this case, guest OS doesn't boot up.

Actually, there are users who don't desire that guest OS goes down due to a 
hardware failure of a log disk only.
Therefore, qemu should offer some way to boot guest OS up even if the log disk 
is broken.

[Solution]
This patch skips error checks in case where opening a log file and creating 
chardev fail.

Signed-off-by: Seiji Aguchi seiji.agu...@hds.com
---
 hw/qdev-properties-system.c |2 +-
 hw/serial-isa.c |   11 +++
 hw/serial-pci.c |   27 ---
 hw/serial.c |   33 +++--
 hw/serial.h |2 +-
 vl.c|4 +++-
 6 files changed, 47 insertions(+), 32 deletions(-)

diff --git a/hw/qdev-properties-system.c b/hw/qdev-properties-system.c
index d9934b5..99eff4f 100644
--- a/hw/qdev-properties-system.c
+++ b/hw/qdev-properties-system.c
@@ -121,7 +121,7 @@ static int parse_chr(DeviceState *dev, const char *str, 
void **ptr)
 {
 CharDriverState *chr = qemu_chr_find(str);
 if (chr == NULL) {
-return -ENOENT;
+return 0;
 }
 if (chr-avail_connections  1) {
 return -EEXIST;
diff --git a/hw/serial-isa.c b/hw/serial-isa.c
index a630a7d..b6cfa0d 100644
--- a/hw/serial-isa.c
+++ b/hw/serial-isa.c
@@ -46,6 +46,7 @@ static int serial_isa_initfn(ISADevice *dev)
 static int index;
 ISASerialState *isa = DO_UPCAST(ISASerialState, dev, dev);
 SerialState *s = isa-state;
+int rc;
 
 if (isa-index == -1) {
 isa-index = index;
@@ -63,11 +64,13 @@ static int serial_isa_initfn(ISADevice *dev)
 
 s-baudbase = 115200;
 isa_init_irq(dev, s-irq, isa-isairq);
-serial_init_core(s);
-qdev_set_legacy_instance_id(dev-qdev, isa-iobase, 3);
+rc = serial_init_core(s);
+if (!rc) {
+qdev_set_legacy_instance_id(dev-qdev, isa-iobase, 3);
 
-memory_region_init_io(s-io, serial_io_ops, s, serial, 8);
-isa_register_ioport(dev, s-io, isa-iobase);
+memory_region_init_io(s-io, serial_io_ops, s, serial, 8);
+isa_register_ioport(dev, s-io, isa-iobase);
+}
 return 0;
 }
 
diff --git a/hw/serial-pci.c b/hw/serial-pci.c
index 954657b..3e8b2d5 100644
--- a/hw/serial-pci.c
+++ b/hw/serial-pci.c
@@ -49,15 +49,18 @@ static int serial_pci_init(PCIDevice *dev)
 {
 PCISerialState *pci = DO_UPCAST(PCISerialState, dev, dev);
 SerialState *s = pci-state;
+int rc;
 
 s-baudbase = 115200;
-serial_init_core(s);
+rc = serial_init_core(s);
 
-pci-dev.config[PCI_INTERRUPT_PIN] = 0x01;
-s-irq = pci-dev.irq[0];
+if (!rc) {
+pci-dev.config[PCI_INTERRUPT_PIN] = 0x01;
+s-irq = pci-dev.irq[0];
 
-memory_region_init_io(s-io, serial_io_ops, s, serial, 8);
-pci_register_bar(pci-dev, 0, PCI_BASE_ADDRESS_SPACE_IO, s-io);
+memory_region_init_io(s-io, serial_io_ops, s, serial, 8);
+pci_register_bar(pci-dev, 0, PCI_BASE_ADDRESS_SPACE_IO, s-io);
+}
 return 0;
 }
 
@@ -80,7 +83,7 @@ static int multi_serial_pci_init(PCIDevice *dev)
 PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
 PCIMultiSerialState *pci = DO_UPCAST(PCIMultiSerialState, dev, dev);
 SerialState *s;
-int i;
+int i, rc;
 
 switch (pc-device_id) {
 case 0x0003:
@@ -102,11 +105,13 @@ static int multi_serial_pci_init(PCIDevice *dev)
 for (i = 0; i  pci-ports; i++) {
 s = pci-state + i;
 s-baudbase = 115200;
-serial_init_core(s);
-s-irq = pci-irqs[i];
-pci-name[i] = g_strdup_printf(uart #%d, i+1);
-memory_region_init_io(s-io, serial_io_ops, s, pci-name[i], 8);
-memory_region_add_subregion(pci-iobar, 8 * i, s-io);
+rc = serial_init_core(s);
+if (!rc) {
+s-irq = pci-irqs[i];
+pci-name[i] = g_strdup_printf(uart #%d, i+1);
+memory_region_init_io(s-io, serial_io_ops, s, pci-name[i], 8);
+memory_region_add_subregion(pci-iobar, 8 * i, s-io);
+}
 }
 return 0;
 }
diff --git a/hw/serial.c b/hw/serial.c
index 0ccc499..ab8929e 100644
--- a/hw/serial.c
+++ b/hw/serial.c
@@ -670,11 +670,11 @@ static void serial_reset(void *opaque)
 qemu_irq_lower(s-irq);
 }
 
-void serial_init_core(SerialState *s)
+int serial_init_core(SerialState *s)
 {
 if (!s-chr) {
 fprintf(stderr, Can't create serial device, empty char device\n);
-   exit(1);
+return 1;
 }
 
 s-modem_status_poll = qemu_new_timer_ns(vm_clock, (QEMUTimerCB *) 
serial_update_msl, s);
@@ -684,6 +684,7 @@ void serial_init_core(SerialState *s)
 
 qemu_chr_add_handlers(s-chr

Re: [Qemu-devel] [RFC][PATCH]Add timestamp to error message

2013-02-04 Thread Seiji Aguchi 
  Do we really want to add timestamps to error messages unconditionally?
  I don't doubt it's useful in your scenario, but most of the time it's
  just annoying clutter.
 
 Agreed, I think it should be an option.  

OK. I will add the timestamp as an option.

 Also remember that management tools can pass a pipe as stderr when starting 
 QEMU, and
 then they can add their preferred timestamping/log formatting outside of QEMU.

To get an exact timestamp, qemu should supply it to the messages.
Also, there are some customers who use qemu in a small environment.
In that case, they don't use the management like vdsm.

Seiji

Re: [Qemu-devel] [RFC][PATCH]Add timestamp to error message

2013-02-04 Thread Seiji Aguchi 
  [TODO]
  Other functions below are used to output error messages in qemu.
   - qerror_report() is called in many source codes.
 
 Not a problem, it ends up in qerror_print(), which calls error_report().

Thanks.

 
   - fprintf() is called in vl.c.
 
 All over the place, not just vl.c.  Also monitor_printf().

I will take a look at the code in monitor_printf() and fprintf() in other 
places.

  On the other hand, qerror_report() and fprintf () seems to be called
  in signal handers.
 
 fprintf() in a signal handler?  Where?

Sorry, I misunderstood the code.
In signal handers, qemu just set a value like shutdown_signal and shutdown_pid 
or so.

Seiji

[Qemu-devel] [RFC][PATCH]Add timestamp to error message

2013-02-01 Thread Seiji Aguchi 
[Issue]
When we offer a customer support service and a problem happens 
in a customer's system, we try to understand the problem by 
comparing what the customer reports with message logs of the 
customer's system.

In this case, we often need to know when the problem happens.

But, currently, there is no timestamp in qemu's error messages.
Therefore, we may not be able to understand the problem based on
error messages.

[Solution]
This patch adds a timestamp to qemu's error message logged by
error_report().

A logic calculating a time is copied from libvirt, src/util/virtime.c.

[TODO]
Other functions below are used to output error messages in qemu.
 - qerror_report() is called in many source codes.
 - fprintf() is called in vl.c.

A timestamp should be added to these messages as well.

Signed-off-by: Seiji Aguchi seiji.agu...@hds.com
---
 util/qemu-error.c |   90 +
 1 files changed, 90 insertions(+), 0 deletions(-)

diff --git a/util/qemu-error.c b/util/qemu-error.c
index 08a36f4..44d0fc1 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -196,6 +196,95 @@ void error_print_loc(void)
 }
 }
 
+
+#define SECS_PER_HOUR   (60 * 60)
+#define SECS_PER_DAY(SECS_PER_HOUR * 24)
+#define DIV(a, b) ((a) / (b) - ((a) % (b)  0))
+#define LEAPS_THRU_END_OF(y) (DIV(y, 4) - DIV(y, 100) + DIV(y, 400))
+
+const unsigned short int __mon_yday[2][13] = {
+/* Normal years.  */
+{ 0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334, 365 },
+/* Leap years.  */
+{ 0, 31, 60, 91, 121, 152, 182, 213, 244, 274, 305, 335, 366 }
+};
+
+#define is_leap_year(y) \
+((y) % 4 == 0  ((y) % 100 != 0 || (y) % 400 == 0))
+
+static void error_print_timestamp(void)
+{
+struct timespec ts;
+unsigned long long now;
+
+struct tm fields;
+long int days, rem, y;
+const unsigned short int *ip;
+unsigned long long whenSecs;
+unsigned int offset = 0; /* We hardcoded GMT */
+
+if (clock_gettime(CLOCK_REALTIME, ts)  0) {
+return;
+}
+
+now = (ts.tv_sec * 1000ull) + (ts.tv_nsec / (1000ull * 1000ull));
+/* This code is taken from GLibC under terms of LGPLv2+ */
+
+whenSecs = now / 1000ull;
+
+days = whenSecs / SECS_PER_DAY;
+rem = whenSecs % SECS_PER_DAY;
+rem += offset;
+while (rem  0) {
+rem += SECS_PER_DAY;
+--days;
+}
+while (rem = SECS_PER_DAY) {
+rem -= SECS_PER_DAY;
+++days;
+}
+fields.tm_hour = rem / SECS_PER_HOUR;
+rem %= SECS_PER_HOUR;
+fields.tm_min = rem / 60;
+fields.tm_sec = rem % 60;
+/* January 1, 1970 was a Thursday.  */
+fields.tm_wday = (4 + days) % 7;
+if (fields.tm_wday  0) {
+fields.tm_wday += 7;
+}
+y = 1970;
+
+while (days  0 || days = (is_leap_year(y) ? 366 : 365)) {
+/* Guess a corrected year, assuming 365 days per year.  */
+long int yg = y + days / 365 - (days % 365  0);
+
+  /* Adjust DAYS and Y to match the guessed year.  */
+  days -= ((yg - y) * 365
+   + LEAPS_THRU_END_OF(yg - 1)
+   - LEAPS_THRU_END_OF(y - 1));
+  y = yg;
+}
+fields.tm_year = y - 1900;
+
+fields.tm_yday = days;
+ip = __mon_yday[is_leap_year(y)];
+for (y = 11; days  (long int) ip[y]; --y) {
+continue;
+}
+
+days -= ip[y];
+fields.tm_mon = y;
+fields.tm_mday = days + 1;
+
+error_printf(
+   %4d-%02d-%02d %02d:%02d:%02d.%03lld+ ,
+   fields.tm_year + 1900, fields.tm_mon + 1, fields.tm_mday,
+   fields.tm_hour, fields.tm_min, fields.tm_sec,
+   now % 1000);
+
+return;
+}
+
 /*
  * Print an error message to current monitor if we have one, else to stderr.
  * Format arguments like sprintf().  The result should not contain
@@ -207,6 +296,7 @@ void error_report(const char *fmt, ...)
 {
 va_list ap;
 
+error_print_timestamp();
 error_print_loc();
 va_start(ap, fmt);
 error_vprintf(fmt, ap);
-- 1.7.1

Re: [Qemu-devel] [RFC][PATCH]Add timestamp to error message

2013-02-01 Thread Seiji Aguchi 
Hi Laszlo,

Thank you for reviewing my patch!
I will update my patch in accordance with your comment, using gmtime() + 
strftime() and gettimeofday().

Seiji

Re: [Qemu-devel] [RFC][PATCH]Add timestamp to error message

2013-02-01 Thread Seiji Aguchi 
  Are gmtime() + strftime() unsuitable for some reason?
 
 They are not async-signal safe, so they are not usable in between a
 fork() and exec*().  Libvirt avoids them because it DOES log information 
 including timestamps in between fork/exec (and prior to
 writing libvirt's virtime.c, we DID hit cases where libvirt would deadlock a 
 child process due to the non-safe use of a more naive
 timestamp generator), but I don't know if qemu suffers from the same 
 restriction of when it has anything worth logging.

In my understanding, gmtime() + strftime() is usable in error_report() because 
it seems to be called 
For just a option check.

[TODO]
Other functions below are used to output error messages in qemu.
 - qerror_report() is called in many source codes.
 - fprintf() is called in vl.c.

On the other hand, qerror_report() and fprintf () seems to be called in signal 
handers.
So, in those cases, some signal-safe functions should be used.

Seiji