diff mbox series

[RFC,16/27] virtio: Expose virtqueue_alloc_element

Message ID 20201120185105.279030-17-eperezma@redhat.com (mailing list archive)
State New, archived
Headers show
Series vDPA software assisted live migration | expand

Commit Message

Eugenio Perez Martin Nov. 20, 2020, 6:50 p.m. UTC
Specify VirtQueueElement * as return type makes no harm at this moment.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 include/hw/virtio/virtio.h | 2 ++
 hw/virtio/virtio.c         | 3 ++-
 2 files changed, 4 insertions(+), 1 deletion(-)

Comments

Stefan Hajnoczi Dec. 8, 2020, 8:25 a.m. UTC | #1
On Fri, Nov 20, 2020 at 07:50:54PM +0100, Eugenio Pérez wrote:
> Specify VirtQueueElement * as return type makes no harm at this moment.

The reason for the void * return type is that C implicitly converts void
pointers to pointers of any type. The function takes a size_t sz
argument so it can allocate a object of user-defined size. The idea is
that the user's struct embeds a VirtQueueElement field. Changing the
return type to VirtQueueElement * means that callers may need to
explicitly cast to the user's struct type.

It's a question of coding style but I think the void * return type
communicates what is going on better than VirtQueueElement *.
Eugenio Perez Martin Dec. 9, 2020, 6:46 p.m. UTC | #2
On Tue, Dec 8, 2020 at 9:26 AM Stefan Hajnoczi <stefanha@gmail.com> wrote:
>
> On Fri, Nov 20, 2020 at 07:50:54PM +0100, Eugenio Pérez wrote:
> > Specify VirtQueueElement * as return type makes no harm at this moment.
>
> The reason for the void * return type is that C implicitly converts void
> pointers to pointers of any type. The function takes a size_t sz
> argument so it can allocate a object of user-defined size. The idea is
> that the user's struct embeds a VirtQueueElement field. Changing the
> return type to VirtQueueElement * means that callers may need to
> explicitly cast to the user's struct type.
>
> It's a question of coding style but I think the void * return type
> communicates what is going on better than VirtQueueElement *.

Right, what I meant with that is that nobody uses that feature, but I
just re-check and I saw that contrib/vhost-user-blk actually uses it
(not checked for more uses). I think it is better just to drop this
commit.

Thanks!
Stefan Hajnoczi Dec. 10, 2020, 11:57 a.m. UTC | #3
On Wed, Dec 09, 2020 at 07:46:49PM +0100, Eugenio Perez Martin wrote:
> On Tue, Dec 8, 2020 at 9:26 AM Stefan Hajnoczi <stefanha@gmail.com> wrote:
> >
> > On Fri, Nov 20, 2020 at 07:50:54PM +0100, Eugenio Pérez wrote:
> > > Specify VirtQueueElement * as return type makes no harm at this moment.
> >
> > The reason for the void * return type is that C implicitly converts void
> > pointers to pointers of any type. The function takes a size_t sz
> > argument so it can allocate a object of user-defined size. The idea is
> > that the user's struct embeds a VirtQueueElement field. Changing the
> > return type to VirtQueueElement * means that callers may need to
> > explicitly cast to the user's struct type.
> >
> > It's a question of coding style but I think the void * return type
> > communicates what is going on better than VirtQueueElement *.
> 
> Right, what I meant with that is that nobody uses that feature, but I
> just re-check and I saw that contrib/vhost-user-blk actually uses it
> (not checked for more uses). I think it is better just to drop this
> commit.

contrib/vhost-user-blk doesn't use hw/virtio/virtio.c. The code is
similar and copy-pasted, but you are free to change this file without
affecting vontrib/vhost-user-blk :).

I still think it's clearer to make it obvious that this function
allocates an object of generic type or at least the change is purely a
question of style and probably not worth making.

Stefan
diff mbox series

Patch

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 79212141a6..ee8fe96f32 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -196,6 +196,8 @@  void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
                     unsigned int len, unsigned int idx);
 
 void virtqueue_map(VirtIODevice *vdev, VirtQueueElement *elem);
+VirtQueueElement *virtqueue_alloc_element(size_t sz, unsigned out_num,
+                                          unsigned in_num);
 void *virtqueue_pop(VirtQueue *vq, size_t sz);
 unsigned int virtqueue_drop_all(VirtQueue *vq);
 void *qemu_get_virtqueue_element(VirtIODevice *vdev, QEMUFile *f, size_t sz);
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index ad9dc5dfa7..a89525f067 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1400,7 +1400,8 @@  void virtqueue_map(VirtIODevice *vdev, VirtQueueElement *elem)
                                                                         false);
 }
 
-static void *virtqueue_alloc_element(size_t sz, unsigned out_num, unsigned in_num)
+VirtQueueElement *virtqueue_alloc_element(size_t sz, unsigned out_num,
+                                          unsigned in_num)
 {
     VirtQueueElement *elem;
     size_t in_addr_ofs = QEMU_ALIGN_UP(sz, __alignof__(elem->in_addr[0]));