Re: [Qemu-devel] [PATCH 03/18] virtio: Return error from virtqueue_get_head

[Michael S. Tsirkin]

On Fri, Apr 17, 2015 at 03:59:18PM +0800, Fam Zheng wrote:
> Return type is changed to int.
> 
> When data is invalid, return -EINVAL with an error.
> 
> Signed-off-by: Fam Zheng <address@hidden>

Problem is, callers can't really handle -EINVAL here, so it's adding
dead code if we just apply patches 1-9.  How about the following
refactoring?  Your patch can go on top, it doesn't add dead code then.
If you like this, feel free to include it.

--->
virtio: get rid of virtqueue_num_heads

We don't really care about # of heads, squash
virtqueue_num_heads and virtqueue_get_head together,
to get a simpler internal API.
Memory barrier logic is also simplified a bit.

Signed-off-by: Michael S. Tsirkin <address@hidden>

--

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 60d10cd..580effe 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -295,8 +295,9 @@ void virtqueue_push(VirtQueue *vq, const VirtQueueElement 
*elem,
     virtqueue_flush(vq, 1);
 }
 
-static int virtqueue_num_heads(VirtQueue *vq, unsigned int idx)
+static int virtqueue_get_head(VirtQueue *vq, unsigned int idx)
 {
+    unsigned int head;
     uint16_t num_heads = vring_avail_idx(vq) - idx;
 
     /* Check it isn't doing very strange things with descriptor numbers. */
@@ -305,18 +306,13 @@ static int virtqueue_num_heads(VirtQueue *vq, unsigned 
int idx)
                      idx, vring_avail_idx(vq));
         exit(1);
     }
-    /* On success, callers read a descriptor at vq->last_avail_idx.
-     * Make sure descriptor read does not bypass avail index read. */
-    if (num_heads) {
-        smp_rmb();
-    }
 
-    return num_heads;
-}
+    if (!num_heads) {
+        return -EAGAIN;
+    }
 
-static unsigned int virtqueue_get_head(VirtQueue *vq, unsigned int idx)
-{
-    unsigned int head;
+    /* Make sure head read below does not bypass avail index read. */
+    smp_rmb();
 
     /* Grab the next descriptor number they're advertising, and increment
      * the index we've seen. */
@@ -360,19 +356,19 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned 
int *in_bytes,
 {
     unsigned int idx;
     unsigned int total_bufs, in_total, out_total;
+    int i;
 
     idx = vq->last_avail_idx;
 
     total_bufs = in_total = out_total = 0;
-    while (virtqueue_num_heads(vq, idx)) {
+    while ((i = virtqueue_get_head(vq, idx)) >= 0) {
         VirtIODevice *vdev = vq->vdev;
         unsigned int max, num_bufs, indirect = 0;
         hwaddr desc_pa;
-        int i;
 
         max = vq->vring.num;
         num_bufs = total_bufs;
-        i = virtqueue_get_head(vq, idx++);
+        idx++;
         desc_pa = vq->vring.desc;
 
         if (vring_desc_flags(vdev, desc_pa, i) & VRING_DESC_F_INDIRECT) {
@@ -462,15 +458,17 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
     hwaddr desc_pa = vq->vring.desc;
     VirtIODevice *vdev = vq->vdev;
 
-    if (!virtqueue_num_heads(vq, vq->last_avail_idx))
+    if ((i = virtqueue_get_head(vq, vq->last_avail_idx)) < 0) {
         return 0;
+    }
 
     /* When we start there are none of either input nor output. */
     elem->out_num = elem->in_num = 0;
 
     max = vq->vring.num;
 
-    i = head = virtqueue_get_head(vq, vq->last_avail_idx++);
+    head = i;
+    vq->last_avail_idx++;
     if (virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
         vring_set_avail_event(vq, vq->last_avail_idx);
     }