[PATCH 0/2] vhost-vdpa: improve parameters error management

[PATCH 0/2] vhost-vdpa: improve parameters error management

[Laurent Vivier]

If vhostdev is not provided QEMU crashes, and more generally
vhost-vdpa doesn't explain why parameters are wrong.

This series fixes that.

Laurent Vivier (2):
  vhost-vdpa: define and use default value for vhostdev
  vhost-vdpa: improve error reporting

 net/vhost-vdpa.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

-- 
2.26.2

[PATCH 1/2] vhost-vdpa: define and use default value for vhostdev

[Laurent Vivier]

vhostdev is defined as optional in net.json, and if not set
/dev/vhost-vdpa-0 should be used.

The default value is not set and if vhostdev is not provided
QEMU crashes with a SIGSEGV exception.

Fixes: 1e0a84ea49b6 ("vhost-vdpa: introduce vhost-vdpa net client")
Cc: lulu@redhat.com
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 net/vhost-vdpa.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index bc0e0d2d35b7..24103ef241e4 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -24,6 +24,9 @@
 #include "monitor/monitor.h"
 #include "hw/virtio/vhost.h"
 
+/* default vhostdev as defined in qapi/net.json */
+#define VHOST_VDPA_DEFAULT_VHOSTDEV "/dev/vhost-vdpa-0"
+
 /* Todo:need to add the multiqueue support here */
 typedef struct VhostVDPAState {
     NetClientState nc;
@@ -224,5 +227,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
                           (char *)name, errp)) {
         return -1;
     }
-    return net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name, opts->vhostdev);
+    return net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name,
+                               opts->has_vhostdev ?
+                               opts->vhostdev : VHOST_VDPA_DEFAULT_VHOSTDEV);
 }
-- 
2.26.2

[PATCH 2/2] vhost-vdpa: improve error reporting

[Laurent Vivier]

Use Error framework to report the id of the device and the details of
the error (vhostdev name and errno).

For instance:

 qemu-system-x86_64 ... -netdev vhost-vdpa,id=hostnet1 ...
 hostnet1: Cannot open '/dev/vhost-vdpa-0': No such file or directory

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 net/vhost-vdpa.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 24103ef241e4..8260902334ae 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -176,7 +176,8 @@ static NetClientInfo net_vhost_vdpa_info = {
 };
 
 static int net_vhost_vdpa_init(NetClientState *peer, const char *device,
-                               const char *name, const char *vhostdev)
+                               const char *name, const char *vhostdev,
+                               Error **errp)
 {
     NetClientState *nc = NULL;
     VhostVDPAState *s;
@@ -189,11 +190,15 @@ static int net_vhost_vdpa_init(NetClientState *peer, const char *device,
     s = DO_UPCAST(VhostVDPAState, nc, nc);
     vdpa_device_fd = qemu_open(vhostdev, O_RDWR);
     if (vdpa_device_fd == -1) {
-        return -errno;
+        error_setg_errno(errp, errno, "%s: Cannot open '%s'", name, vhostdev);
+        return -1;
     }
     s->vhost_vdpa.device_fd = vdpa_device_fd;
     ret = vhost_vdpa_add(nc, (void *)&s->vhost_vdpa);
-    assert(s->vhost_net);
+    if (ret == -1) {
+        error_setg(errp, "%s: Cannot add vhost-vdpa '%s'", name, vhostdev);
+        return -1;
+    }
     return ret;
 }
 
@@ -229,5 +234,6 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
     }
     return net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name,
                                opts->has_vhostdev ?
-                               opts->vhostdev : VHOST_VDPA_DEFAULT_VHOSTDEV);
+                               opts->vhostdev : VHOST_VDPA_DEFAULT_VHOSTDEV,
+                               errp);
 }
-- 
2.26.2

Re: [PATCH 0/2] vhost-vdpa: improve parameters error management

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200903185327.774708-1-lvivier@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

N/A. Internal error while reading log file



The full log is available at
http://patchew.org/logs/20200903185327.774708-1-lvivier@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 0/2] vhost-vdpa: improve parameters error management

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200903185327.774708-1-lvivier@redhat.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.






The full log is available at
http://patchew.org/logs/20200903185327.774708-1-lvivier@redhat.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 2/2] vhost-vdpa: improve error reporting

[Philippe Mathieu-Daudé]

On 9/3/20 8:53 PM, Laurent Vivier wrote:
> Use Error framework to report the id of the device and the details of
> the error (vhostdev name and errno).
> 
> For instance:
> 
>  qemu-system-x86_64 ... -netdev vhost-vdpa,id=hostnet1 ...
>  hostnet1: Cannot open '/dev/vhost-vdpa-0': No such file or directory
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  net/vhost-vdpa.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 24103ef241e4..8260902334ae 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -176,7 +176,8 @@ static NetClientInfo net_vhost_vdpa_info = {
>  };
>  
>  static int net_vhost_vdpa_init(NetClientState *peer, const char *device,
> -                               const char *name, const char *vhostdev)
> +                               const char *name, const char *vhostdev,
> +                               Error **errp)
>  {
>      NetClientState *nc = NULL;
>      VhostVDPAState *s;
> @@ -189,11 +190,15 @@ static int net_vhost_vdpa_init(NetClientState *peer, const char *device,
>      s = DO_UPCAST(VhostVDPAState, nc, nc);
>      vdpa_device_fd = qemu_open(vhostdev, O_RDWR);
>      if (vdpa_device_fd == -1) {
> -        return -errno;
> +        error_setg_errno(errp, errno, "%s: Cannot open '%s'", name, vhostdev);
> +        return -1;
>      }
>      s->vhost_vdpa.device_fd = vdpa_device_fd;
>      ret = vhost_vdpa_add(nc, (void *)&s->vhost_vdpa);
> -    assert(s->vhost_net);
> +    if (ret == -1) {
> +        error_setg(errp, "%s: Cannot add vhost-vdpa '%s'", name, vhostdev);
> +        return -1;
> +    }
>      return ret;
>  }
>  
> @@ -229,5 +234,6 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
>      }
>      return net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name,
>                                 opts->has_vhostdev ?
> -                               opts->vhostdev : VHOST_VDPA_DEFAULT_VHOSTDEV);
> +                               opts->vhostdev : VHOST_VDPA_DEFAULT_VHOSTDEV,
> +                               errp);
>  }
> 

