All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] vhost: added user callbacks for socket open/close
@ 2017-08-21  9:34 Dariusz Stojaczyk
  2017-08-21 15:00 ` Jens Freimann
  2017-08-22 16:24 ` [PATCH v2] " Dariusz Stojaczyk
  0 siblings, 2 replies; 11+ messages in thread
From: Dariusz Stojaczyk @ 2017-08-21  9:34 UTC (permalink / raw)
  To: dev; +Cc: Pawel Wodkowski, Dariusz Stojaczyk

When user receives destroy_device signal, he does not know *why* that
event happened. He does not differ between socket shutdown and virtio
processing pause. User could completely delete device during transition
from BIOS to kernel, causing freeze or possibly kernel panic. Instead
of changing new_device/destroy_device callbacks and breaking the ABI,
a set of new functions new_connection/destroy_connection has been added.

Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
---
 lib/librte_vhost/rte_vhost.h |  5 ++++-
 lib/librte_vhost/socket.c    | 23 +++++++++++++++++++----
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
index 8c974eb..8f86167 100644
--- a/lib/librte_vhost/rte_vhost.h
+++ b/lib/librte_vhost/rte_vhost.h
@@ -107,7 +107,10 @@ struct vhost_device_ops {
 	 */
 	int (*features_changed)(int vid, uint64_t features);
 
-	void *reserved[4]; /**< Reserved for future extension */
+	int (*new_connection)(int vid);		/**< Connect to socket. */
+	void (*destroy_connection)(int vid);	/**< Disconnect from socket */
+
+	void *reserved[2]; /**< Reserved for future extension */
 };
 
 /**
diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
index 41aa3f9..4ab4ff7 100644
--- a/lib/librte_vhost/socket.c
+++ b/lib/librte_vhost/socket.c
@@ -230,24 +230,36 @@ vhost_user_add_connection(int fd, struct vhost_user_socket *vsocket)
 
 	RTE_LOG(INFO, VHOST_CONFIG, "new device, handle is %d\n", vid);
 
+	if (vsocket->notify_ops->new_connection) {
+		ret = vsocket->notify_ops->new_connection(vid);
+		if (ret < 0) {
+			RTE_LOG(ERR, VHOST_CONFIG,
+				"failed to add vhost user connection with fd %d\n",
+				fd);
+			goto err;
+		}
+	}
+
 	conn->connfd = fd;
 	conn->vsocket = vsocket;
 	conn->vid = vid;
 	ret = fdset_add(&vhost_user.fdset, fd, vhost_user_read_cb,
 			NULL, conn);
 	if (ret < 0) {
-		conn->connfd = -1;
-		free(conn);
-		close(fd);
 		RTE_LOG(ERR, VHOST_CONFIG,
 			"failed to add fd %d into vhost server fdset\n",
 			fd);
-		return;
+		goto err;
 	}
 
 	pthread_mutex_lock(&vsocket->conn_mutex);
 	TAILQ_INSERT_TAIL(&vsocket->conn_list, conn, next);
 	pthread_mutex_unlock(&vsocket->conn_mutex);
+	return;
+
+err:
+	free(conn);
+	close(fd);
 }
 
 /* call back when there is new vhost-user connection from client  */
@@ -277,6 +289,9 @@ vhost_user_read_cb(int connfd, void *dat, int *remove)
 		*remove = 1;
 		vhost_destroy_device(conn->vid);
 
+		if (vsocket->notify_ops->destroy_connection)
+			vsocket->notify_ops->destroy_connection(conn->vid);
+
 		pthread_mutex_lock(&vsocket->conn_mutex);
 		TAILQ_REMOVE(&vsocket->conn_list, conn, next);
 		pthread_mutex_unlock(&vsocket->conn_mutex);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] vhost: added user callbacks for socket open/close
  2017-08-21  9:34 [PATCH] vhost: added user callbacks for socket open/close Dariusz Stojaczyk
