From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NnzQe-0001Os-N7 for qemu-devel@nongnu.org; Sat, 06 Mar 2010 14:12:48 -0500 Received: from [199.232.76.173] (port=47515 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NnzQe-0001Ok-1m for qemu-devel@nongnu.org; Sat, 06 Mar 2010 14:12:48 -0500 Received: from Debian-exim by monty-python.gnu.org with spam-scanned (Exim 4.60) (envelope-from ) id 1NnzQc-0000de-KB for qemu-devel@nongnu.org; Sat, 06 Mar 2010 14:12:47 -0500 Received: from mx1.redhat.com ([209.132.183.28]:22507) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NnzQb-0000dK-SA for qemu-devel@nongnu.org; Sat, 06 Mar 2010 14:12:46 -0500 Date: Sat, 6 Mar 2010 21:09:19 +0200 From: "Michael S. Tsirkin" Message-ID: <20100306190918.GD12235@redhat.com> References: <20100305121018.GC22141@amit-x200.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20100305121018.GC22141@amit-x200.redhat.com> Subject: [Qemu-devel] Re: [PATCHv4 05/12] virtio: add APIs for queue fields List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Amit Shah Cc: quintela@redhat.com, qemu-devel@nongnu.org, kraxel@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