[Qemu-devel] [PATCH v2 0/8] virtio: enhance virtio_error messages

[Qemu-devel] [PATCH v2 0/8] 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.

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

Ladi Prosek (8):
  qemu-error: introduce error_report_nolf
  virtio: enhance virtio_error messages
  virtio: introduce virtqueue_error
  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

 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   |  56 ++++++++++-----------
 hw/virtio/virtio.c          | 116 ++++++++++++++++++++++++++++++--------------
 include/hw/virtio/virtio.h  |   1 +
 include/qemu/error-report.h |   3 +-
 util/qemu-error.c           |  32 +++++++++---
 9 files changed, 169 insertions(+), 108 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH v2 1/8] qemu-error: introduce error_report_nolf

[Ladi Prosek]

Callers of error_report may want to compose the error message out of multiple
separate format strings. To save them from always having to combine their
strings into one (a process that would likely involve memory allocation which
is good to avoid on error paths), new "nolf" variants of error_report and
error_vreport are introduced.

Signed-off-by: Ladi Prosek <lprosek@redhat.com>
---
 hw/virtio/virtio.c          |  3 ++-
 include/qemu/error-report.h |  3 ++-
 util/qemu-error.c           | 32 ++++++++++++++++++++++++++------
 3 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 464947f..0e76a73 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2448,8 +2448,9 @@ void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...)
     va_list ap;
 
     va_start(ap, fmt);
-    error_vreport(fmt, ap);
+    error_vreport_nolf(fmt, ap);
     va_end(ap);
+    error_printf("\n");
 
     if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
         virtio_set_status(vdev, vdev->status | VIRTIO_CONFIG_S_NEEDS_RESET);
diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index 3001865..cdee84f 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -35,7 +35,8 @@ void error_printf(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 void error_vprintf_unless_qmp(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
 void error_printf_unless_qmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 void error_set_progname(const char *argv0);
-void error_vreport(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
+void error_vreport_nolf(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
+void error_report_nolf(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 const char *error_get_progname(void);
 extern bool enable_timestamp_msg;
diff --git a/util/qemu-error.c b/util/qemu-error.c
index b331f8f..c7fd127 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -181,11 +181,13 @@ bool enable_timestamp_msg;
 /*
  * Print an error message to current monitor if we have one, else to stderr.
  * Format arguments like vsprintf().  The resulting message should be
- * a single phrase, with no newline or trailing punctuation.
- * Prepend the current location and append a newline.
+ * a single phrase, with no trailing punctuation.  The no-LF version allows
+ * additional text to be appended with error_printf() or error_vprintf().
+ * Make sure to always close with a newline after all text is printed.
+ * Prepends the current location.
  * It's wrong to call this in a QMP monitor.  Use error_setg() there.
  */
-void error_vreport(const char *fmt, va_list ap)
+void error_vreport_nolf(const char *fmt, va_list ap)
 {
     GTimeVal tv;
     gchar *timestr;
@@ -199,14 +201,31 @@ void error_vreport(const char *fmt, va_list ap)
 
     error_print_loc();
     error_vprintf(fmt, ap);
-    error_printf("\n");
+}
+
+/*
+ * Print an error message to current monitor if we have one, else to stderr.
+ * Format arguments like sprintf().  The resulting message should be a
+ * single phrase, with no trailing punctuation.  The no-LF version allows
+ * additional text to be appended with error_printf() or error_vprintf().
+ * Make sure to always close with a newline after all text is printed.
+ * Prepends the current location.
+ * It's wrong to call this in a QMP monitor.  Use error_setg() there.
+ */
+void error_report_nolf(const char *fmt, ...)
+{
+    va_list ap;
+
+    va_start(ap, fmt);
+    error_vreport_nolf(fmt, ap);
+    va_end(ap);
 }
 
 /*
  * Print an error message to current monitor if we have one, else to stderr.
  * Format arguments like sprintf().  The resulting message should be a
  * single phrase, with no newline or trailing punctuation.
- * Prepend the current location and append a newline.
+ * Prepends the current location and appends a newline.
  * It's wrong to call this in a QMP monitor.  Use error_setg() there.
  */
 void error_report(const char *fmt, ...)
@@ -214,6 +233,7 @@ void error_report(const char *fmt, ...)
     va_list ap;
 
     va_start(ap, fmt);
-    error_vreport(fmt, ap);
+    error_vreport_nolf(fmt, ap);
     va_end(ap);
+    error_printf("\n");
 }
-- 
2.9.3

[Qemu-devel] [PATCH v2 2/8] 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.

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

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 0e76a73..a98f681 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,12 +2449,29 @@ 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 = qdev->parent_bus->parent;
+    }
+    return NULL;
+}
+
 void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...)
 {
     va_list ap;
 
+    error_report_nolf("%s (id=%s): ", vdev->name, virtio_get_device_id(vdev));
+
     va_start(ap, fmt);
-    error_vreport_nolf(fmt, ap);
+    error_vprintf(fmt, ap);
     va_end(ap);
     error_printf("\n");
 
-- 
2.9.3

[Qemu-devel] [PATCH v2 3/8] 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. This commit makes
all error output in virtio.c use the newly introduced virtqueue_error.

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

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index a98f681..4a4e977 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) {
@@ -2464,17 +2463,8 @@ static const char *virtio_get_device_id(VirtIODevice *vdev)
     return NULL;
 }
 
-void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...)
+static void virtio_device_set_broken(VirtIODevice *vdev)
 {
-    va_list ap;
-
-    error_report_nolf("%s (id=%s): ", vdev->name, virtio_get_device_id(vdev));
-
-    va_start(ap, fmt);
-    error_vprintf(fmt, ap);
-    va_end(ap);
-    error_printf("\n");
-
     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);
@@ -2483,6 +2473,36 @@ 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;
+
+    error_report_nolf("%s (id=%s): ", vdev->name, virtio_get_device_id(vdev));
+
+    va_start(ap, fmt);
+    error_vprintf(fmt, ap);
+    va_end(ap);
+    error_printf("\n");
+
+    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;
+
+    error_report_nolf("%s (id=%s) queue %d: ", vdev->name,
+                      virtio_get_device_id(vdev), vq->queue_index);
+
+    va_start(ap, fmt);
+    error_vprintf(fmt, ap);
+    va_end(ap);
+    error_printf("\n");
+
+    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 v2 4/8] virtio-9p: use virtqueue_error for errors with queue context

