[Qemu-devel] [PATCH v3 0/9] virtio: enhance virtio_error messages

[Qemu-devel] [PATCH v3 0/9] virtio: enhance virtio_error messages

[Ladi Prosek]

Output like "Virtqueue size exceeded" is not much useful in identifying the
culprit. This series beefs up virtio_error to print the virtio device name
and id, and introduces virtqueue_error which additionally includes the index
of the virtqueue where the error occured.

Patches 1 to 3 lay the groundwork, patches 4 to 8 convert virtio devices to
use virtqueue_error instead of virtio_error. Patch 9 adds virtio_error and
virtqueue_error to the list of error funcs in checkpatch.pl.

v1->v2:
* Modified virtio_error and added virtqueue_error (Stefan)
* Now also printing device id (Stefan)
* Went over all virtio_error call sites and converted them to virtqueue_error
  as appropriate; added virtio device maintainers to cc

v2->v3:
* Removed patch 1 (Stefan, Markus)
* Split patch 3 into 2 (adds virtqueue_error) and 3 (makes virtio.c call it)
  (Cornelia)
* Added patch 9 to modify $qemu_error_funcs in checkpatch.pl (Greg)
* s/includes queue index/includes the queue index/ in patch 3-9 commit
  messages (Cornelia)
* Fixed virtio_get_device_id to return empty string instead of NULL if the
  device doesn't have an id (Stefan)
* Simplified the change in virtio-crypto.c to use vcrypto->ctrl_vq instead
  of propagating the vq pointer in function arguments (Cornelia, Gonglei)

Ladi Prosek (9):
  virtio: enhance virtio_error messages
  virtio: introduce virtqueue_error
  virtio: use virtqueue_error for errors with queue context
  virtio-9p: use virtqueue_error for errors with queue context
  virtio-blk: use virtqueue_error for errors with queue context
  virtio-net: use virtqueue_error for errors with queue context
  virtio-scsi: use virtqueue_error for errors with queue context
  virtio-crypto: use virtqueue_error for errors with queue context
  checkpatch: add virtio_error and virtqueue_error to error funcs

 hw/9pfs/virtio-9p-device.c |  37 ++++++--------
 hw/block/virtio-blk.c      |   6 +--
 hw/net/virtio-net.c        |  24 ++++-----
 hw/scsi/virtio-scsi.c      |   2 +-
 hw/virtio/virtio-crypto.c  |  49 ++++++++++---------
 hw/virtio/virtio.c         | 119 +++++++++++++++++++++++++++++++--------------
 include/hw/virtio/virtio.h |   1 +
 scripts/checkpatch.pl      |   4 +-
 8 files changed, 143 insertions(+), 99 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH v3 1/9] virtio: enhance virtio_error messages

[Ladi Prosek]

Output like "Virtqueue size exceeded" is not much useful in identifying the
culprit. This commit adds virtio device name (e.g. "virtio-input") and id
if set (e.g. "mouse0") to all virtio error messages to improve debuggability.

Some virtio devices (virtio-scsi, virtio-serial) insert a bus between the
proxy object and the virtio backends, so a helper function is added to walk
the object hierarchy and find the right proxy object to extract the id from.

Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Ladi Prosek <lprosek@redhat.com>
---
 hw/virtio/virtio.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 464947f..d7fae54 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -31,6 +31,11 @@
  */
 #define VIRTIO_PCI_VRING_ALIGN         4096
 
+/*
+ * Name of the property linking proxy objects to virtio backend objects.
+ */
+#define VIRTIO_PROP_BACKEND            "virtio-backend"
+
 typedef struct VRingDesc
 {
     uint64_t addr;
@@ -2223,7 +2228,8 @@ void virtio_instance_init_common(Object *proxy_obj, void *data,
     DeviceState *vdev = data;
 
     object_initialize(vdev, vdev_size, vdev_name);
-    object_property_add_child(proxy_obj, "virtio-backend", OBJECT(vdev), NULL);
+    object_property_add_child(proxy_obj, VIRTIO_PROP_BACKEND, OBJECT(vdev),
+                              NULL);
     object_unref(OBJECT(vdev));
     qdev_alias_all_properties(vdev, proxy_obj);
 }
@@ -2443,14 +2449,34 @@ void virtio_device_set_child_bus_name(VirtIODevice *vdev, char *bus_name)
     vdev->bus_name = g_strdup(bus_name);
 }
 
+static const char *virtio_get_device_id(VirtIODevice *vdev)
+{
+    DeviceState *qdev = DEVICE(vdev);
+    while (qdev) {
+        /* Find the proxy object corresponding to the vdev backend */
+        Object *prop = object_property_get_link(OBJECT(qdev),
+                                                VIRTIO_PROP_BACKEND, NULL);
+        if (prop == OBJECT(vdev)) {
+            return qdev->id ? qdev->id : "";
+        }
+        qdev = qdev->parent_bus->parent;
+    }
+    g_assert_not_reached();
+    return "";
+}
+
 void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...)
 {
     va_list ap;
+    char *msg;
 
     va_start(ap, fmt);
-    error_vreport(fmt, ap);
+    msg = g_strdup_vprintf(fmt, ap);
     va_end(ap);
 
+    error_report("%s (id=%s): %s", vdev->name, virtio_get_device_id(vdev), msg);
+    g_free(msg);
+
     if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
         virtio_set_status(vdev, vdev->status | VIRTIO_CONFIG_S_NEEDS_RESET);
         virtio_notify_config(vdev);
-- 
2.9.3

[Qemu-devel] [PATCH v3 2/9] virtio: introduce virtqueue_error

[Ladi Prosek]

Most virtio error output pertains to a specific virtqueue so it makes
sense to include the queue index in error messages.

Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Ladi Prosek <lprosek@redhat.com>
---
 hw/virtio/virtio.c         | 44 +++++++++++++++++++++++++++++++++-----------
 include/hw/virtio/virtio.h |  1 +
 2 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index d7fae54..935a5e3 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2465,18 +2465,8 @@ static const char *virtio_get_device_id(VirtIODevice *vdev)
     return "";
 }
 
-void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...)
+static void virtio_device_set_broken(VirtIODevice *vdev)
 {
-    va_list ap;
-    char *msg;
-
-    va_start(ap, fmt);
-    msg = g_strdup_vprintf(fmt, ap);
-    va_end(ap);
-
-    error_report("%s (id=%s): %s", vdev->name, virtio_get_device_id(vdev), msg);
-    g_free(msg);
-
     if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
         virtio_set_status(vdev, vdev->status | VIRTIO_CONFIG_S_NEEDS_RESET);
         virtio_notify_config(vdev);
@@ -2485,6 +2475,38 @@ void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...)
     vdev->broken = true;
 }
 
