[Qemu-devel] [PATCH 0/4] a few trivials

[Qemu-devel] [PATCH 0/4] a few trivials

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Just cleaning out my trivial pile; a few more traces and
a couple of error reporting tweaks.

Dave


Dr. David Alan Gilbert (4):
  pckbd: Convert DPRINTF->trace
  HMP: Prepend errors with 'Error:'
  kvm: Add kvm_set_ioeventfd* traces
  wavcapture: Convert to error_report

 accel/kvm/kvm-all.c    |  3 +++
 accel/kvm/trace-events |  2 ++
 audio/wavcapture.c     | 39 +++++++++++++++++----------------------
 hmp.c                  |  2 +-
 hw/input/pckbd.c       | 19 ++++++-------------
 hw/input/trace-events  |  7 +++++++
 6 files changed, 36 insertions(+), 36 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 1/4] pckbd: Convert DPRINTF->trace

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/input/pckbd.c      | 19 ++++++-------------
 hw/input/trace-events |  7 +++++++
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index 72e7d5f6cc..47a606f5e3 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -30,14 +30,7 @@
 #include "hw/input/i8042.h"
 #include "sysemu/sysemu.h"
 
-/* debug PC keyboard */
-//#define DEBUG_KBD
-#ifdef DEBUG_KBD
-#define DPRINTF(fmt, ...)                                       \
-    do { printf("KBD: " fmt , ## __VA_ARGS__); } while (0)
-#else
-#define DPRINTF(fmt, ...)
-#endif
+#include "trace.h"
 
 /*	Keyboard Controller Commands */
 #define KBD_CCMD_READ_MODE	0x20	/* Read mode bits */
@@ -210,7 +203,7 @@ static uint64_t kbd_read_status(void *opaque, hwaddr addr,
     KBDState *s = opaque;
     int val;
     val = s->status;
-    DPRINTF("kbd: read status=0x%02x\n", val);
+    trace_pckbd_kbd_read_status(val);
     return val;
 }
 
@@ -224,7 +217,7 @@ static void kbd_queue(KBDState *s, int b, int aux)
 
 static void outport_write(KBDState *s, uint32_t val)
 {
-    DPRINTF("kbd: write outport=0x%02x\n", val);
+    trace_pckbd_outport_write(val);
     s->outport = val;
     qemu_set_irq(s->a20_out, (val >> 1) & 1);
     if (!(val & 1)) {
@@ -237,7 +230,7 @@ static void kbd_write_command(void *opaque, hwaddr addr,
 {
     KBDState *s = opaque;
 
-    DPRINTF("kbd: write cmd=0x%02" PRIx64 "\n", val);
+    trace_pckbd_kbd_write_command(val);
 
     /* Bits 3-0 of the output port P2 of the keyboard controller may be pulsed
      * low for approximately 6 micro seconds. Bits 3-0 of the KBD_CCMD_PULSE
@@ -326,7 +319,7 @@ static uint64_t kbd_read_data(void *opaque, hwaddr addr,
     else
         val = ps2_read_data(s->kbd);
 
-    DPRINTF("kbd: read data=0x%02x\n", val);
+    trace_pckbd_kbd_read_data(val);
     return val;
 }
 
@@ -335,7 +328,7 @@ static void kbd_write_data(void *opaque, hwaddr addr,
 {
     KBDState *s = opaque;
 
-    DPRINTF("kbd: write data=0x%02" PRIx64 "\n", val);
+    trace_pckbd_kbd_write_data(val);
 
     switch(s->write_cmd) {
     case 0:
diff --git a/hw/input/trace-events b/hw/input/trace-events
index 3965a842ae..8e53ae5bbf 100644
--- a/hw/input/trace-events
+++ b/hw/input/trace-events
@@ -14,6 +14,13 @@ adb_mouse_readreg(int reg, uint8_t val0, uint8_t val1) "reg %d obuf[0] 0x%2.2x o
 adb_mouse_request_change_addr(int devaddr) "change addr to 0x%x"
 adb_mouse_request_change_addr_and_handler(int devaddr, int handler) "change addr and handler to 0x%x, 0x%x"
 
+# hw/input/pckbd.c
+pckbd_kbd_read_data(uint32_t val) "0x%02x"
+pckbd_kbd_read_status(int status) "0x%02x"
+pckbd_outport_write(uint32_t val) "0x%02x"
+pckbd_kbd_write_command(uint64_t val) "0x%02"PRIx64
+pckbd_kbd_write_data(uint64_t val) "0x%02"PRIx64
+
 # hw/input/ps2.c
 ps2_put_keycode(void *opaque, int keycode) "%p keycode 0x%02x"
 ps2_keyboard_event(void *opaque, int qcode, int down, unsigned int modifier, unsigned int modifiers) "%p qcode %d down %d modifier 0x%x modifiers 0x%x"
-- 
2.20.1

[Qemu-devel] [PATCH 2/4] HMP: Prepend errors with 'Error:'

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Always make error messages start with 'Error:' as a fallback
to make sure that anything parsing them can tell it failed.

Note: Some places don't use hmp_handle_error

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hmp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hmp.c b/hmp.c
index b2a2b1f84e..4d14718fea 100644
--- a/hmp.c
+++ b/hmp.c
@@ -62,7 +62,7 @@ static void hmp_handle_error(Monitor *mon, Error **errp)
 {
     assert(errp);
     if (*errp) {
-        error_report_err(*errp);
+        error_reportf_err(*errp, "Error: ");
     }
 }
 
-- 
2.20.1

[Qemu-devel] [PATCH 3/4] kvm: Add kvm_set_ioeventfd* traces

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Add a couple of traces around the kvm_set_ioeventfd* calls.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 accel/kvm/kvm-all.c    | 3 +++
 accel/kvm/trace-events | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 4e1de942ce..fd92b6f375 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -657,6 +657,8 @@ static int kvm_set_ioeventfd_mmio(int fd, hwaddr addr, uint32_t val,
         .fd = fd,
     };
 
+    trace_kvm_set_ioeventfd_mmio(fd, (uint64_t)addr, val, assign, size,
+                                 datamatch);
     if (!kvm_enabled()) {
         return -ENOSYS;
     }
@@ -688,6 +690,7 @@ static int kvm_set_ioeventfd_pio(int fd, uint16_t addr, uint16_t val,
         .fd = fd,
     };
     int r;
+    trace_kvm_set_ioeventfd_pio(fd, addr, val, assign, size, datamatch);
     if (!kvm_enabled()) {
         return -ENOSYS;
     }
diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events
index 58e98efe5d..8841025d68 100644
--- a/accel/kvm/trace-events
+++ b/accel/kvm/trace-events
@@ -12,5 +12,7 @@ kvm_irqchip_commit_routes(void) ""
 kvm_irqchip_add_msi_route(char *name, int vector, int virq) "dev %s vector %d virq %d"
 kvm_irqchip_update_msi_route(int virq) "Updating MSI route virq=%d"
 kvm_irqchip_release_virq(int virq) "virq %d"
+kvm_set_ioeventfd_mmio(int fd, uint64_t addr, uint32_t val, bool assign, uint32_t size, bool datamatch) "fd: %d @0x%" PRIx64 " val=0x%x assign: %d size: %d match: %d"
+kvm_set_ioeventfd_pio(int fd, uint16_t addr, uint32_t val, bool assign, uint32_t size, bool datamatch) "fd: %d @0x%x val=0x%x assign: %d size: %d match: %d"
 kvm_set_user_memory(uint32_t slot, uint32_t flags, uint64_t guest_phys_addr, uint64_t memory_size, uint64_t userspace_addr, int ret) "Slot#%d flags=0x%x gpa=0x%"PRIx64 " size=0x%"PRIx64 " ua=0x%"PRIx64 " ret=%d"
 
-- 
2.20.1

[Qemu-devel] [PATCH 4/4] wavcapture: Convert to error_report

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Kill off a pile of monitor_printf's and cur_mon usage.
The only one left in wavcapture.c is the info case.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 audio/wavcapture.c | 39 +++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

diff --git a/audio/wavcapture.c b/audio/wavcapture.c
index cf31ed652c..cd24570aa7 100644
--- a/audio/wavcapture.c
+++ b/audio/wavcapture.c
@@ -38,30 +38,29 @@ static void wav_destroy (void *opaque)
     uint8_t dlen[4];
     uint32_t datalen = wav->bytes;
     uint32_t rifflen = datalen + 36;
-    Monitor *mon = cur_mon;
 
     if (wav->f) {
         le_store (rlen, rifflen, 4);
         le_store (dlen, datalen, 4);
 
         if (fseek (wav->f, 4, SEEK_SET)) {
-            monitor_printf (mon, "wav_destroy: rlen fseek failed\nReason: %s\n",
-                            strerror (errno));
+            error_report("wav_destroy: rlen fseek failed: %s",
+                         strerror(errno));
             goto doclose;
         }
         if (fwrite (rlen, 4, 1, wav->f) != 1) {
-            monitor_printf (mon, "wav_destroy: rlen fwrite failed\nReason %s\n",
-                            strerror (errno));
+            error_report("wav_destroy: rlen fwrite failed: %s",
+                         strerror(errno));
             goto doclose;
         }
         if (fseek (wav->f, 32, SEEK_CUR)) {
-            monitor_printf (mon, "wav_destroy: dlen fseek failed\nReason %s\n",
-                            strerror (errno));
+            error_report("wav_destroy: dlen fseek failed: %s",
+                         strerror(errno));
             goto doclose;
         }
         if (fwrite (dlen, 1, 4, wav->f) != 4) {
-            monitor_printf (mon, "wav_destroy: dlen fwrite failed\nReason %s\n",
-                            strerror (errno));
+            error_report("wav_destroy: dlen fwrite failed: %s",
+                         strerror(errno));
             goto doclose;
         }
     doclose:
@@ -78,8 +77,7 @@ static void wav_capture (void *opaque, void *buf, int size)
     WAVState *wav = opaque;
 
     if (fwrite (buf, size, 1, wav->f) != 1) {
-        monitor_printf (cur_mon, "wav_capture: fwrite error\nReason: %s",
-                        strerror (errno));
+        error_report("wav_capture: fwrite error: %s", strerror(errno));
     }
     wav->bytes += size;
 }
@@ -110,7 +108,6 @@ static struct capture_ops wav_capture_ops = {
 int wav_start_capture (CaptureState *s, const char *path, int freq,
                        int bits, int nchannels)
 {
-    Monitor *mon = cur_mon;
     WAVState *wav;
     uint8_t hdr[] = {
         0x52, 0x49, 0x46, 0x46, 0x00, 0x00, 0x00, 0x00, 0x57, 0x41, 0x56,
@@ -124,13 +121,13 @@ int wav_start_capture (CaptureState *s, const char *path, int freq,
     CaptureVoiceOut *cap;
 
     if (bits != 8 && bits != 16) {
-        monitor_printf (mon, "incorrect bit count %d, must be 8 or 16\n", bits);
+        error_report("incorrect bit count %d, must be 8 or 16", bits);
         return -1;
     }
 
     if (nchannels != 1 && nchannels != 2) {
-        monitor_printf (mon, "incorrect channel count %d, must be 1 or 2\n",
-                        nchannels);
+        error_report("incorrect channel count %d, must be 1 or 2",
+                     nchannels);
         return -1;
     }
 
@@ -158,8 +155,8 @@ int wav_start_capture (CaptureState *s, const char *path, int freq,
 
     wav->f = fopen (path, "wb");
     if (!wav->f) {
-        monitor_printf (mon, "Failed to open wave file `%s'\nReason: %s\n",
-                        path, strerror (errno));
+        error_report("Failed to open wave file `%s': %s",
+                     path, strerror(errno));
         g_free (wav);
         return -1;
     }
@@ -170,14 +167,13 @@ int wav_start_capture (CaptureState *s, const char *path, int freq,
     wav->freq = freq;
 
     if (fwrite (hdr, sizeof (hdr), 1, wav->f) != 1) {
-        monitor_printf (mon, "Failed to write header\nReason: %s\n",
-                        strerror (errno));
+        error_report("Failed to write header: %s", strerror(errno));
         goto error_free;
     }
 
     cap = AUD_add_capture (&as, &ops, wav);
     if (!cap) {
-        monitor_printf (mon, "Failed to add audio capture\n");
+        error_report("Failed to add audio capture");
         goto error_free;
     }
 
@@ -189,8 +185,7 @@ int wav_start_capture (CaptureState *s, const char *path, int freq,
 error_free:
     g_free (wav->path);
     if (fclose (wav->f)) {
-        monitor_printf (mon, "Failed to close wave file\nReason: %s\n",
-                        strerror (errno));
+        error_report("Failed to close wave file: %s", strerror(errno));
     }
     g_free (wav);
     return -1;
-- 
2.20.1

Re: [Qemu-devel] [PATCH 0/4] a few trivials

[Michael S. Tsirkin]

On Tue, Feb 12, 2019 at 01:47:54PM +0000, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Just cleaning out my trivial pile; a few more traces and
> a couple of error reporting tweaks.
> 
> Dave

Looks sane

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> 
> Dr. David Alan Gilbert (4):
>   pckbd: Convert DPRINTF->trace
>   HMP: Prepend errors with 'Error:'
>   kvm: Add kvm_set_ioeventfd* traces
>   wavcapture: Convert to error_report
> 
>  accel/kvm/kvm-all.c    |  3 +++
>  accel/kvm/trace-events |  2 ++
>  audio/wavcapture.c     | 39 +++++++++++++++++----------------------
>  hmp.c                  |  2 +-
>  hw/input/pckbd.c       | 19 ++++++-------------
>  hw/input/trace-events  |  7 +++++++
>  6 files changed, 36 insertions(+), 36 deletions(-)
> 
> -- 
> 2.20.1

Re: [Qemu-devel] [PATCH 1/4] pckbd: Convert DPRINTF->trace

[Philippe Mathieu-Daudé]

On 2/12/19 2:47 PM, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Per https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg03186.html
this one already has:

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

> ---
>  hw/input/pckbd.c      | 19 ++++++-------------
>  hw/input/trace-events |  7 +++++++
>  2 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
> index 72e7d5f6cc..47a606f5e3 100644
> --- a/hw/input/pckbd.c
> +++ b/hw/input/pckbd.c
> @@ -30,14 +30,7 @@
>  #include "hw/input/i8042.h"
>  #include "sysemu/sysemu.h"
>  
> -/* debug PC keyboard */
> -//#define DEBUG_KBD
> -#ifdef DEBUG_KBD
> -#define DPRINTF(fmt, ...)                                       \
> -    do { printf("KBD: " fmt , ## __VA_ARGS__); } while (0)
> -#else
> -#define DPRINTF(fmt, ...)
> -#endif
> +#include "trace.h"
>  
>  /*	Keyboard Controller Commands */
>  #define KBD_CCMD_READ_MODE	0x20	/* Read mode bits */
> @@ -210,7 +203,7 @@ static uint64_t kbd_read_status(void *opaque, hwaddr addr,
>      KBDState *s = opaque;
>      int val;
>      val = s->status;
> -    DPRINTF("kbd: read status=0x%02x\n", val);
> +    trace_pckbd_kbd_read_status(val);
>      return val;
>  }
>  
> @@ -224,7 +217,7 @@ static void kbd_queue(KBDState *s, int b, int aux)
>  
>  static void outport_write(KBDState *s, uint32_t val)
>  {
> -    DPRINTF("kbd: write outport=0x%02x\n", val);
> +    trace_pckbd_outport_write(val);
>      s->outport = val;
>      qemu_set_irq(s->a20_out, (val >> 1) & 1);
>      if (!(val & 1)) {
> @@ -237,7 +230,7 @@ static void kbd_write_command(void *opaque, hwaddr addr,
>  {
>      KBDState *s = opaque;
>  
> -    DPRINTF("kbd: write cmd=0x%02" PRIx64 "\n", val);
> +    trace_pckbd_kbd_write_command(val);
>  
>      /* Bits 3-0 of the output port P2 of the keyboard controller may be pulsed
>       * low for approximately 6 micro seconds. Bits 3-0 of the KBD_CCMD_PULSE
> @@ -326,7 +319,7 @@ static uint64_t kbd_read_data(void *opaque, hwaddr addr,
>      else
>          val = ps2_read_data(s->kbd);
>  
> -    DPRINTF("kbd: read data=0x%02x\n", val);
> +    trace_pckbd_kbd_read_data(val);
>      return val;
>  }
>  
> @@ -335,7 +328,7 @@ static void kbd_write_data(void *opaque, hwaddr addr,
>  {
>      KBDState *s = opaque;
>  
> -    DPRINTF("kbd: write data=0x%02" PRIx64 "\n", val);
> +    trace_pckbd_kbd_write_data(val);
>  
>      switch(s->write_cmd) {
>      case 0:
> diff --git a/hw/input/trace-events b/hw/input/trace-events
> index 3965a842ae..8e53ae5bbf 100644
> --- a/hw/input/trace-events
> +++ b/hw/input/trace-events
> @@ -14,6 +14,13 @@ adb_mouse_readreg(int reg, uint8_t val0, uint8_t val1) "reg %d obuf[0] 0x%2.2x o
>  adb_mouse_request_change_addr(int devaddr) "change addr to 0x%x"
>  adb_mouse_request_change_addr_and_handler(int devaddr, int handler) "change addr and handler to 0x%x, 0x%x"
>  
> +# hw/input/pckbd.c
> +pckbd_kbd_read_data(uint32_t val) "0x%02x"
> +pckbd_kbd_read_status(int status) "0x%02x"
> +pckbd_outport_write(uint32_t val) "0x%02x"
> +pckbd_kbd_write_command(uint64_t val) "0x%02"PRIx64
> +pckbd_kbd_write_data(uint64_t val) "0x%02"PRIx64
> +
>  # hw/input/ps2.c
>  ps2_put_keycode(void *opaque, int keycode) "%p keycode 0x%02x"
>  ps2_keyboard_event(void *opaque, int qcode, int down, unsigned int modifier, unsigned int modifiers) "%p qcode %d down %d modifier 0x%x modifiers 0x%x"
> 

Re: [Qemu-devel] [PATCH 2/4] HMP: Prepend errors with 'Error:'

[Philippe Mathieu-Daudé]

On 2/12/19 2:47 PM, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Always make error messages start with 'Error:' as a fallback
> to make sure that anything parsing them can tell it failed.
> 
> Note: Some places don't use hmp_handle_error
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  hmp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hmp.c b/hmp.c
> index b2a2b1f84e..4d14718fea 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -62,7 +62,7 @@ static void hmp_handle_error(Monitor *mon, Error **errp)
>  {
>      assert(errp);
>      if (*errp) {
> -        error_report_err(*errp);
> +        error_reportf_err(*errp, "Error: ");
>      }
>  }
>  
> 

Re: [Qemu-devel] [PATCH 3/4] kvm: Add kvm_set_ioeventfd* traces

[Philippe Mathieu-Daudé]

On 2/12/19 2:47 PM, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Add a couple of traces around the kvm_set_ioeventfd* calls.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  accel/kvm/kvm-all.c    | 3 +++
>  accel/kvm/trace-events | 2 ++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 4e1de942ce..fd92b6f375 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -657,6 +657,8 @@ static int kvm_set_ioeventfd_mmio(int fd, hwaddr addr, uint32_t val,
>          .fd = fd,
>      };
>  
> +    trace_kvm_set_ioeventfd_mmio(fd, (uint64_t)addr, val, assign, size,
> +                                 datamatch);
>      if (!kvm_enabled()) {
>          return -ENOSYS;
>      }
> @@ -688,6 +690,7 @@ static int kvm_set_ioeventfd_pio(int fd, uint16_t addr, uint16_t val,
>          .fd = fd,
>      };
>      int r;
> +    trace_kvm_set_ioeventfd_pio(fd, addr, val, assign, size, datamatch);
>      if (!kvm_enabled()) {
>          return -ENOSYS;
>      }
> diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events
> index 58e98efe5d..8841025d68 100644
> --- a/accel/kvm/trace-events
> +++ b/accel/kvm/trace-events
> @@ -12,5 +12,7 @@ kvm_irqchip_commit_routes(void) ""
>  kvm_irqchip_add_msi_route(char *name, int vector, int virq) "dev %s vector %d virq %d"
>  kvm_irqchip_update_msi_route(int virq) "Updating MSI route virq=%d"
>  kvm_irqchip_release_virq(int virq) "virq %d"
> +kvm_set_ioeventfd_mmio(int fd, uint64_t addr, uint32_t val, bool assign, uint32_t size, bool datamatch) "fd: %d @0x%" PRIx64 " val=0x%x assign: %d size: %d match: %d"
> +kvm_set_ioeventfd_pio(int fd, uint16_t addr, uint32_t val, bool assign, uint32_t size, bool datamatch) "fd: %d @0x%x val=0x%x assign: %d size: %d match: %d"
>  kvm_set_user_memory(uint32_t slot, uint32_t flags, uint64_t guest_phys_addr, uint64_t memory_size, uint64_t userspace_addr, int ret) "Slot#%d flags=0x%x gpa=0x%"PRIx64 " size=0x%"PRIx64 " ua=0x%"PRIx64 " ret=%d"
>  
> 

Re: [Qemu-devel] [PATCH 3/4] kvm: Add kvm_set_ioeventfd* traces

[Cornelia Huck]

On Tue, 12 Feb 2019 13:47:57 +0000
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Add a couple of traces around the kvm_set_ioeventfd* calls.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  accel/kvm/kvm-all.c    | 3 +++
>  accel/kvm/trace-events | 2 ++
>  2 files changed, 5 insertions(+)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Maybe I'll add a tracepoint for the s390 subchannel ioeventfd as well...

Re: [Qemu-devel] [PATCH 2/4] HMP: Prepend errors with 'Error:'

[Markus Armbruster]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Always make error messages start with 'Error:' as a fallback
> to make sure that anything parsing them can tell it failed.

Feeding HMP output to programs is a bad idea to begin with.  

> Note: Some places don't use hmp_handle_error

True.  A more honest subject would be "HMP: Prepend some errors with
'Error:'".

Several distinct reasons for not using hmp_handle_error():

* Handling an error that isn't transmitted as an Error object

  HMP command handlers generally wrap around QMP command handlers, which
  always use Error objects.  Still, there are other sources of errors
  when the HMP command does more than the wrapped QMP command (typically
  convenience features for humans), or there is no wrapped QMP command.

* Augmenting the error message

  Reporting with error_reportf_err() is easier than first messing with
  the Error object, then passing it to hmp_handle_error().  Example:
  hmp_delvm().

* hmp_handle_error() is static, and we're outside hmp.c

  Tolerable as long as hmp_handle_error() is trivial.  Your patch makes
  it a bit less trivial.

If we truly want to "Prepend errors with 'Error:'", then we should make
hmp command handlers take an Error ** argument, and leave the reporting
to the HMP core.

If you think prepending some errors is useful (and I guess you do), go
right ahead with this patch.  I'd recommend to use the more honest
subject, though.

Re: [Qemu-devel] [Qemu-trivial] [PATCH 1/4] pckbd: Convert DPRINTF->trace

[Laurent Vivier]

On 12/02/2019 15:10, Philippe Mathieu-Daudé wrote:
> On 2/12/19 2:47 PM, Dr. David Alan Gilbert (git) wrote:
>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>
>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> Per https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg03186.html
> this one already has:
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> 


Applied the one from 2018-10
(20181016112232.23241-1-dgilbert@redhat.com), as it is identical and has
the R-b, to my trivial-patches branch.

Thanks,
Laurent

Re: [Qemu-devel] [Qemu-trivial] [PATCH 2/4] HMP: Prepend errors with 'Error:'

[Laurent Vivier]

On 12/02/2019 14:47, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Always make error messages start with 'Error:' as a fallback
> to make sure that anything parsing them can tell it failed.
> 
> Note: Some places don't use hmp_handle_error
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  hmp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hmp.c b/hmp.c
> index b2a2b1f84e..4d14718fea 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -62,7 +62,7 @@ static void hmp_handle_error(Monitor *mon, Error **errp)
>  {
>      assert(errp);
>      if (*errp) {
> -        error_report_err(*errp);
> +        error_reportf_err(*errp, "Error: ");
>      }
>  }
>  
> 

Applied to my trivial-patches branch.

Thanks,
Laurent

Re: [Qemu-devel] [Qemu-trivial] [PATCH 4/4] wavcapture: Convert to error_report

[Laurent Vivier]

On 12/02/2019 14:47, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Kill off a pile of monitor_printf's and cur_mon usage.
> The only one left in wavcapture.c is the info case.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  audio/wavcapture.c | 39 +++++++++++++++++----------------------
>  1 file changed, 17 insertions(+), 22 deletions(-)
> 
> diff --git a/audio/wavcapture.c b/audio/wavcapture.c
> index cf31ed652c..cd24570aa7 100644
> --- a/audio/wavcapture.c
> +++ b/audio/wavcapture.c
> @@ -38,30 +38,29 @@ static void wav_destroy (void *opaque)
>      uint8_t dlen[4];
>      uint32_t datalen = wav->bytes;
>      uint32_t rifflen = datalen + 36;
> -    Monitor *mon = cur_mon;
>  
>      if (wav->f) {
>          le_store (rlen, rifflen, 4);
>          le_store (dlen, datalen, 4);
>  
>          if (fseek (wav->f, 4, SEEK_SET)) {
> -            monitor_printf (mon, "wav_destroy: rlen fseek failed\nReason: %s\n",
> -                            strerror (errno));
> +            error_report("wav_destroy: rlen fseek failed: %s",
> +                         strerror(errno));
>              goto doclose;
>          }
>          if (fwrite (rlen, 4, 1, wav->f) != 1) {
> -            monitor_printf (mon, "wav_destroy: rlen fwrite failed\nReason %s\n",
> -                            strerror (errno));
> +            error_report("wav_destroy: rlen fwrite failed: %s",
> +                         strerror(errno));
>              goto doclose;
>          }
>          if (fseek (wav->f, 32, SEEK_CUR)) {
> -            monitor_printf (mon, "wav_destroy: dlen fseek failed\nReason %s\n",
> -                            strerror (errno));
> +            error_report("wav_destroy: dlen fseek failed: %s",
> +                         strerror(errno));
>              goto doclose;
>          }
>          if (fwrite (dlen, 1, 4, wav->f) != 4) {
> -            monitor_printf (mon, "wav_destroy: dlen fwrite failed\nReason %s\n",
> -                            strerror (errno));
> +            error_report("wav_destroy: dlen fwrite failed: %s",
> +                         strerror(errno));
>              goto doclose;
>          }
>      doclose:
> @@ -78,8 +77,7 @@ static void wav_capture (void *opaque, void *buf, int size)
>      WAVState *wav = opaque;
>  
>      if (fwrite (buf, size, 1, wav->f) != 1) {
> -        monitor_printf (cur_mon, "wav_capture: fwrite error\nReason: %s",
> -                        strerror (errno));
> +        error_report("wav_capture: fwrite error: %s", strerror(errno));
>      }
>      wav->bytes += size;
>  }
> @@ -110,7 +108,6 @@ static struct capture_ops wav_capture_ops = {
>  int wav_start_capture (CaptureState *s, const char *path, int freq,
>                         int bits, int nchannels)
>  {
> -    Monitor *mon = cur_mon;
>      WAVState *wav;
>      uint8_t hdr[] = {
>          0x52, 0x49, 0x46, 0x46, 0x00, 0x00, 0x00, 0x00, 0x57, 0x41, 0x56,
> @@ -124,13 +121,13 @@ int wav_start_capture (CaptureState *s, const char *path, int freq,
>      CaptureVoiceOut *cap;
>  
>      if (bits != 8 && bits != 16) {
> -        monitor_printf (mon, "incorrect bit count %d, must be 8 or 16\n", bits);
> +        error_report("incorrect bit count %d, must be 8 or 16", bits);
>          return -1;
>      }
>  
>      if (nchannels != 1 && nchannels != 2) {
> -        monitor_printf (mon, "incorrect channel count %d, must be 1 or 2\n",
> -                        nchannels);
> +        error_report("incorrect channel count %d, must be 1 or 2",
> +                     nchannels);
>          return -1;
>      }
>  
> @@ -158,8 +155,8 @@ int wav_start_capture (CaptureState *s, const char *path, int freq,
>  
>      wav->f = fopen (path, "wb");
>      if (!wav->f) {
> -        monitor_printf (mon, "Failed to open wave file `%s'\nReason: %s\n",
> -                        path, strerror (errno));
> +        error_report("Failed to open wave file `%s': %s",
> +                     path, strerror(errno));
>          g_free (wav);
>          return -1;
>      }
> @@ -170,14 +167,13 @@ int wav_start_capture (CaptureState *s, const char *path, int freq,
>      wav->freq = freq;
>  
>      if (fwrite (hdr, sizeof (hdr), 1, wav->f) != 1) {
> -        monitor_printf (mon, "Failed to write header\nReason: %s\n",
> -                        strerror (errno));
> +        error_report("Failed to write header: %s", strerror(errno));
>          goto error_free;
>      }
>  
>      cap = AUD_add_capture (&as, &ops, wav);
>      if (!cap) {
> -        monitor_printf (mon, "Failed to add audio capture\n");
> +        error_report("Failed to add audio capture");
>          goto error_free;
>      }
>  
> @@ -189,8 +185,7 @@ int wav_start_capture (CaptureState *s, const char *path, int freq,
>  error_free:
>      g_free (wav->path);
>      if (fclose (wav->f)) {
> -        monitor_printf (mon, "Failed to close wave file\nReason: %s\n",
> -                        strerror (errno));
> +        error_report("Failed to close wave file: %s", strerror(errno));
>      }
>      g_free (wav);
>      return -1;
> 

Applied the one from 2017-03
(20170320173840.3626-3-dgilbert@redhat.com), as it as Gerd's R-b and it
is identical, to my trivial-patches branch.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 4/4] wavcapture: Convert to error_report

[Markus Armbruster]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Kill off a pile of monitor_printf's and cur_mon usage.

You also switch from the two line error format

    <error message>
    Reason: <strerror(errno)>

to the more common

    <error message>: <strerror(errno)>

Suggest to mention that you're changing error messages in the commit
message.

> The only one left in wavcapture.c is the info case.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

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