[libvirt] [PATCH] daemon: Unlink unix socket paths on shutdown
This patch introduces a internal RPC API "virNetServerClose", which is standalone with "virNetServerFree". it closes all the socket fds, and unlinks the unix socket paths, regardless of whether the socket is still referenced or not. This is to address regression bug: https://bugzilla.redhat.com/show_bug.cgi?id=725702 --- daemon/libvirtd.c | 1 + src/rpc/virnetserver.c | 12 ++++++++++++ src/rpc/virnetserver.h | 1 + src/rpc/virnetserverservice.c | 12 ++++++++++++ src/rpc/virnetserverservice.h | 2 ++ src/rpc/virnetsocket.c | 16 ++++++++++++++++ src/rpc/virnetsocket.h | 2 ++ 7 files changed, 46 insertions(+), 0 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 9e044e2..53f1002 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1558,6 +1558,7 @@ int main(int argc, char **argv) { cleanup: virNetServerProgramFree(remoteProgram); virNetServerProgramFree(qemuProgram); + virNetServerClose(srv); virNetServerFree(srv); if (statuswrite != -1) { if (ret != 0) { diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 2dae2ff..d2a6fef 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -794,3 +794,15 @@ void virNetServerFree(virNetServerPtr srv) virMutexDestroy(&srv->lock); VIR_FREE(srv); } + +void virNetServerClose(virNetServerPtr srv) +{ + int i; + + if (!srv) + return; + + for (i = 0; i < srv->nservices; i++) { + virNetServerServiceClose(srv->services[i]); + } +} diff --git a/src/rpc/virnetserver.h b/src/rpc/virnetserver.h index 810d8a3..324cfb7 100644 --- a/src/rpc/virnetserver.h +++ b/src/rpc/virnetserver.h @@ -85,5 +85,6 @@ void virNetServerQuit(virNetServerPtr srv); void virNetServerFree(virNetServerPtr srv); +void virNetServerClose(virNetServerPtr srv); #endif diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c index d5648dc..8c9ed1e 100644 --- a/src/rpc/virnetserverservice.c +++ b/src/rpc/virnetserverservice.c @@ -280,3 +280,15 @@ void virNetServerServiceToggle(virNetServerServicePtr svc, VIR_EVENT_HANDLE_READABLE : 0); } + +void virNetServerServiceClose(virNetServerServicePtr svc) +{ + int i; + + if (!svc) + return; + + for (i = 0; i < svc->nsocks; i++) { + virNetSocketClose(svc->socks[i]); + } +} diff --git a/src/rpc/virnetserverservice.h b/src/rpc/virnetserverservice.h index 9357598..8540bd9 100644 --- a/src/rpc/virnetserverservice.h +++ b/src/rpc/virnetserverservice.h @@ -66,4 +66,6 @@ void virNetServerServiceFree(virNetServerServicePtr svc); void virNetServerServiceToggle(virNetServerServicePtr svc, bool enabled); +void virNetServerServiceClose(virNetServerServicePtr svc); + #endif diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 41b691a..b94b629 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -1222,3 +1222,19 @@ void virNetSocketRemoveIOCallback(virNetSocketPtr sock) virMutexUnlock(&sock->lock); } + +void virNetSocketClose(virNetSocketPtr sock) +{ + if (!sock) + return; + + VIR_FORCE_CLOSE(sock->fd); + +#ifdef HAVE_SYS_UN_H + /* If a server socket, then unlink UNIX path */ + if (!sock->client && + sock->localAddr.data.sa.sa_family == AF_UNIX && + sock->localAddr.data.un.sun_path[0] != '\0') + unlink(sock->localAddr.data.un.sun_path); +#endif +} diff --git a/src/rpc/virnetsocket.h b/src/rpc/virnetsocket.h index dfb3c5d..3735a88 100644 --- a/src/rpc/virnetsocket.h +++ b/src/rpc/virnetsocket.h @@ -118,6 +118,8 @@ void virNetSocketUpdateIOCallback(virNetSocketPtr sock, void virNetSocketRemoveIOCallback(virNetSocketPtr sock); +void virNetSocketClose(virNetSocketPtr sock); + #endif /* __VIR_NET_SOCKET_H__ */ -- 1.7.6
On 08/01/2011 08:38 AM, Osier Yang wrote:
+++ b/src/rpc/virnetsocket.c @@ -1222,3 +1222,19 @@ void virNetSocketRemoveIOCallback(virNetSocketPtr sock)
virMutexUnlock(&sock->lock); } + +void virNetSocketClose(virNetSocketPtr sock) +{
Can this be called on more than one cleanup path? If so,
+ if (!sock) + return; + + VIR_FORCE_CLOSE(sock->fd); + +#ifdef HAVE_SYS_UN_H + /* If a server socket, then unlink UNIX path */ + if (!sock->client && + sock->localAddr.data.sa.sa_family == AF_UNIX && + sock->localAddr.data.un.sun_path[0] != '\0') + unlink(sock->localAddr.data.un.sun_path);
you should clear out sock->localAddr.data.un.sun_path after unlinking the file, so that the next caller doesn't try to unlink a missing file. Also, this probably needs locking. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
于 2011年08月01日 22:28, Eric Blake 写道:
On 08/01/2011 08:38 AM, Osier Yang wrote:
+++ b/src/rpc/virnetsocket.c @@ -1222,3 +1222,19 @@ void virNetSocketRemoveIOCallback(virNetSocketPtr sock)
virMutexUnlock(&sock->lock); } + +void virNetSocketClose(virNetSocketPtr sock) +{ Can this be called on more than one cleanup path? If so,
+ if (!sock) + return; + + VIR_FORCE_CLOSE(sock->fd); + +#ifdef HAVE_SYS_UN_H + /* If a server socket, then unlink UNIX path */ + if (!sock->client&& + sock->localAddr.data.sa.sa_family == AF_UNIX&& + sock->localAddr.data.un.sun_path[0] != '\0') + unlink(sock->localAddr.data.un.sun_path); you should clear out sock->localAddr.data.un.sun_path after unlinking the file, so that the next caller doesn't try to unlink a missing file.
Also, this probably needs locking.
I think it will not only used for the shutdown cleanup, so updated the patch to clear out sock->localAddr.data.un.sub_path. Thanks Osier
This patch introduces a internal RPC API "virNetServerClose", which is standalone with "virNetServerFree". it closes all the socket fds, and unlinks the unix socket paths, regardless of whether the socket is still referenced or not. This is to address regression bug: https://bugzilla.redhat.com/show_bug.cgi?id=725702 --- daemon/libvirtd.c | 1 + src/rpc/virnetserver.c | 12 ++++++++++++ src/rpc/virnetserver.h | 1 + src/rpc/virnetserverservice.c | 12 ++++++++++++ src/rpc/virnetserverservice.h | 2 ++ src/rpc/virnetsocket.c | 22 ++++++++++++++++++++++ src/rpc/virnetsocket.h | 1 + 7 files changed, 51 insertions(+), 0 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 9e044e2..53f1002 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1558,6 +1558,7 @@ int main(int argc, char **argv) { cleanup: virNetServerProgramFree(remoteProgram); virNetServerProgramFree(qemuProgram); + virNetServerClose(srv); virNetServerFree(srv); if (statuswrite != -1) { if (ret != 0) { diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 2dae2ff..d2a6fef 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -794,3 +794,15 @@ void virNetServerFree(virNetServerPtr srv) virMutexDestroy(&srv->lock); VIR_FREE(srv); } + +void virNetServerClose(virNetServerPtr srv) +{ + int i; + + if (!srv) + return; + + for (i = 0; i < srv->nservices; i++) { + virNetServerServiceClose(srv->services[i]); + } +} diff --git a/src/rpc/virnetserver.h b/src/rpc/virnetserver.h index 810d8a3..324cfb7 100644 --- a/src/rpc/virnetserver.h +++ b/src/rpc/virnetserver.h @@ -85,5 +85,6 @@ void virNetServerQuit(virNetServerPtr srv); void virNetServerFree(virNetServerPtr srv); +void virNetServerClose(virNetServerPtr srv); #endif diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c index d5648dc..8c9ed1e 100644 --- a/src/rpc/virnetserverservice.c +++ b/src/rpc/virnetserverservice.c @@ -280,3 +280,15 @@ void virNetServerServiceToggle(virNetServerServicePtr svc, VIR_EVENT_HANDLE_READABLE : 0); } + +void virNetServerServiceClose(virNetServerServicePtr svc) +{ + int i; + + if (!svc) + return; + + for (i = 0; i < svc->nsocks; i++) { + virNetSocketClose(svc->socks[i]); + } +} diff --git a/src/rpc/virnetserverservice.h b/src/rpc/virnetserverservice.h index 9357598..8540bd9 100644 --- a/src/rpc/virnetserverservice.h +++ b/src/rpc/virnetserverservice.h @@ -66,4 +66,6 @@ void virNetServerServiceFree(virNetServerServicePtr svc); void virNetServerServiceToggle(virNetServerServicePtr svc, bool enabled); +void virNetServerServiceClose(virNetServerServicePtr svc); + #endif diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 41b691a..992e33a 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -1222,3 +1222,25 @@ void virNetSocketRemoveIOCallback(virNetSocketPtr sock) virMutexUnlock(&sock->lock); } + +void virNetSocketClose(virNetSocketPtr sock) +{ + if (!sock) + return; + + virMutexLock(&sock->lock); + + VIR_FORCE_CLOSE(sock->fd); + +#ifdef HAVE_SYS_UN_H + /* If a server socket, then unlink UNIX path */ + if (!sock->client && + sock->localAddr.data.sa.sa_family == AF_UNIX && + sock->localAddr.data.un.sun_path[0] != '\0') { + if (unlink(sock->localAddr.data.un.sun_path) == 0) + sock->localAddr.data.un.sun_path[0] = '\0'; + } +#endif + + virMutexUnlock(&sock->lock); +} diff --git a/src/rpc/virnetsocket.h b/src/rpc/virnetsocket.h index dfb3c5d..1e1c63c 100644 --- a/src/rpc/virnetsocket.h +++ b/src/rpc/virnetsocket.h @@ -118,6 +118,7 @@ void virNetSocketUpdateIOCallback(virNetSocketPtr sock, void virNetSocketRemoveIOCallback(virNetSocketPtr sock); +void virNetSocketClose(virNetSocketPtr sock); #endif /* __VIR_NET_SOCKET_H__ */ -- 1.7.6
On 08/01/2011 09:34 AM, Osier Yang wrote:
This patch introduces a internal RPC API "virNetServerClose", which is standalone with "virNetServerFree". it closes all the socket fds, and unlinks the unix socket paths, regardless of whether the socket is still referenced or not.
This is to address regression bug: https://bugzilla.redhat.com/show_bug.cgi?id=725702
+++ b/src/rpc/virnetserver.c @@ -794,3 +794,15 @@ void virNetServerFree(virNetServerPtr srv) virMutexDestroy(&srv->lock); VIR_FREE(srv); } + +void virNetServerClose(virNetServerPtr srv) +{ + int i; + + if (!srv) + return; + + for (i = 0; i < srv->nservices; i++) { + virNetServerServiceClose(srv->services[i]); + }
Don't we need locking at every level of the chain? That is, srv->nservices involves reading srv, so srv must be locked, just as much as:
+ +void virNetServerServiceClose(virNetServerServicePtr svc) +{ + int i; + + if (!svc) + return; + + for (i = 0; i < svc->nsocks; i++) { + virNetSocketClose(svc->socks[i]);
svc->nsocks, as well as...
+void virNetSocketClose(virNetSocketPtr sock) +{ + if (!sock) + return; + + virMutexLock(&sock->lock);
sock, but you only locked sock.
+ + VIR_FORCE_CLOSE(sock->fd); + +#ifdef HAVE_SYS_UN_H + /* If a server socket, then unlink UNIX path */ + if (!sock->client && + sock->localAddr.data.sa.sa_family == AF_UNIX && + sock->localAddr.data.un.sun_path[0] != '\0') { + if (unlink(sock->localAddr.data.un.sun_path) == 0) + sock->localAddr.data.un.sun_path[0] = '\0'; + }
But I think this change is right. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
于 2011年08月01日 23:39, Eric Blake 写道:
On 08/01/2011 09:34 AM, Osier Yang wrote:
This patch introduces a internal RPC API "virNetServerClose", which is standalone with "virNetServerFree". it closes all the socket fds, and unlinks the unix socket paths, regardless of whether the socket is still referenced or not.
This is to address regression bug: https://bugzilla.redhat.com/show_bug.cgi?id=725702 +++ b/src/rpc/virnetserver.c @@ -794,3 +794,15 @@ void virNetServerFree(virNetServerPtr srv) virMutexDestroy(&srv->lock); VIR_FREE(srv); } + +void virNetServerClose(virNetServerPtr srv) +{ + int i; + + if (!srv) + return; + + for (i = 0; i< srv->nservices; i++) { + virNetServerServiceClose(srv->services[i]); + } Don't we need locking at every level of the chain? That is, srv->nservices involves reading srv, so srv must be locked, just as much as:
Yes, srv needs to be locked, but svc doesn't have lock. I'd like push with locking srv if you are fine with it.
+ +void virNetServerServiceClose(virNetServerServicePtr svc) +{ + int i; + + if (!svc) + return; + + for (i = 0; i< svc->nsocks; i++) { + virNetSocketClose(svc->socks[i]); svc->nsocks, as well as...
+void virNetSocketClose(virNetSocketPtr sock) +{ + if (!sock) + return; + + virMutexLock(&sock->lock); sock, but you only locked sock.
+ + VIR_FORCE_CLOSE(sock->fd); + +#ifdef HAVE_SYS_UN_H + /* If a server socket, then unlink UNIX path */ + if (!sock->client&& + sock->localAddr.data.sa.sa_family == AF_UNIX&& + sock->localAddr.data.un.sun_path[0] != '\0') { + if (unlink(sock->localAddr.data.un.sun_path) == 0) + sock->localAddr.data.un.sun_path[0] = '\0'; + } But I think this change is right.
participants (2)
-
Eric Blake -
Osier Yang