| Message ID | 20240924130554.749278-19-marcandre.lureau@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | -Werror=maybe-uninitialized fixes | expand |
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 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 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: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 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. > > >
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();