@ 2017-08-21 15:00 ` Jens Freimann
  2017-08-22  9:55   ` Stojaczyk, DariuszX
  2017-08-22 16:24 ` [PATCH v2] " Dariusz Stojaczyk
  1 sibling, 1 reply; 11+ messages in thread
From: Jens Freimann @ 2017-08-21 15:00 UTC (permalink / raw)
  To: Dariusz Stojaczyk; +Cc: dev, Pawel Wodkowski, Maxime Coquelin (mcoqueli), yliu

Hi Dariusz,

On Mon, Aug 21, 2017 at 11:34:42AM +0200, Dariusz Stojaczyk wrote:
>When user receives destroy_device signal, he does not know *why* that
>event happened. He does not differ between socket shutdown and virtio
>processing pause. User could completely delete device during transition
>from BIOS to kernel, causing freeze or possibly kernel panic. Instead
>of changing new_device/destroy_device callbacks and breaking the ABI,
>a set of new functions new_connection/destroy_connection has been added.
>
>Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
>---
> lib/librte_vhost/rte_vhost.h |  5 ++++-
> lib/librte_vhost/socket.c    | 23 +++++++++++++++++++----
> 2 files changed, 23 insertions(+), 5 deletions(-)
>
>diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
>index 8c974eb..8f86167 100644
>--- a/lib/librte_vhost/rte_vhost.h
>+++ b/lib/librte_vhost/rte_vhost.h
>@@ -107,7 +107,10 @@ struct vhost_device_ops {
> 	 */
> 	int (*features_changed)(int vid, uint64_t features);
>
>-	void *reserved[4]; /**< Reserved for future extension */
>+	int (*new_connection)(int vid);		/**< Connect to socket. */
>+	void (*destroy_connection)(int vid);	/**< Disconnect from socket */

I'm a little uncertain but my gut feeling is that in this context a connection is
something between two sockets, not between devices. I would probably
add these callbacks to struct vhost_user_socket. This is also where we
keep the list of connections.

regards,
Jens 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] vhost: added user callbacks for socket open/close
  2017-08-21 15:00 ` Jens Freimann
@ 2017-08-22  9:55   ` Stojaczyk, DariuszX
  2017-08-22 11:58     ` Jens Freimann
  0 siblings, 1 reply; 11+ messages in thread
From: Stojaczyk, DariuszX @ 2017-08-22  9:55 UTC (permalink / raw)
  To: Jens Freimann; +Cc: dev, Wodkowski, PawelX, Maxime Coquelin (mcoqueli), yliu

Hi Jens,

> I'm a little uncertain but my gut feeling is that in this context a connection is
> something between two sockets, not between devices.

What do you mean?
This is a unix domain socket connection. DPDK can create the socket, then the client may connect to it via connect(2).

> I would probably add
> these callbacks to struct vhost_user_socket. This is also where we keep the
> list of connections.

I get your point. However, it's vhost_device_ops struct that's being set by the user via rte_vhost_driver_callback_register(). The new_connection callback is there just to mark the device as *in use, can't be deleted*. It doesn't transport any connection data.

Regards,
D.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] vhost: added user callbacks for socket open/close
  2017-08-22  9:55   ` Stojaczyk, DariuszX
@ 2017-08-22 11:58     ` Jens Freimann
  2017-08-22 12:10       ` Jens Freimann
  0 siblings, 1 reply; 11+ messages in thread
From: Jens Freimann @ 2017-08-22 11:58 UTC (permalink / raw)
  To: Stojaczyk, DariuszX
  Cc: dev, Wodkowski, PawelX, Maxime Coquelin (mcoqueli), yliu

On Tue, Aug 22, 2017 at 09:55:19AM +0000, Stojaczyk, DariuszX wrote:
>Hi Jens,
>
>> I'm a little uncertain but my gut feeling is that in this context a connection is
>> something between two sockets, not between devices.
>
>What do you mean?
>This is a unix domain socket connection. DPDK can create the socket, then the client may connect to it via connect(2).

yes, I get that. 

>
>> I would probably add
>> these callbacks to struct vhost_user_socket. This is also where we keep the
>> list of connections.
>
>I get your point. However, it's vhost_device_ops struct that's being set by the user via rte_vhost_driver_callback_register(). The new_connection callback is there just to mark the device as *in use, can't be deleted*. It doesn't transport any connection data.

You're right, I overlooked that it needs to be set by the user. In
this case your patch is the smallest possible change and looks good to
me.

Do we need a documentation change for this?


regards,
Jens 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] vhost: added user callbacks for socket open/close
  2017-08-22 11:58     ` Jens Freimann
