Re: [dpdk-dev] [PATCH] vhost: fix vhost-user init failed

[Yuanhan Liu <yliu@fridaylinux.org>]

On Mon, Jul 10, 2017 at 11:48:27AM +0200, Jens Freimann wrote:
> >@@ -668,7 +668,7 @@ rte_vhost_driver_register(const char *path, uint64_t flags)
> >	}
> >
> >	vhost_user.vsockets[vhost_user.vsocket_cnt++] = vsocket;
> >-
> >+	goto out;
> >out_mutex:
> >	if (pthread_mutex_destroy(&vsocket->conn_mutex)) {
> >		RTE_LOG(ERR, VHOST_CONFIG,
> 
> Thanks for fixing this!
> 
> Sorry for introducing this bug, I was about to send this before I saw
> your fix:
> 
> diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
> index 57b86c0..b2158a7 100644
> --- a/lib/librte_vhost/socket.c
> +++ b/lib/librte_vhost/socket.c
> @@ -668,6 +668,9 @@ rte_vhost_driver_register(const char *path, uint64_t flags)
>        }
> 
>        vhost_user.vsockets[vhost_user.vsocket_cnt++] = vsocket;
> +out:
> +       pthread_mutex_unlock(&vhost_user.mutex);
> +       return ret;
> 
> out_mutex:
>        if (pthread_mutex_destroy(&vsocket->conn_mutex)) {
> @@ -677,9 +680,7 @@ rte_vhost_driver_register(const char *path, uint64_t flags)
> out_free:
>        free(vsocket->path);
>        free(vsocket);
> -out:
>        pthread_mutex_unlock(&vhost_user.mutex);
> -
>        return ret;
> }
> 
> Both works fine, so I leave it up to the maintainers how to fix.
> 
> Reviewed-by: Jens Freimann <jfreimann@redhat.com>

I actually prefers below: I don't want a simple "out" on top of "out_mutex"
and "out_free", and an unconditional "goto" looks weird to me.

---
@@ -669,6 +669,9 @@ rte_vhost_driver_register(const char *path, uint64_t flags)

        vhost_user.vsockets[vhost_user.vsocket_cnt++] = vsocket;

+       pthread_mutex_unlock(&vhost_user.mutex);
+       return ret;
+
 out_mutex:
        if (pthread_mutex_destroy(&vsocket->conn_mutex)) {
                RTE_LOG(ERR, VHOST_CONFIG,


Applied to dpdk-next-virtio, with above changes.

Thanks!

	--yliu