On Tue, Sep 24, 2024 at 05:05:53PM GMT, marcandre.lureau@redhat.com wrote:
>From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
>vhost_svq_get_buf() may return a VirtQueueElement that should be freed.
>
>It's unclear to me if the vhost_svq_get_buf() call should always return NULL.
>
>Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>---
> hw/virtio/vhost-shadow-virtqueue.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
>diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
>index cd29cc795b..93742d9ddc 100644
>--- a/hw/virtio/vhost-shadow-virtqueue.c
>+++ b/hw/virtio/vhost-shadow-virtqueue.c
>@@ -414,6 +414,7 @@ static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq,
> return i;
> }
>
>+G_GNUC_WARN_UNUSED_RESULT
> static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
> uint32_t *len)
> {
>@@ -529,6 +530,7 @@ size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num)
> uint32_t r = 0;
>
> while (num--) {
>+ g_autofree VirtQueueElement *elem = NULL;
Yes, indeed it sounds like we should release the buffer, although from
the name of the function here, it sounds like we are just trying to
figure out if the queue has elements, so I expect there is another
function that is then called to process the buffers.
There's still a potential problem here that I pointed out in the other
patch, but I think we need Eugenio here.
> int64_t start_us = g_get_monotonic_time();
>
> do {
>@@ -541,7 +543,7 @@ size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num)
> }
> } while (true);
>
>- vhost_svq_get_buf(svq, &r);
>+ elem = vhost_svq_get_buf(svq, &r);
> len += r;
> }
>
>--
>2.45.2.827.g557ae147e6
>