[Ladi Prosek]

virtqueue_error includes 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/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 v2 5/8] virtio-blk: use virtqueue_error for errors with queue context

[Ladi Prosek]

virtqueue_error includes 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/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 c0bd247..e82d2a3 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 v2 6/8] virtio-net: use virtqueue_error for errors with queue context

[Ladi Prosek]

virtqueue_error includes 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/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 v2 7/8] virtio-scsi: use virtqueue_error for errors with queue context

[Ladi Prosek]

virtqueue_error includes 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/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 d076fe7..a633d51 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 v2 8/8] virtio-crypto: use virtqueue_error for errors with queue context

[Ladi Prosek]

virtqueue_error includes 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 | 56 ++++++++++++++++++++++++-----------------------
 1 file changed, 29 insertions(+), 27 deletions(-)

diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
index 0353eb6..08e6c03 100644
--- a/hw/virtio/virtio-crypto.c
+++ b/hw/virtio/virtio-crypto.c
@@ -35,6 +35,7 @@ static inline int virtio_crypto_vq2q(int queue_index)
 
 static int
 virtio_crypto_cipher_session_helper(VirtIODevice *vdev,
+           VirtQueue *ctrl_vq,
            CryptoDevBackendSymSessionInfo *info,
            struct virtio_crypto_cipher_session_para *cipher_para,
            struct iovec **iov, unsigned int *out_num)
@@ -61,7 +62,7 @@ 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(ctrl_vq, "virtio-crypto cipher key incorrect");
             return -EFAULT;
         }
         iov_discard_front(iov, &num, info->key_len);
