[Qemu-devel] Re: [PATCHv4 09/12] vhost: vhost net support

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Mar 05, 2010 at 11:49:11PM +0530, Amit Shah wrote:
> On (Wed) Mar 03 2010 [19:16:35], Michael S. Tsirkin wrote:
> 
> > +static int vhost_virtqueue_init(struct vhost_dev *dev,
> > +                                struct VirtIODevice *vdev,
> > +                                struct vhost_virtqueue *vq,
> > +                                unsigned idx)
> > +{
> > +    target_phys_addr_t s, l, a;
> > +    int r;
> > +    struct vhost_vring_file file = {
> > +        .index = idx,
> > +    };
> > +    struct vhost_vring_state state = {
> > +        .index = idx,
> > +    };
> > +    struct VirtQueue *q = virtio_queue(vdev, idx);
> 
> Why depart from using 'vq' for VirtQueue? Why not use 'hvq' for
> vhost_virtqueue? That'll make reading through this code easier... Also,
> 'hvdev' for vhost_dev will be apt as well.

I'll think of better names, thanks.

> > +    vq->num = state.num = virtio_queue_get_num(vdev, idx);
> 
> I think this should be named 'virtio_queue_get_vq_num' for clarity.

This is the name upstream. If you like, send a patch to rename it :).

> > +    r = ioctl(dev->control, VHOST_SET_VRING_NUM, &state);
> > +    if (r) {
> > +        return -errno;
> > +    }
> > +
> > +    state.num = virtio_queue_last_avail_idx(vdev, idx);
> > +    r = ioctl(dev->control, VHOST_SET_VRING_BASE, &state);
> > +    if (r) {
> > +        return -errno;
> > +    }
> > +
> > +    s = l = virtio_queue_get_desc_size(vdev, idx);
> > +    a = virtio_queue_get_desc(vdev, idx);
> > +    vq->desc = cpu_physical_memory_map(a, &l, 0);
> > +    if (!vq->desc || l != s) {
> > +        r = -ENOMEM;
> > +        goto fail_alloc_desc;
> > +    }
> > +    s = l = virtio_queue_get_avail_size(vdev, idx);
> > +    a = virtio_queue_get_avail(vdev, idx);
> > +    vq->avail = cpu_physical_memory_map(a, &l, 0);
> > +    if (!vq->avail || l != s) {
> > +        r = -ENOMEM;
> > +        goto fail_alloc_avail;
> > +    }
> > +    vq->used_size = s = l = virtio_queue_get_used_size(vdev, idx);
> > +    vq->used_phys = a = virtio_queue_get_used(vdev, idx);
> > +    vq->used = cpu_physical_memory_map(a, &l, 1);
> > +    if (!vq->used || l != s) {
> > +        r = -ENOMEM;
> > +        goto fail_alloc_used;
> > +    }
> > +
> > +    vq->ring_size = s = l = virtio_queue_get_ring_size(vdev, idx);
> > +    vq->ring_phys = a = virtio_queue_get_ring(vdev, idx);
> > +    vq->ring = cpu_physical_memory_map(a, &l, 1);
> > +    if (!vq->ring || l != s) {
> > +        r = -ENOMEM;
> > +        goto fail_alloc_ring;
> > +    }
> > +
> > +    r = vhost_virtqueue_set_addr(dev, vq, idx, dev->log_enabled);
> > +    if (r < 0) {
> > +        r = -errno;
> > +        goto fail_alloc;
> > +    }
> > +    if (!vdev->binding->guest_notifier || !vdev->binding->host_notifier) {
> > +        fprintf(stderr, "binding does not support irqfd/queuefd\n");
> > +        r = -ENOSYS;
> > +        goto fail_alloc;
> > +    }
> 
> This could be checked much earlier on in the function; so that we avoid
> doing all that stuff above and the cleanup.
> 
> 		Amit


Whatever order we put checks in, we'll have to undo stuff
done beforehand on error.

-- 
MST