Re: [PULL 05/26] vhost-vdpa: allow passing opened vhostfd to vhost-vdpa

[Peter Maydell]

On Fri, 28 Oct 2022 at 06:55, Jason Wang <jasowang@redhat.com> wrote:
>
> From: Si-Wei Liu <si-wei.liu@oracle.com>
>
> Similar to other vhost backends, vhostfd can be passed to vhost-vdpa
> backend as another parameter to instantiate vhost-vdpa net client.
> This would benefit the use case where only open file descriptors, as
> opposed to raw vhost-vdpa device paths, are accessible from the QEMU
> process.
>
> (qemu) netdev_add type=vhost-vdpa,vhostfd=61,id=vhost-vdpa1
>

> @@ -634,14 +634,29 @@ int net_init_vhost_vdpa(const Netdev *netdev, const 
> char *name,
>
>      assert(netdev->type == NET_CLIENT_DRIVER_VHOST_VDPA);
>      opts = &netdev->u.vhost_vdpa;
> -    if (!opts->vhostdev) {
> -        error_setg(errp, "vdpa character device not specified with 
> vhostdev");
> +    if (!opts->has_vhostdev && !opts->has_vhostfd) {
> +        error_setg(errp,
> +                   "vhost-vdpa: neither vhostdev= nor vhostfd= was 
> specified");
>          return -1;
>      }
>
> -    vdpa_device_fd = qemu_open(opts->vhostdev, O_RDWR, errp);
> -    if (vdpa_device_fd == -1) {
> -        return -errno;
> +    if (opts->has_vhostdev && opts->has_vhostfd) {
> +        error_setg(errp,
> +                   "vhost-vdpa: vhostdev= and vhostfd= are mutually 
> exclusive");
> +        return -1;
> +    }
> +
> +    if (opts->has_vhostdev) {
> +        vdpa_device_fd = qemu_open(opts->vhostdev, O_RDWR, errp);
> +        if (vdpa_device_fd == -1) {
> +            return -errno;
> +        }
> +    } else if (opts->has_vhostfd) {
> +        vdpa_device_fd = monitor_fd_param(monitor_cur(), opts->vhostfd, 
> errp);
> +        if (vdpa_device_fd == -1) {
> +            error_prepend(errp, "vhost-vdpa: unable to parse vhostfd: ");
> +            return -1;
> +        }
>      }

Hi; this doesn't compile with clang:


../../net/vhost-vdpa.c:654:16: error: variable 'vdpa_device_fd' is
used uninitialized whenever 'if' condition is false
[-Werror,-Wsometimes-uninitialized]
    } else if (opts->has_vhostfd) {
               ^~~~~~~~~~~~~~~~~
../../net/vhost-vdpa.c:662:33: note: uninitialized use occurs here
    r = vhost_vdpa_get_features(vdpa_device_fd, &features, errp);
                                ^~~~~~~~~~~~~~
../../net/vhost-vdpa.c:654:12: note: remove the 'if' if its condition
is always true
    } else if (opts->has_vhostfd) {
           ^~~~~~~~~~~~~~~~~~~~~~~
../../net/vhost-vdpa.c:629:23: note: initialize the variable
'vdpa_device_fd' to silence this warning
    int vdpa_device_fd;
                      ^
                       = 0
1 error generated.


(clang version 10.0.0-4ubuntu1).

It's a false positive -- the compiler doesn't manage to figure out
that the error checks further up mean that there's no code path where
vdpa_device_fd isn't initialized. Put another way, the problem is
that we check "if (opts->has_vhostfd)" when in fact that condition
must always be true.

We could fix this by for instance:

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 854ebd61ae6..b1572ea00bc 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -634,11 +634,6 @@ int net_init_vhost_vdpa(const Netdev *netdev,
const char *name,

     assert(netdev->type == NET_CLIENT_DRIVER_VHOST_VDPA);
     opts = &netdev->u.vhost_vdpa;
-    if (!opts->has_vhostdev && !opts->has_vhostfd) {
-        error_setg(errp,
-                   "vhost-vdpa: neither vhostdev= nor vhostfd= was specified");
-        return -1;
-    }

     if (opts->has_vhostdev && opts->has_vhostfd) {
         error_setg(errp,
@@ -657,6 +652,10 @@ int net_init_vhost_vdpa(const Netdev *netdev,
const char *name,
             error_prepend(errp, "vhost-vdpa: unable to parse vhostfd: ");
             return -1;
         }
+    } else {
+        error_setg(errp,
+                   "vhost-vdpa: neither vhostdev= nor vhostfd= was specified");
+        return -1;
     }

     r = vhost_vdpa_get_features(vdpa_device_fd, &features, errp);



or alternatively by

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 854ebd61ae6..e370ecb8ebd 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -651,7 +651,8 @@ int net_init_vhost_vdpa(const Netdev *netdev,
const char *name,
         if (vdpa_device_fd == -1) {
             return -errno;
         }
-    } else if (opts->has_vhostfd) {
+    } else {
+        /* has_vhostfd */
         vdpa_device_fd = monitor_fd_param(monitor_cur(), opts->vhostfd, errp);
         if (vdpa_device_fd == -1) {
             error_prepend(errp, "vhost-vdpa: unable to parse vhostfd: ");

I think I prefer the second of these.

(I'll post a patch properly in a moment I guess.)

-- PMM