| Message ID | 20200831082737.10983-1-jasowang@redhat.com |
|---|---|
| Headers | show |
| Series | refine vhost-vdpa initialization | expand |
On 31/08/2020 10:27, Jason Wang wrote: > vhost-dev is mandatory for vhost-vdpa to be initialized otherwise > net_vhost_vdpa_init will pass an uninitialized pointer to qemu_open() > which will lead a SIGSEV: > > #0 0x0000555555c3a640 in strstart (str=str@entry=0x0, val=val@entry=0x555555dbcb65 "/dev/fdset/", ptr=ptr@entry=0x7fffffffdfb8) at ../util/cutils.c:77 > #1 0x0000555555c572a7 in qemu_open (name=name@entry=0x0, flags=flags@entry=2) at ../util/osdep.c:294 > #2 0x000055555584314a in net_vhost_vdpa_init (device=0x555555c81baa "vhost-vdpa", vhostdev=0x0, name=0x555556396600 "hn0", peer=0x0) at ../net/vhost-vdpa.c:187 > #3 0x000055555584314a in net_init_vhost_vdpa (netdev=<optimized out>, name=0x555556396600 "hn0", peer=0x0, errp=<optimized out>) at ../net/vhost-vdpa.c:227 > #4 0x000055555587e8c9 in net_client_init1 (netdev=0x555556398790, is_netdev=is_netdev@entry=true, errp=errp@entry=0x7fffffffe290) at ../net/net.c:1008 > #5 0x000055555587ecc7 in net_client_init (opts=0x555556192ff0, is_netdev=<optimized out>, errp=0x7fffffffe290) at ../net/net.c:1113 > #6 0x0000555555c33082 in qemu_opts_foreach > (list=<optimized out>, func=func@entry=0x55555587ed50 <net_init_netdev>, opaque=opaque@entry=0x0, errp=errp@entry=0x7fffffffe290) at ../util/qemu-option.c:1172 > #7 0x0000555555880057 in net_init_clients (errp=errp@entry=0x7fffffffe290) at ../net/net.c:1494 > #8 0x0000555555a7f8e9 in qemu_init (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at ../softmmu/vl.c:4250 > #9 0x00005555557f26cd in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at ../softmmu/main.c:49 > > Cc: qemu-stable@nongnu.org > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > net/vhost-vdpa.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c > index bc0e0d2d35..b7221beaa1 100644 > --- a/net/vhost-vdpa.c > +++ b/net/vhost-vdpa.c > @@ -206,7 +206,7 @@ static int net_vhost_check_net(void *opaque, QemuOpts *opts, Error **errp) > } > if (strcmp(netdev, name) == 0 && > !g_str_has_prefix(driver, "virtio-net-")) { > - error_setg(errp, "vhost-vdpa requires frontend driver virtio-net-*"); > + error_setg(errp, "Vhost-vdpa requires frontend driver virtio-net-*"); > return -1; > } > return 0; > @@ -224,5 +224,9 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name, > (char *)name, errp)) { > return -1; > } > + if (!opts->has_vhostdev) { > + error_setg(errp, "vhost-vdpa requires vhostdev to be set"); > + return -1; > + } Useless if you remove the '*' in qapi/net.json. Thanks, Laurent
On 31/08/2020 10:27, Jason Wang wrote: > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > net/vhost-vdpa.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c > index 397d4d3082..bcbf49d55b 100644 > --- a/net/vhost-vdpa.c > +++ b/net/vhost-vdpa.c > @@ -175,10 +175,10 @@ static NetClientInfo net_vhost_vdpa_info = { > static int net_vhost_vdpa_init(NetClientState *peer, const char *device, > const char *name, const char *vhostdev) > { > - NetClientState *nc = NULL; > + NetClientState *nc; > VhostVDPAState *s; > - int vdpa_device_fd = -1; > - int ret = 0; > + int vdpa_device_fd; > + int ret; > assert(name); > nc = qemu_new_net_client(&net_vhost_vdpa_info, peer, device, name); > snprintf(nc->info_str, sizeof(nc->info_str), "vhostdev=%s", vhostdev); > Reviewed-by: Laurent Vivier <lvivier@redhat.com>
On 31/08/2020 10:27, Jason Wang wrote: > The nc is allocated through g_malloc0(), so no need to assign its > queue_index to zero. > > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > net/vhost-vdpa.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c > index bcbf49d55b..1d3ac89135 100644 > --- a/net/vhost-vdpa.c > +++ b/net/vhost-vdpa.c > @@ -182,7 +182,6 @@ static int net_vhost_vdpa_init(NetClientState *peer, const char *device, > assert(name); > nc = qemu_new_net_client(&net_vhost_vdpa_info, peer, device, name); > snprintf(nc->info_str, sizeof(nc->info_str), "vhostdev=%s", vhostdev); > - nc->queue_index = 0; > s = DO_UPCAST(VhostVDPAState, nc, nc); > vdpa_device_fd = qemu_open(vhostdev, O_RDWR); > if (vdpa_device_fd == -1) { > Reviewed-by: Laurent Vivier <lvivier@redhat.com>
On 31/08/2020 10:27, Jason Wang wrote: > This patch squashes net_vhost_vdpa_init() into > net_init_vhost_vdpa(). This will simplify adding pre open file > descriptor support. > > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > net/vhost-vdpa.c | 41 +++++++++++++++++++---------------------- > 1 file changed, 19 insertions(+), 22 deletions(-) > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c > index 1d3ac89135..f5cc4e8326 100644 > --- a/net/vhost-vdpa.c > +++ b/net/vhost-vdpa.c > @@ -172,27 +172,6 @@ static NetClientInfo net_vhost_vdpa_info = { > .has_ufo = vhost_vdpa_has_ufo, > }; > > -static int net_vhost_vdpa_init(NetClientState *peer, const char *device, > - const char *name, const char *vhostdev) > -{ > - NetClientState *nc; > - VhostVDPAState *s; > - int vdpa_device_fd; > - int ret; > - assert(name); > - nc = qemu_new_net_client(&net_vhost_vdpa_info, peer, device, name); > - snprintf(nc->info_str, sizeof(nc->info_str), "vhostdev=%s", vhostdev); > - s = DO_UPCAST(VhostVDPAState, nc, nc); > - vdpa_device_fd = qemu_open(vhostdev, O_RDWR); > - if (vdpa_device_fd == -1) { > - return -errno; > - } > - s->vhost_vdpa.device_fd = vdpa_device_fd; > - ret = vhost_vdpa_add(nc, (void *)&s->vhost_vdpa); > - > - return ret; > -} > - > static int net_vhost_check_net(void *opaque, QemuOpts *opts, Error **errp) > { > const char *name = opaque; > @@ -215,6 +194,10 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name, > NetClientState *peer, Error **errp) > { > const NetdevVhostVDPAOptions *opts; > + NetClientState *nc; > + VhostVDPAState *s; > + int vdpa_device_fd; > + int ret; > > assert(netdev->type == NET_CLIENT_DRIVER_VHOST_VDPA); > opts = &netdev->u.vhost_vdpa; > @@ -227,5 +210,19 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name, > error_setg(errp, "vhost-vdpa requires vhostdev to be set"); > return -1; > } > - return net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name, opts->vhostdev); > + > + assert(name); > + > + nc = qemu_new_net_client(&net_vhost_vdpa_info, peer, TYPE_VHOST_VDPA, name); > + snprintf(nc->info_str, sizeof(nc->info_str), "vhostdev=%s", opts->vhostdev); > + > + s = DO_UPCAST(VhostVDPAState, nc, nc); > + vdpa_device_fd = qemu_open(opts->vhostdev, O_RDWR); > + if (vdpa_device_fd == -1) { > + return -errno; > + } > + s->vhost_vdpa.device_fd = vdpa_device_fd; > + ret = vhost_vdpa_add(nc, (void *)&s->vhost_vdpa); > + > + return ret; You can avoid to declare and use "ret" by doing directly "return vhost_vdpa_add()" Anyway, the code move is correct: Reviewed-by: Laurent Vivier <lvivier@redhat.com>
Hi all: This series refine vhost-vdpa initialization by: - fixing SIGSEV when vhostdev is not specificed - tweak the codes for vhost-vdpa initialization - allow to accept preopen vhost-vdpa file descriptor Please review. Thanks Jason Wang (9): vhost-vdpa: remove the default devname vhost-vdpa: mandate vhostdev vhost-vdpa: remove the unnecessary assert(s->vhost_net) vhost-vdpa: refine info string vhost-vdpa: remove the unnecessary initialization vhost-vdpa: remove the unnecessary queue index assignment vhost-vdpa: squash net_vhost_vdpa_init() into net_init_vhost_vdpa() vhost-vdpa: add accurate error string when fail to open vhost vDPA device vhost-vdpa: allow pre-opend file descriptor net/vhost-vdpa.c | 63 ++++++++++++++++++++++++++++++------------------ qapi/net.json | 6 +++-- 2 files changed, 43 insertions(+), 26 deletions(-)