Re: [PATCH 1/2] vhost-vdpa: define and use default value for vhostdev

[Jason Wang]

On 2020/9/4 上午2:53, Laurent Vivier wrote:
> vhostdev is defined as optional in net.json, and if not set
> /dev/vhost-vdpa-0 should be used.
>
> The default value is not set and if vhostdev is not provided
> QEMU crashes with a SIGSEGV exception.
>
> Fixes: 1e0a84ea49b6 ("vhost-vdpa: introduce vhost-vdpa net client")
> Cc: lulu@redhat.com
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>   net/vhost-vdpa.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index bc0e0d2d35b7..24103ef241e4 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -24,6 +24,9 @@
>   #include "monitor/monitor.h"
>   #include "hw/virtio/vhost.h"
>   
> +/* default vhostdev as defined in qapi/net.json */
> +#define VHOST_VDPA_DEFAULT_VHOSTDEV "/dev/vhost-vdpa-0"
> +
>   /* Todo:need to add the multiqueue support here */
>   typedef struct VhostVDPAState {
>       NetClientState nc;
> @@ -224,5 +227,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
>                             (char *)name, errp)) {
>           return -1;
>       }
> -    return net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name, opts->vhostdev);
> +    return net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name,
> +                               opts->has_vhostdev ?
> +                               opts->vhostdev : VHOST_VDPA_DEFAULT_VHOSTDEV);
>   }


Hi Laurent:

I think having a default path could introduce more confusion here.

So I post a patch to remove the default [1].

Thanks

[1] 
https://lore.kernel.org/qemu-devel/20200831082737.10983-2-jasowang@redhat.com/

Re: [PATCH 2/2] vhost-vdpa: improve error reporting

[Jason Wang]

On 2020/9/4 上午2:53, Laurent Vivier wrote:
> Use Error framework to report the id of the device and the details of
> the error (vhostdev name and errno).
>
> For instance:
>
>   qemu-system-x86_64 ... -netdev vhost-vdpa,id=hostnet1 ...
>   hostnet1: Cannot open '/dev/vhost-vdpa-0': No such file or directory
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>   net/vhost-vdpa.c | 14 ++++++++++----
>   1 file changed, 10 insertions(+), 4 deletions(-)


Hi Laurent:

If you don't mind I will add this patch to v2 of my series[1]

Thanks

[1] 
https://lore.kernel.org/qemu-devel/20200831082737.10983-1-jasowang@redhat.com/


> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 24103ef241e4..8260902334ae 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -176,7 +176,8 @@ static NetClientInfo net_vhost_vdpa_info = {
>   };
>   
>   static int net_vhost_vdpa_init(NetClientState *peer, const char *device,
> -                               const char *name, const char *vhostdev)
> +                               const char *name, const char *vhostdev,
> +                               Error **errp)
>   {
>       NetClientState *nc = NULL;
>       VhostVDPAState *s;
> @@ -189,11 +190,15 @@ static int net_vhost_vdpa_init(NetClientState *peer, const char *device,
>       s = DO_UPCAST(VhostVDPAState, nc, nc);
>       vdpa_device_fd = qemu_open(vhostdev, O_RDWR);
>       if (vdpa_device_fd == -1) {
> -        return -errno;
> +        error_setg_errno(errp, errno, "%s: Cannot open '%s'", name, vhostdev);
> +        return -1;
>       }
>       s->vhost_vdpa.device_fd = vdpa_device_fd;
>       ret = vhost_vdpa_add(nc, (void *)&s->vhost_vdpa);
> -    assert(s->vhost_net);
> +    if (ret == -1) {
> +        error_setg(errp, "%s: Cannot add vhost-vdpa '%s'", name, vhostdev);
> +        return -1;
> +    }
>       return ret;
>   }
>   
> @@ -229,5 +234,6 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
>       }
>       return net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name,
>                                  opts->has_vhostdev ?
> -                               opts->vhostdev : VHOST_VDPA_DEFAULT_VHOSTDEV);
> +                               opts->vhostdev : VHOST_VDPA_DEFAULT_VHOSTDEV,
> +                               errp);
>   }

Re: [PATCH 2/2] vhost-vdpa: improve error reporting

[Laurent Vivier]

On 09/09/2020 03:53, Jason Wang wrote:
> 
> On 2020/9/4 上午2:53, Laurent Vivier wrote:
>> Use Error framework to report the id of the device and the details of
>> the error (vhostdev name and errno).
>>
>> For instance:
>>
>>   qemu-system-x86_64 ... -netdev vhost-vdpa,id=hostnet1 ...
>>   hostnet1: Cannot open '/dev/vhost-vdpa-0': No such file or directory
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>>   net/vhost-vdpa.c | 14 ++++++++++----
>>   1 file changed, 10 insertions(+), 4 deletions(-)
> 
> 
> Hi Laurent:
> 
> If you don't mind I will add this patch to v2 of my series[1]

Yes, please. I think it could replace the PATCH 3 of your series.

Thanks,
Laurent