vdpa: fix gcc cvq_isolated uninitialized variable warning

[PATCH] vdpa: fix gcc cvq_isolated uninitialized variable warning

Posted by Stefan Hajnoczi 1 year, 2 months ago

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,
-- 
2.41.0

Re: [PATCH] vdpa: fix gcc cvq_isolated uninitialized variable warning

Posted by Jason Wang 1 year, 2 months ago

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
>

Re: [PATCH] vdpa: fix gcc cvq_isolated uninitialized variable warning

Posted by Eugenio Perez Martin 1 year, 2 months ago

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
>

Re: [PATCH] vdpa: fix gcc cvq_isolated uninitialized variable warning

Posted by Philippe Mathieu-Daudé 1 year, 2 months ago

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>

Re: [PATCH] vdpa: fix gcc cvq_isolated uninitialized variable warning

Posted by Stefan Hajnoczi 1 year, 2 months ago

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

Re: [PATCH] vdpa: fix gcc cvq_isolated uninitialized variable warning

Posted by Philippe Mathieu-Daudé 1 year, 2 months ago

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