[Qemu-devel] Re: [PATCHv4 05/12] virtio: add APIs for queue fields

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

On Fri, Mar 05, 2010 at 05:40:18PM +0530, Amit Shah wrote:
> On (Wed) Mar 03 2010 [19:16:09], Michael S. Tsirkin wrote:
> > vhost needs physical addresses for ring and other queue fields,
> > so add APIs for these.
> 
> Already discussed on IRC, but mentioning here so that it doesn't get
> lost:
> 
> > +target_phys_addr_t virtio_queue_get_desc(VirtIODevice *vdev, int n)
> > +{
> > +    return vdev->vq[n].vring.desc;
> > +}
> > +
> > +target_phys_addr_t virtio_queue_get_avail(VirtIODevice *vdev, int n)
> > +{
> > +    return vdev->vq[n].vring.avail;
> > +}
> > +
> > +target_phys_addr_t virtio_queue_get_used(VirtIODevice *vdev, int n)
> > +{
> > +    return vdev->vq[n].vring.used;
> > +}
> > +
> > +target_phys_addr_t virtio_queue_get_ring(VirtIODevice *vdev, int n)
> > +{
> > +    return vdev->vq[n].vring.desc;
> > +}
> 
> All these functions return the start address of these fields; any better
> way to name them?
> 
> eg, just by looking at, eg, 'virtio_queue_get_used()', I'd expect that
> the function returns the number of used buffers in the ring, not the
> start address of the used buffers.
> 
> > +target_phys_addr_t virtio_queue_get_desc_size(VirtIODevice *vdev, int n)
> > +{
> > +    return sizeof(VRingDesc) * vdev->vq[n].vring.num;
> > +}
> > +
> > +target_phys_addr_t virtio_queue_get_avail_size(VirtIODevice *vdev, int n)
> > +{
> > +    return offsetof(VRingAvail, ring) +
> > +        sizeof(u_int64_t) * vdev->vq[n].vring.num;
> > +}
> > +
> > +target_phys_addr_t virtio_queue_get_used_size(VirtIODevice *vdev, int n)
> > +{
> > +    return offsetof(VRingUsed, ring) +
> > +        sizeof(VRingUsedElem) * vdev->vq[n].vring.num;
> > +}
> > +
> > +
> 
> Extra newline
> 
> > +target_phys_addr_t virtio_queue_get_ring_size(VirtIODevice *vdev, int n)
> > +{
> > +    return vdev->vq[n].vring.used - vdev->vq[n].vring.desc +
> > +	    virtio_queue_get_used_size(vdev, n);
> > +}
> > +
> > +uint16_t virtio_queue_last_avail_idx(VirtIODevice *vdev, int n)
> > +{
> > +    return vdev->vq[n].last_avail_idx;
> > +}
> > +
> > +void virtio_queue_set_last_avail_idx(VirtIODevice *vdev, int n, uint16_t idx)
> > +{
> > +    vdev->vq[n].last_avail_idx = idx;
> > +}
> 
> virtio_queue_last_avail_idx() does make sense, but since you have a
> 'set_last_avail_idx', better name the previous one 'get_..'?
> 
> > +VirtQueue *virtio_queue(VirtIODevice *vdev, int n)
> > +{
> > +    return vdev->vq + n;
> > +}
> 
> This really doesn't mean anything; I suggest virtio_queue_get_vq().
> 
> > +EventNotifier *virtio_queue_guest_notifier(VirtQueue *vq)
> > +{
> > +    return &vq->guest_notifier;
> > +}
> > +EventNotifier *virtio_queue_host_notifier(VirtQueue *vq)
> > +{
> > +    return &vq->host_notifier;
> > +}
> 
> Why drop the 'get_' for these functions?
> 
> virtio_queue_get_guest_notifier()
> and
> virtio_queue_get_host_notifier()
> 
> might be better.
> 
> 		Amit

Not sure we want 'get' all around, but I'll think about the names, thanks!

-- 
MST