| Message ID | 20230911215435.4156314-1-stefanha@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vdpa: fix gcc cvq_isolated uninitialized variable warning | expand |
On 11/9/23 23:54, Stefan Hajnoczi wrote: > gcc 13.2.1 emits the following warning: > > net/vhost-vdpa.c: In function ‘net_vhost_vdpa_init.constprop’: > net/vhost-vdpa.c:1394:25: error: ‘cvq_isolated’ may be used uninitialized [-Werror=maybe-uninitialized] > 1394 | s->cvq_isolated = cvq_isolated; > | ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~ > net/vhost-vdpa.c:1355:9: note: ‘cvq_isolated’ was declared here > 1355 | int cvq_isolated; > | ^~~~~~~~~~~~ > cc1: all warnings being treated as errors > > Cc: Eugenio Pérez <eperezma@redhat.com> > Cc: Michael S. Tsirkin <mst@redhat.com> > Cc: Jason Wang <jasowang@redhat.com> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > net/vhost-vdpa.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c > index 34202ca009..7eaee841aa 100644 > --- a/net/vhost-vdpa.c > +++ b/net/vhost-vdpa.c > @@ -1352,7 +1352,7 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer, > VhostVDPAState *s; > int ret = 0; > assert(name); > - int cvq_isolated; > + int cvq_isolated = 0; > > if (is_datapath) { > nc = qemu_new_net_client(&net_vhost_vdpa_info, peer, device, Alternatively: -- >8 -- diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c index 34202ca009..218fe0c305 100644 --- a/net/vhost-vdpa.c +++ b/net/vhost-vdpa.c @@ -1352,13 +1352,12 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer, VhostVDPAState *s; int ret = 0; assert(name); - int cvq_isolated; if (is_datapath) { nc = qemu_new_net_client(&net_vhost_vdpa_info, peer, device, name); } else { - cvq_isolated = vhost_vdpa_probe_cvq_isolation(vdpa_device_fd, features, + int cvq_isolated = vhost_vdpa_probe_cvq_isolation(vdpa_device_fd, features, queue_pair_index * 2, errp); if (unlikely(cvq_isolated < 0)) { @@ -1391,7 +1390,7 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer, s->vhost_vdpa.shadow_vq_ops = &vhost_vdpa_net_svq_ops; s->vhost_vdpa.shadow_vq_ops_opaque = s; - s->cvq_isolated = cvq_isolated; + s->cvq_isolated = true; --- Whichever you prefer: Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On Mon, Sep 11, 2023 at 11:54 PM Stefan Hajnoczi <stefanha@redhat.com> wrote: > > gcc 13.2.1 emits the following warning: > > net/vhost-vdpa.c: In function ‘net_vhost_vdpa_init.constprop’: > net/vhost-vdpa.c:1394:25: error: ‘cvq_isolated’ may be used uninitialized [-Werror=maybe-uninitialized] > 1394 | s->cvq_isolated = cvq_isolated; > | ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~ > net/vhost-vdpa.c:1355:9: note: ‘cvq_isolated’ was declared here > 1355 | int cvq_isolated; > | ^~~~~~~~~~~~ > cc1: all warnings being treated as errors > I think GCC is evaluating the case where (queue_pair_index == 0 && !is_datapath), which should be not possible. If it is useful we can refactor the conditionals so we don't need the variable at that level of the function. If not, Acked-by: Eugenio Pérez <eperezma@redhat.com> Thanks! > Cc: Eugenio Pérez <eperezma@redhat.com> > Cc: Michael S. Tsirkin <mst@redhat.com> > Cc: Jason Wang <jasowang@redhat.com> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > net/vhost-vdpa.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c > index 34202ca009..7eaee841aa 100644 > --- a/net/vhost-vdpa.c > +++ b/net/vhost-vdpa.c > @@ -1352,7 +1352,7 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer, > VhostVDPAState *s; > int ret = 0; > assert(name); > - int cvq_isolated; > + int cvq_isolated = 0; > > if (is_datapath) { > nc = qemu_new_net_client(&net_vhost_vdpa_info, peer, device, > -- > 2.41.0 >
On Tue, 12 Sept 2023 at 02:19, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > On 11/9/23 23:54, Stefan Hajnoczi wrote: > > gcc 13.2.1 emits the following warning: > > > > net/vhost-vdpa.c: In function ‘net_vhost_vdpa_init.constprop’: > > net/vhost-vdpa.c:1394:25: error: ‘cvq_isolated’ may be used uninitialized [-Werror=maybe-uninitialized] > > 1394 | s->cvq_isolated = cvq_isolated; > > | ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~ > > net/vhost-vdpa.c:1355:9: note: ‘cvq_isolated’ was declared here > > 1355 | int cvq_isolated; > > | ^~~~~~~~~~~~ > > cc1: all warnings being treated as errors > > > > Cc: Eugenio Pérez <eperezma@redhat.com> > > Cc: Michael S. Tsirkin <mst@redhat.com> > > Cc: Jason Wang <jasowang@redhat.com> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > --- > > net/vhost-vdpa.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c > > index 34202ca009..7eaee841aa 100644 > > --- a/net/vhost-vdpa.c > > +++ b/net/vhost-vdpa.c > > @@ -1352,7 +1352,7 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer, > > VhostVDPAState *s; > > int ret = 0; > > assert(name); > > - int cvq_isolated; > > + int cvq_isolated = 0; > > > > if (is_datapath) { > > nc = qemu_new_net_client(&net_vhost_vdpa_info, peer, device, > > Alternatively: > > -- >8 -- > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c > index 34202ca009..218fe0c305 100644 > --- a/net/vhost-vdpa.c > +++ b/net/vhost-vdpa.c > @@ -1352,13 +1352,12 @@ static NetClientState > *net_vhost_vdpa_init(NetClientState *peer, > VhostVDPAState *s; > int ret = 0; > assert(name); > - int cvq_isolated; > > if (is_datapath) { > nc = qemu_new_net_client(&net_vhost_vdpa_info, peer, device, > name); > } else { > - cvq_isolated = vhost_vdpa_probe_cvq_isolation(vdpa_device_fd, > features, > + int cvq_isolated = > vhost_vdpa_probe_cvq_isolation(vdpa_device_fd, features, > queue_pair_index > * 2, > errp); > if (unlikely(cvq_isolated < 0)) { > @@ -1391,7 +1390,7 @@ static NetClientState > *net_vhost_vdpa_init(NetClientState *peer, > > s->vhost_vdpa.shadow_vq_ops = &vhost_vdpa_net_svq_ops; > s->vhost_vdpa.shadow_vq_ops_opaque = s; > - s->cvq_isolated = cvq_isolated; > + s->cvq_isolated = true; This is incorrect because the return value of vhost_vdpa_probe_cvq_isolation() is -errno for errors, 0 for no cvq isolation, and 1 for cvq isolation. A variable is still needed to distinguish between no cvq isolation and cvq isolation. > > --- > > Whichever you prefer: > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > >
On 12/9/23 12:48, Stefan Hajnoczi wrote: > On Tue, 12 Sept 2023 at 02:19, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >> >> On 11/9/23 23:54, Stefan Hajnoczi wrote: >>> gcc 13.2.1 emits the following warning: >>> >>> net/vhost-vdpa.c: In function ‘net_vhost_vdpa_init.constprop’: >>> net/vhost-vdpa.c:1394:25: error: ‘cvq_isolated’ may be used uninitialized [-Werror=maybe-uninitialized] >>> 1394 | s->cvq_isolated = cvq_isolated; >>> | ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~ >>> net/vhost-vdpa.c:1355:9: note: ‘cvq_isolated’ was declared here >>> 1355 | int cvq_isolated; >>> | ^~~~~~~~~~~~ >>> cc1: all warnings being treated as errors >>> >>> Cc: Eugenio Pérez <eperezma@redhat.com> >>> Cc: Michael S. Tsirkin <mst@redhat.com> >>> Cc: Jason Wang <jasowang@redhat.com> >>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> >>> --- >>> net/vhost-vdpa.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c >>> index 34202ca009..7eaee841aa 100644 >>> --- a/net/vhost-vdpa.c >>> +++ b/net/vhost-vdpa.c >>> @@ -1352,7 +1352,7 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer, >>> VhostVDPAState *s; >>> int ret = 0; >>> assert(name); >>> - int cvq_isolated; >>> + int cvq_isolated = 0; >>> >>> if (is_datapath) { >>> nc = qemu_new_net_client(&net_vhost_vdpa_info, peer, device, >> >> Alternatively: >> >> -- >8 -- >> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c >> index 34202ca009..218fe0c305 100644 >> --- a/net/vhost-vdpa.c >> +++ b/net/vhost-vdpa.c >> @@ -1352,13 +1352,12 @@ static NetClientState >> *net_vhost_vdpa_init(NetClientState *peer, >> VhostVDPAState *s; >> int ret = 0; >> assert(name); >> - int cvq_isolated; >> >> if (is_datapath) { >> nc = qemu_new_net_client(&net_vhost_vdpa_info, peer, device, >> name); >> } else { >> - cvq_isolated = vhost_vdpa_probe_cvq_isolation(vdpa_device_fd, >> features, >> + int cvq_isolated = >> vhost_vdpa_probe_cvq_isolation(vdpa_device_fd, features, >> queue_pair_index >> * 2, >> errp); >> if (unlikely(cvq_isolated < 0)) { >> @@ -1391,7 +1390,7 @@ static NetClientState >> *net_vhost_vdpa_init(NetClientState *peer, >> >> s->vhost_vdpa.shadow_vq_ops = &vhost_vdpa_net_svq_ops; >> s->vhost_vdpa.shadow_vq_ops_opaque = s; >> - s->cvq_isolated = cvq_isolated; >> + s->cvq_isolated = true; > > This is incorrect because the return value of > vhost_vdpa_probe_cvq_isolation() is -errno for errors, 0 for no cvq > isolation, and 1 for cvq isolation. A variable is still needed to > distinguish between no cvq isolation and cvq isolation. Oh, I was expecting a boolean for a field named 'cvq_isolated', my bad. R-b stands for your patch. Regards, Phil. > >> >> --- >> >> Whichever you prefer: >> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> >>
On Tue, Sep 12, 2023 at 5:54 AM Stefan Hajnoczi <stefanha@redhat.com> wrote: > > gcc 13.2.1 emits the following warning: > > net/vhost-vdpa.c: In function ‘net_vhost_vdpa_init.constprop’: > net/vhost-vdpa.c:1394:25: error: ‘cvq_isolated’ may be used uninitialized [-Werror=maybe-uninitialized] > 1394 | s->cvq_isolated = cvq_isolated; > | ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~ > net/vhost-vdpa.c:1355:9: note: ‘cvq_isolated’ was declared here > 1355 | int cvq_isolated; > | ^~~~~~~~~~~~ > cc1: all warnings being treated as errors > > Cc: Eugenio Pérez <eperezma@redhat.com> > Cc: Michael S. Tsirkin <mst@redhat.com> > Cc: Jason Wang <jasowang@redhat.com> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Acked-by: Jason Wang <jasowang@redhat.com> Thanks > --- > net/vhost-vdpa.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c > index 34202ca009..7eaee841aa 100644 > --- a/net/vhost-vdpa.c > +++ b/net/vhost-vdpa.c > @@ -1352,7 +1352,7 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer, > VhostVDPAState *s; > int ret = 0; > assert(name); > - int cvq_isolated; > + int cvq_isolated = 0; > > if (is_datapath) { > nc = qemu_new_net_client(&net_vhost_vdpa_info, peer, device, > -- > 2.41.0 >
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c index 34202ca009..7eaee841aa 100644 --- a/net/vhost-vdpa.c +++ b/net/vhost-vdpa.c @@ -1352,7 +1352,7 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer, VhostVDPAState *s; int ret = 0; assert(name); - int cvq_isolated; + int cvq_isolated = 0; if (is_datapath) { nc = qemu_new_net_client(&net_vhost_vdpa_info, peer, device,
gcc 13.2.1 emits the following warning: net/vhost-vdpa.c: In function ‘net_vhost_vdpa_init.constprop’: net/vhost-vdpa.c:1394:25: error: ‘cvq_isolated’ may be used uninitialized [-Werror=maybe-uninitialized] 1394 | s->cvq_isolated = cvq_isolated; | ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~ net/vhost-vdpa.c:1355:9: note: ‘cvq_isolated’ was declared here 1355 | int cvq_isolated; | ^~~~~~~~~~~~ cc1: all warnings being treated as errors Cc: Eugenio Pérez <eperezma@redhat.com> Cc: Michael S. Tsirkin <mst@redhat.com> Cc: Jason Wang <jasowang@redhat.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- net/vhost-vdpa.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)