[Qemu-devel] [PATCH 0/4] virtio: Convert fprintf() to error_report()

[Qemu-devel] [PATCH 0/4] virtio: Convert fprintf() to error_report()

[Stefan Hajnoczi]

The virtio hardware emulation code uses fprintf(stderr, ...) error messages.
Improve things slightly by moving to error_report() so error messages will be
printed to the monitor, if present.

We want to handle device error states properly instead of bailing out with
exit(1) but this series does not attempt to fix that yet.

Leave virtio-9p for now where there are many cases of fprintf(stderr, ...) and
development is still very active.

[Qemu-devel] [PATCH 1/4] virtio-blk: Convert fprintf() to error_report()

[Stefan Hajnoczi]

Errors should be logged using error_report() so they go to the
appropriate monitor.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 hw/virtio-blk.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 49528a9..e5f9b27 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -324,13 +324,13 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req,
     MultiReqBuffer *mrb)
 {
     if (req->elem.out_num < 1 || req->elem.in_num < 1) {
-        fprintf(stderr, "virtio-blk missing headers\n");
+        error_report("virtio-blk missing headers");
         exit(1);
     }
 
     if (req->elem.out_sg[0].iov_len < sizeof(*req->out) ||
         req->elem.in_sg[req->elem.in_num - 1].iov_len < sizeof(*req->in)) {
-        fprintf(stderr, "virtio-blk header not in correct element\n");
+        error_report("virtio-blk header not in correct element");
         exit(1);
     }
 
-- 
1.7.2.3

[Qemu-devel] [PATCH 2/4] virtio: Convert fprintf() to error_report()

[Stefan Hajnoczi]

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 hw/virtio.c |   35 ++++++++++++++++++-----------------
 1 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/hw/virtio.c b/hw/virtio.c
index a2a657e..849a60f 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -14,6 +14,7 @@
 #include <inttypes.h>
 
 #include "trace.h"
+#include "qemu-error.h"
 #include "virtio.h"
 #include "sysemu.h"
 
@@ -253,8 +254,8 @@ static int virtqueue_num_heads(VirtQueue *vq, unsigned int idx)
 
     /* Check it isn't doing very strange things with descriptor numbers. */
     if (num_heads > vq->vring.num) {
-        fprintf(stderr, "Guest moved used index from %u to %u",
-                idx, vring_avail_idx(vq));
+        error_report("Guest moved used index from %u to %u",
+                     idx, vring_avail_idx(vq));
         exit(1);
     }
 
@@ -271,7 +272,7 @@ static unsigned int virtqueue_get_head(VirtQueue *vq, unsigned int idx)
 
     /* If their number is silly, that's a fatal mistake. */
     if (head >= vq->vring.num) {
-        fprintf(stderr, "Guest says index %u is available", head);
+        error_report("Guest says index %u is available", head);
         exit(1);
     }
 
@@ -293,7 +294,7 @@ static unsigned virtqueue_next_desc(target_phys_addr_t desc_pa,
     wmb();
 
     if (next >= max) {
-        fprintf(stderr, "Desc next is %u", next);
+        error_report("Desc next is %u", next);
         exit(1);
     }
 
@@ -320,13 +321,13 @@ int virtqueue_avail_bytes(VirtQueue *vq, int in_bytes, int out_bytes)
 
         if (vring_desc_flags(desc_pa, i) & VRING_DESC_F_INDIRECT) {
             if (vring_desc_len(desc_pa, i) % sizeof(VRingDesc)) {
-                fprintf(stderr, "Invalid size for indirect buffer table\n");
+                error_report("Invalid size for indirect buffer table");
                 exit(1);
             }
 
             /* If we've got too many, that implies a descriptor loop. */
             if (num_bufs >= max) {
-                fprintf(stderr, "Looped descriptor");
+                error_report("Looped descriptor");
                 exit(1);
             }
 
@@ -340,7 +341,7 @@ int virtqueue_avail_bytes(VirtQueue *vq, int in_bytes, int out_bytes)
         do {
             /* If we've got too many, that implies a descriptor loop. */
             if (++num_bufs > max) {
-                fprintf(stderr, "Looped descriptor");
+                error_report("Looped descriptor");
                 exit(1);
             }
 
@@ -374,7 +375,7 @@ void virtqueue_map_sg(struct iovec *sg, target_phys_addr_t *addr,
         len = sg[i].iov_len;
         sg[i].iov_base = cpu_physical_memory_map(addr[i], &len, is_write);
         if (sg[i].iov_base == NULL || len != sg[i].iov_len) {
-            fprintf(stderr, "virtio: trying to map MMIO memory\n");
+            error_report("virtio: trying to map MMIO memory");
             exit(1);
         }
     }
@@ -397,7 +398,7 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
 
     if (vring_desc_flags(desc_pa, i) & VRING_DESC_F_INDIRECT) {
         if (vring_desc_len(desc_pa, i) % sizeof(VRingDesc)) {
-            fprintf(stderr, "Invalid size for indirect buffer table\n");
+            error_report("Invalid size for indirect buffer table");
             exit(1);
         }
 
@@ -423,7 +424,7 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
 
         /* If we've got too many, that implies a descriptor loop. */
         if ((elem->in_num + elem->out_num) > max) {
-            fprintf(stderr, "Looped descriptor");
+            error_report("Looped descriptor");
             exit(1);
         }
     } while ((i = virtqueue_next_desc(desc_pa, i, max)) != max);
@@ -694,8 +695,8 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
     qemu_get_be16s(f, &vdev->queue_sel);
     qemu_get_be32s(f, &features);
     if (features & ~supported_features) {
-        fprintf(stderr, "Features 0x%x unsupported. Allowed features: 0x%x\n",
-                features, supported_features);
+        error_report("Features 0x%x unsupported. Allowed features: 0x%x",
+                     features, supported_features);
         return -1;
     }
     if (vdev->set_features)
@@ -717,11 +718,11 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
 	num_heads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
 	/* Check it isn't doing very strange things with descriptor numbers. */
 	if (num_heads > vdev->vq[i].vring.num) {
-		fprintf(stderr, "VQ %d size 0x%x Guest index 0x%x "
-                        "inconsistent with Host index 0x%x: delta 0x%x\n",
-			i, vdev->vq[i].vring.num,
-                        vring_avail_idx(&vdev->vq[i]),
-                        vdev->vq[i].last_avail_idx, num_heads);
+		error_report("VQ %d size 0x%x Guest index 0x%x "
+		             "inconsistent with Host index 0x%x: delta 0x%x",
+		             i, vdev->vq[i].vring.num,
+		             vring_avail_idx(&vdev->vq[i]),
+		             vdev->vq[i].last_avail_idx, num_heads);
 		return -1;
 	}
         if (vdev->binding->load_queue) {
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH 2/4] virtio: Convert fprintf() to error_report()

[Markus Armbruster]

Worth mentioning that this fixes several messages lacking newline.

[Qemu-devel] [PATCH 3/4] virtio-net: Convert fprintf() to error_report()

[Stefan Hajnoczi]

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 hw/virtio-net.c |   41 ++++++++++++++++++++---------------------
 1 files changed, 20 insertions(+), 21 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 7e1688c..1d61f19 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -120,8 +120,8 @@ static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
     if (!n->vhost_started) {
         int r = vhost_net_start(tap_get_vhost_net(n->nic->nc.peer), &n->vdev);
         if (r < 0) {
-            fprintf(stderr, "unable to start vhost net: %d: "
-                    "falling back on userspace virtio\n", -r);
+            error_report("unable to start vhost net: %d: "
+                         "falling back on userspace virtio", -r);
         } else {
             n->vhost_started = 1;
         }
@@ -271,7 +271,7 @@ static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
     uint8_t on;
 
     if (elem->out_num != 2 || elem->out_sg[1].iov_len != sizeof(on)) {
-        fprintf(stderr, "virtio-net ctrl invalid rx mode command\n");
+        error_report("virtio-net ctrl invalid rx mode command");
         exit(1);
     }
 
@@ -353,7 +353,7 @@ static int virtio_net_handle_vlan_table(VirtIONet *n, uint8_t cmd,
     uint16_t vid;
 
     if (elem->out_num != 2 || elem->out_sg[1].iov_len != sizeof(vid)) {
-        fprintf(stderr, "virtio-net ctrl invalid vlan command\n");
+        error_report("virtio-net ctrl invalid vlan command");
         return VIRTIO_NET_ERR;
     }
 
@@ -381,13 +381,13 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
 
     while (virtqueue_pop(vq, &elem)) {
         if ((elem.in_num < 1) || (elem.out_num < 1)) {
-            fprintf(stderr, "virtio-net ctrl missing headers\n");
+            error_report("virtio-net ctrl missing headers");
             exit(1);
         }
 
         if (elem.out_sg[0].iov_len < sizeof(ctrl) ||
             elem.in_sg[elem.in_num - 1].iov_len < sizeof(status)) {
-            fprintf(stderr, "virtio-net ctrl header not in correct element\n");
+            error_report("virtio-net ctrl header not in correct element");
             exit(1);
         }
 
@@ -591,21 +591,21 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_
         if (virtqueue_pop(n->rx_vq, &elem) == 0) {
             if (i == 0)
                 return -1;
-            fprintf(stderr, "virtio-net unexpected empty queue: "
+            error_report("virtio-net unexpected empty queue: "
                     "i %zd mergeable %d offset %zd, size %zd, "
-                    "guest hdr len %zd, host hdr len %zd guest features 0x%x\n",
+                    "guest hdr len %zd, host hdr len %zd guest features 0x%x",
                     i, n->mergeable_rx_bufs, offset, size,
                     guest_hdr_len, host_hdr_len, n->vdev.guest_features);
             exit(1);
         }
 
         if (elem.in_num < 1) {
-            fprintf(stderr, "virtio-net receive queue contains no in buffers\n");
+            error_report("virtio-net receive queue contains no in buffers");
             exit(1);
         }
 
         if (!n->mergeable_rx_bufs && elem.in_sg[0].iov_len != guest_hdr_len) {
-            fprintf(stderr, "virtio-net header not in first element\n");
+            error_report("virtio-net header not in first element");
             exit(1);
         }
 
@@ -630,12 +630,11 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_
          * Otherwise, drop it. */
         if (!n->mergeable_rx_bufs && offset < size) {
 #if 0
-            fprintf(stderr, "virtio-net truncated non-mergeable packet: "
-
-                    "i %zd mergeable %d offset %zd, size %zd, "
-                    "guest hdr len %zd, host hdr len %zd\n",
-                    i, n->mergeable_rx_bufs,
-                    offset, size, guest_hdr_len, host_hdr_len);
+            error_report("virtio-net truncated non-mergeable packet: "
+                         "i %zd mergeable %d offset %zd, size %zd, "
+                         "guest hdr len %zd, host hdr len %zd",
+                         i, n->mergeable_rx_bufs,
+                         offset, size, guest_hdr_len, host_hdr_len);
 #endif
             return size;
         }
@@ -695,7 +694,7 @@ static int32_t virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq)
             sizeof(struct virtio_net_hdr);
 
         if (out_num < 1 || out_sg->iov_len != hdr_len) {
-            fprintf(stderr, "virtio-net header not in first element\n");
+            error_report("virtio-net header not in first element");
             exit(1);
         }
 
@@ -981,10 +980,10 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
     n->rx_vq = virtio_add_queue(&n->vdev, 256, virtio_net_handle_rx);
 
     if (net->tx && strcmp(net->tx, "timer") && strcmp(net->tx, "bh")) {
-        fprintf(stderr, "virtio-net: "
-                "Unknown option tx=%s, valid options: \"timer\" \"bh\"\n",
-                net->tx);
-        fprintf(stderr, "Defaulting to \"bh\"\n");
+        error_report("virtio-net: "
+                     "Unknown option tx=%s, valid options: \"timer\" \"bh\"",
+                     net->tx);
+        error_report("Defaulting to \"bh\"");
     }
 
     if (net->tx && !strcmp(net->tx, "timer")) {
-- 
1.7.2.3

[Qemu-devel] [PATCH 4/4] virtio-pci: Convert fprintf() to error_report()

[Stefan Hajnoczi]

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 hw/virtio-pci.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 729917d..b9d0349 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -254,8 +254,8 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
         virtio_queue_set_vector(vdev, vdev->queue_sel, val);
         break;
     default:
-        fprintf(stderr, "%s: unexpected address 0x%x value 0x%x\n",
-                __func__, addr, val);
+        error_report("%s: unexpected address 0x%x value 0x%x",
+                     __func__, addr, val);
         break;
     }
 }
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH 0/4] virtio: Convert fprintf() to error_report()

[Anthony Liguori]

On 11/15/2010 02:44 PM, Stefan Hajnoczi wrote:
> The virtio hardware emulation code uses fprintf(stderr, ...) error messages.
> Improve things slightly by moving to error_report() so error messages will be
> printed to the monitor, if present.
>
> We want to handle device error states properly instead of bailing out with
> exit(1) but this series does not attempt to fix that yet.
>
> Leave virtio-9p for now where there are many cases of fprintf(stderr, ...) and
> development is still very active.
>    

Applied all.  Thanks.

Regards,

Anthony Liguori

>
>