@@ -73,6 +74,7 @@ virtio_crypto_cipher_session_helper(VirtIODevice *vdev,
 
 static int64_t
 virtio_crypto_create_sym_session(VirtIOCrypto *vcrypto,
+               VirtQueue *ctrl_vq,
                struct virtio_crypto_sym_create_session_req *sess_req,
                uint32_t queue_id,
                uint32_t opcode,
@@ -92,7 +94,7 @@ virtio_crypto_create_sym_session(VirtIOCrypto *vcrypto,
     info.op_code = opcode;
 
     if (op_type == VIRTIO_CRYPTO_SYM_OP_CIPHER) {
-        ret = virtio_crypto_cipher_session_helper(vdev, &info,
+        ret = virtio_crypto_cipher_session_helper(vdev, ctrl_vq, &info,
                            &sess_req->u.cipher.para,
                            &iov, &out_num);
         if (ret < 0) {
@@ -101,7 +103,7 @@ virtio_crypto_create_sym_session(VirtIOCrypto *vcrypto,
     } else if (op_type == VIRTIO_CRYPTO_SYM_OP_ALGORITHM_CHAINING) {
         size_t s;
         /* cipher part */
-        ret = virtio_crypto_cipher_session_helper(vdev, &info,
+        ret = virtio_crypto_cipher_session_helper(vdev, ctrl_vq, &info,
                            &sess_req->u.chain.para.cipher_param,
                            &iov, &out_num);
         if (ret < 0) {
@@ -131,7 +133,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(ctrl_vq,
                           "virtio-crypto authenticated key incorrect");
                     ret = -EFAULT;
                     goto err;
@@ -229,7 +231,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 +243,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;
@@ -254,7 +256,7 @@ static void virtio_crypto_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
         switch (opcode) {
         case VIRTIO_CRYPTO_CIPHER_CREATE_SESSION:
             memset(&input, 0, sizeof(input));
-            session_id = virtio_crypto_create_sym_session(vcrypto,
+            session_id = virtio_crypto_create_sym_session(vcrypto, vq,
                              &ctrl.u.sym_create_session,
                              queue_id, opcode,
                              out_iov, out_num);
@@ -274,7 +276,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 +292,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 +308,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 +372,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 +385,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 +416,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 +457,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 +478,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 +492,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 +507,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 +535,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 +575,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 +591,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 +601,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 +631,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

Re: [Qemu-devel] [PATCH v2 1/8] qemu-error: introduce error_report_nolf

[Stefan Hajnoczi]

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

On Thu, Jul 13, 2017 at 01:02:30PM +0200, Ladi Prosek wrote:
> +/*
> + * Print an error message to current monitor if we have one, else to stderr.
> + * Format arguments like sprintf().  The resulting message should be a
> + * single phrase, with no trailing punctuation.  The no-LF version allows
> + * additional text to be appended with error_printf() or error_vprintf().
> + * Make sure to always close with a newline after all text is printed.
> + * Prepends the current location.
> + * It's wrong to call this in a QMP monitor.  Use error_setg() there.
> + */
> +void error_report_nolf(const char *fmt, ...)
> +{
> +    va_list ap;
> +
> +    va_start(ap, fmt);
> +    error_vreport_nolf(fmt, ap);
> +    va_end(ap);
>  }

Each call to this function prepends the timestamp, so it cannot really
be used for a sequence of prints in a single line.

It's a little ugly but I expected something along the lines of
g_strdup_vprintf() in virtio_error():

  char *msg;

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

  error_report("%s: %s", DEVICE(vdev)->id, msg);

  g_free(msg);

https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html#g-strdup-vprintf

Stefan

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

Re: [Qemu-devel] [PATCH v2 2/8] virtio: enhance virtio_error messages

[Stefan Hajnoczi]

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

On Thu, Jul 13, 2017 at 01:02:31PM +0200, Ladi Prosek wrote:
> +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 = qdev->parent_bus->parent;
> +    }
> +    return NULL;
> +}
> +
>  void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...)
>  {
>      va_list ap;
>  
> +    error_report_nolf("%s (id=%s): ", vdev->name, virtio_get_device_id(vdev));

virtio_get_device_id() can return NULL.  POSIX does not guarantee that
the printf(3) family functions handle "%s", NULL safely.  glibc prints
"(null)" but other libc implementations crash (e.g. Solaris).

http://pubs.opengroup.org/onlinepubs/9699919799/functions/fprintf.html

Should the return NULL above have g_assert_not_reached()?  That would
communicate the assumption that we never reach return NULL and it might
silence static checkers like Coverity but I'm not sure.

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

Re: [Qemu-devel] [PATCH v2 1/8] qemu-error: introduce error_report_nolf

[Ladi Prosek]

On Thu, Jul 13, 2017 at 3:32 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Thu, Jul 13, 2017 at 01:02:30PM +0200, Ladi Prosek wrote:
>> +/*
>> + * Print an error message to current monitor if we have one, else to stderr.
>> + * Format arguments like sprintf().  The resulting message should be a
>> + * single phrase, with no trailing punctuation.  The no-LF version allows
>> + * additional text to be appended with error_printf() or error_vprintf().
>> + * Make sure to always close with a newline after all text is printed.
>> + * Prepends the current location.
>> + * It's wrong to call this in a QMP monitor.  Use error_setg() there.
>> + */
>> +void error_report_nolf(const char *fmt, ...)
>> +{
>> +    va_list ap;
>> +
>> +    va_start(ap, fmt);
>> +    error_vreport_nolf(fmt, ap);
>> +    va_end(ap);
>>  }
>
> Each call to this function prepends the timestamp, so it cannot really
> be used for a sequence of prints in a single line.

True, the _nolf means "does not append \n" rather than "can be called
repeatedly". You would use error_printf() / error_vprintf() for
subsequent prints, as mentioned in the comment above this function.

> It's a little ugly but I expected something along the lines of
> g_strdup_vprintf() in virtio_error():
>
>   char *msg;
>
>   va_start(ap, fmt);
>   msg = g_strdup_vprintf(fmt, ap);
>   va_end(ap);
>
>   error_report("%s: %s", DEVICE(vdev)->id, msg);
>
>   g_free(msg);
>
> https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html#g-strdup-vprintf

I wanted to avoid the memory allocation, that's all. Not for
performance, obviously, but to increase the chances that it will come
through and always look the same if the process is under memory
pressure, the heap corrupted or otherwise in a bad state.

Both approaches look about the same ugly to me but I can certainly
change it to the one you're suggesting.

> Stefan

Re: [Qemu-devel] [PATCH v2 2/8] virtio: enhance virtio_error messages

[Ladi Prosek]

On Thu, Jul 13, 2017 at 3:40 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Thu, Jul 13, 2017 at 01:02:31PM +0200, Ladi Prosek wrote:
>> +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 = qdev->parent_bus->parent;
>> +    }
>> +    return NULL;
>> +}
>> +
>>  void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...)
>>  {
>>      va_list ap;
>>
>> +    error_report_nolf("%s (id=%s): ", vdev->name, virtio_get_device_id(vdev));
>
> virtio_get_device_id() can return NULL.  POSIX does not guarantee that
> the printf(3) family functions handle "%s", NULL safely.  glibc prints
> "(null)" but other libc implementations crash (e.g. Solaris).
>
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/fprintf.html
>
> Should the return NULL above have g_assert_not_reached()?  That would
> communicate the assumption that we never reach return NULL and it might
> silence static checkers like Coverity but I'm not sure.

virtio_get_device_id is expected to return NULL if the device has no
id assigned and I kind of liked the "(null)" output. I just failed to
realize that not all printf's will handle it. I'll definitely fix
this, thanks!

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

[Greg Kurz]

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

On Thu, 13 Jul 2017 13:02:33 +0200
Ladi Prosek <lprosek@redhat.com> wrote:

> virtqueue_error includes 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>

>  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;


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

Re: [Qemu-devel] [PATCH v2 0/8] virtio: enhance virtio_error messages

[Greg Kurz]

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

On Thu, 13 Jul 2017 13:02:29 +0200
Ladi Prosek <lprosek@redhat.com> 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.
> 
> 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
> 
> Ladi Prosek (8):
>   qemu-error: introduce error_report_nolf
>   virtio: enhance virtio_error messages
>   virtio: introduce virtqueue_error
>   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
> 

While here, maybe you can add all the virtio_error related functions to the
$qemu_error_funcs variable in scripts/checkpatch.pl ?

Cheers,

--
Greg

>  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   |  56 ++++++++++-----------
>  hw/virtio/virtio.c          | 116 ++++++++++++++++++++++++++++++--------------
>  include/hw/virtio/virtio.h  |   1 +
>  include/qemu/error-report.h |   3 +-
>  util/qemu-error.c           |  32 +++++++++---
>  9 files changed, 169 insertions(+), 108 deletions(-)
> 


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

Re: [Qemu-devel] [PATCH v2 3/8] virtio: introduce virtqueue_error

[Cornelia Huck]

On Thu, 13 Jul 2017 13:02:32 +0200
Ladi Prosek <lprosek@redhat.com> wrote:

> Most virtio error output pertains to a specific virtqueue so it makes
> sense to include the queue index in error messages. This commit makes
> all error output in virtio.c use the newly introduced virtqueue_error.

Split out the change introducing virtqueue_error(), as this patch is
quite large?

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

I like the change. Feel free to add

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

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

[Cornelia Huck]

On Thu, 13 Jul 2017 13:02:33 +0200
Ladi Prosek <lprosek@redhat.com> wrote:

> virtqueue_error includes queue index in the error output and is preferred

s/includes queue index/includes the queue index/

(also the other patches in this series)

> for errors that pertain to a virtqueue rather than to the device as a whole.
> 
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> ---
>  hw/9pfs/virtio-9p-device.c | 37 +++++++++++++++----------------------
>  1 file changed, 15 insertions(+), 22 deletions(-)

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

Re: [Qemu-devel] [PATCH v2 5/8] virtio-blk: use virtqueue_error for errors with queue context

[Cornelia Huck]

On Thu, 13 Jul 2017 13:02:34 +0200
Ladi Prosek <lprosek@redhat.com> wrote:

> virtqueue_error includes 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/block/virtio-blk.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

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

Re: [Qemu-devel] [PATCH v2 0/8] virtio: enhance virtio_error messages

[Ladi Prosek]

On Thu, Jul 13, 2017 at 4:36 PM, Greg Kurz <groug@kaod.org> wrote:
> On Thu, 13 Jul 2017 13:02:29 +0200
> Ladi Prosek <lprosek@redhat.com> 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.
>>
>> 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
>>
>> Ladi Prosek (8):
>>   qemu-error: introduce error_report_nolf
>>   virtio: enhance virtio_error messages
>>   virtio: introduce virtqueue_error
>>   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
>>
>
> While here, maybe you can add all the virtio_error related functions to the
> $qemu_error_funcs variable in scripts/checkpatch.pl ?

Will do, thanks!

> Cheers,
>
> --
> Greg
>
>>  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   |  56 ++++++++++-----------
>>  hw/virtio/virtio.c          | 116 ++++++++++++++++++++++++++++++--------------
>>  include/hw/virtio/virtio.h  |   1 +
>>  include/qemu/error-report.h |   3 +-
>>  util/qemu-error.c           |  32 +++++++++---
>>  9 files changed, 169 insertions(+), 108 deletions(-)
>>
>

Re: [Qemu-devel] [PATCH v2 6/8] virtio-net: use virtqueue_error for errors with queue context

[Cornelia Huck]

On Thu, 13 Jul 2017 13:02:35 +0200
Ladi Prosek <lprosek@redhat.com> wrote:

> virtqueue_error includes 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/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");

This does not add much information, as there's just one control queue
anyway, but it doesn't hurt either.

>              virtqueue_detach_element(vq, elem, 0);
>              g_free(elem);
>              break;

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

Re: [Qemu-devel] [PATCH v2 3/8] virtio: introduce virtqueue_error

[Ladi Prosek]

On Thu, Jul 13, 2017 at 4:42 PM, Cornelia Huck <cohuck@redhat.com> wrote:
> On Thu, 13 Jul 2017 13:02:32 +0200
> Ladi Prosek <lprosek@redhat.com> wrote:
>
>> Most virtio error output pertains to a specific virtqueue so it makes
>> sense to include the queue index in error messages. This commit makes
>> all error output in virtio.c use the newly introduced virtqueue_error.
>
> Split out the change introducing virtqueue_error(), as this patch is
> quite large?

Will do, thanks!

>>
>> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
>> ---
>>  hw/virtio/virtio.c         | 98 ++++++++++++++++++++++++++++------------------
>>  include/hw/virtio/virtio.h |  1 +
>>  2 files changed, 60 insertions(+), 39 deletions(-)
>
> I like the change. Feel free to add
>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [Qemu-devel] [PATCH v2 7/8] virtio-scsi: use virtqueue_error for errors with queue context

[Cornelia Huck]

On Thu, 13 Jul 2017 13:02:36 +0200
Ladi Prosek <lprosek@redhat.com> wrote:

> virtqueue_error includes 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/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 d076fe7..a633d51 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);
>  }

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

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

[Cornelia Huck]

On Thu, 13 Jul 2017 13:02:37 +0200
Ladi Prosek <lprosek@redhat.com> wrote:

> virtqueue_error includes 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 | 56 ++++++++++++++++++++++++-----------------------
>  1 file changed, 29 insertions(+), 27 deletions(-)
> 
> diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
> index 0353eb6..08e6c03 100644
> --- a/hw/virtio/virtio-crypto.c
> +++ b/hw/virtio/virtio-crypto.c
> @@ -35,6 +35,7 @@ static inline int virtio_crypto_vq2q(int queue_index)
>  
>  static int
>  virtio_crypto_cipher_session_helper(VirtIODevice *vdev,
> +           VirtQueue *ctrl_vq,
>             CryptoDevBackendSymSessionInfo *info,
>             struct virtio_crypto_cipher_session_para *cipher_para,
>             struct iovec **iov, unsigned int *out_num)
> @@ -61,7 +62,7 @@ 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(ctrl_vq, "virtio-crypto cipher key incorrect");

As virtio-crypto devices always have one control queue, I would just
use the pointer stored in the vcrypto structure. This avoids some of
the cascading function signature changes.

>              return -EFAULT;
>          }
>          iov_discard_front(iov, &num, info->key_len);

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

[Ladi Prosek]

On Thu, Jul 13, 2017 at 5:20 PM, Cornelia Huck <cohuck@redhat.com> wrote:
> On Thu, 13 Jul 2017 13:02:37 +0200
> Ladi Prosek <lprosek@redhat.com> wrote:
>
>> virtqueue_error includes 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 | 56 ++++++++++++++++++++++++-----------------------
>>  1 file changed, 29 insertions(+), 27 deletions(-)
>>
>> diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
>> index 0353eb6..08e6c03 100644
>> --- a/hw/virtio/virtio-crypto.c
>> +++ b/hw/virtio/virtio-crypto.c
>> @@ -35,6 +35,7 @@ static inline int virtio_crypto_vq2q(int queue_index)
>>
>>  static int
>>  virtio_crypto_cipher_session_helper(VirtIODevice *vdev,
>> +           VirtQueue *ctrl_vq,
>>             CryptoDevBackendSymSessionInfo *info,
>>             struct virtio_crypto_cipher_session_para *cipher_para,
>>             struct iovec **iov, unsigned int *out_num)
>> @@ -61,7 +62,7 @@ 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(ctrl_vq, "virtio-crypto cipher key incorrect");
>
> As virtio-crypto devices always have one control queue, I would just
> use the pointer stored in the vcrypto structure. This avoids some of
> the cascading function signature changes.

Will do. I wanted to be explicit to make the change more future-proof,
but maybe it is better to keep it simple. Thanks!

>>              return -EFAULT;
>>          }
>>          iov_discard_front(iov, &num, info->key_len);

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

[Cornelia Huck]

On Thu, 13 Jul 2017 17:31:59 +0200
Ladi Prosek <lprosek@redhat.com> wrote:

> On Thu, Jul 13, 2017 at 5:20 PM, Cornelia Huck <cohuck@redhat.com> wrote:
> > On Thu, 13 Jul 2017 13:02:37 +0200
> > Ladi Prosek <lprosek@redhat.com> wrote:
> >  
> >> virtqueue_error includes 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 | 56 ++++++++++++++++++++++++-----------------------
> >>  1 file changed, 29 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
> >> index 0353eb6..08e6c03 100644
> >> --- a/hw/virtio/virtio-crypto.c
> >> +++ b/hw/virtio/virtio-crypto.c
> >> @@ -35,6 +35,7 @@ static inline int virtio_crypto_vq2q(int queue_index)
> >>
> >>  static int
> >>  virtio_crypto_cipher_session_helper(VirtIODevice *vdev,
> >> +           VirtQueue *ctrl_vq,
> >>             CryptoDevBackendSymSessionInfo *info,
> >>             struct virtio_crypto_cipher_session_para *cipher_para,
> >>             struct iovec **iov, unsigned int *out_num)
> >> @@ -61,7 +62,7 @@ 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(ctrl_vq, "virtio-crypto cipher key incorrect");  
> >
> > As virtio-crypto devices always have one control queue, I would just
> > use the pointer stored in the vcrypto structure. This avoids some of
> > the cascading function signature changes.  
> 
> Will do. I wanted to be explicit to make the change more future-proof,
> but maybe it is better to keep it simple. Thanks!

I don't see a case for multiple virtio-crypto control vqs, even in the
future, but let's see what the virtio-crypto maintainer thinks.

> 
> >>              return -EFAULT;
> >>          }
> >>          iov_discard_front(iov, &num, info->key_len);  

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

[Gonglei (Arei)]

> -----Original Message-----
> From: Cornelia Huck [mailto:cohuck@redhat.com]
> Sent: Thursday, July 13, 2017 11:37 PM
> To: Ladi Prosek
> Cc: qemu-devel; Fernando Casas Schössow; Michael S. Tsirkin; Jason Wang;
> Markus Armbruster; Greg Kurz; Gonglei (Arei);
> aneesh.kumar@linux.vnet.ibm.com; Stefan Hajnoczi
> Subject: Re: [Qemu-devel] [PATCH v2 8/8] virtio-crypto: use virtqueue_error for
> errors with queue context
> 
> On Thu, 13 Jul 2017 17:31:59 +0200
> Ladi Prosek <lprosek@redhat.com> wrote:
> 
> > On Thu, Jul 13, 2017 at 5:20 PM, Cornelia Huck <cohuck@redhat.com>
> wrote:
> > > On Thu, 13 Jul 2017 13:02:37 +0200
> > > Ladi Prosek <lprosek@redhat.com> wrote:
> > >
> > >> virtqueue_error includes 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 | 56
> ++++++++++++++++++++++++-----------------------
> > >>  1 file changed, 29 insertions(+), 27 deletions(-)
> > >>
> > >> diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
> > >> index 0353eb6..08e6c03 100644
> > >> --- a/hw/virtio/virtio-crypto.c
> > >> +++ b/hw/virtio/virtio-crypto.c
> > >> @@ -35,6 +35,7 @@ static inline int virtio_crypto_vq2q(int queue_index)
> > >>
> > >>  static int
> > >>  virtio_crypto_cipher_session_helper(VirtIODevice *vdev,
> > >> +           VirtQueue *ctrl_vq,
> > >>             CryptoDevBackendSymSessionInfo *info,
> > >>             struct virtio_crypto_cipher_session_para *cipher_para,
> > >>             struct iovec **iov, unsigned int *out_num)
> > >> @@ -61,7 +62,7 @@ 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(ctrl_vq, "virtio-crypto cipher key
> incorrect");
> > >
> > > As virtio-crypto devices always have one control queue, I would just
> > > use the pointer stored in the vcrypto structure. This avoids some of
> > > the cascading function signature changes.
> >
> > Will do. I wanted to be explicit to make the change more future-proof,
> > but maybe it is better to keep it simple. Thanks!
> 
> I don't see a case for multiple virtio-crypto control vqs, even in the
> future, but let's see what the virtio-crypto maintainer thinks.
> 
Yes, Cornelia is right. Pls go ahead.

Thanks,
-Gonglei

Re: [Qemu-devel] [PATCH v2 1/8] qemu-error: introduce error_report_nolf

[Stefan Hajnoczi]

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

On Thu, Jul 13, 2017 at 03:48:02PM +0200, Ladi Prosek wrote:
> On Thu, Jul 13, 2017 at 3:32 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > On Thu, Jul 13, 2017 at 01:02:30PM +0200, Ladi Prosek wrote:
> >> +/*
> >> + * Print an error message to current monitor if we have one, else to stderr.
> >> + * Format arguments like sprintf().  The resulting message should be a
> >> + * single phrase, with no trailing punctuation.  The no-LF version allows
> >> + * additional text to be appended with error_printf() or error_vprintf().
> >> + * Make sure to always close with a newline after all text is printed.
> >> + * Prepends the current location.
> >> + * It's wrong to call this in a QMP monitor.  Use error_setg() there.
> >> + */
> >> +void error_report_nolf(const char *fmt, ...)
> >> +{
> >> +    va_list ap;
> >> +
> >> +    va_start(ap, fmt);
> >> +    error_vreport_nolf(fmt, ap);
> >> +    va_end(ap);
> >>  }
> >
> > Each call to this function prepends the timestamp, so it cannot really
> > be used for a sequence of prints in a single line.
> 
> True, the _nolf means "does not append \n" rather than "can be called
> repeatedly". You would use error_printf() / error_vprintf() for
> subsequent prints, as mentioned in the comment above this function.
> 
> > It's a little ugly but I expected something along the lines of
> > g_strdup_vprintf() in virtio_error():
> >
> >   char *msg;
> >
> >   va_start(ap, fmt);
> >   msg = g_strdup_vprintf(fmt, ap);
> >   va_end(ap);
> >
> >   error_report("%s: %s", DEVICE(vdev)->id, msg);
> >
> >   g_free(msg);
> >
> > https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html#g-strdup-vprintf
> 
> I wanted to avoid the memory allocation, that's all. Not for
> performance, obviously, but to increase the chances that it will come
> through and always look the same if the process is under memory
> pressure, the heap corrupted or otherwise in a bad state.
> 
> Both approaches look about the same ugly to me but I can certainly
> change it to the one you're suggesting.

The chance that error_report_nolf() will be used incorrectly is high.
Performance isn't a big concern for printing error messages.  That's why
I suggest a single error_report() call instead.

Stefan

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

Re: [Qemu-devel] [PATCH v2 3/8] virtio: introduce virtqueue_error

[Stefan Hajnoczi]

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

On Thu, Jul 13, 2017 at 01:02:32PM +0200, Ladi Prosek wrote:
> Most virtio error output pertains to a specific virtqueue so it makes
> sense to include the queue index in error messages. This commit makes
> all error output in virtio.c use the newly introduced virtqueue_error.
> 
> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> ---
>  hw/virtio/virtio.c         | 98 ++++++++++++++++++++++++++++------------------
>  include/hw/virtio/virtio.h |  1 +
>  2 files changed, 60 insertions(+), 39 deletions(-)

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

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

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

[Stefan Hajnoczi]

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

On Thu, Jul 13, 2017 at 01:02:33PM +0200, Ladi Prosek wrote:
> virtqueue_error includes 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/9pfs/virtio-9p-device.c | 37 +++++++++++++++----------------------
>  1 file changed, 15 insertions(+), 22 deletions(-)

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

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

Re: [Qemu-devel] [PATCH v2 5/8] virtio-blk: use virtqueue_error for errors with queue context

[Stefan Hajnoczi]

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

On Thu, Jul 13, 2017 at 01:02:34PM +0200, Ladi Prosek wrote:
> virtqueue_error includes 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/block/virtio-blk.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

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

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

Re: [Qemu-devel] [PATCH v2 6/8] virtio-net: use virtqueue_error for errors with queue context

[Stefan Hajnoczi]

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

On Thu, Jul 13, 2017 at 01:02:35PM +0200, Ladi Prosek wrote:
> virtqueue_error includes 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/net/virtio-net.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)

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

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

Re: [Qemu-devel] [PATCH v2 7/8] virtio-scsi: use virtqueue_error for errors with queue context

[Stefan Hajnoczi]

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

On Thu, Jul 13, 2017 at 01:02:36PM +0200, Ladi Prosek wrote:
> virtqueue_error includes 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/scsi/virtio-scsi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

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

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

[Stefan Hajnoczi]

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

On Thu, Jul 13, 2017 at 01:02:37PM +0200, Ladi Prosek wrote:
> virtqueue_error includes 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 | 56 ++++++++++++++++++++++++-----------------------
>  1 file changed, 29 insertions(+), 27 deletions(-)

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

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

Re: [Qemu-devel] [PATCH v2 1/8] qemu-error: introduce error_report_nolf

[Daniel P. Berrange]

On Thu, Jul 13, 2017 at 02:32:06PM +0100, Stefan Hajnoczi wrote:
> On Thu, Jul 13, 2017 at 01:02:30PM +0200, Ladi Prosek wrote:
> > +/*
> > + * Print an error message to current monitor if we have one, else to stderr.
> > + * Format arguments like sprintf().  The resulting message should be a
> > + * single phrase, with no trailing punctuation.  The no-LF version allows
> > + * additional text to be appended with error_printf() or error_vprintf().
> > + * Make sure to always close with a newline after all text is printed.
> > + * Prepends the current location.
> > + * It's wrong to call this in a QMP monitor.  Use error_setg() there.
> > + */
> > +void error_report_nolf(const char *fmt, ...)
> > +{
> > +    va_list ap;
> > +
> > +    va_start(ap, fmt);
> > +    error_vreport_nolf(fmt, ap);
> > +    va_end(ap);
> >  }
> 
> Each call to this function prepends the timestamp, so it cannot really
> be used for a sequence of prints in a single line.
> 
> It's a little ugly but I expected something along the lines of
> g_strdup_vprintf() in virtio_error():
> 
>   char *msg;
> 
>   va_start(ap, fmt);
>   msg = g_strdup_vprintf(fmt, ap);
>   va_end(ap);
> 
>   error_report("%s: %s", DEVICE(vdev)->id, msg);
> 
>   g_free(msg);

You could get the same thing by turning virtio_Error into a macro with
a few games. Rename the current method to virtio_error_impl() and then
define:

  #define virtio_error(dev, fmt, ...) \
     virtio_error_impl(dev, "%s: " fmt, DEVICE(dev)->id, __VA_ARGS__)

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 1/8] qemu-error: introduce error_report_nolf

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> On Thu, Jul 13, 2017 at 03:48:02PM +0200, Ladi Prosek wrote:
>> On Thu, Jul 13, 2017 at 3:32 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>> > On Thu, Jul 13, 2017 at 01:02:30PM +0200, Ladi Prosek wrote:
>> >> +/*
>> >> + * Print an error message to current monitor if we have one, else to stderr.
>> >> + * Format arguments like sprintf().  The resulting message should be a
>> >> + * single phrase, with no trailing punctuation.  The no-LF version allows
>> >> + * additional text to be appended with error_printf() or error_vprintf().
>> >> + * Make sure to always close with a newline after all text is printed.
>> >> + * Prepends the current location.
>> >> + * It's wrong to call this in a QMP monitor.  Use error_setg() there.
>> >> + */
>> >> +void error_report_nolf(const char *fmt, ...)
>> >> +{
>> >> +    va_list ap;
>> >> +
>> >> +    va_start(ap, fmt);
>> >> +    error_vreport_nolf(fmt, ap);
>> >> +    va_end(ap);
>> >>  }
>> >
>> > Each call to this function prepends the timestamp, so it cannot really
>> > be used for a sequence of prints in a single line.
>> 
>> True, the _nolf means "does not append \n" rather than "can be called
>> repeatedly". You would use error_printf() / error_vprintf() for
>> subsequent prints, as mentioned in the comment above this function.
>> 
>> > It's a little ugly but I expected something along the lines of
>> > g_strdup_vprintf() in virtio_error():
>> >
>> >   char *msg;
>> >
>> >   va_start(ap, fmt);
>> >   msg = g_strdup_vprintf(fmt, ap);
>> >   va_end(ap);
>> >
>> >   error_report("%s: %s", DEVICE(vdev)->id, msg);
>> >
>> >   g_free(msg);
>> >
>> > https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html#g-strdup-vprintf
>> 
>> I wanted to avoid the memory allocation, that's all. Not for
>> performance, obviously, but to increase the chances that it will come
>> through and always look the same if the process is under memory
>> pressure, the heap corrupted or otherwise in a bad state.
>> 
>> Both approaches look about the same ugly to me but I can certainly
>> change it to the one you're suggesting.
>
> The chance that error_report_nolf() will be used incorrectly is high.

Concur.

When malloc() starts to fail or crash, the process is basically screwed.
This should be rare.  Keeping error paths simple to improve the chances
of a useful error message getting through before the process dies is a
laudable idea anyway.  But:

* When timestamps are enabled, error_vreport() already allocates.

* When error messages go to the monitor, monitor_vprintf() already
  allocates().

We could get rid of both allocations with some effort.  Wouldn't affect
the interface.  Until we do, avoiding another small allocation is
unlikely to help.

> Performance isn't a big concern for printing error messages.  That's why
> I suggest a single error_report() call instead.

Re: [Qemu-devel] [PATCH v2 1/8] qemu-error: introduce error_report_nolf

[Ladi Prosek]

On Sat, Jul 15, 2017 at 7:50 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
>
>> On Thu, Jul 13, 2017 at 03:48:02PM +0200, Ladi Prosek wrote:
>>> On Thu, Jul 13, 2017 at 3:32 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>>> > On Thu, Jul 13, 2017 at 01:02:30PM +0200, Ladi Prosek wrote:
>>> >> +/*
>>> >> + * Print an error message to current monitor if we have one, else to stderr.
>>> >> + * Format arguments like sprintf().  The resulting message should be a
>>> >> + * single phrase, with no trailing punctuation.  The no-LF version allows
>>> >> + * additional text to be appended with error_printf() or error_vprintf().
>>> >> + * Make sure to always close with a newline after all text is printed.
>>> >> + * Prepends the current location.
>>> >> + * It's wrong to call this in a QMP monitor.  Use error_setg() there.
>>> >> + */
>>> >> +void error_report_nolf(const char *fmt, ...)
>>> >> +{
>>> >> +    va_list ap;
>>> >> +
>>> >> +    va_start(ap, fmt);
>>> >> +    error_vreport_nolf(fmt, ap);
>>> >> +    va_end(ap);
>>> >>  }
>>> >
>>> > Each call to this function prepends the timestamp, so it cannot really
>>> > be used for a sequence of prints in a single line.
>>>
>>> True, the _nolf means "does not append \n" rather than "can be called
>>> repeatedly". You would use error_printf() / error_vprintf() for
>>> subsequent prints, as mentioned in the comment above this function.
>>>
>>> > It's a little ugly but I expected something along the lines of
>>> > g_strdup_vprintf() in virtio_error():
>>> >
>>> >   char *msg;
>>> >
>>> >   va_start(ap, fmt);
>>> >   msg = g_strdup_vprintf(fmt, ap);
>>> >   va_end(ap);
>>> >
>>> >   error_report("%s: %s", DEVICE(vdev)->id, msg);
>>> >
>>> >   g_free(msg);
>>> >
>>> > https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html#g-strdup-vprintf
>>>
>>> I wanted to avoid the memory allocation, that's all. Not for
>>> performance, obviously, but to increase the chances that it will come
>>> through and always look the same if the process is under memory
>>> pressure, the heap corrupted or otherwise in a bad state.
>>>
>>> Both approaches look about the same ugly to me but I can certainly
>>> change it to the one you're suggesting.
>>
>> The chance that error_report_nolf() will be used incorrectly is high.
>
> Concur.
>
> When malloc() starts to fail or crash, the process is basically screwed.
> This should be rare.  Keeping error paths simple to improve the chances
> of a useful error message getting through before the process dies is a
> laudable idea anyway.  But:
>
> * When timestamps are enabled, error_vreport() already allocates.
>
> * When error messages go to the monitor, monitor_vprintf() already
>   allocates().
>
> We could get rid of both allocations with some effort.  Wouldn't affect
> the interface.  Until we do, avoiding another small allocation is
> unlikely to help.

Makes sense. v3 will have what you guys suggest. Thanks!

>> Performance isn't a big concern for printing error messages.  That's why
>> I suggest a single error_report() call instead.

Re: [Qemu-devel] [PATCH v2 1/8] qemu-error: introduce error_report_nolf

[Ladi Prosek]

On Fri, Jul 14, 2017 at 12:41 PM, Daniel P. Berrange
<berrange@redhat.com> wrote:
> On Thu, Jul 13, 2017 at 02:32:06PM +0100, Stefan Hajnoczi wrote:
>> On Thu, Jul 13, 2017 at 01:02:30PM +0200, Ladi Prosek wrote:
>> > +/*
>> > + * Print an error message to current monitor if we have one, else to stderr.
>> > + * Format arguments like sprintf().  The resulting message should be a
>> > + * single phrase, with no trailing punctuation.  The no-LF version allows
>> > + * additional text to be appended with error_printf() or error_vprintf().
>> > + * Make sure to always close with a newline after all text is printed.
>> > + * Prepends the current location.
>> > + * It's wrong to call this in a QMP monitor.  Use error_setg() there.
>> > + */
>> > +void error_report_nolf(const char *fmt, ...)
>> > +{
>> > +    va_list ap;
>> > +
>> > +    va_start(ap, fmt);
>> > +    error_vreport_nolf(fmt, ap);
>> > +    va_end(ap);
>> >  }
>>
>> Each call to this function prepends the timestamp, so it cannot really
>> be used for a sequence of prints in a single line.
>>
>> It's a little ugly but I expected something along the lines of
>> g_strdup_vprintf() in virtio_error():
>>
>>   char *msg;
>>
>>   va_start(ap, fmt);
>>   msg = g_strdup_vprintf(fmt, ap);
>>   va_end(ap);
>>
>>   error_report("%s: %s", DEVICE(vdev)->id, msg);
>>
>>   g_free(msg);
>
> You could get the same thing by turning virtio_Error into a macro with
> a few games. Rename the current method to virtio_error_impl() and then
> define:
>
>   #define virtio_error(dev, fmt, ...) \
>      virtio_error_impl(dev, "%s: " fmt, DEVICE(dev)->id, __VA_ARGS__)

Neat! I think I'll stick with a function though. This doesn't allocate
but it adds a little bit of code to each call site which has the
potential of slowing down the fast no-error path (I have no data, just
the general keeping-the-code-compact-is-good principle). Holler if you
disagree!

> Regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 1/8] qemu-error: introduce error_report_nolf