@ 2017-08-22 12:10       ` Jens Freimann
  0 siblings, 0 replies; 11+ messages in thread
From: Jens Freimann @ 2017-08-22 12:10 UTC (permalink / raw)
  To: Stojaczyk, DariuszX
  Cc: dev, Wodkowski, PawelX, Maxime Coquelin (mcoqueli), yliu

On Tue, Aug 22, 2017 at 01:58:44PM +0200, Jens Freimann wrote:
>On Tue, Aug 22, 2017 at 09:55:19AM +0000, Stojaczyk, DariuszX wrote:
>Do we need a documentation change for this?

To answer my own question, I think doc/guides/prog_guide/vhost_lib.rst
needs an update.

regards,
Jens 
>
>
>regards,
>Jens

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] vhost: added user callbacks for socket open/close
  2017-08-21  9:34 [PATCH] vhost: added user callbacks for socket open/close Dariusz Stojaczyk
  2017-08-21 15:00 ` Jens Freimann
@ 2017-08-22 16:24 ` Dariusz Stojaczyk
  2017-08-25  9:22   ` Jens Freimann
  2017-08-30 10:50   ` [PATCH v3] rte_vhost: " Dariusz Stojaczyk
  1 sibling, 2 replies; 11+ messages in thread
From: Dariusz Stojaczyk @ 2017-08-22 16:24 UTC (permalink / raw)
  To: dev; +Cc: Pawel Wodkowski, Dariusz Stojaczyk

When user receives destroy_device signal, he does not know *why* that
event happened. He does not differ between socket shutdown and virtio
processing pause. User could completely delete device during transition
from BIOS to kernel, causing freeze or possibly kernel panic. Instead
of changing new_device/destroy_device callbacks and breaking the ABI,
a set of new functions new_connection/destroy_connection has been added.

Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
---
v2: also updated vhost_lib.rst
 doc/guides/prog_guide/vhost_lib.rst | 15 +++++++++++++--
 lib/librte_vhost/rte_vhost.h        |  5 ++++-
 lib/librte_vhost/socket.c           | 23 +++++++++++++++++++----
 3 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/doc/guides/prog_guide/vhost_lib.rst b/doc/guides/prog_guide/vhost_lib.rst
index 5979290..861a0e2 100644
--- a/doc/guides/prog_guide/vhost_lib.rst
+++ b/doc/guides/prog_guide/vhost_lib.rst
@@ -129,8 +129,7 @@ The following is an overview of some key Vhost API functions:
 
   * ``destroy_device(int vid)``
 
-    This callback is invoked when a virtio device shuts down (or when the
-    vhost connection is broken).
+    This callback is invoked when a virtio device is paused or shut down.
 
   * ``vring_state_changed(int vid, uint16_t queue_id, int enable)``
 
@@ -143,6 +142,18 @@ The following is an overview of some key Vhost API functions:
     ``VHOST_F_LOG_ALL`` will be set/cleared at the start/end of live
     migration, respectively.
 
+  * ``new_connection(int vid)``
+
+    This callback is invoked on new vhost-user socket connection. If DPDK
+    acts as the server the device should not be deleted before
+     ``destroy_connection`` callback is received.
+
+  * ``destroy_connection(int vid)``
+
+    This callback is invoked when vhost-user socket connection is closed.
+    It indicates that device with id ``vid`` is no longer in use and can be
+    safely deleted.
+
 * ``rte_vhost_driver_disable/enable_features(path, features))``
 
   This function disables/enables some features. For example, it can be used to
diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
index 8c974eb..8f86167 100644
--- a/lib/librte_vhost/rte_vhost.h
+++ b/lib/librte_vhost/rte_vhost.h
@@ -107,7 +107,10 @@ struct vhost_device_ops {
 	 */
 	int (*features_changed)(int vid, uint64_t features);
 
-	void *reserved[4]; /**< Reserved for future extension */
+	int (*new_connection)(int vid);		/**< Connect to socket. */
+	void (*destroy_connection)(int vid);	/**< Disconnect from socket */
+
+	void *reserved[2]; /**< Reserved for future extension */
 };
 
 /**
diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
index 41aa3f9..4ab4ff7 100644
--- a/lib/librte_vhost/socket.c
+++ b/lib/librte_vhost/socket.c
@@ -230,24 +230,36 @@ vhost_user_add_connection(int fd, struct vhost_user_socket *vsocket)
 
 	RTE_LOG(INFO, VHOST_CONFIG, "new device, handle is %d\n", vid);
 
+	if (vsocket->notify_ops->new_connection) {
+		ret = vsocket->notify_ops->new_connection(vid);
+		if (ret < 0) {
+			RTE_LOG(ERR, VHOST_CONFIG,
+				"failed to add vhost user connection with fd %d\n",
+				fd);
+			goto err;
+		}
+	}
+
 	conn->connfd = fd;
 	conn->vsocket = vsocket;
 	conn->vid = vid;
 	ret = fdset_add(&vhost_user.fdset, fd, vhost_user_read_cb,
 			NULL, conn);
 	if (ret < 0) {
-		conn->connfd = -1;
-		free(conn);
-		close(fd);
 		RTE_LOG(ERR, VHOST_CONFIG,
 			"failed to add fd %d into vhost server fdset\n",
 			fd);
-		return;
+		goto err;
 	}
 
 	pthread_mutex_lock(&vsocket->conn_mutex);
 	TAILQ_INSERT_TAIL(&vsocket->conn_list, conn, next);
 	pthread_mutex_unlock(&vsocket->conn_mutex);
+	return;
+
+err:
+	free(conn);
+	close(fd);
 }
 
 /* call back when there is new vhost-user connection from client  */
@@ -277,6 +289,9 @@ vhost_user_read_cb(int connfd, void *dat, int *remove)
 		*remove = 1;
 		vhost_destroy_device(conn->vid);
 
+		if (vsocket->notify_ops->destroy_connection)
+			vsocket->notify_ops->destroy_connection(conn->vid);
+
 		pthread_mutex_lock(&vsocket->conn_mutex);
 		TAILQ_REMOVE(&vsocket->conn_list, conn, next);
 		pthread_mutex_unlock(&vsocket->conn_mutex);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] vhost: added user callbacks for socket open/close
  2017-08-22 16:24 ` [PATCH v2] " Dariusz Stojaczyk
