diff mbox

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

Message ID 20170710094827.zdykowebtm3m3w2k@dhcp-192-218.str.redhat.com (mailing list archive)
State Not Applicable, archived
Headers

Checks

Context Check Description
ci/Intel-compilation fail apply patch file failure
ci/checkpatch warning coding style issues

Commit Message

Jens Freimann July 10, 2017, 9:48 a.m. UTC 
  On Mon, Jul 10, 2017 at 04:06:48PM +0800, Zhiyong Yang wrote:
>Exception handling is executed in the normal path and it will cause
>vhost-user init failure.
>Fixes: d6983a70e259("vhost: check return of pthread calls")
>
>Reported-by: Lei Yao <lei.a.yao@intel.com>
>Signed-off-by: Zhiyong Yang <zhiyong.yang@intel.com>
>---
> lib/librte_vhost/socket.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
>index 57b86c0..9d2049c 100644
>--- a/lib/librte_vhost/socket.c
>+++ b/lib/librte_vhost/socket.c
>@@ -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: 

Both works fine, so I leave it up to the maintainers how to fix. 

Reviewed-by: Jens Freimann <jfreimann@redhat.com>

Comments

Yuanhan Liu July 14, 2017, 2:43 a.m. UTC | #1 
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
diff mbox

Patch

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