Re: [dpdk-dev] [PATCH] vhost: fix passing destroyed device to destroy callback

[Ilya Maximets <i.maximets@samsung.com>]

On 10.04.2019 10:24, David Marchand wrote:
> 
> 
> On Tue, Apr 9, 2019 at 3:36 PM Ilya Maximets <i.maximets@samsung.com <mailto:i.maximets@samsung.com>> wrote:
> 
>     Application should be able to obtain information like 'ifname' from
>     the 'vid' passed to 'destroy_connection' callback. Currently, all the
>     API calls with passed 'vid' fails with 'device not found'.
> 
>     Fixes: efba12a78ddf ("vhost: add user callbacks for socket open/close")
>     Cc: stable@dpdk.org <mailto:stable@dpdk.org>
> 
>     Signed-off-by: Ilya Maximets <i.maximets@samsung.com <mailto:i.maximets@samsung.com>>
>     ---
>      lib/librte_vhost/socket.c | 3 ++-
>      1 file changed, 2 insertions(+), 1 deletion(-)
> 
>     diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
>     index 3da9de62c..43f091d10 100644
>     --- a/lib/librte_vhost/socket.c
>     +++ b/lib/librte_vhost/socket.c
>     @@ -297,11 +297,12 @@ vhost_user_read_cb(int connfd, void *dat, int *remove)
>             if (ret < 0) {
>                     close(connfd);
>                     *remove = 1;
>     -               vhost_destroy_device(conn->vid);
> 
>                     if (vsocket->notify_ops->destroy_connection)
>                             vsocket->notify_ops->destroy_connection(conn->vid);
> 
>     +               vhost_destroy_device(conn->vid);
>     +
>                     pthread_mutex_lock(&vsocket->conn_mutex);
>                     TAILQ_REMOVE(&vsocket->conn_list, conn, next);
>                     pthread_mutex_unlock(&vsocket->conn_mutex);
>     -- 
>     2.17.1
> 
> 
> Reviewed-by: David Marchand <david.marchand@redhat.com <mailto:david.marchand@redhat.com>>
> 
> For vhost maintainers, looking at vhost_user_add_connection, aren't we leaking a vid on errors ? either when new_connection notifier returns an error, or after calling destroy_connection.

I think that you're right.
I spotted that too yesterday while preparing this patch, just had no time to
check deeper. It should be safe to call 'vhost_destroy_device' in these cases.

Best regards, Ilya Maximets.