On Fri, Sep 27, 2024 at 3:08 PM Eugenio Perez Martin
<epere...@redhat.com> wrote:
>
> On Wed, Sep 25, 2024 at 10:08 AM Stefano Garzarella <sgarz...@redhat.com>
> wrote:
> >
> > On Tue, Sep 24, 2024 at 05:05:49PM GMT, marcandre.lur...@redhat.com wrote:
> > >From: Marc-André Lureau <marcandre.lur...@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.lur...@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.
I don't think so, I mean if we embed it and check the error path
better, yes, but now in vhost_svq_get_buf() if we fail, we return
NULL, but we don't set "len" to 0, so we would have the same warning.
Thanks,
Stefano
>
> But I understand it is better to change this function than trust the
> reviews long term.
>