+void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...)
+{
+    va_list ap;
+    char *msg;
+
+    va_start(ap, fmt);
+    msg = g_strdup_vprintf(fmt, ap);
+    va_end(ap);
+
+    error_report("%s (id=%s): %s", vdev->name, virtio_get_device_id(vdev), msg);
+    g_free(msg);
+
+    virtio_device_set_broken(vdev);
+}
+
+void GCC_FMT_ATTR(2, 3) virtqueue_error(VirtQueue *vq, const char *fmt, ...)
+{
+    VirtIODevice *vdev = vq->vdev;
+    va_list ap;
+    char *msg;
+
+    va_start(ap, fmt);
+    msg = g_strdup_vprintf(fmt, ap);
+    va_end(ap);
+
+    error_report("%s (id=%s) queue %d: %s", vdev->name,
+                 virtio_get_device_id(vdev), vq->queue_index, msg);
+    g_free(msg);
+
+    virtio_device_set_broken(vdev);
+}
+
 static void virtio_memory_listener_commit(MemoryListener *listener)
 {
     VirtIODevice *vdev = container_of(listener, VirtIODevice, listener);
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 80c45c3..c6c56a0 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -151,6 +151,7 @@ void virtio_init(VirtIODevice *vdev, const char *name,
 void virtio_cleanup(VirtIODevice *vdev);
 
 void virtio_error(VirtIODevice *vdev, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
+void virtqueue_error(VirtQueue *vq, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
 
 /* Set the child bus name. */
 void virtio_device_set_child_bus_name(VirtIODevice *vdev, char *bus_name);
-- 
2.9.3

[Qemu-devel] [PATCH v3 3/9] virtio: use virtqueue_error for errors with queue context

[Ladi Prosek]

virtqueue_error includes the queue index in the error output and is preferred
for errors that pertain to a virtqueue rather than to the device as a whole.

Signed-off-by: Ladi Prosek <lprosek@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio.c | 57 +++++++++++++++++++++++++++---------------------------
 1 file changed, 28 insertions(+), 29 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 935a5e3..de4dd32 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -148,7 +148,7 @@ static void virtio_init_region_cache(VirtIODevice *vdev, int n)
     len = address_space_cache_init(&new->desc, vdev->dma_as,
                                    addr, size, false);
     if (len < size) {
-        virtio_error(vdev, "Cannot map desc");
+        virtqueue_error(vq, "Cannot map desc");
         goto err_desc;
     }
 
@@ -156,7 +156,7 @@ static void virtio_init_region_cache(VirtIODevice *vdev, int n)
     len = address_space_cache_init(&new->used, vdev->dma_as,
                                    vq->vring.used, size, true);
     if (len < size) {
-        virtio_error(vdev, "Cannot map used");
+        virtqueue_error(vq, "Cannot map used");
         goto err_used;
     }
 
@@ -164,7 +164,7 @@ static void virtio_init_region_cache(VirtIODevice *vdev, int n)
     len = address_space_cache_init(&new->avail, vdev->dma_as,
                                    vq->vring.avail, size, false);
     if (len < size) {
-        virtio_error(vdev, "Cannot map avail");
+        virtqueue_error(vq, "Cannot map avail");
         goto err_avail;
     }
 
@@ -522,7 +522,7 @@ 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) {
-        virtio_error(vq->vdev, "Guest moved used index from %u to %u",
+        virtqueue_error(vq, "Guest moved used index from %u to %u",
                      idx, vq->shadow_avail_idx);
         return -EINVAL;
     }
@@ -545,7 +545,7 @@ static bool virtqueue_get_head(VirtQueue *vq, unsigned int idx,
 
     /* If their number is silly, that's a fatal mistake. */
     if (*head >= vq->vring.num) {
-        virtio_error(vq->vdev, "Guest says index %u is available", *head);
+        virtqueue_error(vq, "Guest says index %u is available", *head);
         return false;
     }
 
@@ -558,7 +558,7 @@ enum {
     VIRTQUEUE_READ_DESC_MORE = 1,   /* more buffers in chain */
 };
 
-static int virtqueue_read_next_desc(VirtIODevice *vdev, VRingDesc *desc,
+static int virtqueue_read_next_desc(VirtQueue *vq, VRingDesc *desc,
                                     MemoryRegionCache *desc_cache, unsigned int max,
                                     unsigned int *next)
 {
@@ -573,11 +573,11 @@ static int virtqueue_read_next_desc(VirtIODevice *vdev, VRingDesc *desc,
     smp_wmb();
 
     if (*next >= max) {
-        virtio_error(vdev, "Desc next is %u", *next);
+        virtqueue_error(vq, "Desc next is %u", *next);
         return VIRTQUEUE_READ_DESC_ERROR;
     }
 
-    vring_desc_read(vdev, desc, desc_cache, *next);
+    vring_desc_read(vq->vdev, desc, desc_cache, *next);
     return VIRTQUEUE_READ_DESC_MORE;
 }
 
@@ -610,7 +610,7 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
     max = vq->vring.num;
     caches = vring_get_region_caches(vq);
     if (caches->desc.len < max * sizeof(VRingDesc)) {
-        virtio_error(vdev, "Cannot map descriptor ring");
+        virtqueue_error(vq, "Cannot map descriptor ring");
         goto err;
     }
 
@@ -630,13 +630,13 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
 
         if (desc.flags & VRING_DESC_F_INDIRECT) {
             if (desc.len % sizeof(VRingDesc)) {
-                virtio_error(vdev, "Invalid size for indirect buffer table");
+                virtqueue_error(vq, "Invalid size for indirect buffer table");
                 goto err;
             }
 
             /* If we've got too many, that implies a descriptor loop. */
             if (num_bufs >= max) {
-                virtio_error(vdev, "Looped descriptor");
+                virtqueue_error(vq, "Looped descriptor");
                 goto err;
             }
 
@@ -646,7 +646,7 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
                                            desc.addr, desc.len, false);
             desc_cache = &indirect_desc_cache;
             if (len < desc.len) {
-                virtio_error(vdev, "Cannot map indirect buffer");
+                virtqueue_error(vq, "Cannot map indirect buffer");
                 goto err;
             }
 
@@ -658,7 +658,7 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
         do {
             /* If we've got too many, that implies a descriptor loop. */
             if (++num_bufs > max) {
-                virtio_error(vdev, "Looped descriptor");
+                virtqueue_error(vq, "Looped descriptor");
                 goto err;
             }
 
@@ -671,7 +671,7 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
                 goto done;
             }
 
-            rc = virtqueue_read_next_desc(vdev, &desc, desc_cache, max, &i);
+            rc = virtqueue_read_next_desc(vq, &desc, desc_cache, max, &i);
         } while (rc == VIRTQUEUE_READ_DESC_MORE);
 
         if (rc == VIRTQUEUE_READ_DESC_ERROR) {
@@ -715,7 +715,7 @@ int virtqueue_avail_bytes(VirtQueue *vq, unsigned int in_bytes,
     return in_bytes <= in_total && out_bytes <= out_total;
 }
 
-static bool virtqueue_map_desc(VirtIODevice *vdev, unsigned int *p_num_sg,
+static bool virtqueue_map_desc(VirtQueue *vq, unsigned int *p_num_sg,
                                hwaddr *addr, struct iovec *iov,
                                unsigned int max_num_sg, bool is_write,
                                hwaddr pa, size_t sz)
@@ -725,7 +725,7 @@ static bool virtqueue_map_desc(VirtIODevice *vdev, unsigned int *p_num_sg,
     assert(num_sg <= max_num_sg);
 
     if (!sz) {
-        virtio_error(vdev, "virtio: zero sized buffers are not allowed");
+        virtqueue_error(vq, "Zero sized buffers are not allowed");
         goto out;
     }
 
@@ -733,17 +733,16 @@ static bool virtqueue_map_desc(VirtIODevice *vdev, unsigned int *p_num_sg,
         hwaddr len = sz;
 
         if (num_sg == max_num_sg) {
-            virtio_error(vdev, "virtio: too many write descriptors in "
-                               "indirect table");
+            virtqueue_error(vq, "Too many write descriptors in indirect table");
             goto out;
         }
 
-        iov[num_sg].iov_base = dma_memory_map(vdev->dma_as, pa, &len,
+        iov[num_sg].iov_base = dma_memory_map(vq->vdev->dma_as, pa, &len,
                                               is_write ?
                                               DMA_DIRECTION_FROM_DEVICE :
                                               DMA_DIRECTION_TO_DEVICE);
         if (!iov[num_sg].iov_base) {
-            virtio_error(vdev, "virtio: bogus descriptor or out of resources");
+            virtqueue_error(vq, "Bogus descriptor or out of resources");
             goto out;
         }
 
@@ -862,7 +861,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
     max = vq->vring.num;
 
     if (vq->inuse >= vq->vring.num) {
-        virtio_error(vdev, "Virtqueue size exceeded");
+        virtqueue_error(vq, "Virtqueue size exceeded");
         goto done;
     }
 
@@ -878,7 +877,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
 
     caches = vring_get_region_caches(vq);
     if (caches->desc.len < max * sizeof(VRingDesc)) {
-        virtio_error(vdev, "Cannot map descriptor ring");
+        virtqueue_error(vq, "Cannot map descriptor ring");
         goto done;
     }
 
@@ -886,7 +885,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
     vring_desc_read(vdev, &desc, desc_cache, i);
     if (desc.flags & VRING_DESC_F_INDIRECT) {
         if (desc.len % sizeof(VRingDesc)) {
-            virtio_error(vdev, "Invalid size for indirect buffer table");
+            virtqueue_error(vq, "Invalid size for indirect buffer table");
             goto done;
         }
 
@@ -895,7 +894,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
                                        desc.addr, desc.len, false);
         desc_cache = &indirect_desc_cache;
         if (len < desc.len) {
-            virtio_error(vdev, "Cannot map indirect buffer");
+            virtqueue_error(vq, "Cannot map indirect buffer");
             goto done;
         }
 
@@ -909,16 +908,16 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
         bool map_ok;
 
         if (desc.flags & VRING_DESC_F_WRITE) {
-            map_ok = virtqueue_map_desc(vdev, &in_num, addr + out_num,
+            map_ok = virtqueue_map_desc(vq, &in_num, addr + out_num,
                                         iov + out_num,
                                         VIRTQUEUE_MAX_SIZE - out_num, true,
                                         desc.addr, desc.len);
         } else {
             if (in_num) {
-                virtio_error(vdev, "Incorrect order for descriptors");
+                virtqueue_error(vq, "Incorrect order for descriptors");
                 goto err_undo_map;
             }
-            map_ok = virtqueue_map_desc(vdev, &out_num, addr, iov,
+            map_ok = virtqueue_map_desc(vq, &out_num, addr, iov,
                                         VIRTQUEUE_MAX_SIZE, false,
                                         desc.addr, desc.len);
         }
@@ -928,11 +927,11 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
 
         /* If we've got too many, that implies a descriptor loop. */
         if ((in_num + out_num) > max) {
-            virtio_error(vdev, "Looped descriptor");
+            virtqueue_error(vq, "Looped descriptor");
             goto err_undo_map;
         }
 
-        rc = virtqueue_read_next_desc(vdev, &desc, desc_cache, max, &i);
+        rc = virtqueue_read_next_desc(vq, &desc, desc_cache, max, &i);
     } while (rc == VIRTQUEUE_READ_DESC_MORE);
 
     if (rc == VIRTQUEUE_READ_DESC_ERROR) {
-- 
2.9.3

[Qemu-devel] [PATCH v3 4/9] virtio-9p: use virtqueue_error for errors with queue context

[Ladi Prosek]

virtqueue_error includes the queue index in the error output and is preferred
for errors that pertain to a virtqueue rather than to the device as a whole.

Signed-off-by: Ladi Prosek <lprosek@redhat.com>
Acked-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/9pfs/virtio-9p-device.c | 37 +++++++++++++++----------------------
 1 file changed, 15 insertions(+), 22 deletions(-)

diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 62650b0..cf16b4b 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -54,16 +54,16 @@ static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
         }
 
         if (iov_size(elem->in_sg, elem->in_num) < 7) {
-            virtio_error(vdev,
-                         "The guest sent a VirtFS request without space for "
-                         "the reply");
+            virtqueue_error(vq,
+                            "The guest sent a VirtFS request without space for "
+                            "the reply");
             goto out_free_req;
         }
 
         len = iov_to_buf(elem->out_sg, elem->out_num, 0, &out, 7);
         if (len != 7) {
-            virtio_error(vdev, "The guest sent a malformed VirtFS request: "
-                         "header size is %zd, should be 7", len);
+            virtqueue_error(vq, "The guest sent a malformed VirtFS request: "
+                            "header size is %zd, should be 7", len);
             goto out_free_req;
         }
 
@@ -150,10 +150,8 @@ static ssize_t virtio_pdu_vmarshal(V9fsPDU *pdu, size_t offset,
 
     ret = v9fs_iov_vmarshal(elem->in_sg, elem->in_num, offset, 1, fmt, ap);
     if (ret < 0) {
-        VirtIODevice *vdev = VIRTIO_DEVICE(v);
-
-        virtio_error(vdev, "Failed to encode VirtFS reply type %d",
-                     pdu->id + 1);
+        virtqueue_error(v->vq, "Failed to encode VirtFS reply type %d",
+                        pdu->id + 1);
     }
     return ret;
 }
@@ -168,9 +166,8 @@ static ssize_t virtio_pdu_vunmarshal(V9fsPDU *pdu, size_t offset,
 
     ret = v9fs_iov_vunmarshal(elem->out_sg, elem->out_num, offset, 1, fmt, ap);
     if (ret < 0) {
-        VirtIODevice *vdev = VIRTIO_DEVICE(v);
-
-        virtio_error(vdev, "Failed to decode VirtFS request type %d", pdu->id);
+        virtqueue_error(v->vq, "Failed to decode VirtFS request type %d",
+                        pdu->id);
     }
     return ret;
 }
@@ -184,11 +181,9 @@ static void virtio_init_in_iov_from_pdu(V9fsPDU *pdu, struct iovec **piov,
     size_t buf_size = iov_size(elem->in_sg, elem->in_num);
 
     if (buf_size < size) {
-        VirtIODevice *vdev = VIRTIO_DEVICE(v);
-
-        virtio_error(vdev,
-                     "VirtFS reply type %d needs %zu bytes, buffer has %zu",
-                     pdu->id + 1, size, buf_size);
+        virtqueue_error(v->vq,
+                        "VirtFS reply type %d needs %zu bytes, buffer has %zu",
+                        pdu->id + 1, size, buf_size);
     }
 
     *piov = elem->in_sg;
@@ -204,11 +199,9 @@ static void virtio_init_out_iov_from_pdu(V9fsPDU *pdu, struct iovec **piov,
     size_t buf_size = iov_size(elem->out_sg, elem->out_num);
 
     if (buf_size < size) {
-        VirtIODevice *vdev = VIRTIO_DEVICE(v);
-
-        virtio_error(vdev,
-                     "VirtFS request type %d needs %zu bytes, buffer has %zu",
-                     pdu->id, size, buf_size);
+        virtqueue_error(v->vq,
+                        "VirtFS request type %d needs %zu bytes, "
+                        "buffer has %zu", pdu->id, size, buf_size);
     }
 
     *piov = elem->out_sg;
-- 
2.9.3

[Qemu-devel] [PATCH v3 5/9] virtio-blk: use virtqueue_error for errors with queue context

[Ladi Prosek]

virtqueue_error includes the queue index in the error output and is preferred
for errors that pertain to a virtqueue rather than to the device as a whole.

Signed-off-by: Ladi Prosek <lprosek@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/virtio-blk.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index b750bd8..359d8bd 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -490,20 +490,20 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
 
     if (req->elem.out_num < 1 || req->elem.in_num < 1) {
-        virtio_error(vdev, "virtio-blk missing headers");
+        virtqueue_error(req->vq, "virtio-blk missing headers");
         return -1;
     }
 
     if (unlikely(iov_to_buf(iov, out_num, 0, &req->out,
                             sizeof(req->out)) != sizeof(req->out))) {
-        virtio_error(vdev, "virtio-blk request outhdr too short");
+        virtqueue_error(req->vq, "virtio-blk request outhdr too short");
         return -1;
     }
 
     iov_discard_front(&iov, &out_num, sizeof(req->out));
 
     if (in_iov[in_num - 1].iov_len < sizeof(struct virtio_blk_inhdr)) {
-        virtio_error(vdev, "virtio-blk request inhdr too short");
+        virtqueue_error(req->vq, "virtio-blk request inhdr too short");
         return -1;
     }
 
-- 
2.9.3

[Qemu-devel] [PATCH v3 6/9] virtio-net: use virtqueue_error for errors with queue context

[Ladi Prosek]

virtqueue_error includes the queue index in the error output and is preferred
for errors that pertain to a virtqueue rather than to the device as a whole.

Signed-off-by: Ladi Prosek <lprosek@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/net/virtio-net.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 5630a9e..b3d0b85 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -970,7 +970,7 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
         }
         if (iov_size(elem->in_sg, elem->in_num) < sizeof(status) ||
             iov_size(elem->out_sg, elem->out_num) < sizeof(ctrl)) {
-            virtio_error(vdev, "virtio-net ctrl missing headers");
+            virtqueue_error(vq, "virtio-net ctrl missing headers");
             virtqueue_detach_element(vq, elem, 0);
             g_free(elem);
             break;
@@ -1204,20 +1204,20 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
         elem = virtqueue_pop(q->rx_vq, sizeof(VirtQueueElement));
         if (!elem) {
             if (i) {
-                virtio_error(vdev, "virtio-net unexpected empty queue: "
-                             "i %zd mergeable %d offset %zd, size %zd, "
-                             "guest hdr len %zd, host hdr len %zd "
-                             "guest features 0x%" PRIx64,
-                             i, n->mergeable_rx_bufs, offset, size,
-                             n->guest_hdr_len, n->host_hdr_len,
-                             vdev->guest_features);
+                virtqueue_error(q->rx_vq, "virtio-net unexpected empty queue: "
+                                "i %zd mergeable %d offset %zd, size %zd, "
+                                "guest hdr len %zd, host hdr len %zd "
+                                "guest features 0x%" PRIx64,
+                                i, n->mergeable_rx_bufs, offset, size,
+                                n->guest_hdr_len, n->host_hdr_len,
+                                vdev->guest_features);
             }
             return -1;
         }
 
         if (elem->in_num < 1) {
-            virtio_error(vdev,
-                         "virtio-net receive queue contains no in buffers");
+            virtqueue_error(q->rx_vq,
+                            "virtio-net receive queue contains no in buffers");
             virtqueue_detach_element(q->rx_vq, elem, 0);
             g_free(elem);
             return -1;
@@ -1333,7 +1333,7 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
         out_num = elem->out_num;
         out_sg = elem->out_sg;
         if (out_num < 1) {
-            virtio_error(vdev, "virtio-net header not in first element");
+            virtqueue_error(q->tx_vq, "virtio-net header not in first element");
             virtqueue_detach_element(q->tx_vq, elem, 0);
             g_free(elem);
             return -EINVAL;
@@ -1342,7 +1342,7 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
         if (n->has_vnet_hdr) {
             if (iov_to_buf(out_sg, out_num, 0, &mhdr, n->guest_hdr_len) <
                 n->guest_hdr_len) {
-                virtio_error(vdev, "virtio-net header incorrect");
+                virtqueue_error(q->tx_vq, "virtio-net header incorrect");
                 virtqueue_detach_element(q->tx_vq, elem, 0);
                 g_free(elem);
                 return -EINVAL;
-- 
2.9.3

[Qemu-devel] [PATCH v3 7/9] virtio-scsi: use virtqueue_error for errors with queue context

[Ladi Prosek]

virtqueue_error includes the queue index in the error output and is preferred
for errors that pertain to a virtqueue rather than to the device as a whole.

Signed-off-by: Ladi Prosek <lprosek@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/scsi/virtio-scsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index eb63944..436d9b9 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -84,7 +84,7 @@ static void virtio_scsi_complete_req(VirtIOSCSIReq *req)
 
 static void virtio_scsi_bad_req(VirtIOSCSIReq *req)
 {
-    virtio_error(VIRTIO_DEVICE(req->dev), "wrong size for virtio-scsi headers");
+    virtqueue_error(req->vq, "wrong size for virtio-scsi headers");
     virtqueue_detach_element(req->vq, &req->elem, 0);
     virtio_scsi_free_req(req);
 }
-- 
2.9.3

[Qemu-devel] [PATCH v3 8/9] virtio-crypto: use virtqueue_error for errors with queue context

[Ladi Prosek]

virtqueue_error includes the queue index in the error output and is preferred
for errors that pertain to a virtqueue rather than to the device as a whole.

Signed-off-by: Ladi Prosek <lprosek@redhat.com>
---
 hw/virtio/virtio-crypto.c | 49 ++++++++++++++++++++++++-----------------------
 1 file changed, 25 insertions(+), 24 deletions(-)

diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
index 19c82e0..eaf2ed3 100644
--- a/hw/virtio/virtio-crypto.c
+++ b/hw/virtio/virtio-crypto.c
@@ -61,7 +61,8 @@ virtio_crypto_cipher_session_helper(VirtIODevice *vdev,
         info->cipher_key = g_malloc(info->key_len);
         s = iov_to_buf(*iov, num, 0, info->cipher_key, info->key_len);
         if (unlikely(s != info->key_len)) {
-            virtio_error(vdev, "virtio-crypto cipher key incorrect");
+            virtqueue_error(vcrypto->ctrl_vq,
+                            "virtio-crypto cipher key incorrect");
             return -EFAULT;
         }
         iov_discard_front(iov, &num, info->key_len);
@@ -131,7 +132,7 @@ virtio_crypto_create_sym_session(VirtIOCrypto *vcrypto,
                 s = iov_to_buf(iov, out_num, 0, info.auth_key,
                                info.auth_key_len);
                 if (unlikely(s != info.auth_key_len)) {
-                    virtio_error(vdev,
+                    virtqueue_error(vcrypto->ctrl_vq,
                           "virtio-crypto authenticated key incorrect");
                     ret = -EFAULT;
                     goto err;
@@ -229,7 +230,7 @@ static void virtio_crypto_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
             break;
         }
         if (elem->out_num < 1 || elem->in_num < 1) {
-            virtio_error(vdev, "virtio-crypto ctrl missing headers");
+            virtqueue_error(vq, "virtio-crypto ctrl missing headers");
             virtqueue_detach_element(vq, elem, 0);
             g_free(elem);
             break;
@@ -241,7 +242,7 @@ static void virtio_crypto_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
         in_iov = elem->in_sg;
         if (unlikely(iov_to_buf(out_iov, out_num, 0, &ctrl, sizeof(ctrl))
                     != sizeof(ctrl))) {
-            virtio_error(vdev, "virtio-crypto request ctrl_hdr too short");
+            virtqueue_error(vq, "virtio-crypto request ctrl_hdr too short");
             virtqueue_detach_element(vq, elem, 0);
             g_free(elem);
             break;
@@ -274,7 +275,7 @@ static void virtio_crypto_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
 
             s = iov_from_buf(in_iov, in_num, 0, &input, sizeof(input));
             if (unlikely(s != sizeof(input))) {
-                virtio_error(vdev, "virtio-crypto input incorrect");
+                virtqueue_error(vq, "virtio-crypto input incorrect");
                 virtqueue_detach_element(vq, elem, 0);
                 break;
             }
@@ -290,7 +291,7 @@ static void virtio_crypto_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
             /* The status only occupy one byte, we can directly use it */
             s = iov_from_buf(in_iov, in_num, 0, &status, sizeof(status));
             if (unlikely(s != sizeof(status))) {
-                virtio_error(vdev, "virtio-crypto status incorrect");
+                virtqueue_error(vq, "virtio-crypto status incorrect");
                 virtqueue_detach_element(vq, elem, 0);
                 break;
             }
@@ -306,7 +307,7 @@ static void virtio_crypto_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
             stl_le_p(&input.status, VIRTIO_CRYPTO_NOTSUPP);
             s = iov_from_buf(in_iov, in_num, 0, &input, sizeof(input));
             if (unlikely(s != sizeof(input))) {
-                virtio_error(vdev, "virtio-crypto input incorrect");
+                virtqueue_error(vq, "virtio-crypto input incorrect");
                 virtqueue_detach_element(vq, elem, 0);
                 break;
             }
@@ -370,7 +371,7 @@ virtio_crypto_sym_input_data_helper(VirtIODevice *vdev,
     /* Save the cipher result */
     s = iov_from_buf(req->in_iov, req->in_num, 0, sym_op_info->dst, len);
     if (s != len) {
-        virtio_error(vdev, "virtio-crypto dest data incorrect");
+        virtqueue_error(req->vq, "virtio-crypto dest data incorrect");
         return;
     }
 
@@ -383,7 +384,7 @@ virtio_crypto_sym_input_data_helper(VirtIODevice *vdev,
                          sym_op_info->digest_result,
                          sym_op_info->digest_result_len);
         if (s != sym_op_info->digest_result_len) {
-            virtio_error(vdev, "virtio-crypto digest result incorrect");
+            virtqueue_error(req->vq, "virtio-crypto digest result incorrect");
         }
     }
 }
@@ -414,12 +415,13 @@ virtio_crypto_get_request(VirtIOCrypto *s, VirtQueue *vq)
 }
 
 static CryptoDevBackendSymOpInfo *
-virtio_crypto_sym_op_helper(VirtIODevice *vdev,
+virtio_crypto_sym_op_helper(VirtIOCryptoReq *request,
            struct virtio_crypto_cipher_para *cipher_para,
            struct virtio_crypto_alg_chain_data_para *alg_chain_para,
            struct iovec *iov, unsigned int out_num)
 {
-    VirtIOCrypto *vcrypto = VIRTIO_CRYPTO(vdev);
+    VirtIOCrypto *vcrypto = VIRTIO_CRYPTO(request->vcrypto);
+    VirtQueue *vq = request->vq;
     CryptoDevBackendSymOpInfo *op_info;
     uint32_t src_len = 0, dst_len = 0;
     uint32_t iv_len = 0;
@@ -454,7 +456,7 @@ virtio_crypto_sym_op_helper(VirtIODevice *vdev,
 
     max_len = (uint64_t)iv_len + aad_len + src_len + dst_len + hash_result_len;
     if (unlikely(max_len > vcrypto->conf.max_size)) {
-        virtio_error(vdev, "virtio-crypto too big length");
+        virtqueue_error(vq, "virtio-crypto too big length");
         return NULL;
     }
 
@@ -475,7 +477,7 @@ virtio_crypto_sym_op_helper(VirtIODevice *vdev,
 
         s = iov_to_buf(iov, out_num, 0, op_info->iv, op_info->iv_len);
         if (unlikely(s != op_info->iv_len)) {
-            virtio_error(vdev, "virtio-crypto iv incorrect");
+            virtqueue_error(vq, "virtio-crypto iv incorrect");
             goto err;
         }
         iov_discard_front(&iov, &out_num, op_info->iv_len);
@@ -489,7 +491,7 @@ virtio_crypto_sym_op_helper(VirtIODevice *vdev,
 
         s = iov_to_buf(iov, out_num, 0, op_info->aad_data, op_info->aad_len);
         if (unlikely(s != op_info->aad_len)) {
-            virtio_error(vdev, "virtio-crypto additional auth data incorrect");
+            virtqueue_error(vq, "virtio-crypto additional auth data incorrect");
             goto err;
         }
         iov_discard_front(&iov, &out_num, op_info->aad_len);
@@ -504,7 +506,7 @@ virtio_crypto_sym_op_helper(VirtIODevice *vdev,
 
         s = iov_to_buf(iov, out_num, 0, op_info->src, op_info->src_len);
         if (unlikely(s != op_info->src_len)) {
-            virtio_error(vdev, "virtio-crypto source data incorrect");
+            virtqueue_error(vq, "virtio-crypto source data incorrect");
             goto err;
         }
         iov_discard_front(&iov, &out_num, op_info->src_len);
@@ -532,26 +534,25 @@ err:
 }
 
 static int
-virtio_crypto_handle_sym_req(VirtIOCrypto *vcrypto,
+virtio_crypto_handle_sym_req(VirtIOCryptoReq *request,
                struct virtio_crypto_sym_data_req *req,
                CryptoDevBackendSymOpInfo **sym_op_info,
                struct iovec *iov, unsigned int out_num)
 {
-    VirtIODevice *vdev = VIRTIO_DEVICE(vcrypto);
     uint32_t op_type;
     CryptoDevBackendSymOpInfo *op_info;
 
     op_type = ldl_le_p(&req->op_type);
 
     if (op_type == VIRTIO_CRYPTO_SYM_OP_CIPHER) {
-        op_info = virtio_crypto_sym_op_helper(vdev, &req->u.cipher.para,
+        op_info = virtio_crypto_sym_op_helper(request, &req->u.cipher.para,
                                               NULL, iov, out_num);
         if (!op_info) {
             return -EFAULT;
         }
         op_info->op_type = op_type;
     } else if (op_type == VIRTIO_CRYPTO_SYM_OP_ALGORITHM_CHAINING) {
-        op_info = virtio_crypto_sym_op_helper(vdev, NULL,
+        op_info = virtio_crypto_sym_op_helper(request, NULL,
                                               &req->u.chain.para,
                                               iov, out_num);
         if (!op_info) {
@@ -573,7 +574,7 @@ static int
 virtio_crypto_handle_request(VirtIOCryptoReq *request)
 {
     VirtIOCrypto *vcrypto = request->vcrypto;
-    VirtIODevice *vdev = VIRTIO_DEVICE(vcrypto);
+    VirtQueue *vq = request->vq;
     VirtQueueElement *elem = &request->elem;
     int queue_index = virtio_crypto_vq2q(virtio_get_queue_index(request->vq));
     struct virtio_crypto_op_data_req req;
@@ -589,7 +590,7 @@ virtio_crypto_handle_request(VirtIOCryptoReq *request)
     Error *local_err = NULL;
 
     if (elem->out_num < 1 || elem->in_num < 1) {
-        virtio_error(vdev, "virtio-crypto dataq missing headers");
+        virtqueue_error(vq, "virtio-crypto dataq missing headers");
         return -1;
     }
 
@@ -599,14 +600,14 @@ virtio_crypto_handle_request(VirtIOCryptoReq *request)
     in_iov = elem->in_sg;
     if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
                 != sizeof(req))) {
-        virtio_error(vdev, "virtio-crypto request outhdr too short");
+        virtqueue_error(vq, "virtio-crypto request outhdr too short");
         return -1;
     }
     iov_discard_front(&out_iov, &out_num, sizeof(req));
 
     if (in_iov[in_num - 1].iov_len <
             sizeof(struct virtio_crypto_inhdr)) {
-        virtio_error(vdev, "virtio-crypto request inhdr too short");
+        virtqueue_error(vq, "virtio-crypto request inhdr too short");
         return -1;
     }
     /* We always touch the last byte, so just see how big in_iov is. */
@@ -629,7 +630,7 @@ virtio_crypto_handle_request(VirtIOCryptoReq *request)
     switch (opcode) {
     case VIRTIO_CRYPTO_CIPHER_ENCRYPT:
     case VIRTIO_CRYPTO_CIPHER_DECRYPT:
-        ret = virtio_crypto_handle_sym_req(vcrypto,
+        ret = virtio_crypto_handle_sym_req(request,
                          &req.u.sym_req,
                          &sym_op_info,
                          out_iov, out_num);
-- 
2.9.3

[Qemu-devel] [PATCH v3 9/9] checkpatch: add virtio_error and virtqueue_error to error funcs

[Ladi Prosek]

Two more error functions that should not contain newlines.

Suggested-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Ladi Prosek <lprosek@redhat.com>
---
 scripts/checkpatch.pl | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4e91122..2cd2713 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2540,7 +2540,9 @@ sub process {
 				info_vreport|
 				error_report|
 				warn_report|
-				info_report}x;
+				info_report|
+				virtio_error|
+				virtqueue_error}x;
 
 	if ($rawline =~ /\b(?:$qemu_error_funcs)\s*\(.*\".*\\n/) {
 		ERROR("Error messages should not contain newlines\n" . $herecurr);
-- 
2.9.3

Re: [Qemu-devel] [PATCH v3 8/9] virtio-crypto: use virtqueue_error for errors with queue context

[Gonglei (Arei)]

> -----Original Message-----
> From: Ladi Prosek [mailto:lprosek@redhat.com]
> Sent: Monday, July 17, 2017 4:12 PM
> To: qemu-devel@nongnu.org
> Cc: casasfernando@hotmail.com; mst@redhat.com; jasowang@redhat.com;
> cohuck@redhat.com; armbru@redhat.com; groug@kaod.org; Gonglei (Arei);
> aneesh.kumar@linux.vnet.ibm.com
> Subject: [PATCH v3 8/9] virtio-crypto: use virtqueue_error for errors with queue
> context
> 
> virtqueue_error includes the queue index in the error output and is preferred
> for errors that pertain to a virtqueue rather than to the device as a whole.
> 
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> ---
>  hw/virtio/virtio-crypto.c | 49 ++++++++++++++++++++++++-----------------------
>  1 file changed, 25 insertions(+), 24 deletions(-)
> 

Reviewed-by: Gonglei <arei.gonglei@huawei.com>

Re: [Qemu-devel] [PATCH v3 1/9] virtio: enhance virtio_error messages

[Cornelia Huck]

On Mon, 17 Jul 2017 10:11:44 +0200
Ladi Prosek <lprosek@redhat.com> wrote:

> Output like "Virtqueue size exceeded" is not much useful in identifying the

s/much/very/

> culprit. This commit adds virtio device name (e.g. "virtio-input") and id
> if set (e.g. "mouse0") to all virtio error messages to improve debuggability.
> 
> Some virtio devices (virtio-scsi, virtio-serial) insert a bus between the
> proxy object and the virtio backends, so a helper function is added to walk
> the object hierarchy and find the right proxy object to extract the id from.
> 
> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> ---
>  hw/virtio/virtio.c | 30 ++++++++++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)

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

Re: [Qemu-devel] [PATCH v3 2/9] virtio: introduce virtqueue_error

[Cornelia Huck]

On Mon, 17 Jul 2017 10:11:45 +0200
Ladi Prosek <lprosek@redhat.com> wrote:

> Most virtio error output pertains to a specific virtqueue so it makes

s/pertains/pertain/

> sense to include the queue index in error messages.
> 
> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> ---
>  hw/virtio/virtio.c         | 44 +++++++++++++++++++++++++++++++++-----------
>  include/hw/virtio/virtio.h |  1 +
>  2 files changed, 34 insertions(+), 11 deletions(-)

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

Re: [Qemu-devel] [PATCH v3 8/9] virtio-crypto: use virtqueue_error for errors with queue context

[Cornelia Huck]

On Mon, 17 Jul 2017 10:11:51 +0200
Ladi Prosek <lprosek@redhat.com> wrote:

> virtqueue_error includes the queue index in the error output and is preferred
> for errors that pertain to a virtqueue rather than to the device as a whole.
> 
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> ---
>  hw/virtio/virtio-crypto.c | 49 ++++++++++++++++++++++++-----------------------
>  1 file changed, 25 insertions(+), 24 deletions(-)

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

Re: [Qemu-devel] [PATCH v3 9/9] checkpatch: add virtio_error and virtqueue_error to error funcs

[Cornelia Huck]

On Mon, 17 Jul 2017 10:11:52 +0200
Ladi Prosek <lprosek@redhat.com> wrote:

> Two more error functions that should not contain newlines.
> 
> Suggested-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> ---
>  scripts/checkpatch.pl | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

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

Re: [Qemu-devel] [PATCH v3 9/9] checkpatch: add virtio_error and virtqueue_error to error funcs

[Markus Armbruster]

Ladi Prosek <lprosek@redhat.com> writes:

> Two more error functions that should not contain newlines.
>
> Suggested-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> ---
>  scripts/checkpatch.pl | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 4e91122..2cd2713 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2540,7 +2540,9 @@ sub process {
>  				info_vreport|
>  				error_report|
>  				warn_report|
> -				info_report}x;
> +				info_report|
> +				virtio_error|
> +				virtqueue_error}x;
>  
>  	if ($rawline =~ /\b(?:$qemu_error_funcs)\s*\(.*\".*\\n/) {
>  		ERROR("Error messages should not contain newlines\n" . $herecurr);

What makes the two functions appropriate for this list is this pattern:

    va_start(ap, fmt);
    msg = g_strdup_vprintf(fmt, ap);
    va_end(ap);

    error_report("... %s", ..., msg);

There's a related pattern using error_vreport().

Do we have more instances of these patterns?

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

Re: [Qemu-devel] [PATCH v3 9/9] checkpatch: add virtio_error and virtqueue_error to error funcs

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 370 bytes --]

On Mon, Jul 17, 2017 at 10:11:52AM +0200, Ladi Prosek wrote:
> Two more error functions that should not contain newlines.
> 
> Suggested-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> ---
>  scripts/checkpatch.pl | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v3 8/9] virtio-crypto: use virtqueue_error for errors with queue context

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 476 bytes --]

On Mon, Jul 17, 2017 at 10:11:51AM +0200, Ladi Prosek wrote:
> virtqueue_error includes the queue index in the error output and is preferred
> for errors that pertain to a virtqueue rather than to the device as a whole.
> 
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> ---
>  hw/virtio/virtio-crypto.c | 49 ++++++++++++++++++++++++-----------------------
>  1 file changed, 25 insertions(+), 24 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v3 0/9] virtio: enhance virtio_error messages

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2674 bytes --]

On Mon, Jul 17, 2017 at 10:11:43AM +0200, Ladi Prosek wrote:
> Output like "Virtqueue size exceeded" is not much useful in identifying the
> culprit. This series beefs up virtio_error to print the virtio device name
> and id, and introduces virtqueue_error which additionally includes the index
> of the virtqueue where the error occured.
> 
> Patches 1 to 3 lay the groundwork, patches 4 to 8 convert virtio devices to
> use virtqueue_error instead of virtio_error. Patch 9 adds virtio_error and
> virtqueue_error to the list of error funcs in checkpatch.pl.
> 
> v1->v2:
> * Modified virtio_error and added virtqueue_error (Stefan)
> * Now also printing device id (Stefan)
> * Went over all virtio_error call sites and converted them to virtqueue_error
>   as appropriate; added virtio device maintainers to cc
> 
> v2->v3:
> * Removed patch 1 (Stefan, Markus)
> * Split patch 3 into 2 (adds virtqueue_error) and 3 (makes virtio.c call it)
>   (Cornelia)
> * Added patch 9 to modify $qemu_error_funcs in checkpatch.pl (Greg)
> * s/includes queue index/includes the queue index/ in patch 3-9 commit
>   messages (Cornelia)
> * Fixed virtio_get_device_id to return empty string instead of NULL if the
>   device doesn't have an id (Stefan)
> * Simplified the change in virtio-crypto.c to use vcrypto->ctrl_vq instead
>   of propagating the vq pointer in function arguments (Cornelia, Gonglei)
> 
> Ladi Prosek (9):
>   virtio: enhance virtio_error messages
>   virtio: introduce virtqueue_error
>   virtio: use virtqueue_error for errors with queue context
>   virtio-9p: use virtqueue_error for errors with queue context
>   virtio-blk: use virtqueue_error for errors with queue context
>   virtio-net: use virtqueue_error for errors with queue context
>   virtio-scsi: use virtqueue_error for errors with queue context
>   virtio-crypto: use virtqueue_error for errors with queue context
>   checkpatch: add virtio_error and virtqueue_error to error funcs
> 
>  hw/9pfs/virtio-9p-device.c |  37 ++++++--------
>  hw/block/virtio-blk.c      |   6 +--
>  hw/net/virtio-net.c        |  24 ++++-----
>  hw/scsi/virtio-scsi.c      |   2 +-
>  hw/virtio/virtio-crypto.c  |  49 ++++++++++---------
>  hw/virtio/virtio.c         | 119 +++++++++++++++++++++++++++++++--------------
>  include/hw/virtio/virtio.h |   1 +
>  scripts/checkpatch.pl      |   4 +-
>  8 files changed, 143 insertions(+), 99 deletions(-)

Looks good.  QEMU is currently in freeze for the 2.10 release.  You may
need to resend so Michael Tsirkin can merge it after the release, or if
he maintainers a -next branch he could merge it right away.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v3 9/9] checkpatch: add virtio_error and virtqueue_error to error funcs

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 880 bytes --]

On Mon, 17 Jul 2017 10:11:52 +0200
Ladi Prosek <lprosek@redhat.com> wrote:

> Two more error functions that should not contain newlines.
> 
> Suggested-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  scripts/checkpatch.pl | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 4e91122..2cd2713 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2540,7 +2540,9 @@ sub process {
>  				info_vreport|
>  				error_report|
>  				warn_report|
> -				info_report}x;
> +				info_report|
> +				virtio_error|
> +				virtqueue_error}x;
>  
>  	if ($rawline =~ /\b(?:$qemu_error_funcs)\s*\(.*\".*\\n/) {
>  		ERROR("Error messages should not contain newlines\n" . $herecurr);


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH v3 0/9] virtio: enhance virtio_error messages

[Ladi Prosek]

On Fri, Jul 21, 2017 at 5:21 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Mon, Jul 17, 2017 at 10:11:43AM +0200, Ladi Prosek wrote:
>> Output like "Virtqueue size exceeded" is not much useful in identifying the
>> culprit. This series beefs up virtio_error to print the virtio device name
>> and id, and introduces virtqueue_error which additionally includes the index
>> of the virtqueue where the error occured.
>>
>> Patches 1 to 3 lay the groundwork, patches 4 to 8 convert virtio devices to
>> use virtqueue_error instead of virtio_error. Patch 9 adds virtio_error and
>> virtqueue_error to the list of error funcs in checkpatch.pl.
>>
>> v1->v2:
>> * Modified virtio_error and added virtqueue_error (Stefan)
>> * Now also printing device id (Stefan)
>> * Went over all virtio_error call sites and converted them to virtqueue_error
>>   as appropriate; added virtio device maintainers to cc
>>
>> v2->v3:
>> * Removed patch 1 (Stefan, Markus)
>> * Split patch 3 into 2 (adds virtqueue_error) and 3 (makes virtio.c call it)
>>   (Cornelia)
>> * Added patch 9 to modify $qemu_error_funcs in checkpatch.pl (Greg)
>> * s/includes queue index/includes the queue index/ in patch 3-9 commit
>>   messages (Cornelia)
>> * Fixed virtio_get_device_id to return empty string instead of NULL if the
>>   device doesn't have an id (Stefan)
>> * Simplified the change in virtio-crypto.c to use vcrypto->ctrl_vq instead
>>   of propagating the vq pointer in function arguments (Cornelia, Gonglei)
>>
>> Ladi Prosek (9):
>>   virtio: enhance virtio_error messages
>>   virtio: introduce virtqueue_error
>>   virtio: use virtqueue_error for errors with queue context
>>   virtio-9p: use virtqueue_error for errors with queue context
>>   virtio-blk: use virtqueue_error for errors with queue context
>>   virtio-net: use virtqueue_error for errors with queue context
>>   virtio-scsi: use virtqueue_error for errors with queue context
>>   virtio-crypto: use virtqueue_error for errors with queue context
>>   checkpatch: add virtio_error and virtqueue_error to error funcs
>>
>>  hw/9pfs/virtio-9p-device.c |  37 ++++++--------
>>  hw/block/virtio-blk.c      |   6 +--
>>  hw/net/virtio-net.c        |  24 ++++-----
>>  hw/scsi/virtio-scsi.c      |   2 +-
>>  hw/virtio/virtio-crypto.c  |  49 ++++++++++---------
>>  hw/virtio/virtio.c         | 119 +++++++++++++++++++++++++++++++--------------
>>  include/hw/virtio/virtio.h |   1 +
>>  scripts/checkpatch.pl      |   4 +-
>>  8 files changed, 143 insertions(+), 99 deletions(-)
>
> Looks good.  QEMU is currently in freeze for the 2.10 release.  You may
> need to resend so Michael Tsirkin can merge it after the release, or if
> he maintainers a -next branch he could merge it right away.

Should I resend the series, Michael?

Thanks,
Ladi

Re: [Qemu-devel] [PATCH v3 0/9] virtio: enhance virtio_error messages

[Ladi Prosek]

On Thu, Sep 14, 2017 at 7:59 AM, Ladi Prosek <lprosek@redhat.com> wrote:
> On Fri, Jul 21, 2017 at 5:21 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Mon, Jul 17, 2017 at 10:11:43AM +0200, Ladi Prosek wrote:
>>> Output like "Virtqueue size exceeded" is not much useful in identifying the
>>> culprit. This series beefs up virtio_error to print the virtio device name
>>> and id, and introduces virtqueue_error which additionally includes the index
>>> of the virtqueue where the error occured.
>>>
>>> Patches 1 to 3 lay the groundwork, patches 4 to 8 convert virtio devices to
>>> use virtqueue_error instead of virtio_error. Patch 9 adds virtio_error and
>>> virtqueue_error to the list of error funcs in checkpatch.pl.
>>>
>>> v1->v2:
>>> * Modified virtio_error and added virtqueue_error (Stefan)
>>> * Now also printing device id (Stefan)
>>> * Went over all virtio_error call sites and converted them to virtqueue_error
>>>   as appropriate; added virtio device maintainers to cc
>>>
>>> v2->v3:
>>> * Removed patch 1 (Stefan, Markus)
>>> * Split patch 3 into 2 (adds virtqueue_error) and 3 (makes virtio.c call it)
>>>   (Cornelia)
>>> * Added patch 9 to modify $qemu_error_funcs in checkpatch.pl (Greg)
>>> * s/includes queue index/includes the queue index/ in patch 3-9 commit
>>>   messages (Cornelia)
>>> * Fixed virtio_get_device_id to return empty string instead of NULL if the
>>>   device doesn't have an id (Stefan)
>>> * Simplified the change in virtio-crypto.c to use vcrypto->ctrl_vq instead
>>>   of propagating the vq pointer in function arguments (Cornelia, Gonglei)
>>>
>>> Ladi Prosek (9):
>>>   virtio: enhance virtio_error messages
>>>   virtio: introduce virtqueue_error
>>>   virtio: use virtqueue_error for errors with queue context
>>>   virtio-9p: use virtqueue_error for errors with queue context
>>>   virtio-blk: use virtqueue_error for errors with queue context
>>>   virtio-net: use virtqueue_error for errors with queue context
>>>   virtio-scsi: use virtqueue_error for errors with queue context
>>>   virtio-crypto: use virtqueue_error for errors with queue context
>>>   checkpatch: add virtio_error and virtqueue_error to error funcs
>>>
>>>  hw/9pfs/virtio-9p-device.c |  37 ++++++--------
>>>  hw/block/virtio-blk.c      |   6 +--
>>>  hw/net/virtio-net.c        |  24 ++++-----
>>>  hw/scsi/virtio-scsi.c      |   2 +-
>>>  hw/virtio/virtio-crypto.c  |  49 ++++++++++---------
>>>  hw/virtio/virtio.c         | 119 +++++++++++++++++++++++++++++++--------------
>>>  include/hw/virtio/virtio.h |   1 +
>>>  scripts/checkpatch.pl      |   4 +-
>>>  8 files changed, 143 insertions(+), 99 deletions(-)
>>
>> Looks good.  QEMU is currently in freeze for the 2.10 release.  You may
>> need to resend so Michael Tsirkin can merge it after the release, or if
>> he maintainers a -next branch he could merge it right away.
>
> Should I resend the series, Michael?

Ping. The series applies cleanly on current master.