Re: [PATCH v2 18/22] hw/virtio: fix -Werror=maybe-uninitialized false-positive

[Eugenio Perez Martin]

On Wed, Sep 25, 2024 at 10:08 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Tue, Sep 24, 2024 at 05:05:49PM GMT, marcandre.lureau@redhat.com wrote:
> >From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> For the title: I don't think it is a false positive, but a real fix,
> indeed maybe not a complete one.
>
> >
> >../hw/virtio/vhost-shadow-virtqueue.c:545:13: error: ‘r’ may be used 
> >uninitialized [-Werror=maybe-uninitialized]
> >
> >Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >---
> > hw/virtio/vhost-shadow-virtqueue.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/hw/virtio/vhost-shadow-virtqueue.c 
> >b/hw/virtio/vhost-shadow-virtqueue.c
> >index fc5f408f77..cd29cc795b 100644
> >--- a/hw/virtio/vhost-shadow-virtqueue.c
> >+++ b/hw/virtio/vhost-shadow-virtqueue.c
> >@@ -526,7 +526,7 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
> > size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num)
> > {
> >     size_t len = 0;
> >-    uint32_t r;
> >+    uint32_t r = 0;
> >
> >     while (num--) {
>
> I think we should move the initialization to 0 here in the loop:
>
>            uint32_t r = 0;
>
> >         int64_t start_us = g_get_monotonic_time();
>
> ...
>
>            vhost_svq_get_buf(svq, &r);
>            len += r;
>        }
>
> This because we don't check vhost_svq_get_buf() return value.
>
> IIUC, in that function, `r` is set only if the return value of
> vhost_svq_get_buf() is not null, so if we don't check its return value,
> we should set `r` to 0 on every cycle (or check the return value of
> course).
>

Sorry I missed this mail and I proposed the same :). I do think it is
a real false positive though, in the sense that if we embed the
vhost_svq_get_buf here the warning would go away.

But I understand it is better to change this function than trust the
reviews long term.