On Thu, Jun 27, 2019 at 10:54:31AM +0100, Daniel P. Berrangé wrote: Unrelated whitespace. Reviewed-by: Ján Tomko <jtomko(a)redhat.com&gt; Jano

On Thu, Jun 27, 2019 at 10:54:33AM +0100, Daniel P. Berrangé wrote: commit a4bfc2521f8aeff8f4e4431a8e2332cc74806b8a util: Move the VIR_AUTO(CLEAN|PTR) helper macros into a separate header moved the macros not depending on other stuff from viralloc.h to virautoclean.h This macro would better fit there. Also, sc_require_attribute_cleanup_initialization will need a touch-up. Reviewed-by: Ján Tomko <jtomko(a)redhat.com&gt; Jano

The getservent() APIs are not re-entrant safe so cannot be used in any threaded program. Add a wrapper around getaddrinfo() for resolving the service names to a port number. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/util/virsocketaddr.c | 51 ++++++++++++++++++++++++++++++++++++++++ src/util/virsocketaddr.h | 2 ++ 3 files changed, 54 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ce614e04bd..1adf735a38 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2919,6 +2919,7 @@ virSocketAddrParseIPv4; virSocketAddrParseIPv6; virSocketAddrPrefixToNetmask; virSocketAddrPTRDomain; +virSocketAddrResolveService; virSocketAddrSetIPv4Addr; virSocketAddrSetIPv4AddrNetOrder; virSocketAddrSetIPv6Addr; diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index 7a50cbe040..790bc0ebec 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -235,6 +235,57 @@ virSocketAddrParseIPv6(virSocketAddrPtr addr, const char *val) return virSocketAddrParse(addr, val, AF_INET6); } +/** + * virSocketAddrResolveService: + * @service: a service name or port number + * + * Resolve a service, which might be a plain port or service name, + * into a port number for IPv4/IPv6 usage + * + * Returns a numberic port number + */ +int virSocketAddrResolveService(const char *service) +{ + struct addrinfo *res, *tmp; + struct addrinfo hints; + int err; + int port = -1; + + memset(&hints, 0, sizeof(hints)); + + if ((err = getaddrinfo(NULL, service, &hints, &res)) != 0) { + virReportError(VIR_ERR_SYSTEM_ERROR, + _("Cannot parse socket service '%s': %s"), + service, gai_strerror(err)); + return -1; + } + + tmp = res; + while (tmp) { + if (tmp->ai_family == AF_INET) { + struct sockaddr_in in; + memcpy(&in, tmp->ai_addr, sizeof(in)); + port = in.sin_port; + goto cleanup; + } else if (tmp->ai_family == AF_INET6) { + struct sockaddr_in6 in; + memcpy(&in, tmp->ai_addr, sizeof(in)); + port = in.sin6_port; + goto cleanup; + } + tmp++; + } + + virReportError(VIR_ERR_SYSTEM_ERROR, + _("No matches for socket service '%s': %s"), + service, gai_strerror(err)); + + cleanup: + freeaddrinfo(res); + + return port; +} + /* * virSocketAddrSetIPv4AddrNetOrder: * @addr: the location to store the result diff --git a/src/util/virsocketaddr.h b/src/util/virsocketaddr.h index 9dbd8caa0d..bb97e6e3a0 100644 --- a/src/util/virsocketaddr.h +++ b/src/util/virsocketaddr.h @@ -98,6 +98,8 @@ int virSocketAddrParseIPv4(virSocketAddrPtr addr, int virSocketAddrParseIPv6(virSocketAddrPtr addr, const char *val); +int virSocketAddrResolveService(const char *service); + void virSocketAddrSetIPv4AddrNetOrder(virSocketAddrPtr s, uint32_t addr); void virSocketAddrSetIPv4Addr(virSocketAddrPtr s, uint32_t addr); void virSocketAddrSetIPv6AddrNetOrder(virSocketAddrPtr s, uint32_t addr[4]); -- 2.21.0

When receiving multiple FDs from systemd during service activation it is neccessary to identify which purpose each FD is used for. While this could be inferred by looking for the specific IP ports or UNIX socket paths, this requires the systemd config to always match what is expected by the code. Using systemd FD names we can remove this restriction and simply identify FDs based on an arbitrary name. The FD names are passed by systemd in the LISTEN_FDNAMES env variable which is populated with the socket unit file names, unless overriden by using the FileDescriptorName setting. This is supported since the system 227 release and unfortunately RHEL7 lacks this version. Thus the code has some back compat support whereby we look at the TCP ports or the UNIX socket paths to identify what socket maps to which name. This back compat code is written such that is it easly deleted when we are able to mandate newer systemd. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- src/libvirt_private.syms | 5 + src/util/virsystemd.c | 362 +++++++++++++++++++++++++++++++++++++++ src/util/virsystemd.h | 30 ++++ tests/virsystemdtest.c | 169 ++++++++++++++++++ 4 files changed, 566 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1adf735a38..ee1073e680 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3102,6 +3102,11 @@ virSysinfoReadS390; # util/virsystemd.h +virSystemdGetActivation; +virSystemdActivationClaimFDs; +virSystemdActivationComplete; +virSystemdActivationFree; +virSystemdActivationHasName; virSystemdCanHibernate; virSystemdCanHybridSleep; virSystemdCanSuspend; diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c index 3f03e3bd63..ae8401343d 100644 --- a/src/util/virsystemd.c +++ b/src/util/virsystemd.c @@ -39,6 +39,8 @@ #include "virlog.h" #include "virerror.h" #include "virfile.h" +#include "virhash.h" +#include "virsocketaddr.h" #define VIR_FROM_THIS VIR_FROM_SYSTEMD @@ -48,6 +50,18 @@ VIR_LOG_INIT("util.systemd"); # define MSG_NOSIGNAL 0 #endif +struct _virSystemdActivation { + virHashTablePtr fds; +}; + +typedef struct _virSystemdActivationEntry virSystemdActivationEntry; +typedef virSystemdActivationEntry *virSystemdActivationEntryPtr; + +struct _virSystemdActivationEntry { + int *fds; + size_t nfds; +}; + static void virSystemdEscapeName(virBufferPtr buf, const char *name) { @@ -561,3 +575,351 @@ int virSystemdCanHybridSleep(bool *result) { return virSystemdPMSupportTarget("CanHybridSleep", result); } + + +static void +virSystemdActivationEntryFree(void *data, const void *name) +{ + virSystemdActivationEntryPtr ent = data; + size_t i; + + VIR_DEBUG("Closing activation FDs for %s", (const char *)name); + for (i = 0; i < ent->nfds; i++) { + VIR_DEBUG("Closing activation FD %d", ent->fds[i]); + VIR_FORCE_CLOSE(ent->fds[i]); + } + + VIR_FREE(ent->fds); + VIR_FREE(ent); +} + + +static int +virSystemdActivationAddFD(virSystemdActivationPtr act, + const char *name, + int fd) +{ + virSystemdActivationEntryPtr ent = virHashLookup(act->fds, name); + + if (!ent) { + if (VIR_ALLOC(ent) < 0) + return -1; + + if (VIR_ALLOC_N(ent->fds, 1) < 0) { + virSystemdActivationEntryFree(ent, name); + return -1; + } + + ent->fds[ent->nfds++] = fd; + + VIR_DEBUG("Record first FD %d with name %s", fd, name); + if (virHashAddEntry(act->fds, name, ent) < 0) { + virSystemdActivationEntryFree(ent, name); + return -1; + } + + return 0; + } + + if (VIR_EXPAND_N(ent->fds, ent->nfds, 1) < 0) + return -1; + + VIR_DEBUG("Record extra FD %d with name %s", fd, name); + ent->fds[ent->nfds - 1] = fd; + + return 0; +} + + +static int +virSystemdActivationInitFromNames(virSystemdActivationPtr act, + int nfds, + const char *fdnames) +{ + VIR_AUTOSTRINGLIST fdnamelistptr = NULL; + char **fdnamelist; + size_t nfdnames; + size_t i; + int nextfd = STDERR_FILENO + 1; + + VIR_DEBUG("FD names %s", fdnames); + + if (!(fdnamelistptr = virStringSplitCount(fdnames, ":", 0, &nfdnames))) + goto error; + + if (nfdnames != nfds) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Expecting %d FD names but got %zu"), + nfds, nfdnames); + goto error; + } + + fdnamelist = fdnamelistptr; + while (nfds) { + if (virSystemdActivationAddFD(act, *fdnamelist, nextfd) < 0) + goto error; + + fdnamelist++; + nextfd++; + nfds--; + } + + return 0; + + error: + for (i = 0; i < nfds; i++) { + int fd = nextfd + i; + VIR_FORCE_CLOSE(fd); + } + return -1; +} + + +/* + * Back compat for systemd < v227 which lacks LISTEN_FDNAMES. + * Delete when min systemd is increased ie RHEL7 dropped + */ +static int +virSystemdActivationInitFromMap(virSystemdActivationPtr act, + int nfds, + virSystemdActivationMap *map, + size_t nmap) +{ + int nextfd = STDERR_FILENO + 1; + size_t i; + + while (nfds) { + virSocketAddr addr; + const char *name = NULL; + + memset(&addr, 0, sizeof(addr)); + + addr.len = sizeof(addr.data); + if (getsockname(nextfd, &addr.data.sa, &addr.len) < 0) { + virReportSystemError(errno, "%s", _("Unable to get local socket name")); + goto error; + } + + for (i = 0; i < nmap && !name; i++) { + if (map[i].name == NULL) + continue; + + if (addr.data.sa.sa_family == AF_INET) { + if (map[i].family == AF_INET && + addr.data.inet4.sin_port == htons(map[i].port)) + name = map[i].name; + } else if (addr.data.sa.sa_family == AF_INET6) { + /* NB use of AF_INET here is correct. The "map" struct + * only refers to AF_INET. The socket may be AF_INET + * or AF_INET6 + */ + if (map[i].family == AF_INET && + addr.data.inet6.sin6_port == htons(map[i].port)) + name = map[i].name; +#ifndef WIN32 + } else if (addr.data.sa.sa_family == AF_UNIX) { + if (map[i].family == AF_UNIX && + STREQLEN(map[i].path, + addr.data.un.sun_path, + sizeof(addr.data.un.sun_path))) + name = map[i].name; +#endif + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected socket family %d"), + addr.data.sa.sa_family); + goto error; + } + } + + if (!name) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot find name for FD %d socket family %d"), + nextfd, addr.data.sa.sa_family); + goto error; + } + + if (virSystemdActivationAddFD(act, name, nextfd) < 0) + goto error; + + nfds--; + nextfd++; + } + + return 0; + + error: + for (i = 0; i < nfds; i++) { + int fd = nextfd + i; + VIR_FORCE_CLOSE(fd); + } + return -1; +} + + +static virSystemdActivationPtr +virSystemdActivationNew(virSystemdActivationMap *map, + size_t nmap, + int nfds) +{ + virSystemdActivationPtr act; + const char *fdnames; + + VIR_DEBUG("Activated with %d FDs", nfds); + if (VIR_ALLOC(act) < 0) + return NULL; + + if (!(act->fds = virHashCreate(10, virSystemdActivationEntryFree))) + goto error; + + fdnames = virGetEnvAllowSUID("LISTEN_FDNAMES"); + if (fdnames) { + if (virSystemdActivationInitFromNames(act, nfds, fdnames) < 0) + goto error; + } else { + if (virSystemdActivationInitFromMap(act, nfds, map, nmap) < 0) + goto error; + } + + VIR_DEBUG("Created activation object for %d FDs", nfds); + return act; + + error: + virSystemdActivationFree(&act); + return NULL; +} + + +/** + * virSystemdGetActivation: + * @map: mapping of socket addresses to names + * @nmap: number of entries in @map + * @act: filled with allocated activation object + * + * Acquire an object for handling systemd activation. + * If no activation FDs have been provided the returned object + * will be NULL, indicating normal sevice setup can be performed + * If the returned object is non-NULL then at least one file + * descriptor will be present. No normal service setup should + * be performed. + * + * Returns: 0 on success, -1 on failure + */ +int +virSystemdGetActivation(virSystemdActivationMap *map, + size_t nmap, + virSystemdActivationPtr *act) +{ + int nfds = 0; + + if ((nfds = virGetListenFDs()) < 0) + return -1; + + if (nfds == 0) { + VIR_DEBUG("No activation FDs present"); + *act = NULL; + return 0; + } + + *act = virSystemdActivationNew(map, nmap, nfds); + return 0; +} + + +/** + * virSystemdActivationHasName: + * @act: the activation object + * @name: the file descriptor name + * + * Check whether there is a file descriptor present + * for the requested name. + * + * Returns: true if a FD is present, false otherwise + */ +bool +virSystemdActivationHasName(virSystemdActivationPtr act, + const char *name) +{ + return virHashLookup(act->fds, name) != NULL; +} + + +/** + * virSystemdActivationComplete: + * @act: the activation object + * + * Indicate that processing of activation has been + * completed. All provided file descriptors should + * have been claimed. If any are unclaimed then + * an error will be reported + * + * Returns: 0 on success, -1 if some FDs are unclaimed + */ +int +virSystemdActivationComplete(virSystemdActivationPtr act) +{ + if (virHashSize(act->fds) != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Some activation file descriptors are unclaimed")); + return -1; + } + + return 0; +} + + +/** + * virSystemdActivationClaimFDs: + * @act: the activation object + * @name: the file descriptor name + * @fds: to be filled with claimed FDs + * @nfds: to be filled with number of FDs in @fds + * + * Claims the file descriptors associated with + * @name. + * + * The caller is responsible for closing all + * returned file descriptors when they are no + * longer required. The caller must also free + * the array memory in @fds. + */ +void +virSystemdActivationClaimFDs(virSystemdActivationPtr act, + const char *name, + int **fds, + size_t *nfds) +{ + virSystemdActivationEntryPtr ent = virHashSteal(act->fds, name); + + if (!ent) { + *fds = NULL; + *nfds = 0; + VIR_DEBUG("No FD with name %s", name); + return; + } + + VIR_DEBUG("Found %zu FDs with name %s", ent->nfds, name); + *fds = ent->fds; + *nfds = ent->nfds; + + VIR_FREE(ent); +} + + +/** + * virSystemdActivationFree: + * @act: the activation object + * + * Free memory and close unclaimed file descriptors + * associated with the activation object + */ +void +virSystemdActivationFree(virSystemdActivationPtr *act) +{ + if (!*act) + return; + + virHashFree((*act)->fds); + + VIR_FREE(*act); +} diff --git a/src/util/virsystemd.h b/src/util/virsystemd.h index db4ecbff60..2858732d00 100644 --- a/src/util/virsystemd.h +++ b/src/util/virsystemd.h @@ -23,6 +23,20 @@ #include "internal.h" +typedef struct _virSystemdActivation virSystemdActivation; +typedef virSystemdActivation *virSystemdActivationPtr; + +/* + * Back compat for systemd < v227 which lacks LISTEN_FDNAMES. + * Delete when min systemd is increased ie RHEL7 dropped + */ +typedef struct _virSystemdActivationMap { + const char *name; + int family; + int port; /* if family == AF_INET/AF_INET6 */ + const char *path; /* if family == AF_UNIX */ +} virSystemdActivationMap; + char *virSystemdMakeScopeName(const char *name, const char *drivername, bool legacy_behaviour); @@ -49,3 +63,19 @@ int virSystemdCanHibernate(bool *result); int virSystemdCanHybridSleep(bool *result); char *virSystemdGetMachineNameByPID(pid_t pid); + +int virSystemdGetActivation(virSystemdActivationMap *map, + size_t nmap, + virSystemdActivationPtr *act); + +bool virSystemdActivationHasName(virSystemdActivationPtr act, + const char *name); + +int virSystemdActivationComplete(virSystemdActivationPtr act); + +void virSystemdActivationClaimFDs(virSystemdActivationPtr act, + const char *name, + int **fds, + size_t *nfds); + +void virSystemdActivationFree(virSystemdActivationPtr *act); diff --git a/tests/virsystemdtest.c b/tests/virsystemdtest.c index 82c02decd1..edca45d9f0 100644 --- a/tests/virsystemdtest.c +++ b/tests/virsystemdtest.c @@ -31,6 +31,8 @@ # include "virdbus.h" # include "virlog.h" # include "virmock.h" +# include "rpc/virnetsocket.h" +# include "intprops.h" # define VIR_FROM_THIS VIR_FROM_NONE VIR_LOG_INIT("tests.systemdtest"); @@ -507,6 +509,166 @@ static int testPMSupportSystemdNotRunning(const void *opaque) return 0; } + +static int +testActivationCreateFDs(virNetSocketPtr *sockUNIX, + virNetSocketPtr **sockIP, + size_t *nsockIP) +{ + *sockUNIX = NULL; + *sockIP = NULL; + *nsockIP = 0; + + if (virNetSocketNewListenUNIX("virsystemdtest.sock", + 0777, + 0, + 0, + sockUNIX) < 0) + return -1; + + if (virNetSocketNewListenTCP("localhost", + NULL, + AF_UNSPEC, + sockIP, + nsockIP) < 0) { + virObjectUnref(*sockUNIX); + return -1; + } + + return 0; +} + + +static int +testActivation(bool useNames) +{ + virNetSocketPtr sockUNIX; + virNetSocketPtr *sockIP; + size_t nsockIP; + int ret = -1; + size_t i; + const char *names2 = "demo-unix.socket:demo-ip.socket"; + const char *names3 = "demo-unix.socket:demo-ip.socket:demo-ip.socket"; + char nfdstr[INT_BUFSIZE_BOUND(size_t)]; + char pidstr[INT_BUFSIZE_BOUND(pid_t)]; + virSystemdActivationMap map[2]; + int *fds = NULL; + size_t nfds = 0; + VIR_AUTOSTRUCT(virSystemdActivation) *act = NULL; + + if (testActivationCreateFDs(&sockUNIX, &sockIP, &nsockIP) < 0) + return -1; + + if (nsockIP != 1 && nsockIP != 2) { + fprintf(stderr, "Got %zu IP sockets but expected only 1 or 2\n", nsockIP); + goto cleanup; + } + + snprintf(nfdstr, sizeof(nfdstr), "%zu", 1 + nsockIP); + snprintf(pidstr, sizeof(pidstr), "%lld", (long long)getpid()); + + setenv("LISTEN_FDS", nfdstr, 1); + setenv("LISTEN_PID", pidstr, 1); + + if (useNames) + setenv("LISTEN_FDNAMES", nsockIP == 1 ? names2 : names3, 1); + else + unsetenv("LISTEN_FDNAMES"); + + map[0].name = "demo-unix.socket"; + map[0].family = AF_UNIX; + map[0].path = virNetSocketGetPath(sockUNIX); + + map[1].name = "demo-ip.socket"; + map[1].family = AF_INET; + map[1].port = virNetSocketGetPort(sockIP[0]); + + if (virSystemdGetActivation(map, ARRAY_CARDINALITY(map), &act) < 0) + goto cleanup; + + if (act == NULL) { + fprintf(stderr, "Activation object was not created: %s", virGetLastErrorMessage()); + goto cleanup; + } + + if (virSystemdActivationComplete(act) == 0) { + fprintf(stderr, "Activation did not report unclaimed FDs"); + goto cleanup; + } + + virSystemdActivationClaimFDs(act, "demo-unix.socket", &fds, &nfds); + + if (nfds != 1) { + fprintf(stderr, "Expected 1 UNIX fd, but got %zu\n", nfds); + goto cleanup; + } + VIR_FREE(fds); + + virSystemdActivationClaimFDs(act, "demo-ip.socket", &fds, &nfds); + + if (nfds != nsockIP) { + fprintf(stderr, "Expected %zu IP fd, but got %zu\n", nsockIP, nfds); + goto cleanup; + } + VIR_FREE(fds); + + virSystemdActivationClaimFDs(act, "demo-ip-alt.socket", &fds, &nfds); + + if (nfds != 0) { + fprintf(stderr, "Expected 0 IP fd, but got %zu\n", nfds); + goto cleanup; + } + + if (virSystemdActivationComplete(act) < 0) { + fprintf(stderr, "Action was not complete: %s\n", virGetLastErrorMessage()); + goto cleanup; + } + + ret = 0; + cleanup: + virObjectUnref(sockUNIX); + for (i = 0; i < nsockIP; i++) + virObjectUnref(sockIP[i]); + VIR_FREE(sockIP); + VIR_FREE(fds); + return ret; +} + + +static int +testActivationEmpty(const void *opaque ATTRIBUTE_UNUSED) +{ + virSystemdActivationPtr act; + + unsetenv("LISTEN_FDS"); + + if (virSystemdGetActivation(NULL, 0, &act) < 0) + return -1; + + if (act != NULL) { + fprintf(stderr, "Unexpectedly got activation object"); + virSystemdActivationFree(&act); + return -1; + } + + return 0; +} + + +static int +testActivationFDNames(const void *opaque ATTRIBUTE_UNUSED) +{ + return testActivation(true); +} + + +static int +testActivationFDAddrs(const void *opaque ATTRIBUTE_UNUSED) +{ + return testActivation(false); +} + + static int mymain(void) { @@ -598,6 +760,13 @@ mymain(void) TESTS_PM_SUPPORT_HELPER("canHibernate", &virSystemdCanHibernate); TESTS_PM_SUPPORT_HELPER("canHybridSleep", &virSystemdCanHybridSleep); + if (virTestRun("Test activation empty", testActivationEmpty, NULL) < 0) + ret = -1; + if (virTestRun("Test activation names", testActivationFDNames, NULL) < 0) + ret = -1; + if (virTestRun("Test activation addrs", testActivationFDAddrs, NULL) < 0) + ret = -1; + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.21.0

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

The virNetServerServiceNewFD API only accepts a single FD, but it is easily changed to allow for an array of FDs to be passed in. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- src/libvirt_remote.syms | 2 +- src/locking/lock_daemon.c | 9 ++++-- src/logging/log_daemon.c | 9 ++++-- src/rpc/virnetserverservice.c | 53 +++++++++++++++++++++-------------- src/rpc/virnetserverservice.h | 13 +++++---- 5 files changed, 52 insertions(+), 34 deletions(-) diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms index 99fe3dd07c..e0f10400db 100644 --- a/src/libvirt_remote.syms +++ b/src/libvirt_remote.syms @@ -202,7 +202,7 @@ virNetServerServiceGetMaxRequests; virNetServerServiceGetPort; virNetServerServiceGetTLSContext; virNetServerServiceIsReadonly; -virNetServerServiceNewFD; +virNetServerServiceNewFDs; virNetServerServiceNewFDOrUNIX; virNetServerServiceNewPostExecRestart; virNetServerServiceNewTCP; diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index bc2fb4a7fb..c10b2d383c 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -597,6 +597,7 @@ virLockDaemonSetupNetworkingSystemD(virNetServerPtr lockSrv, virNetServerPtr adm virNetServerServicePtr svc; char *path = virGetUNIXSocketPath(3 + i); virNetServerPtr srv; + int fds[] = { 3 + i }; if (!path) return -1; @@ -616,9 +617,11 @@ virLockDaemonSetupNetworkingSystemD(virNetServerPtr lockSrv, virNetServerPtr adm /* Systemd passes FDs, starting immediately after stderr, * so the first FD we'll get is '3'. */ - if (!(svc = virNetServerServiceNewFD(3 + i, 0, - NULL, - false, 0, 1))) + if (!(svc = virNetServerServiceNewFDs(fds, + ARRAY_CARDINALITY(fds), + 0, + NULL, + false, 0, 1))) return -1; if (virNetServerAddService(srv, svc) < 0) { diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c index 014596b280..6531999381 100644 --- a/src/logging/log_daemon.c +++ b/src/logging/log_daemon.c @@ -532,6 +532,7 @@ virLogDaemonSetupNetworkingSystemD(virNetServerPtr logSrv, virNetServerPtr admin virNetServerServicePtr svc; char *path = virGetUNIXSocketPath(3 + i); virNetServerPtr srv; + int fds[] = { 3 + i }; if (!path) return -1; @@ -551,9 +552,11 @@ virLogDaemonSetupNetworkingSystemD(virNetServerPtr logSrv, virNetServerPtr admin /* Systemd passes FDs, starting immediately after stderr, * so the first FD we'll get is '3'. */ - if (!(svc = virNetServerServiceNewFD(3 + i, 0, - NULL, - false, 0, 1))) + if (!(svc = virNetServerServiceNewFDs(fds, + ARRAY_CARDINALITY(fds), + 0, + NULL, + false, 0, 1))) return -1; if (virNetServerAddService(srv, svc) < 0) { diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c index 69043ccc0e..0d2f264696 100644 --- a/src/rpc/virnetserverservice.c +++ b/src/rpc/virnetserverservice.c @@ -112,18 +112,20 @@ virNetServerServiceNewFDOrUNIX(const char *path, nrequests_client_max); } else { + int fds[] = {(*cur_fd)++}; /* * There's still enough file descriptors. In this case we'll * use the current one and increment it afterwards. Take care * with order of operation for pointer arithmetic and auto * increment on cur_fd - the parentheses are necessary. */ - return virNetServerServiceNewFD((*cur_fd)++, - auth, - tls, - readonly, - max_queued_clients, - nrequests_client_max); + return virNetServerServiceNewFDs(fds, + ARRAY_CARDINALITY(fds), + auth, + tls, + readonly, + max_queued_clients, + nrequests_client_max); } } @@ -253,30 +255,39 @@ virNetServerServicePtr virNetServerServiceNewUNIX(const char *path, return svc; } -virNetServerServicePtr virNetServerServiceNewFD(int fd, - int auth, - virNetTLSContextPtr tls, - bool readonly, - size_t max_queued_clients, - size_t nrequests_client_max) +virNetServerServicePtr virNetServerServiceNewFDs(int *fds, + size_t nfds, + int auth, + virNetTLSContextPtr tls, + bool readonly, + size_t max_queued_clients, + size_t nrequests_client_max) { - virNetServerServicePtr svc; - virNetSocketPtr sock; + virNetServerServicePtr svc = NULL; + virNetSocketPtr *socks; + size_t i; - if (virNetSocketNewListenFD(fd, - &sock) < 0) - return NULL; + if (VIR_ALLOC_N(socks, nfds) < 0) + goto cleanup; - svc = virNetServerServiceNewSocket(&sock, - 1, + for (i = 0; i < nfds; i++) { + if (virNetSocketNewListenFD(fds[i], + &socks[i]) < 0) + goto cleanup; + } + + svc = virNetServerServiceNewSocket(socks, + nfds, auth, tls, readonly, max_queued_clients, nrequests_client_max); - virObjectUnref(sock); - + cleanup: + for (i = 0; i < nfds && socks; i++) + virObjectUnref(socks[i]); + VIR_FREE(socks); return svc; } diff --git a/src/rpc/virnetserverservice.h b/src/rpc/virnetserverservice.h index 5dd22bd929..59ee51e5ee 100644 --- a/src/rpc/virnetserverservice.h +++ b/src/rpc/virnetserverservice.h @@ -60,12 +60,13 @@ virNetServerServicePtr virNetServerServiceNewUNIX(const char *path, bool readonly, size_t max_queued_clients, size_t nrequests_client_max); -virNetServerServicePtr virNetServerServiceNewFD(int fd, - int auth, - virNetTLSContextPtr tls, - bool readonly, - size_t max_queued_clients, - size_t nrequests_client_max); +virNetServerServicePtr virNetServerServiceNewFDs(int *fd, + size_t nfds, + int auth, + virNetTLSContextPtr tls, + bool readonly, + size_t max_queued_clients, + size_t nrequests_client_max); virNetServerServicePtr virNetServerServiceNewPostExecRestart(virJSONValuePtr object); -- 2.21.0

Currently code has to first create the service and then separately register it with the server. If the socket associated with a particular service is not passed from systemd we want to skip creating the service altogether. This means we can't put the systemd activation logic into the constructors for virNetServerService. This patch thus creates some helper methods against virNetServer which combine systemd activation, service creation and service registration into one single operation. This operation is automatically a no-op if systemd activation is present and no sockets were passed in. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- src/libvirt_remote.syms | 2 + src/rpc/virnetserver.c | 145 ++++++++++++++++++++++++++++++++++++++++ src/rpc/virnetserver.h | 23 +++++++ 3 files changed, 170 insertions(+) diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms index e0f10400db..892091dd83 100644 --- a/src/libvirt_remote.syms +++ b/src/libvirt_remote.syms @@ -114,6 +114,8 @@ virNetMessageSaveError; virNetServerAddClient; virNetServerAddProgram; virNetServerAddService; +virNetServerAddServiceTCP; +virNetServerAddServiceUNIX; virNetServerClose; virNetServerGetClient; virNetServerGetClients; diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 0f3fa63fbb..894feae406 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -668,6 +668,151 @@ int virNetServerAddService(virNetServerPtr srv, return -1; } + +static int +virNetServerAddServiceActivation(virNetServerPtr srv, + virSystemdActivationPtr act, + const char *actname, + int auth, + virNetTLSContextPtr tls, + bool readonly, + size_t max_queued_clients, + size_t nrequests_client_max) +{ + int *fds; + size_t nfds; + + if (act == NULL) + return 0; + + virSystemdActivationClaimFDs(act, actname, &fds, &nfds); + + if (nfds) { + virNetServerServicePtr svc; + + svc = virNetServerServiceNewFDs(fds, + nfds, + false, + auth, + tls, + readonly, + max_queued_clients, + nrequests_client_max); + if (!svc) + return -1; + + if (virNetServerAddService(srv, svc) < 0) { + virObjectUnref(svc); + return -1; + } + } + + /* Intentionally return 1 any time activation is present, + * even if we didn't find any sockets with the matching + * name. The user needs to be free to disable some of the + * services via unit files without causing us to fallback + * to creating the service manually. + */ + return 1; +} + + +int virNetServerAddServiceTCP(virNetServerPtr srv, + virSystemdActivationPtr act, + const char *actname, + 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 = NULL; + int ret; + + ret = virNetServerAddServiceActivation(srv, act, actname, + auth, + tls, + readonly, + max_queued_clients, + nrequests_client_max); + if (ret < 0) + return -1; + + if (ret == 1) + return 0; + + if (!(svc = virNetServerServiceNewTCP(nodename, + service, + family, + auth, + tls, + readonly, + max_queued_clients, + nrequests_client_max))) + return -1; + + if (virNetServerAddService(srv, svc) < 0) { + virObjectUnref(svc); + return -1; + } + + virObjectUnref(svc); + + return 0; +} + + +int virNetServerAddServiceUNIX(virNetServerPtr srv, + virSystemdActivationPtr act, + const char *actname, + const char *path, + mode_t mask, + gid_t grp, + int auth, + virNetTLSContextPtr tls, + bool readonly, + size_t max_queued_clients, + size_t nrequests_client_max) +{ + virNetServerServicePtr svc = NULL; + int ret; + + ret = virNetServerAddServiceActivation(srv, act, actname, + auth, + tls, + readonly, + max_queued_clients, + nrequests_client_max); + if (ret < 0) + return -1; + + if (ret == 1) + return 0; + + if (!(svc = virNetServerServiceNewUNIX(path, + mask, + grp, + auth, + tls, + readonly, + max_queued_clients, + nrequests_client_max))) + return -1; + + if (virNetServerAddService(srv, svc) < 0) { + virObjectUnref(svc); + return -1; + } + + virObjectUnref(svc); + + return 0; +} + + int virNetServerAddProgram(virNetServerPtr srv, virNetServerProgramPtr prog) { diff --git a/src/rpc/virnetserver.h b/src/rpc/virnetserver.h index b47b71b4b2..3205dde78f 100644 --- a/src/rpc/virnetserver.h +++ b/src/rpc/virnetserver.h @@ -27,6 +27,7 @@ #include "virnetserverservice.h" #include "virobject.h" #include "virjson.h" +#include "virsystemd.h" virNetServerPtr virNetServerNew(const char *name, @@ -60,6 +61,28 @@ virJSONValuePtr virNetServerPreExecRestart(virNetServerPtr srv); int virNetServerAddService(virNetServerPtr srv, virNetServerServicePtr svc); +int virNetServerAddServiceTCP(virNetServerPtr srv, + virSystemdActivationPtr act, + const char *actname, + const char *nodename, + const char *service, + int family, + int auth, + virNetTLSContextPtr tls, + bool readonly, + size_t max_queued_clients, + size_t nrequests_client_max); +int virNetServerAddServiceUNIX(virNetServerPtr srv, + virSystemdActivationPtr act, + const char *actname, + const char *path, + mode_t mask, + gid_t grp, + int auth, + virNetTLSContextPtr tls, + bool readonly, + size_t max_queued_clients, + size_t nrequests_client_max); int virNetServerAddProgram(virNetServerPtr srv, virNetServerProgramPtr prog); -- 2.21.0

Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- src/remote/remote_daemon.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c index fdc9e4333a..0dabd3dff8 100644 --- a/src/remote/remote_daemon.c +++ b/src/remote/remote_daemon.c @@ -534,15 +534,10 @@ daemonSetupNetworking(virNetServerPtr srv, } #if WITH_SASL - if (config->auth_unix_rw == REMOTE_AUTH_SASL || - (sock_path_ro && config->auth_unix_ro == REMOTE_AUTH_SASL) || - (ipsock && config->listen_tls && config->auth_tls == REMOTE_AUTH_SASL) || - (ipsock && config->listen_tcp && config->auth_tcp == REMOTE_AUTH_SASL)) { - saslCtxt = virNetSASLContextNewServer( - (const char *const*)config->sasl_allowed_username_list); - if (!saslCtxt) + if (virNetServerNeedsAuth(srv, REMOTE_AUTH_SASL) && + !(saslCtxt = virNetSASLContextNewServer( + (const char *const*)config->sasl_allowed_username_list))) goto cleanup; - } #endif ret = 0; -- 2.21.0

The virNetServerServiceNewFDOrUNIX method cannot be correctly used when dealing with systemd activation of a service which can receive more than one socket FD as there is not guaranteed ordering of FDs. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- src/libvirt_remote.syms | 1 - src/rpc/virnetserverservice.c | 46 ----------------------------------- src/rpc/virnetserverservice.h | 10 -------- 3 files changed, 57 deletions(-) diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms index f4a62491e5..3307d74324 100644 --- a/src/libvirt_remote.syms +++ b/src/libvirt_remote.syms @@ -206,7 +206,6 @@ virNetServerServiceGetPort; virNetServerServiceGetTLSContext; virNetServerServiceIsReadonly; virNetServerServiceNewFDs; -virNetServerServiceNewFDOrUNIX; virNetServerServiceNewPostExecRestart; virNetServerServiceNewTCP; virNetServerServiceNewUNIX; diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c index d5df5d5c20..66af27d9f7 100644 --- a/src/rpc/virnetserverservice.c +++ b/src/rpc/virnetserverservice.c @@ -88,52 +88,6 @@ static void virNetServerServiceAccept(virNetSocketPtr sock, } -virNetServerServicePtr -virNetServerServiceNewFDOrUNIX(const char *path, - mode_t mask, - gid_t grp, - int auth, - virNetTLSContextPtr tls, - bool readonly, - size_t max_queued_clients, - size_t nrequests_client_max, - unsigned int nfds, - unsigned int *cur_fd) -{ - if (*cur_fd - STDERR_FILENO > nfds) { - /* - * There are no more file descriptors to use, so we have to - * fallback to UNIX socket. - */ - return virNetServerServiceNewUNIX(path, - mask, - grp, - auth, - tls, - readonly, - max_queued_clients, - nrequests_client_max); - - } else { - int fds[] = {(*cur_fd)++}; - /* - * There's still enough file descriptors. In this case we'll - * use the current one and increment it afterwards. Take care - * with order of operation for pointer arithmetic and auto - * increment on cur_fd - the parentheses are necessary. - */ - return virNetServerServiceNewFDs(fds, - ARRAY_CARDINALITY(fds), - false, - auth, - tls, - readonly, - max_queued_clients, - nrequests_client_max); - } -} - - static virNetServerServicePtr virNetServerServiceNewSocket(virNetSocketPtr *socks, size_t nsocks, diff --git a/src/rpc/virnetserverservice.h b/src/rpc/virnetserverservice.h index 73d61dde99..d58fc43437 100644 --- a/src/rpc/virnetserverservice.h +++ b/src/rpc/virnetserverservice.h @@ -34,16 +34,6 @@ typedef int (*virNetServerServiceDispatchFunc)(virNetServerServicePtr svc, virNetSocketPtr sock, void *opaque); -virNetServerServicePtr virNetServerServiceNewFDOrUNIX(const char *path, - mode_t mask, - gid_t grp, - int auth, - virNetTLSContextPtr tls, - bool readonly, - size_t max_queued_clients, - size_t nrequests_client_max, - unsigned int nfds, - unsigned int *cur_fd); virNetServerServicePtr virNetServerServiceNewTCP(const char *nodename, const char *service, int family, -- 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 ee1073e680..c560dda5e8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1699,7 +1699,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..2dd91d3c3d 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_AUTOSTRUCT(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 37a8ff1285..600a4dea13 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3250,7 +3250,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