On Mon, Sep 30, 2024 at 10:11 AM Stefano Garzarella <sgarz...@redhat.com> wrote:
>
> 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.
>
Ohh got it, I missed that path!
> Thanks,
> Stefano
>
> >
> > But I understand it is better to change this function than trust the
> > reviews long term.
> >
>
