When the service passed to getaddrinfo is NULL the kernel will choose a free port to bind to. In a dual stack though we will get separate sockets for IPv4 and IPv6 and we need them to bind to the same port number. Thus once the kerel has auto-selected a port for the first socket, we must disable auto-select for subsequent IP sockets and force reuse of the first port. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- src/rpc/virnetsocket.c | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 254f39c4ec..fc13b1654a 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -311,6 +311,7 @@ int virNetSocketNewListenTCP(const char *nodename, int socketErrno = 0; int bindErrno = 0; virSocketAddr tmp_addr; + int port = 0; *retsocks = NULL; *nretsocks = 0; @@ -379,7 +380,24 @@ int virNetSocketNewListenTCP(const char *nodename, } #endif - if (bind(fd, runp->ai_addr, runp->ai_addrlen) < 0) { + addr.len = runp->ai_addrlen; + memcpy(&addr.data.sa, runp->ai_addr, runp->ai_addrlen); + + /* When service is NULL, we let the kernel auto-select the + * port. Once we've selected a port for one IP protocol + * though, we want to ensure we pick the same port for the + * other IP protocol + */ + if (port != 0 && service == NULL) { + if (runp->ai_addr->sa_family == AF_INET) { + addr.data.inet4.sin_port = port; + } else if (addr.data.sa.sa_family == AF_INET6) { + addr.data.inet6.sin6_port = port; + } + VIR_DEBUG("Used saved port %d", port); + } + + if (bind(fd, &addr.data.sa, addr.len) < 0) { if (errno != EADDRINUSE && errno != EADDRNOTAVAIL) { virReportSystemError(errno, "%s", _("Unable to bind to port")); goto error; @@ -396,6 +414,14 @@ int virNetSocketNewListenTCP(const char *nodename, goto error; } + if (port == 0 && service == NULL) { + if (addr.data.sa.sa_family == AF_INET) + port = addr.data.inet4.sin_port; + else if (addr.data.sa.sa_family == AF_INET6) + port = addr.data.inet6.sin6_port; + VIR_DEBUG("Saved port %d", port); + } + VIR_DEBUG("%p f=%d f=%d", &addr, runp->ai_family, addr.data.sa.sa_family); if (VIR_EXPAND_N(socks, nsocks, 1) < 0) -- 2.21.0

Introduce a virNetServerServiceNewSocket API that allows the various constructors to share more code. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- src/rpc/virnetserverservice.c | 165 +++++++++++++++------------------- 1 file changed, 74 insertions(+), 91 deletions(-) diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c index 97341d1546..69043ccc0e 100644 --- a/src/rpc/virnetserverservice.c +++ b/src/rpc/virnetserverservice.c @@ -128,14 +128,14 @@ virNetServerServiceNewFDOrUNIX(const char *path, } -virNetServerServicePtr virNetServerServiceNewTCP(const char *nodename, - const char *service, - int family, - int auth, - virNetTLSContextPtr tls, - bool readonly, - size_t max_queued_clients, - size_t nrequests_client_max) +static virNetServerServicePtr +virNetServerServiceNewSocket(virNetSocketPtr *socks, + size_t nsocks, + int auth, + virNetTLSContextPtr tls, + bool readonly, + size_t max_queued_clients, + size_t nrequests_client_max) { virNetServerServicePtr svc; size_t i; @@ -146,18 +146,18 @@ virNetServerServicePtr virNetServerServiceNewTCP(const char *nodename, if (!(svc = virObjectNew(virNetServerServiceClass))) return NULL; + if (VIR_ALLOC_N(svc->socks, nsocks) < 0) + goto error; + svc->nsocks = nsocks; + for (i = 0; i < svc->nsocks; i++) { + svc->socks[i] = socks[i]; + virObjectRef(svc->socks[i]); + } svc->auth = auth; svc->readonly = readonly; svc->nrequests_client_max = nrequests_client_max; svc->tls = virObjectRef(tls); - if (virNetSocketNewListenTCP(nodename, - service, - family, - &svc->socks, - &svc->nsocks) < 0) - goto error; - for (i = 0; i < svc->nsocks; i++) { if (virNetSocketListen(svc->socks[i], max_queued_clients) < 0) goto error; @@ -184,6 +184,43 @@ virNetServerServicePtr virNetServerServiceNewTCP(const char *nodename, } +virNetServerServicePtr virNetServerServiceNewTCP(const char *nodename, + const char *service, + int family, + int auth, + virNetTLSContextPtr tls, + bool readonly, + size_t max_queued_clients, + size_t nrequests_client_max) +{ + virNetServerServicePtr svc; + size_t i; + virNetSocketPtr *socks; + size_t nsocks; + + if (virNetSocketNewListenTCP(nodename, + service, + family, + &socks, + &nsocks) < 0) + return NULL; + + svc = virNetServerServiceNewSocket(socks, + nsocks, + auth, + tls, + readonly, + max_queued_clients, + nrequests_client_max); + + for (i = 0; i < nsocks; i++) + virObjectUnref(socks[i]); + VIR_FREE(socks); + + return svc; +} + + virNetServerServicePtr virNetServerServiceNewUNIX(const char *path, mode_t mask, gid_t grp, @@ -194,53 +231,26 @@ virNetServerServicePtr virNetServerServiceNewUNIX(const char *path, size_t nrequests_client_max) { virNetServerServicePtr svc; - size_t i; - - if (virNetServerServiceInitialize() < 0) - return NULL; - - if (!(svc = virObjectNew(virNetServerServiceClass))) - return NULL; - - svc->auth = auth; - svc->readonly = readonly; - svc->nrequests_client_max = nrequests_client_max; - svc->tls = virObjectRef(tls); - - if (VIR_ALLOC_N(svc->socks, 1) < 0) - goto error; - svc->nsocks = 1; + virNetSocketPtr sock; if (virNetSocketNewListenUNIX(path, mask, -1, grp, - &svc->socks[0]) < 0) - goto error; - - for (i = 0; i < svc->nsocks; i++) { - if (virNetSocketListen(svc->socks[i], max_queued_clients) < 0) - goto error; + &sock) < 0) + return NULL; - /* IO callback is initially disabled, until we're ready - * to deal with incoming clients */ - virObjectRef(svc); - if (virNetSocketAddIOCallback(svc->socks[i], - 0, - virNetServerServiceAccept, - svc, - virObjectFreeCallback) < 0) { - virObjectUnref(svc); - goto error; - } - } + svc = virNetServerServiceNewSocket(&sock, + 1, + auth, + tls, + readonly, + max_queued_clients, + nrequests_client_max); + virObjectUnref(sock); return svc; - - error: - virObjectUnref(svc); - return NULL; } virNetServerServicePtr virNetServerServiceNewFD(int fd, @@ -251,50 +261,23 @@ virNetServerServicePtr virNetServerServiceNewFD(int fd, size_t nrequests_client_max) { virNetServerServicePtr svc; - size_t i; - - if (virNetServerServiceInitialize() < 0) - return NULL; - - if (!(svc = virObjectNew(virNetServerServiceClass))) - return NULL; - - svc->auth = auth; - svc->readonly = readonly; - svc->nrequests_client_max = nrequests_client_max; - svc->tls = virObjectRef(tls); - - if (VIR_ALLOC_N(svc->socks, 1) < 0) - goto error; - svc->nsocks = 1; + virNetSocketPtr sock; if (virNetSocketNewListenFD(fd, - &svc->socks[0]) < 0) - goto error; - - for (i = 0; i < svc->nsocks; i++) { - if (virNetSocketListen(svc->socks[i], max_queued_clients) < 0) - goto error; + &sock) < 0) + return NULL; - /* IO callback is initially disabled, until we're ready - * to deal with incoming clients */ - virObjectRef(svc); - if (virNetSocketAddIOCallback(svc->socks[i], - 0, - virNetServerServiceAccept, - svc, - virObjectFreeCallback) < 0) { - virObjectUnref(svc); - goto error; - } - } + svc = virNetServerServiceNewSocket(&sock, + 1, + auth, + tls, + readonly, + max_queued_clients, + nrequests_client_max); + virObjectUnref(sock); return svc; - - error: - virObjectUnref(svc); - return NULL; } -- 2.21.0

Currently the socket code will unlink any UNIX socket path which is associated with a server socket. This is not fine grained enough, as we need to avoid unlinking server sockets we were passed by systemd. To deal with this we must explicitly track whether each socket needs to be unlinked when closed, separately of the client vs server state. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- src/locking/lock_daemon.c | 1 + src/logging/log_daemon.c | 1 + src/rpc/virnetserverservice.c | 3 + src/rpc/virnetserverservice.h | 1 + src/rpc/virnetsocket.c | 55 ++++++++++++------- src/rpc/virnetsocket.h | 1 + .../input-data-anon-clients.json | 12 ++-- .../output-data-admin-server-names.json | 24 +++++--- tests/virnetdaemondata/output-data-admin.json | 24 +++++--- .../output-data-anon-clients.json | 12 ++-- .../output-data-client-auth-pending.json | 12 ++-- .../output-data-client-ids.json | 12 ++-- .../output-data-client-timestamp.json | 12 ++-- .../virnetdaemondata/output-data-initial.json | 12 ++-- .../output-data-no-keepalive-required.json | 24 +++++--- 15 files changed, 138 insertions(+), 68 deletions(-) diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index c10b2d383c..0f90606be6 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -619,6 +619,7 @@ virLockDaemonSetupNetworkingSystemD(virNetServerPtr lockSrv, virNetServerPtr adm * so the first FD we'll get is '3'. */ if (!(svc = virNetServerServiceNewFDs(fds, ARRAY_CARDINALITY(fds), + false, 0, NULL, false, 0, 1))) diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c index 6531999381..30c70a20dd 100644 --- a/src/logging/log_daemon.c +++ b/src/logging/log_daemon.c @@ -554,6 +554,7 @@ virLogDaemonSetupNetworkingSystemD(virNetServerPtr logSrv, virNetServerPtr admin * so the first FD we'll get is '3'. */ if (!(svc = virNetServerServiceNewFDs(fds, ARRAY_CARDINALITY(fds), + false, 0, NULL, false, 0, 1))) diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c index 0d2f264696..315a4950df 100644 --- a/src/rpc/virnetserverservice.c +++ b/src/rpc/virnetserverservice.c @@ -121,6 +121,7 @@ virNetServerServiceNewFDOrUNIX(const char *path, */ return virNetServerServiceNewFDs(fds, ARRAY_CARDINALITY(fds), + false, auth, tls, readonly, @@ -257,6 +258,7 @@ virNetServerServicePtr virNetServerServiceNewUNIX(const char *path, virNetServerServicePtr virNetServerServiceNewFDs(int *fds, size_t nfds, + bool unlinkUNIX, int auth, virNetTLSContextPtr tls, bool readonly, @@ -272,6 +274,7 @@ virNetServerServicePtr virNetServerServiceNewFDs(int *fds, for (i = 0; i < nfds; i++) { if (virNetSocketNewListenFD(fds[i], + unlinkUNIX, &socks[i]) < 0) goto cleanup; } diff --git a/src/rpc/virnetserverservice.h b/src/rpc/virnetserverservice.h index 59ee51e5ee..73d61dde99 100644 --- a/src/rpc/virnetserverservice.h +++ b/src/rpc/virnetserverservice.h @@ -62,6 +62,7 @@ virNetServerServicePtr virNetServerServiceNewUNIX(const char *path, size_t nrequests_client_max); virNetServerServicePtr virNetServerServiceNewFDs(int *fd, size_t nfds, + bool unlinkUNIX, int auth, virNetTLSContextPtr tls, bool readonly, diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index fc13b1654a..e23f6a648c 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -78,9 +78,10 @@ struct _virNetSocket { int watch; pid_t pid; int errfd; - bool client; + bool isClient; bool ownsFd; bool quietEOF; + bool unlinkUNIX; /* Event callback fields */ virNetSocketIOFunc func; @@ -216,10 +217,14 @@ int virNetSocketCheckProtocols(bool *hasIPv4, } -static virNetSocketPtr virNetSocketNew(virSocketAddrPtr localAddr, - virSocketAddrPtr remoteAddr, - bool isClient, - int fd, int errfd, pid_t pid) +static virNetSocketPtr +virNetSocketNew(virSocketAddrPtr localAddr, + virSocketAddrPtr remoteAddr, + bool isClient, + int fd, + int errfd, + pid_t pid, + bool unlinkUNIX) { virNetSocketPtr sock; int no_slow_start = 1; @@ -254,6 +259,8 @@ static virNetSocketPtr virNetSocketNew(virSocketAddrPtr localAddr, sock->pid = pid; sock->watch = -1; sock->ownsFd = true; + sock->isClient = isClient; + sock->unlinkUNIX = unlinkUNIX; /* Disable nagle for TCP sockets */ if (sock->localAddr.data.sa.sa_family == AF_INET || @@ -280,8 +287,6 @@ static virNetSocketPtr virNetSocketNew(virSocketAddrPtr localAddr, !(sock->remoteAddrStrURI = virSocketAddrFormatFull(remoteAddr, true, NULL))) goto error; - sock->client = isClient; - PROBE(RPC_SOCKET_NEW, "sock=%p fd=%d errfd=%d pid=%lld localAddr=%s, remoteAddr=%s", sock, fd, errfd, (long long)pid, @@ -427,7 +432,7 @@ int virNetSocketNewListenTCP(const char *nodename, if (VIR_EXPAND_N(socks, nsocks, 1) < 0) goto error; - if (!(socks[nsocks-1] = virNetSocketNew(&addr, NULL, false, fd, -1, 0))) + if (!(socks[nsocks-1] = virNetSocketNew(&addr, NULL, false, fd, -1, 0, false))) goto error; runp = runp->ai_next; fd = -1; @@ -513,7 +518,7 @@ int virNetSocketNewListenUNIX(const char *path, goto error; } - if (!(*retsock = virNetSocketNew(&addr, NULL, false, fd, -1, 0))) + if (!(*retsock = virNetSocketNew(&addr, NULL, false, fd, -1, 0, true))) goto error; return 0; @@ -538,6 +543,7 @@ int virNetSocketNewListenUNIX(const char *path ATTRIBUTE_UNUSED, #endif int virNetSocketNewListenFD(int fd, + bool unlinkUNIX, virNetSocketPtr *retsock) { virSocketAddr addr; @@ -551,7 +557,7 @@ int virNetSocketNewListenFD(int fd, return -1; } - if (!(*retsock = virNetSocketNew(&addr, NULL, false, fd, -1, 0))) + if (!(*retsock = virNetSocketNew(&addr, NULL, false, fd, -1, 0, unlinkUNIX))) return -1; return 0; @@ -627,7 +633,7 @@ int virNetSocketNewConnectTCP(const char *nodename, goto error; } - if (!(*retsock = virNetSocketNew(&localAddr, &remoteAddr, true, fd, -1, 0))) + if (!(*retsock = virNetSocketNew(&localAddr, &remoteAddr, true, fd, -1, 0, false))) goto error; freeaddrinfo(ai); @@ -752,7 +758,7 @@ int virNetSocketNewConnectUNIX(const char *path, goto cleanup; } - if (!(*retsock = virNetSocketNew(&localAddr, &remoteAddr, true, fd, -1, 0))) + if (!(*retsock = virNetSocketNew(&localAddr, &remoteAddr, true, fd, -1, 0, false))) goto cleanup; ret = 0; @@ -820,7 +826,7 @@ int virNetSocketNewConnectCommand(virCommandPtr cmd, VIR_FORCE_CLOSE(sv[1]); VIR_FORCE_CLOSE(errfd[1]); - if (!(*retsock = virNetSocketNew(NULL, NULL, true, sv[0], errfd[0], pid))) + if (!(*retsock = virNetSocketNew(NULL, NULL, true, sv[0], errfd[0], pid, false))) goto error; virCommandFree(cmd); @@ -1219,7 +1225,7 @@ int virNetSocketNewConnectSockFD(int sockfd, return -1; } - if (!(*retsock = virNetSocketNew(&localAddr, NULL, true, sockfd, -1, -1))) + if (!(*retsock = virNetSocketNew(&localAddr, NULL, true, sockfd, -1, -1, false))) return -1; return 0; @@ -1232,6 +1238,7 @@ virNetSocketPtr virNetSocketNewPostExecRestart(virJSONValuePtr object) virSocketAddr remoteAddr; int fd, thepid, errfd; bool isClient; + bool unlinkUNIX; if (virJSONValueObjectGetNumberInt(object, "fd", &fd) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -1250,12 +1257,16 @@ virNetSocketPtr virNetSocketNewPostExecRestart(virJSONValuePtr object) _("Missing errfd data in JSON document")); return NULL; } + if (virJSONValueObjectGetBoolean(object, "isClient", &isClient) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing isClient data in JSON document")); return NULL; } + if (virJSONValueObjectGetBoolean(object, "unlinkUNIX", &unlinkUNIX) < 0) + unlinkUNIX = !isClient; + memset(&localAddr, 0, sizeof(localAddr)); memset(&remoteAddr, 0, sizeof(remoteAddr)); @@ -1271,8 +1282,8 @@ virNetSocketPtr virNetSocketNewPostExecRestart(virJSONValuePtr object) return NULL; } - return virNetSocketNew(&localAddr, &remoteAddr, - isClient, fd, errfd, thepid); + return virNetSocketNew(&localAddr, &remoteAddr, isClient, + fd, errfd, thepid, unlinkUNIX); } @@ -1309,7 +1320,10 @@ virJSONValuePtr virNetSocketPreExecRestart(virNetSocketPtr sock) if (virJSONValueObjectAppendNumberInt(object, "pid", sock->pid) < 0) goto error; - if (virJSONValueObjectAppendBoolean(object, "isClient", sock->client) < 0) + if (virJSONValueObjectAppendBoolean(object, "isClient", sock->isClient) < 0) + goto error; + + if (virJSONValueObjectAppendBoolean(object, "unlinkUNIX", sock->unlinkUNIX) < 0) goto error; if (virSetInherit(sock->fd, true) < 0) { @@ -1350,7 +1364,7 @@ void virNetSocketDispose(void *obj) #ifdef HAVE_SYS_UN_H /* If a server socket, then unlink UNIX path */ - if (!sock->client && + if (sock->unlinkUNIX && sock->localAddr.data.sa.sa_family == AF_UNIX && sock->localAddr.data.un.sun_path[0] != '\0') unlink(sock->localAddr.data.un.sun_path); @@ -2141,7 +2155,8 @@ int virNetSocketAccept(virNetSocketPtr sock, virNetSocketPtr *clientsock) if (!(*clientsock = virNetSocketNew(&localAddr, &remoteAddr, true, - fd, -1, 0))) + fd, -1, 0, + false))) goto cleanup; fd = -1; @@ -2272,7 +2287,7 @@ void virNetSocketClose(virNetSocketPtr sock) #ifdef HAVE_SYS_UN_H /* If a server socket, then unlink UNIX path */ - if (!sock->client && + if (sock->unlinkUNIX && 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) diff --git a/src/rpc/virnetsocket.h b/src/rpc/virnetsocket.h index de5a465cde..2f626cb08f 100644 --- a/src/rpc/virnetsocket.h +++ b/src/rpc/virnetsocket.h @@ -58,6 +58,7 @@ int virNetSocketNewListenUNIX(const char *path, virNetSocketPtr *addr); int virNetSocketNewListenFD(int fd, + bool unlinkUNIX, virNetSocketPtr *addr); int virNetSocketNewConnectTCP(const char *nodename, diff --git a/tests/virnetdaemondata/input-data-anon-clients.json b/tests/virnetdaemondata/input-data-anon-clients.json index 8058fe0a43..3ca2af3899 100644 --- a/tests/virnetdaemondata/input-data-anon-clients.json +++ b/tests/virnetdaemondata/input-data-anon-clients.json @@ -17,7 +17,8 @@ "fd": 100, "errfd": -1, "pid": 0, - "isClient": false + "isClient": false, + "unlinkUNIX": true } ] }, @@ -30,7 +31,8 @@ "fd": 101, "errfd": -1, "pid": 0, - "isClient": false + "isClient": false, + "unlinkUNIX": true } ] } @@ -44,7 +46,8 @@ "fd": 102, "errfd": -1, "pid": -1, - "isClient": true + "isClient": true, + "unlinkUNIX": false }, "privateData": 1729 }, @@ -56,7 +59,8 @@ "fd": 103, "errfd": -1, "pid": -1, - "isClient": true + "isClient": true, + "unlinkUNIX": false }, "privateData": 1729 } diff --git a/tests/virnetdaemondata/output-data-admin-server-names.json b/tests/virnetdaemondata/output-data-admin-server-names.json index 04cb5e6bb3..4488a23291 100644 --- a/tests/virnetdaemondata/output-data-admin-server-names.json +++ b/tests/virnetdaemondata/output-data-admin-server-names.json @@ -19,7 +19,8 @@ "fd": 100, "errfd": -1, "pid": 0, - "isClient": false + "isClient": false, + "unlinkUNIX": true } ] }, @@ -32,7 +33,8 @@ "fd": 101, "errfd": -1, "pid": 0, - "isClient": false + "isClient": false, + "unlinkUNIX": true } ] } @@ -48,7 +50,8 @@ "fd": 102, "errfd": -1, "pid": -1, - "isClient": true + "isClient": true, + "unlinkUNIX": false }, "privateData": 1729 }, @@ -62,7 +65,8 @@ "fd": 103, "errfd": -1, "pid": -1, - "isClient": true + "isClient": true, + "unlinkUNIX": false }, "privateData": 1729 } @@ -87,7 +91,8 @@ "fd": 100, "errfd": -1, "pid": 0, - "isClient": false + "isClient": false, + "unlinkUNIX": true } ] }, @@ -100,7 +105,8 @@ "fd": 101, "errfd": -1, "pid": 0, - "isClient": false + "isClient": false, + "unlinkUNIX": true } ] } @@ -116,7 +122,8 @@ "fd": 102, "errfd": -1, "pid": -1, - "isClient": true + "isClient": true, + "unlinkUNIX": false }, "privateData": 1729 }, @@ -130,7 +137,8 @@ "fd": 103, "errfd": -1, "pid": -1, - "isClient": true + "isClient": true, + "unlinkUNIX": false }, "privateData": 1729 } diff --git a/tests/virnetdaemondata/output-data-admin.json b/tests/virnetdaemondata/output-data-admin.json index 04cb5e6bb3..4488a23291 100644 --- a/tests/virnetdaemondata/output-data-admin.json +++ b/tests/virnetdaemondata/output-data-admin.json @@ -19,7 +19,8 @@ "fd": 100, "errfd": -1, "pid": 0, - "isClient": false + "isClient": false, + "unlinkUNIX": true } ] }, @@ -32,7 +33,8 @@ "fd": 101, "errfd": -1, "pid": 0, - "isClient": false + "isClient": false, + "unlinkUNIX": true } ] } @@ -48,7 +50,8 @@ "fd": 102, "errfd": -1, "pid": -1, - "isClient": true + "isClient": true, + "unlinkUNIX": false }, "privateData": 1729 }, @@ -62,7 +65,8 @@ "fd": 103, "errfd": -1, "pid": -1, - "isClient": true + "isClient": true, + "unlinkUNIX": false }, "privateData": 1729 } @@ -87,7 +91,8 @@ "fd": 100, "errfd": -1, "pid": 0, - "isClient": false + "isClient": false, + "unlinkUNIX": true } ] }, @@ -100,7 +105,8 @@ "fd": 101, "errfd": -1, "pid": 0, - "isClient": false + "isClient": false, + "unlinkUNIX": true } ] } @@ -116,7 +122,8 @@ "fd": 102, "errfd": -1, "pid": -1, - "isClient": true + "isClient": true, + "unlinkUNIX": false }, "privateData": 1729 }, @@ -130,7 +137,8 @@ "fd": 103, "errfd": -1, "pid": -1, - "isClient": true + "isClient": true, + "unlinkUNIX": false }, "privateData": 1729 } diff --git a/tests/virnetdaemondata/output-data-anon-clients.json b/tests/virnetdaemondata/output-data-anon-clients.json index 49fe89be48..c7090d2ed8 100644 --- a/tests/virnetdaemondata/output-data-anon-clients.json +++ b/tests/virnetdaemondata/output-data-anon-clients.json @@ -19,7 +19,8 @@ "fd": 100, "errfd": -1, "pid": 0, - "isClient": false + "isClient": false, + "unlinkUNIX": true } ] }, @@ -32,7 +33,8 @@ "fd": 101, "errfd": -1, "pid": 0, - "isClient": false + "isClient": false, + "unlinkUNIX": true } ] } @@ -48,7 +50,8 @@ "fd": 102, "errfd": -1, "pid": -1, - "isClient": true + "isClient": true, + "unlinkUNIX": false }, "privateData": 1729 }, @@ -62,7 +65,8 @@ "fd": 103, "errfd": -1, "pid": -1, - "isClient": true + "isClient": true, + "unlinkUNIX": false }, "privateData": 1729 } diff --git a/tests/virnetdaemondata/output-data-client-auth-pending.json b/tests/virnetdaemondata/output-data-client-auth-pending.json index 0675404e6c..9011588d8d 100644 --- a/tests/virnetdaemondata/output-data-client-auth-pending.json +++ b/tests/virnetdaemondata/output-data-client-auth-pending.json @@ -19,7 +19,8 @@ "fd": 100, "errfd": -1, "pid": 0, - "isClient": false + "isClient": false, + "unlinkUNIX": true } ] }, @@ -32,7 +33,8 @@ "fd": 101, "errfd": -1, "pid": 0, - "isClient": false + "isClient": false, + "unlinkUNIX": true } ] } @@ -48,7 +50,8 @@ "fd": 102, "errfd": -1, "pid": -1, - "isClient": true + "isClient": true, + "unlinkUNIX": false }, "privateData": 1729 }, @@ -62,7 +65,8 @@ "fd": 103, "errfd": -1, "pid": -1, - "isClient": true + "isClient": true, + "unlinkUNIX": false }, "privateData": 1729 } diff --git a/tests/virnetdaemondata/output-data-client-ids.json b/tests/virnetdaemondata/output-data-client-ids.json index 90c3383a93..5840757614 100644 --- a/tests/virnetdaemondata/output-data-client-ids.json +++ b/tests/virnetdaemondata/output-data-client-ids.json @@ -19,7 +19,8 @@ "fd": 100, "errfd": -1, "pid": 0, - "isClient": false + "isClient": false, + "unlinkUNIX": true } ] }, @@ -32,7 +33,8 @@ "fd": 101, "errfd": -1, "pid": 0, - "isClient": false + "isClient": false, + "unlinkUNIX": true } ] } @@ -48,7 +50,8 @@ "fd": 102, "errfd": -1, "pid": -1, - "isClient": true + "isClient": true, + "unlinkUNIX": false }, "privateData": 1729 }, @@ -62,7 +65,8 @@ "fd": 103, "errfd": -1, "pid": -1, - "isClient": true + "isClient": true, + "unlinkUNIX": false }, "privateData": 1729 } diff --git a/tests/virnetdaemondata/output-data-client-timestamp.json b/tests/virnetdaemondata/output-data-client-timestamp.json index 9cfb069793..e685475c4a 100644 --- a/tests/virnetdaemondata/output-data-client-timestamp.json +++ b/tests/virnetdaemondata/output-data-client-timestamp.json @@ -19,7 +19,8 @@ "fd": 100, "errfd": -1, "pid": 0, - "isClient": false + "isClient": false, + "unlinkUNIX": true } ] }, @@ -32,7 +33,8 @@ "fd": 101, "errfd": -1, "pid": 0, - "isClient": false + "isClient": false, + "unlinkUNIX": true } ] } @@ -49,7 +51,8 @@ "fd": 102, "errfd": -1, "pid": -1, - "isClient": true + "isClient": true, + "unlinkUNIX": false }, "privateData": 1729 }, @@ -64,7 +67,8 @@ "fd": 103, "errfd": -1, "pid": -1, - "isClient": true + "isClient": true, + "unlinkUNIX": false }, "privateData": 1729 } diff --git a/tests/virnetdaemondata/output-data-initial.json b/tests/virnetdaemondata/output-data-initial.json index 916297c59d..f887d37379 100644 --- a/tests/virnetdaemondata/output-data-initial.json +++ b/tests/virnetdaemondata/output-data-initial.json @@ -19,7 +19,8 @@ "fd": 100, "errfd": -1, "pid": 0, - "isClient": false + "isClient": false, + "unlinkUNIX": true } ] }, @@ -32,7 +33,8 @@ "fd": 101, "errfd": -1, "pid": 0, - "isClient": false + "isClient": false, + "unlinkUNIX": true } ] } @@ -48,7 +50,8 @@ "fd": 102, "errfd": -1, "pid": -1, - "isClient": true + "isClient": true, + "unlinkUNIX": false }, "privateData": 1729 }, @@ -62,7 +65,8 @@ "fd": 103, "errfd": -1, "pid": -1, - "isClient": true + "isClient": true, + "unlinkUNIX": false }, "privateData": 1729 } diff --git a/tests/virnetdaemondata/output-data-no-keepalive-required.json b/tests/virnetdaemondata/output-data-no-keepalive-required.json index 04cb5e6bb3..4488a23291 100644 --- a/tests/virnetdaemondata/output-data-no-keepalive-required.json +++ b/tests/virnetdaemondata/output-data-no-keepalive-required.json @@ -19,7 +19,8 @@ "fd": 100, "errfd": -1, "pid": 0, - "isClient": false + "isClient": false, + "unlinkUNIX": true } ] }, @@ -32,7 +33,8 @@ "fd": 101, "errfd": -1, "pid": 0, - "isClient": false + "isClient": false, + "unlinkUNIX": true } ] } @@ -48,7 +50,8 @@ "fd": 102, "errfd": -1, "pid": -1, - "isClient": true + "isClient": true, + "unlinkUNIX": false }, "privateData": 1729 }, @@ -62,7 +65,8 @@ "fd": 103, "errfd": -1, "pid": -1, - "isClient": true + "isClient": true, + "unlinkUNIX": false }, "privateData": 1729 } @@ -87,7 +91,8 @@ "fd": 100, "errfd": -1, "pid": 0, - "isClient": false + "isClient": false, + "unlinkUNIX": true } ] }, @@ -100,7 +105,8 @@ "fd": 101, "errfd": -1, "pid": 0, - "isClient": false + "isClient": false, + "unlinkUNIX": true } ] } @@ -116,7 +122,8 @@ "fd": 102, "errfd": -1, "pid": -1, - "isClient": true + "isClient": true, + "unlinkUNIX": false }, "privateData": 1729 }, @@ -130,7 +137,8 @@ "fd": 103, "errfd": -1, "pid": -1, - "isClient": true + "isClient": true, + "unlinkUNIX": false }, "privateData": 1729 } -- 2.21.0

Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- src/libvirt_remote.syms | 1 + src/rpc/virnetserver.c | 17 +++++++++++++++++ src/rpc/virnetserver.h | 3 +++ 3 files changed, 21 insertions(+) diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms index d855078186..386137c0d8 100644 --- a/src/libvirt_remote.syms +++ b/src/libvirt_remote.syms @@ -126,6 +126,7 @@ virNetServerGetMaxUnauthClients; virNetServerGetName; virNetServerGetThreadPoolParameters; virNetServerHasClients; +virNetServerNeedsAuth; virNetServerNew; virNetServerNewPostExecRestart; virNetServerNextClientID; diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 894feae406..e229f57bab 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -1098,6 +1098,23 @@ virNetServerGetCurrentUnauthClients(virNetServerPtr srv) return ret; } + +bool virNetServerNeedsAuth(virNetServerPtr srv, + int auth) +{ + bool ret = false; + size_t i; + + virObjectLock(srv); + for (i = 0; i < srv->nservices; i++) { + if (virNetServerServiceGetAuth(srv->services[i]) == auth) + ret = true; + } + virObjectUnlock(srv); + + return ret; +} + int virNetServerGetClients(virNetServerPtr srv, virNetServerClientPtr **clts) diff --git a/src/rpc/virnetserver.h b/src/rpc/virnetserver.h index 1b4184733f..260c99b22d 100644 --- a/src/rpc/virnetserver.h +++ b/src/rpc/virnetserver.h @@ -119,6 +119,9 @@ unsigned long long virNetServerNextClientID(virNetServerPtr srv); virNetServerClientPtr virNetServerGetClient(virNetServerPtr srv, unsigned long long id); +bool virNetServerNeedsAuth(virNetServerPtr srv, + int auth); + int virNetServerGetClients(virNetServerPtr srv, virNetServerClientPtr **clients); -- 2.21.0

The current libvirtd code for systemd socket activation assumes socket FDs are passed in the order unix-rw, unix-ro, unix-admin. There is in fact no ordering guarantee made by systemd. Applications are expected to check the address or name associated with each FD to figure out its identity. This rewrites libvirtd to make use of the new systemd activation APIs to make it robust wrt socket ordering changes. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- src/remote/remote_daemon.c | 244 ++++++++++++++++------------------ src/rpc/virnetserverservice.c | 7 + 2 files changed, 125 insertions(+), 126 deletions(-) diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c index 0dabd3dff8..b5228e8176 100644 --- a/src/remote/remote_daemon.c +++ b/src/remote/remote_daemon.c @@ -56,6 +56,7 @@ #include "virutil.h" #include "virgettext.h" #include "util/virnetdevopenvswitch.h" +#include "virsystemd.h" #include "driver.h" @@ -367,30 +368,34 @@ daemonSetupNetworking(virNetServerPtr srv, bool ipsock, bool privileged) { - virNetServerServicePtr svc = NULL; - virNetServerServicePtr svcAdm = NULL; - virNetServerServicePtr svcRO = NULL; - virNetServerServicePtr svcTCP = NULL; - virNetServerServicePtr svcTLS = NULL; gid_t unix_sock_gid = 0; int unix_sock_ro_mask = 0; int unix_sock_rw_mask = 0; int unix_sock_adm_mask = 0; int ret = -1; + VIR_AUTOPTR(virSystemdActivation) act = NULL; + virSystemdActivationMap actmap[] = { + { .name = "libvirtd.socket", .family = AF_UNIX, .path = sock_path }, + { .name = "libvirtd-ro.socket", .family = AF_UNIX, .path = sock_path_ro }, + { .name = "libvirtd-admin.socket", .family = AF_UNIX, .path = sock_path_adm }, + { .name = "libvirtd-tcp.socket", .family = AF_INET }, + { .name = "libvirtd-tls.socket", .family = AF_INET }, + }; + + if ((actmap[3].port = virSocketAddrResolveService(config->tcp_port)) < 0) + return -1; + + if ((actmap[4].port = virSocketAddrResolveService(config->tls_port)) < 0) + return -1; - unsigned int cur_fd = STDERR_FILENO + 1; - unsigned int nfds = virGetListenFDs(); + if (virSystemdGetActivation(actmap, ARRAY_CARDINALITY(actmap), &act) < 0) + return -1; if (config->unix_sock_group) { if (virGetGroupID(config->unix_sock_group, &unix_sock_gid) < 0) return ret; } - if (nfds > (sock_path_ro ? 2 : 1)) { - VIR_ERROR(_("Too many (%u) FDs passed from caller"), nfds); - return ret; - } - if (virStrToLong_i(config->unix_sock_ro_perms, NULL, 8, &unix_sock_ro_mask) != 0) { VIR_ERROR(_("Failed to parse mode '%s'"), config->unix_sock_ro_perms); goto cleanup; @@ -406,148 +411,135 @@ daemonSetupNetworking(virNetServerPtr srv, goto cleanup; } - if (!(svc = virNetServerServiceNewFDOrUNIX(sock_path, - unix_sock_rw_mask, - unix_sock_gid, - config->auth_unix_rw, - NULL, - false, - config->max_queued_clients, - config->max_client_requests, - nfds, &cur_fd))) + if (virNetServerAddServiceUNIX(srv, + act, + "libvirtd.socket", + sock_path, + unix_sock_rw_mask, + unix_sock_gid, + config->auth_unix_rw, + NULL, + false, + config->max_queued_clients, + config->max_client_requests) < 0) goto cleanup; - if (sock_path_ro) { - if (!(svcRO = virNetServerServiceNewFDOrUNIX(sock_path_ro, - unix_sock_ro_mask, - unix_sock_gid, - config->auth_unix_ro, - NULL, - true, - config->max_queued_clients, - config->max_client_requests, - nfds, &cur_fd))) - goto cleanup; - } - - if (virNetServerAddService(srv, svc) < 0) + if (sock_path_ro && + virNetServerAddServiceUNIX(srv, + act, + "libvirtd-ro.socket", + sock_path_ro, + unix_sock_ro_mask, + unix_sock_gid, + config->auth_unix_ro, + NULL, + true, + config->max_queued_clients, + config->max_client_requests) < 0) goto cleanup; - if (svcRO && - virNetServerAddService(srv, svcRO) < 0) + if (sock_path_adm && + virNetServerAddServiceUNIX(srvAdm, + act, + "libvirtd-admin.socket", + sock_path_adm, + unix_sock_adm_mask, + unix_sock_gid, + REMOTE_AUTH_NONE, + NULL, + false, + config->admin_max_queued_clients, + config->admin_max_client_requests) < 0) goto cleanup; - if (sock_path_adm) { - VIR_DEBUG("Registering unix socket %s", sock_path_adm); - if (!(svcAdm = virNetServerServiceNewUNIX(sock_path_adm, - unix_sock_adm_mask, - unix_sock_gid, - REMOTE_AUTH_NONE, - NULL, - false, - config->admin_max_queued_clients, - config->admin_max_client_requests))) - goto cleanup; + if (((ipsock && config->listen_tcp) || act) && + virNetServerAddServiceTCP(srv, + act, + "libvirtd-tcp.socket", + config->listen_addr, + config->tcp_port, + AF_UNSPEC, + config->auth_tcp, + NULL, + false, + config->max_queued_clients, + config->max_client_requests) < 0) + goto cleanup; - if (virNetServerAddService(srvAdm, svcAdm) < 0) - goto cleanup; - } + if (((ipsock && config->listen_tls) || (act && virSystemdActivationHasName(act, "ip-tls")))) { + virNetTLSContextPtr ctxt = NULL; - if (ipsock) { - if (config->listen_tcp) { - VIR_DEBUG("Registering TCP socket %s:%s", - config->listen_addr, config->tcp_port); - if (!(svcTCP = virNetServerServiceNewTCP(config->listen_addr, - config->tcp_port, - AF_UNSPEC, - config->auth_tcp, - NULL, - false, - config->max_queued_clients, - config->max_client_requests))) + if (config->ca_file || + config->cert_file || + config->key_file) { + if (!config->ca_file) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("No CA certificate path set to match server key/cert")); goto cleanup; - - if (virNetServerAddService(srv, svcTCP) < 0) + } + if (!config->cert_file) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("No server certificate path set to match server key")); goto cleanup; - } - - if (config->listen_tls) { - virNetTLSContextPtr ctxt = NULL; - - if (config->ca_file || - config->cert_file || - config->key_file) { - if (!config->ca_file) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("No CA certificate path set to match server key/cert")); - goto cleanup; - } - if (!config->cert_file) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("No server certificate path set to match server key")); - goto cleanup; - } - if (!config->key_file) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("No server key path set to match server cert")); - goto cleanup; - } - VIR_DEBUG("Using CA='%s' cert='%s' key='%s'", - config->ca_file, config->cert_file, config->key_file); - if (!(ctxt = virNetTLSContextNewServer(config->ca_file, - config->crl_file, - config->cert_file, - config->key_file, + } + if (!config->key_file) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("No server key path set to match server cert")); + goto cleanup; + } + VIR_DEBUG("Using CA='%s' cert='%s' key='%s'", + config->ca_file, config->cert_file, config->key_file); + if (!(ctxt = virNetTLSContextNewServer(config->ca_file, + config->crl_file, + config->cert_file, + config->key_file, + (const char *const*)config->tls_allowed_dn_list, + config->tls_priority, + config->tls_no_sanity_certificate ? false : true, + config->tls_no_verify_certificate ? false : true))) + goto cleanup; + } else { + if (!(ctxt = virNetTLSContextNewServerPath(NULL, + !privileged, (const char *const*)config->tls_allowed_dn_list, config->tls_priority, config->tls_no_sanity_certificate ? false : true, config->tls_no_verify_certificate ? false : true))) - goto cleanup; - } else { - if (!(ctxt = virNetTLSContextNewServerPath(NULL, - !privileged, - (const char *const*)config->tls_allowed_dn_list, - config->tls_priority, - config->tls_no_sanity_certificate ? false : true, - config->tls_no_verify_certificate ? false : true))) - goto cleanup; - } - - VIR_DEBUG("Registering TLS socket %s:%s", - config->listen_addr, config->tls_port); - if (!(svcTLS = - virNetServerServiceNewTCP(config->listen_addr, - config->tls_port, - AF_UNSPEC, - config->auth_tls, - ctxt, - false, - config->max_queued_clients, - config->max_client_requests))) { - virObjectUnref(ctxt); - goto cleanup; - } - if (virNetServerAddService(srv, svcTLS) < 0) goto cleanup; + } + VIR_DEBUG("Registering TLS socket %s:%s", + config->listen_addr, config->tls_port); + if (virNetServerAddServiceTCP(srv, + act, + "libvirtd-tls.socket", + config->listen_addr, + config->tls_port, + AF_UNSPEC, + config->auth_tls, + ctxt, + false, + config->max_queued_clients, + config->max_client_requests) < 0) { virObjectUnref(ctxt); + goto cleanup; } + virObjectUnref(ctxt); } + if (act && + virSystemdActivationComplete(act) < 0) + goto cleanup; + #if WITH_SASL if (virNetServerNeedsAuth(srv, REMOTE_AUTH_SASL) && !(saslCtxt = virNetSASLContextNewServer( (const char *const*)config->sasl_allowed_username_list))) - goto cleanup; + goto cleanup; #endif ret = 0; cleanup: - virObjectUnref(svcTLS); - virObjectUnref(svcTCP); - virObjectUnref(svcRO); - virObjectUnref(svcAdm); - virObjectUnref(svc); return ret; } diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c index 315a4950df..d5df5d5c20 100644 --- a/src/rpc/virnetserverservice.c +++ b/src/rpc/virnetserverservice.c @@ -28,9 +28,12 @@ #include "viralloc.h" #include "virerror.h" #include "virthread.h" +#include "virlog.h" #define VIR_FROM_THIS VIR_FROM_RPC +VIR_LOG_INIT("rpc.netserverservice"); + struct _virNetServerService { virObject parent; @@ -201,6 +204,8 @@ virNetServerServicePtr virNetServerServiceNewTCP(const char *nodename, virNetSocketPtr *socks; size_t nsocks; + VIR_DEBUG("Creating new TCP server nodename='%s' service='%s'", + NULLSTR(nodename), NULLSTR(service)); if (virNetSocketNewListenTCP(nodename, service, family, @@ -236,6 +241,8 @@ virNetServerServicePtr virNetServerServiceNewUNIX(const char *path, virNetServerServicePtr svc; virNetSocketPtr sock; + VIR_DEBUG("Creating new UNIX server path='%s' mask=%o gid=%u", + path, mask, grp); if (virNetSocketNewListenUNIX(path, mask, -1, -- 2.21.0

We don't do socket activation of libvirtd, since we need to unconditionally start libvirtd in order to perform autostart. This doesn't mean we can't have systemd socket units. Some use cases will not need libvirt's autostart & are thus free to use activation. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- libvirt.spec.in | 24 +++++++++++++++++++- src/remote/Makefile.inc.am | 35 +++++++++++++++++++++++++++++ src/remote/libvirtd-admin.socket.in | 13 +++++++++++ src/remote/libvirtd-ro.socket.in | 13 +++++++++++ src/remote/libvirtd-tcp.socket.in | 12 ++++++++++ src/remote/libvirtd-tls.socket.in | 12 ++++++++++ src/remote/libvirtd.service.in | 10 ++++----- src/remote/libvirtd.socket.in | 11 +++++++++ 8 files changed, 124 insertions(+), 6 deletions(-) create mode 100644 src/remote/libvirtd-admin.socket.in create mode 100644 src/remote/libvirtd-ro.socket.in create mode 100644 src/remote/libvirtd-tcp.socket.in create mode 100644 src/remote/libvirtd-tls.socket.in create mode 100644 src/remote/libvirtd.socket.in diff --git a/libvirt.spec.in b/libvirt.spec.in index d54f58f1d4..ec562d5f7a 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1342,6 +1342,8 @@ exit 0 %systemd_post virtlockd.socket virtlockd-admin.socket %systemd_post virtlogd.socket virtlogd-admin.socket +%systemd_post libvirtd.socket libvirtd-ro.socket libvirtd-admin.socket +%systemd_post libvirtd-tcp.socket libvirtd-tls.socket %systemd_post libvirtd.service # request daemon restart in posttrans @@ -1350,6 +1352,8 @@ touch %{_localstatedir}/lib/rpm-state/libvirt/restart || : %preun daemon %systemd_preun libvirtd.service +%systemd_preun libvirtd-tcp.socket libvirtd-tls.socket +%systemd_preun libvirtd.socket libvirtd-ro.socket libvirtd-admin.socket %systemd_preun virtlogd.socket virtlogd-admin.socket virtlogd.service %systemd_preun virtlockd.socket virtlockd-admin.socket virtlockd.service @@ -1374,7 +1378,20 @@ fi %posttrans daemon if [ -f %{_localstatedir}/lib/rpm-state/libvirt/restart ]; then - /bin/systemctl try-restart libvirtd.service >/dev/null 2>&1 || : + /bin/systemctl is-active libvirtd.service 1>/dev/null 2>&1 + # Old libvirtd owns the sockets and will delete them on + # shutdown. Can't use a try-restart as libvirtd will simply + # own the sockets again when it comes back up. Thus we must + # do this particular ordering + if test $? == 0 ; then + /bin/systemctl stop libvirtd.service >/dev/null 2>&1 || : + + /bin/systemctl try-restart libvirtd.socket >/dev/null 2>&1 || : + /bin/systemctl try-restart libvirtd-ro.socket >/dev/null 2>&1 || : + /bin/systemctl try-restart libvirtd-admin.socket >/dev/null 2>&1 || : + + /bin/systemctl start libvirtd.service >/dev/null 2>&1 || : + fi fi rm -rf %{_localstatedir}/lib/rpm-state/libvirt || : @@ -1505,6 +1522,11 @@ exit 0 %dir %attr(0700, root, root) %{_sysconfdir}/libvirt/ %{_unitdir}/libvirtd.service +%{_unitdir}/libvirtd.socket +%{_unitdir}/libvirtd-ro.socket +%{_unitdir}/libvirtd-admin.socket +%{_unitdir}/libvirtd-tcp.socket +%{_unitdir}/libvirtd-tls.socket %{_unitdir}/virt-guest-shutdown.target %{_unitdir}/virtlogd.service %{_unitdir}/virtlogd.socket diff --git a/src/remote/Makefile.inc.am b/src/remote/Makefile.inc.am index 851ab903fd..0cf00cb902 100644 --- a/src/remote/Makefile.inc.am +++ b/src/remote/Makefile.inc.am @@ -51,6 +51,11 @@ MANINFILES += libvirtd.8.in SYSTEMD_UNIT_FILES_IN += \ remote/libvirtd.service.in \ + remote/libvirtd.socket.in \ + remote/libvirtd-ro.socket.in \ + remote/libvirtd-admin.socket.in \ + remote/libvirtd-tcp.socket.in \ + remote/libvirtd-tls.socket.in \ remote/virt-guest-shutdown.target.in \ $(NULL) @@ -267,6 +272,36 @@ libvirtd.service: remote/libvirtd.service.in $(top_builddir)/config.status < $< > $@-t && \ mv $@-t $@ +libvirtd.socket: remote/libvirtd.socket.in $(top_builddir)/config.status + $(AM_V_GEN)sed \ + -e 's|[@]localstatedir[@]|$(localstatedir)|g' \ + < $< > $@-t && \ + mv $@-t $@ + +libvirtd-ro.socket: remote/libvirtd-ro.socket.in $(top_builddir)/config.status + $(AM_V_GEN)sed \ + -e 's|[@]localstatedir[@]|$(localstatedir)|g' \ + < $< > $@-t && \ + mv $@-t $@ + +libvirtd-admin.socket: remote/libvirtd-admin.socket.in $(top_builddir)/config.status + $(AM_V_GEN)sed \ + -e 's|[@]localstatedir[@]|$(localstatedir)|g' \ + < $< > $@-t && \ + mv $@-t $@ + +libvirtd-tcp.socket: remote/libvirtd-tcp.socket.in $(top_builddir)/config.status + $(AM_V_GEN)sed \ + -e 's|[@]localstatedir[@]|$(localstatedir)|g' \ + < $< > $@-t && \ + mv $@-t $@ + +libvirtd-tls.socket: remote/libvirtd-tls.socket.in $(top_builddir)/config.status + $(AM_V_GEN)sed \ + -e 's|[@]localstatedir[@]|$(localstatedir)|g' \ + < $< > $@-t && \ + mv $@-t $@ + virt-guest-shutdown.target: remote/virt-guest-shutdown.target.in \ $(top_builddir)/config.status $(AM_V_GEN)cp $< $@ diff --git a/src/remote/libvirtd-admin.socket.in b/src/remote/libvirtd-admin.socket.in new file mode 100644 index 0000000000..b791a2eb1b --- /dev/null +++ b/src/remote/libvirtd-admin.socket.in @@ -0,0 +1,13 @@ +[Unit] +Description=Libvirt admin socket +Before=libvirtd.service +BindsTo=libvirtd.socket +After=libvirtd.socket + +[Socket] +ListenStream=@localstatedir@/run/libvirt/libvirt-admin-sock +Service=libvirtd.service +SocketMode=0600 + +[Install] +WantedBy=sockets.target diff --git a/src/remote/libvirtd-ro.socket.in b/src/remote/libvirtd-ro.socket.in new file mode 100644 index 0000000000..55c44944b4 --- /dev/null +++ b/src/remote/libvirtd-ro.socket.in @@ -0,0 +1,13 @@ +[Unit] +Description=Libvirt local read-only socket +Before=libvirtd.service +BindsTo=libvirtd.socket +After=libvirtd.socket + +[Socket] +ListenStream=@localstatedir@/run/libvirt/libvirt-sock-ro +Service=libvirtd.service +SocketMode=0666 + +[Install] +WantedBy=sockets.target diff --git a/src/remote/libvirtd-tcp.socket.in b/src/remote/libvirtd-tcp.socket.in new file mode 100644 index 0000000000..09d5d3d67a --- /dev/null +++ b/src/remote/libvirtd-tcp.socket.in @@ -0,0 +1,12 @@ +[Unit] +Description=Libvirt non-TLS IP socket +Before=libvirtd.service +BindsTo=libvirtd.socket +After=libvirtd.socket + +[Socket] +ListenStream=16509 +Service=libvirtd.service + +[Install] +WantedBy=sockets.target diff --git a/src/remote/libvirtd-tls.socket.in b/src/remote/libvirtd-tls.socket.in new file mode 100644 index 0000000000..c60f0c9c77 --- /dev/null +++ b/src/remote/libvirtd-tls.socket.in @@ -0,0 +1,12 @@ +[Unit] +Description=Libvirt TLS IP socket +Before=libvirtd.service +BindsTo=libvirtd.socket +After=libvirtd.socket + +[Socket] +ListenStream=16514 +Service=libvirtd.service + +[Install] +WantedBy=sockets.target diff --git a/src/remote/libvirtd.service.in b/src/remote/libvirtd.service.in index 7f689e08a8..047620f79b 100644 --- a/src/remote/libvirtd.service.in +++ b/src/remote/libvirtd.service.in @@ -1,12 +1,10 @@ -# NB we don't use socket activation. When libvirtd starts it will -# spawn any virtual machines registered for autostart. We want this -# to occur on every boot, regardless of whether any client connects -# to a socket. Thus socket activation doesn't have any benefit - [Unit] Description=Virtualization daemon Requires=virtlogd.socket Requires=virtlockd.socket +Requires=libvirtd.socket +Requires=libvirtd-ro.socket +Requires=libvirtd-admin.socket Wants=systemd-machined.service Before=libvirt-guests.service After=network.target @@ -42,3 +40,5 @@ TasksMax=32768 WantedBy=multi-user.target Also=virtlockd.socket Also=virtlogd.socket +Also=libvirtd.socket +Also=libvirtd-ro.socket diff --git a/src/remote/libvirtd.socket.in b/src/remote/libvirtd.socket.in new file mode 100644 index 0000000000..e194c6e76e --- /dev/null +++ b/src/remote/libvirtd.socket.in @@ -0,0 +1,11 @@ +[Unit] +Description=Libvirt local socket +Before=libvirtd.service + +[Socket] +ListenStream=@localstatedir@/run/libvirt/libvirt-sock +Service=libvirtd.service +SocketMode=0666 + +[Install] +WantedBy=sockets.target -- 2.21.0

Since we have socket activation available now, we can let the system libvirtd exit when it is idle. This allows it to still do autostart when the host boots up, but when nothing was started it will quickly exit again until some mgmt app connects to the socket. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- src/remote/libvirtd.service.in | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/remote/libvirtd.service.in b/src/remote/libvirtd.service.in index 047620f79b..2e51429e7a 100644 --- a/src/remote/libvirtd.service.in +++ b/src/remote/libvirtd.service.in @@ -21,7 +21,11 @@ Documentation=https://libvirt.org [Service] Type=notify EnvironmentFile=-/etc/sysconfig/libvirtd -ExecStart=@sbindir@/libvirtd $LIBVIRTD_ARGS +# libvirtd.service is set to run on boot so that autostart of +# VMs can be performed. We don't want it to stick around if +# unused though, so we set a timeout. The socket activation +# then ensures it gets started again if anything needs it +ExecStart=@sbindir@/libvirtd --timeout 30 $LIBVIRTD_ARGS ExecReload=/bin/kill -HUP $MAINPID KillMode=process Restart=on-failure -- 2.21.0

The only use of this code was removed by: commit be78814ae07f092d9c4e71fd82dd1947aba2f029 Author: Michal Privoznik <mprivozn(a)redhat.com&gt; Date: Thu Apr 2 14:41:17 2015 +0200 virNetSocketNewConnectUNIX: Use flocks when spawning a daemon less than a year after it was first introduced in commit 1b807f92dbb617db5b9d551777d3026d8ff0903f Author: Martin Kletzander <mkletzan(a)redhat.com&gt; Date: Wed Jul 16 08:00:19 2014 +0200 rpc: pass listen FD to the daemon being started --- src/libvirt_private.syms | 1 - src/util/vircommand.c | 99 ------------------------------------ src/util/vircommand.h | 2 - tests/commanddata/test24.log | 8 --- tests/commandtest.c | 58 --------------------- 5 files changed, 168 deletions(-) delete mode 100644 tests/commanddata/test24.log diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a19ba1d798..1e32dd090e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1700,7 +1700,6 @@ virCommandNewVAList; virCommandNonblockingFDs; virCommandPassFD; virCommandPassFDGetFDIndex; -virCommandPassListenFDs; virCommandRawStatus; virCommandRequireHandshake; virCommandRun; diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 8695c98d1b..c81ddfc0d0 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -66,7 +66,6 @@ enum { VIR_EXEC_CLEAR_CAPS = (1 << 2), VIR_EXEC_RUN_SYNC = (1 << 3), VIR_EXEC_ASYNC_IO = (1 << 4), - VIR_EXEC_LISTEN_FDS = (1 << 5), }; typedef struct _virCommandFD virCommandFD; @@ -205,78 +204,6 @@ virCommandFDSet(virCommandPtr cmd, #ifndef WIN32 -static void -virCommandReorderFDs(virCommandPtr cmd) -{ - int maxfd = 0; - int openmax = 0; - size_t i = 0; - - if (!cmd || cmd->has_error || !cmd->npassfd) - return; - - for (i = 0; i < cmd->npassfd; i++) - maxfd = MAX(cmd->passfd[i].fd, maxfd); - - openmax = sysconf(_SC_OPEN_MAX); - if (openmax < 0 || - maxfd + cmd->npassfd > openmax) - goto error; - - /* - * Simple two-pass sort, nothing fancy. This is not designed for - * anything else than passing around 2 FDs into the child. - * - * So first dup2() them somewhere else. - */ - for (i = 0; i < cmd->npassfd; i++) { - int newfd = maxfd + i + 1; - int oldfd = cmd->passfd[i].fd; - if (dup2(oldfd, newfd) != newfd) { - virReportSystemError(errno, - _("Cannot dup2() fd %d before " - "passing it to the child"), - oldfd); - goto error; - } - VIR_FORCE_CLOSE(cmd->passfd[i].fd); - } - - VIR_DEBUG("First reorder pass done"); - - /* - * And then dup2() them in orderly manner. - */ - for (i = 0; i < cmd->npassfd; i++) { - int newfd = STDERR_FILENO + i + 1; - int oldfd = maxfd + i + 1; - if (dup2(oldfd, newfd) != newfd) { - virReportSystemError(errno, - _("Cannot dup2() fd %d before " - "passing it to the child"), - oldfd); - goto error; - } - if (virSetInherit(newfd, true) < 0) { - virReportSystemError(errno, - _("Cannot set O_CLOEXEC on fd %d before " - "passing it to the child"), - newfd); - goto error; - } - VIR_FORCE_CLOSE(oldfd); - cmd->passfd[i].fd = newfd; - } - - VIR_DEBUG("Second reorder pass done"); - - return; - - error: - cmd->has_error = -1; - return; -} - /** * virFork: * @@ -763,15 +690,6 @@ virExec(virCommandPtr cmd) goto fork_error; } - if (cmd->flags & VIR_EXEC_LISTEN_FDS) { - virCommandReorderFDs(cmd); - virCommandAddEnvFormat(cmd, "LISTEN_PID=%u", getpid()); - virCommandAddEnvFormat(cmd, "LISTEN_FDS=%zu", cmd->npassfd); - - if (cmd->has_error) - goto fork_error; - } - /* Close logging again to ensure no FDs leak to child */ virLogReset(); @@ -1002,23 +920,6 @@ virCommandPassFD(virCommandPtr cmd, int fd, unsigned int flags) } } -/** - * virCommandPassListenFDs: - * @cmd: the command to modify - * - * Pass LISTEN_FDS and LISTEN_PID environment variables into the - * child. LISTEN_PID has the value of the child's PID and LISTEN_FDS - * is a number of passed file descriptors starting from 3. - */ -void -virCommandPassListenFDs(virCommandPtr cmd) -{ - if (!cmd || cmd->has_error) - return; - - cmd->flags |= VIR_EXEC_LISTEN_FDS; -} - /* * virCommandPassFDGetFDIndex: * @cmd: pointer to virCommand diff --git a/src/util/vircommand.h b/src/util/vircommand.h index c9a8d3c41c..2a9ee5cdc7 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -60,8 +60,6 @@ void virCommandPassFD(virCommandPtr cmd, int fd, unsigned int flags) ATTRIBUTE_NOINLINE; -void virCommandPassListenFDs(virCommandPtr cmd); - int virCommandPassFDGetFDIndex(virCommandPtr cmd, int fd); diff --git a/tests/commanddata/test24.log b/tests/commanddata/test24.log deleted file mode 100644 index 38cbb5451b..0000000000 --- a/tests/commanddata/test24.log +++ /dev/null @@ -1,8 +0,0 @@ -FD:0 -FD:1 -FD:2 -FD:3 -FD:4 -DAEMON:yes -CWD:/ -UMASK:0022 diff --git a/tests/commandtest.c b/tests/commandtest.c index 146cc4c1bf..ce0832fb0c 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -1003,63 +1003,6 @@ test23(const void *unused ATTRIBUTE_UNUSED) return ret; } -static int test24(const void *unused ATTRIBUTE_UNUSED) -{ - char *pidfile = virPidFileBuildPath(abs_builddir, "commandhelper"); - char *prefix = NULL; - int newfd1 = dup(STDERR_FILENO); - int newfd2 = dup(STDERR_FILENO); - int newfd3 = dup(STDERR_FILENO); - int ret = -1; - pid_t pid; - virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); - - if (!pidfile) - goto cleanup; - - if (VIR_CLOSE(newfd1) < 0) - printf("Cannot close fd %d\n", newfd1); - - virCommandSetPidFile(cmd, pidfile); - virCommandDaemonize(cmd); - virCommandPassFD(cmd, newfd2, VIR_COMMAND_PASS_FD_CLOSE_PARENT); - virCommandPassFD(cmd, newfd3, VIR_COMMAND_PASS_FD_CLOSE_PARENT); - newfd2 = newfd3 = -1; - virCommandPassListenFDs(cmd); - - if (virCommandRun(cmd, NULL) < 0) { - printf("Cannot run child %s\n", virGetLastErrorMessage()); - goto cleanup; - } - - if (virPidFileRead(abs_builddir, "commandhelper", &pid) < 0) { - printf("cannot read pidfile\n"); - goto cleanup; - } - - if (virAsprintf(&prefix, - "ENV:LISTEN_FDS=2\nENV:LISTEN_PID=%u\n", - pid) < 0) - goto cleanup; - - while (kill(pid, 0) != -1) - usleep(100*1000); - - ret = checkoutput("test24", prefix); - - cleanup: - if (pidfile) - unlink(pidfile); - VIR_FREE(pidfile); - VIR_FREE(prefix); - virCommandFree(cmd); - VIR_FORCE_CLOSE(newfd1); - VIR_FORCE_CLOSE(newfd2); - VIR_FORCE_CLOSE(newfd3); - return ret; -} - - static int test25(const void *unused ATTRIBUTE_UNUSED) { int ret = -1; @@ -1347,7 +1290,6 @@ mymain(void) DO_TEST(test21); DO_TEST(test22); DO_TEST(test23); - DO_TEST(test24); DO_TEST(test25); DO_TEST(test26); -- 2.21.0

Using the new system activation APIs allows for simpler code setting up the network services. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- src/logging/log_daemon.c | 125 ++++++++++++--------------------------- 1 file changed, 37 insertions(+), 88 deletions(-) diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c index 30c70a20dd..8df04dbab1 100644 --- a/src/logging/log_daemon.c +++ b/src/logging/log_daemon.c @@ -517,78 +517,6 @@ virLogDaemonSetupSignals(virNetDaemonPtr dmn) } -static int -virLogDaemonSetupNetworkingSystemD(virNetServerPtr logSrv, virNetServerPtr adminSrv) -{ - unsigned int nfds; - size_t i; - - if ((nfds = virGetListenFDs()) == 0) - return 0; - if (nfds > 2) - VIR_DEBUG("Too many (%d) file descriptors from systemd", nfds); - - for (i = 0; i < nfds && i < 2; i++) { - virNetServerServicePtr svc; - char *path = virGetUNIXSocketPath(3 + i); - virNetServerPtr srv; - int fds[] = { 3 + i }; - - if (!path) - return -1; - - if (strstr(path, "virtlogd-admin-sock")) { - srv = adminSrv; - } else if (strstr(path, "virtlogd-sock")) { - srv = logSrv; - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unknown UNIX socket %s passed in"), - path); - VIR_FREE(path); - return -1; - } - VIR_FREE(path); - - /* Systemd passes FDs, starting immediately after stderr, - * so the first FD we'll get is '3'. */ - if (!(svc = virNetServerServiceNewFDs(fds, - ARRAY_CARDINALITY(fds), - false, - 0, - NULL, - false, 0, 1))) - return -1; - - if (virNetServerAddService(srv, svc) < 0) { - virObjectUnref(svc); - return -1; - } - } - return 1; -} - - -static int -virLogDaemonSetupNetworkingNative(virNetServerPtr srv, const char *sock_path) -{ - virNetServerServicePtr svc; - - VIR_DEBUG("Setting up networking natively"); - - if (!(svc = virNetServerServiceNewUNIX(sock_path, 0700, 0, 0, - NULL, - false, 0, 1))) - return -1; - - if (virNetServerAddService(srv, svc) < 0) { - virObjectUnref(svc); - return -1; - } - return 0; -} - - static void virLogDaemonClientFree(void *opaque) { @@ -1129,6 +1057,12 @@ int main(int argc, char **argv) { * scratch if rv == 0 */ if (rv == 0) { + VIR_AUTOPTR(virSystemdActivation) act = NULL; + virSystemdActivationMap actmap[] = { + { .name = "virtlogd.socket", .family = AF_UNIX, .path = sock_file }, + { .name = "virtlogd-admin.socket", .family = AF_UNIX, .path = admin_sock_file }, + }; + if (godaemon) { char ebuf[1024]; @@ -1156,31 +1090,46 @@ int main(int argc, char **argv) { goto cleanup; } + if (virSystemdGetActivation(actmap, + ARRAY_CARDINALITY(actmap), + &act) < 0) { + ret = VIR_LOG_DAEMON_ERR_NETWORK; + goto cleanup; + } + logSrv = virNetDaemonGetServer(logDaemon->dmn, "virtlogd"); adminSrv = virNetDaemonGetServer(logDaemon->dmn, "admin"); - if ((rv = virLogDaemonSetupNetworkingSystemD(logSrv, adminSrv)) < 0) { + + if (virNetServerAddServiceUNIX(logSrv, + act, "virtlogd.socket", + sock_file, 0700, 0, 0, + NULL, + false, 0, 1) < 0) { + ret = VIR_LOG_DAEMON_ERR_NETWORK; + goto cleanup; + } + if (virNetServerAddServiceUNIX(adminSrv, + act, "virtlogd-admin.socket", + admin_sock_file, 0700, 0, 0, + NULL, + false, 0, 1) < 0) { ret = VIR_LOG_DAEMON_ERR_NETWORK; goto cleanup; } - /* Only do this, if systemd did not pass a FD */ - if (rv == 0) { - if (virLogDaemonSetupNetworkingNative(logSrv, sock_file) < 0 || - virLogDaemonSetupNetworkingNative(adminSrv, admin_sock_file) < 0) { - ret = VIR_LOG_DAEMON_ERR_NETWORK; - goto cleanup; - } + if (act && + virSystemdActivationComplete(act) < 0) { + ret = VIR_LOG_DAEMON_ERR_NETWORK; + goto cleanup; } - virObjectUnref(logSrv); - virObjectUnref(adminSrv); + } else { + logSrv = virNetDaemonGetServer(logDaemon->dmn, "virtlogd"); + /* If exec-restarting from old virtlogd, we won't have an + * admin server present */ + if (virNetDaemonHasServer(logDaemon->dmn, "admin")) + adminSrv = virNetDaemonGetServer(logDaemon->dmn, "admin"); } - logSrv = virNetDaemonGetServer(logDaemon->dmn, "virtlogd"); - /* If exec-restarting from old virtlogd, we won't have an - * admin server present */ - if (virNetDaemonHasServer(logDaemon->dmn, "admin")) - adminSrv = virNetDaemonGetServer(logDaemon->dmn, "admin"); - if (timeout != -1) { VIR_DEBUG("Registering shutdown timeout %d", timeout); virNetDaemonAutoShutdown(logDaemon->dmn, -- 2.21.0

The new systemd activation APIs mean there is no longer a need to get the UNIX socket path associated with a plain FD. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- src/libvirt_private.syms | 1 - src/util/virutil.c | 44 ---------------------------------------- src/util/virutil.h | 2 -- 3 files changed, 47 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 11134e2e34..7dfa5af3b3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3234,7 +3234,6 @@ virGetHostnameQuiet; virGetSelfLastChanged; virGetSystemPageSize; virGetSystemPageSizeKB; -virGetUNIXSocketPath; virGetUnprivSGIOSysfsPath; virGetUserCacheDirectory; virGetUserConfigDirectory; diff --git a/src/util/virutil.c b/src/util/virutil.c index 4bd719127b..019009be8c 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1785,50 +1785,6 @@ void virUpdateSelfLastChanged(const char *path) } -#ifdef HAVE_SYS_UN_H -char *virGetUNIXSocketPath(int fd) -{ - union { - struct sockaddr sa; - struct sockaddr_storage ss; - struct sockaddr_un un; - } addr = { .ss = { 0 } }; - socklen_t len = sizeof(addr.ss); - char *path; - - if (getsockname(fd, &addr.sa, &len) < 0) { - virReportSystemError(errno, _("Unable to get address of FD %d"), fd); - return NULL; - } - - if (addr.ss.ss_family != AF_UNIX) { - virReportSystemError(EINVAL, _("FD %d is not a UNIX socket, has af=%d"), - fd, addr.ss.ss_family); - return NULL; - } - - if (addr.un.sun_path[0] == '\0') - addr.un.sun_path[0] = '@'; - - if (VIR_ALLOC_N(path, sizeof(addr.un.sun_path) + 1) < 0) - return NULL; - - memcpy(path, addr.un.sun_path, sizeof(addr.un.sun_path)); - path[sizeof(addr.un.sun_path)] = '\0'; - return path; -} - -#else /* HAVE_SYS_UN_H */ - -char *virGetUNIXSocketPath(int fd ATTRIBUTE_UNUSED) -{ - virReportSystemError(ENOSYS, "%s", - _("UNIX sockets not supported on this platform")); - return NULL; -} - -#endif /* HAVE_SYS_UN_H */ - #ifndef WIN32 long virGetSystemPageSize(void) { diff --git a/src/util/virutil.h b/src/util/virutil.h index b9715e5e66..7ea702f27a 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -149,8 +149,6 @@ bool virIsSUID(void); time_t virGetSelfLastChanged(void); void virUpdateSelfLastChanged(const char *path); -char *virGetUNIXSocketPath(int fd); - long virGetSystemPageSize(void) ATTRIBUTE_NOINLINE; long virGetSystemPageSizeKB(void) ATTRIBUTE_NOINLINE; -- 2.21.0