@ 2017-08-25  9:22   ` Jens Freimann
  2017-08-29  6:08     ` Stojaczyk, DariuszX
  2017-08-30 10:50   ` [PATCH v3] rte_vhost: " Dariusz Stojaczyk
  1 sibling, 1 reply; 11+ messages in thread
From: Jens Freimann @ 2017-08-25  9:22 UTC (permalink / raw)
  To: Dariusz Stojaczyk; +Cc: dev, Pawel Wodkowski, maxime.coquelin, yliu


Hi Dariusz,

On Tue, Aug 22, 2017 at 06:24:52PM +0200, Dariusz Stojaczyk wrote:
>When user receives destroy_device signal, he does not know *why* that
>event happened. He does not differ between socket shutdown and virtio
>processing pause. User could completely delete device during transition
>from BIOS to kernel, causing freeze or possibly kernel panic. Instead
>of changing new_device/destroy_device callbacks and breaking the ABI,
>a set of new functions new_connection/destroy_connection has been added.
>
>Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
>---
>v2: also updated vhost_lib.rst
> doc/guides/prog_guide/vhost_lib.rst | 15 +++++++++++++--
> lib/librte_vhost/rte_vhost.h        |  5 ++++-
> lib/librte_vhost/socket.c           | 23 +++++++++++++++++++----
> 3 files changed, 36 insertions(+), 7 deletions(-)

thanks for adding documentation!

I'm still not sure I understand the use case. So just for my
understanding: users need to distinct between "the device is going
away temporarily, keep the connection" and "we're shutting down for good", is that it?
Maybe it's just me or maybe it means you could explain your example in
the commit message a bit more.  I think the code looks sane, so 

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

Oh, and you should put the maintainers on Cc to get a faster review. 

regards,
Jens 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] vhost: added user callbacks for socket open/close
  2017-08-25  9:22   ` Jens Freimann
@ 2017-08-29  6:08     ` Stojaczyk, DariuszX
  2017-08-30  6:33       ` Jens Freimann
  0 siblings, 1 reply; 11+ messages in thread
From: Stojaczyk, DariuszX @ 2017-08-29  6:08 UTC (permalink / raw)
  To: 'Jens Freimann'; +Cc: dev, Wodkowski, PawelX, maxime.coquelin, yliu

Hi Jens,

> I'm still not sure I understand the use case. So just for my
> understanding: users need to distinct between "the device is going away
> temporarily, keep the connection" and "we're shutting down for good", is
> that it?

Yes, exactly.

> Maybe it's just me or maybe it means you could explain your example in the
> commit message a bit more.

Ok. How about the following commit message instead:
```
rte_vhost: added user callbacks for socket open/close

Added new callbacks to notify about socket connection status.
As destroy_device is used for virtqueue processing *pause* as
well as connection close, the user has no distinction between those.

Consider the following scenario:
rte_vhost: received SET_VRING_BASE message,
                  calling destroy_device() as usual

user:  end-user asks to remove the device (together with socket file),
          OK, device is not *in use* - that's NOT the behavior we want
          calling rte_vhost_driver_unregister() etc.

Instead of changing new_device/destroy_device callbacks and breaking
the ABI, a set of new functions new_connection/destroy_connection
has been added.
```

