From: Marc-André Lureau <marcandre.lureau@redhat.com>

../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--) {
         int64_t start_us = g_get_monotonic_time();
-- 
2.45.2.827.g557ae147e6

On Tue, Sep 24, 2024 at 3:07 PM <marcandre.lureau@redhat.com> wrote:
>
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> ../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;
>

I understand this is a bulk changeset to avoid the warning, but does
this mean we cannot use pointer arguments to just return information
anymore? vhost_svq_get_buf just write to it, it never reads it.

If you post a second version and it is convenient for you, it would be
useful to move it inside of the while.

Any way we solve it,

Acked-by: Eugenio Pérez <eperezma@redhat.com>

>      while (num--) {
>          int64_t start_us = g_get_monotonic_time();
> --
> 2.45.2.827.g557ae147e6
>

On Fri, Sep 27, 2024 at 3:05 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Tue, Sep 24, 2024 at 3:07 PM <marcandre.lureau@redhat.com> wrote:
> >
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > ../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;
> >
>
> I understand this is a bulk changeset to avoid the warning, but does
> this mean we cannot use pointer arguments to just return information
> anymore? vhost_svq_get_buf just write to it, it never reads it.

Sure we can, the problem here is that vhost_svq_get_buf() might return
without having written there (in the error path).

>
> If you post a second version and it is convenient for you, it would be
> useful to move it inside of the while.

I think it is the only way, if we keep it out we have the problem from
the second loop on (always in the error path).

>
> Any way we solve it,
>
> Acked-by: Eugenio Pérez <eperezma@redhat.com>
>
> >      while (num--) {
> >          int64_t start_us = g_get_monotonic_time();
> > --
> > 2.45.2.827.g557ae147e6
> >
>

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).

Thanks,
Stefano

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.

On Fri, Sep 27, 2024 at 3:08 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> 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.

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.
>

On Mon, Sep 30, 2024 at 10:11 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Fri, Sep 27, 2024 at 3:08 PM Eugenio Perez Martin
> <eperezma@redhat.com> wrote:
> >
> > 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.
>
> 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.
> >
>