Re: [PATCH] vhost: fix driver unregister for client mode

Thanks. Fixed.

Best regards, Ilya Maximets.

On 21.07.2016 11:24, Yuanhan Liu wrote:

On Wed, Jul 20, 2016 at 11:32:43AM +0300, Ilya Maximets wrote:

quoted

Currently while calling of 'rte_vhost_driver_unregister()' connection
to QEMU will not be closed. This leads to inability to register driver
again and reconnect to same virtual machine.

This scenario is reproducible with OVS. While executing of the following
command vhost port will be re-created (will be executed
'rte_vhost_driver_register()' followed by 'rte_vhost_driver_unregister()')
network will be broken and QEMU possibly will crash:

	ovs-vsctl set Interface vhost1 ofport_request=15

Fix this by closing all established connections on driver unregister and
removing of pending connections from reconnection list.

Fixes: 64ab701c3d1e ("vhost: add vhost-user client mode")
Signed-off-by: Ilya Maximets <redacted>

Again, thanks for the fix.

quoted

---

Patch prepared for master branch because dpdk-next-virtio doesn't contain
commit acbff5c67ea7 ("vhost: fix crash when exceeding file descriptors").
Porting to dpdk-next-virtio/master is trivial and may be performed on
demand.

Yeah, my bad, I haven't updated it after rc2, since Thomas no longer
pull request from it.  Anyway, you just remind me that I should have
done that.

quoted

 /**
  * Unregister the specified vhost socket
  */

@@ -672,20 +700,34 @@ rte_vhost_driver_unregister(const char *path)
 {
 	int i;
 	int count;
+	struct vhost_user_connection *conn;
 
 	pthread_mutex_lock(&vhost_user.mutex);
 
 	for (i = 0; i < vhost_user.vsocket_cnt; i++) {
-		if (!strcmp(vhost_user.vsockets[i]->path, path)) {
-			if (vhost_user.vsockets[i]->is_server) {
-				fdset_del(&vhost_user.fdset,
-					vhost_user.vsockets[i]->listenfd);
-				close(vhost_user.vsockets[i]->listenfd);
+		struct vhost_user_socket *vsocket = vhost_user.vsockets[i];
+
+		if (!strcmp(vsocket->path, path)) {
+			if (vsocket->is_server) {
+				(void) fdset_del(&vhost_user.fdset,
+				                 vsocket->listenfd);

I would think the (void) cast is not neceessary here.

quoted

+				close(vsocket->listenfd);
 				unlink(path);
+			} else if (vsocket->reconnect) {
+				vhost_user_remove_reconnect(vsocket);
+			}
+
+			conn = fdset_del(&vhost_user.fdset, vsocket->connfd);
+			if (conn) {
+				RTE_LOG(INFO, VHOST_CONFIG, "free connfd = %d"
+				    "for device '%s'\n", vsocket->connfd, path);

We should try not to break a log message into several lines, which
hurts "git grep".  Here, it could be:

				RTE_LOG(INFO, VHOST_CONFIG,
					"free connfd = %d for device '%s'\n",
					vsocket->connfd, path);

Besides the two minor nits,

Acked-by: Yuanhan Liu <redacted>

	--yliu