> Oh, and you should put the maintainers on Cc to get a faster review.

Thanks, I will!
Regards,
D. 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] vhost: added user callbacks for socket open/close
  2017-08-29  6:08     ` Stojaczyk, DariuszX
@ 2017-08-30  6:33       ` Jens Freimann
  0 siblings, 0 replies; 11+ messages in thread
From: Jens Freimann @ 2017-08-30  6:33 UTC (permalink / raw)
  To: Stojaczyk, DariuszX; +Cc: dev, Wodkowski, PawelX, maxime.coquelin, yliu

On Tue, Aug 29, 2017 at 06:08:45AM +0000, Stojaczyk, DariuszX wrote:
>Hi Jens,
>
>> I'm still not sure I understand the use case. So just for my
>> understanding: users need to distinct between "the device is going away
>> temporarily, keep the connection" and "we're shutting down for good", is
>> that it?
>
>Yes, exactly.
>
>> Maybe it's just me or maybe it means you could explain your example in the
>> commit message a bit more.
>
>Ok. How about the following commit message instead:
>```
>rte_vhost: added user callbacks for socket open/close
>
>Added new callbacks to notify about socket connection status.
>As destroy_device is used for virtqueue processing *pause* as
>well as connection close, the user has no distinction between those.
>
>Consider the following scenario:
>rte_vhost: received SET_VRING_BASE message,
>                  calling destroy_device() as usual
>
>user:  end-user asks to remove the device (together with socket file),
>          OK, device is not *in use* - that's NOT the behavior we want
>          calling rte_vhost_driver_unregister() etc.
>
>Instead of changing new_device/destroy_device callbacks and breaking
>the ABI, a set of new functions new_connection/destroy_connection
>has been added.
>```

Sounds good to me. Thanks!

regards,
Jens 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v3] rte_vhost: added user callbacks for socket open/close
  2017-08-22 16:24 ` [PATCH v2] " Dariusz Stojaczyk
  2017-08-25  9:22   ` Jens Freimann
@ 2017-08-30 10:50   ` Dariusz Stojaczyk
  2017-10-10  3:14     ` Yuanhan Liu
  1 sibling, 1 reply; 11+ messages in thread
From: Dariusz Stojaczyk @ 2017-08-30 10:50 UTC (permalink / raw)
  To: dev; +Cc: Pawel Wodkowski, jfreimann, maxime.coquelin, yliu, Dariusz Stojaczyk

Added new callbacks to notify about socket connection status.
As destroy_device is used for virtqueue processing *pause* as well as
connection close, the user has no distinction between those.

Consider the following scenario:
rte_vhost: received SET_VRING_BASE message,
           calling destroy_device() as usual

user:  end-user asks to remove the device (together with socket file),
       OK, device is not *in use* - that's NOT the behavior we want
       calling rte_vhost_driver_unregister() etc.

Instead of changing new_device/destroy_device callbacks and breaking
the ABI, a set of new functions new_connection/destroy_connection
has been added.

Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
---
v3: improved err-handling path and updated commit msg
v2: also updated vhost_lib.rst
 lib/librte_vhost/rte_vhost.h |  5 ++++-
 lib/librte_vhost/socket.c    | 31 ++++++++++++++++++++++++-------
 2 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
