| Message ID | 20201120185105.279030-17-eperezma@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | vDPA software assisted live migration | expand |
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 *.
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!
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 --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]));
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(-)