[Daniel P. Berrange]

On Mon, Jul 17, 2017 at 08:54:18AM +0200, Ladi Prosek wrote:
> On Fri, Jul 14, 2017 at 12:41 PM, Daniel P. Berrange
> <berrange@redhat.com> wrote:
> > On Thu, Jul 13, 2017 at 02:32:06PM +0100, Stefan Hajnoczi wrote:
> >> On Thu, Jul 13, 2017 at 01:02:30PM +0200, Ladi Prosek wrote:
> >> > +/*
> >> > + * Print an error message to current monitor if we have one, else to stderr.
> >> > + * Format arguments like sprintf().  The resulting message should be a
> >> > + * single phrase, with no trailing punctuation.  The no-LF version allows
> >> > + * additional text to be appended with error_printf() or error_vprintf().
> >> > + * Make sure to always close with a newline after all text is printed.
> >> > + * Prepends the current location.
> >> > + * It's wrong to call this in a QMP monitor.  Use error_setg() there.
> >> > + */
> >> > +void error_report_nolf(const char *fmt, ...)
> >> > +{
> >> > +    va_list ap;
> >> > +
> >> > +    va_start(ap, fmt);
> >> > +    error_vreport_nolf(fmt, ap);
> >> > +    va_end(ap);
> >> >  }
> >>
> >> Each call to this function prepends the timestamp, so it cannot really
> >> be used for a sequence of prints in a single line.
> >>
> >> It's a little ugly but I expected something along the lines of
> >> g_strdup_vprintf() in virtio_error():
> >>
> >>   char *msg;
> >>
> >>   va_start(ap, fmt);
> >>   msg = g_strdup_vprintf(fmt, ap);
> >>   va_end(ap);
> >>
> >>   error_report("%s: %s", DEVICE(vdev)->id, msg);
> >>
> >>   g_free(msg);
> >
> > You could get the same thing by turning virtio_Error into a macro with
> > a few games. Rename the current method to virtio_error_impl() and then
> > define:
> >
> >   #define virtio_error(dev, fmt, ...) \
> >      virtio_error_impl(dev, "%s: " fmt, DEVICE(dev)->id, __VA_ARGS__)
> 
> Neat! I think I'll stick with a function though. This doesn't allocate
> but it adds a little bit of code to each call site which has the
> potential of slowing down the fast no-error path (I have no data, just
> the general keeping-the-code-compact-is-good principle). Holler if you
> disagree!