index 8c974eb..fe5c94c 100644
--- a/lib/librte_vhost/rte_vhost.h
+++ b/lib/librte_vhost/rte_vhost.h
@@ -107,7 +107,10 @@ struct vhost_device_ops {
 	 */
 	int (*features_changed)(int vid, uint64_t features);
 
-	void *reserved[4]; /**< Reserved for future extension */
+	int (*new_connection)(int vid);
+	void (*destroy_connection)(int vid);
+
+	void *reserved[2]; /**< Reserved for future extension */
 };
 
 /**
diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
index 41aa3f9..7018150 100644
--- a/lib/librte_vhost/socket.c
+++ b/lib/librte_vhost/socket.c
@@ -217,9 +217,7 @@ vhost_user_add_connection(int fd, struct vhost_user_socket *vsocket)
 
 	vid = vhost_new_device();
 	if (vid == -1) {
-		close(fd);
-		free(conn);
-		return;
+		goto err;
 	}
 
 	size = strnlen(vsocket->path, PATH_MAX);
@@ -230,24 +228,40 @@ vhost_user_add_connection(int fd, struct vhost_user_socket *vsocket)
 
 	RTE_LOG(INFO, VHOST_CONFIG, "new device, handle is %d\n", vid);
 
+	if (vsocket->notify_ops->new_connection) {
+		ret = vsocket->notify_ops->new_connection(vid);
+		if (ret < 0) {
+			RTE_LOG(ERR, VHOST_CONFIG,
+				"failed to add vhost user connection with fd %d\n",
+				fd);
+			goto err;
+		}
+	}
+
 	conn->connfd = fd;
 	conn->vsocket = vsocket;
 	conn->vid = vid;
 	ret = fdset_add(&vhost_user.fdset, fd, vhost_user_read_cb,
 			NULL, conn);
 	if (ret < 0) {
-		conn->connfd = -1;
-		free(conn);
-		close(fd);
 		RTE_LOG(ERR, VHOST_CONFIG,
 			"failed to add fd %d into vhost server fdset\n",
 			fd);
-		return;
+
+		if (vsocket->notify_ops->destroy_connection)
+			vsocket->notify_ops->destroy_connection(conn->vid);
+
+		goto err;
 	}
 
 	pthread_mutex_lock(&vsocket->conn_mutex);
 	TAILQ_INSERT_TAIL(&vsocket->conn_list, conn, next);
 	pthread_mutex_unlock(&vsocket->conn_mutex);
+	return;
+
+err:
+	free(conn);
+	close(fd);
 }
 
 /* call back when there is new vhost-user connection from client  */
@@ -277,6 +291,9 @@ vhost_user_read_cb(int connfd, void *dat, int *remove)
 		*remove = 1;
 		vhost_destroy_device(conn->vid);
 
+		if (vsocket->notify_ops->destroy_connection)
+			vsocket->notify_ops->destroy_connection(conn->vid);
+
 		pthread_mutex_lock(&vsocket->conn_mutex);
 		TAILQ_REMOVE(&vsocket->conn_list, conn, next);
 		pthread_mutex_unlock(&vsocket->conn_mutex);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] rte_vhost: added user callbacks for socket open/close
  2017-08-30 10:50   ` [PATCH v3] rte_vhost: " Dariusz Stojaczyk
@ 2017-10-10  3:14     ` Yuanhan Liu
  0 siblings, 0 replies; 11+ messages in thread
From: Yuanhan Liu @ 2017-10-10  3:14 UTC (permalink / raw)
  To: Dariusz Stojaczyk; +Cc: dev, Pawel Wodkowski, jfreimann, maxime.coquelin

On Wed, Aug 30, 2017 at 12:50:58PM +0200, Dariusz Stojaczyk wrote:
> Added new callbacks to notify about socket connection status.
> As destroy_device is used for virtqueue processing *pause* as well as
> connection close, the user has no distinction between those.
> 
> Consider the following scenario:
> rte_vhost: received SET_VRING_BASE message,
>            calling destroy_device() as usual
> 
> user:  end-user asks to remove the device (together with socket file),
>        OK, device is not *in use* - that's NOT the behavior we want
>        calling rte_vhost_driver_unregister() etc.
> 
> Instead of changing new_device/destroy_device callbacks and breaking
> the ABI, a set of new functions new_connection/destroy_connection
> has been added.
> 
> Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
> ---
> v3: improved err-handling path and updated commit msg
> v2: also updated vhost_lib.rst

The doc update is missing. I have cherry-picked it from v2.

Applied to dpdk-next-virtio.

Thanks.

	--yliu

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2017-10-10  3:14 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-21  9:34 [PATCH] vhost: added user callbacks for socket open/close Dariusz Stojaczyk
2017-08-21 15:00 ` Jens Freimann
2017-08-22  9:55   ` Stojaczyk, DariuszX
2017-08-22 11:58     ` Jens Freimann
2017-08-22 12:10       ` Jens Freimann
2017-08-22 16:24 ` [PATCH v2] " Dariusz Stojaczyk
2017-08-25  9:22   ` Jens Freimann
2017-08-29  6:08     ` Stojaczyk, DariuszX
2017-08-30  6:33       ` Jens Freimann
2017-08-30 10:50   ` [PATCH v3] rte_vhost: " Dariusz Stojaczyk
2017-10-10  3:14     ` Yuanhan Liu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.