Re: [PATCH] vhost: fix driver unregister for client mode
From: Yuanhan Liu <hidden>
Date: 2016-07-21 08:20:14
On Wed, Jul 20, 2016 at 11:32:43AM +0300, Ilya Maximets wrote:
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.
---
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 hunk ↗ jump to hunk
/** * 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.
+ 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