IMHO that would be uneccessary optimization, particular since this is in
the error scenario and so is not performance critical to normal operation.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 1/8] qemu-error: introduce error_report_nolf

[Ladi Prosek]

On Mon, Jul 17, 2017 at 10:58 AM, Daniel P. Berrange
<berrange@redhat.com> wrote:
> On Mon, Jul 17, 2017 at 08:54:18AM +0200, Ladi Prosek wrote:
>> On Fri, Jul 14, 2017 at 12:41 PM, Daniel P. Berrange
>> <berrange@redhat.com> wrote:
>> > On Thu, Jul 13, 2017 at 02:32:06PM +0100, Stefan Hajnoczi wrote:
>> >> On Thu, Jul 13, 2017 at 01:02:30PM +0200, Ladi Prosek wrote:
>> >> > +/*
>> >> > + * Print an error message to current monitor if we have one, else to stderr.
>> >> > + * Format arguments like sprintf().  The resulting message should be a
>> >> > + * single phrase, with no trailing punctuation.  The no-LF version allows
>> >> > + * additional text to be appended with error_printf() or error_vprintf().
>> >> > + * Make sure to always close with a newline after all text is printed.
>> >> > + * Prepends the current location.
>> >> > + * It's wrong to call this in a QMP monitor.  Use error_setg() there.
>> >> > + */
>> >> > +void error_report_nolf(const char *fmt, ...)
>> >> > +{
>> >> > +    va_list ap;
>> >> > +
>> >> > +    va_start(ap, fmt);
>> >> > +    error_vreport_nolf(fmt, ap);
>> >> > +    va_end(ap);
>> >> >  }
>> >>
>> >> Each call to this function prepends the timestamp, so it cannot really
>> >> be used for a sequence of prints in a single line.
>> >>
>> >> It's a little ugly but I expected something along the lines of
>> >> g_strdup_vprintf() in virtio_error():
>> >>
>> >>   char *msg;
>> >>
>> >>   va_start(ap, fmt);
>> >>   msg = g_strdup_vprintf(fmt, ap);
>> >>   va_end(ap);
>> >>
>> >>   error_report("%s: %s", DEVICE(vdev)->id, msg);
>> >>
>> >>   g_free(msg);
>> >
>> > You could get the same thing by turning virtio_Error into a macro with
>> > a few games. Rename the current method to virtio_error_impl() and then
>> > define:
>> >
>> >   #define virtio_error(dev, fmt, ...) \
>> >      virtio_error_impl(dev, "%s: " fmt, DEVICE(dev)->id, __VA_ARGS__)
>>
>> Neat! I think I'll stick with a function though. This doesn't allocate
>> but it adds a little bit of code to each call site which has the
>> potential of slowing down the fast no-error path (I have no data, just
>> the general keeping-the-code-compact-is-good principle). Holler if you
>> disagree!
>
> IMHO that would be uneccessary optimization, particular since this is in
> the error scenario and so is not performance critical to normal operation.

Yeah, what I mean is that code getting bigger may have negative impact
even if it doesn't execute - it takes up space in caches and such.
Maybe it's an overkill but some of this common virtio code is pretty
low-level and every cache line counts. Actually, I'm tempted to wrap
the error conditions in virtio.c in unlikely() so the compiler knows
it's not part of normal operation. Thanks!

> Regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|