Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virsocketaddr.c | 42 ++++++++++++++++++++++++++++++++++++++++ src/util/virsocketaddr.h | 2 ++ 3 files changed, 45 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 34937adc5d..ce614e04bd 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2903,6 +2903,7 @@ virSocketAddrFormat; virSocketAddrFormatFull; virSocketAddrGetIPPrefix; virSocketAddrGetNumNetmaskBits; +virSocketAddrGetPath; virSocketAddrGetPort; virSocketAddrGetRange; virSocketAddrIsNetmask; diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index 182e18aa8c..7a50cbe040 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -500,6 +500,7 @@ virSocketAddrSetPort(virSocketAddrPtr addr, int port) return 0; } + /* * virSocketGetPort: * @addr: an initialized virSocketAddrPtr @@ -522,6 +523,47 @@ virSocketAddrGetPort(virSocketAddrPtr addr) return -1; } + +/* + * virSocketGetPath: + * @addr: an initialized virSocketAddrPtr + * + * Returns the UNIX socket path of the given virtSocketAddr + * + * Returns -1 if @addr is invalid or does not refer to an + * address of type AF_UNIX; + */ +char * +virSocketAddrGetPath(virSocketAddrPtr addr ATTRIBUTE_UNUSED) +{ +#ifndef WIN32 + char *path = NULL; + if (addr == NULL) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("No socket address provided")); + return NULL; + } + + if (addr->data.sa.sa_family != AF_UNIX) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("UNIX socket address is required")); + return NULL; + } + + if (VIR_STRNDUP(path, + addr->data.un.sun_path, + sizeof(addr->data.un.sun_path)) < 0) + return NULL; + + return path; +#else + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("UNIX sockets not supported on this platform")); + return NULL; +#endif +} + + /** * virSocketAddrIsNetmask: * @netmask: the netmask address diff --git a/src/util/virsocketaddr.h b/src/util/virsocketaddr.h index b2ecb3c748..9dbd8caa0d 100644 --- a/src/util/virsocketaddr.h +++ b/src/util/virsocketaddr.h @@ -108,6 +108,8 @@ char *virSocketAddrFormatFull(const virSocketAddr *addr, bool withService, const char *separator); +char *virSocketAddrGetPath(virSocketAddrPtr addr); + int virSocketAddrSetPort(virSocketAddrPtr addr, int port); int virSocketAddrGetPort(virSocketAddrPtr addr); -- 2.21.0

The current VIR_AUTOPTR macro assumes that the struct needs to have a auto-free function auto-generated to call the real free function. The new VIR_AUTOSTRUCT macro allows for structs which already have a free function which takes a pointer to a pointer to the struct and can thus be used directly for auto-cleanup. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/viralloc.h | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 2b82096fde..5de18b9ea0 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -614,3 +614,16 @@ void virAllocTestHook(void (*func)(int, void*), void *data); * when the variable goes out of scope. */ #define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type + +/** + * VIR_AUTOSTRUCT: + * @type: type of the struct variable to be freed automatically + * + * Macro to automatically free the memory allocated to + * the struct variable declared with it by calling vir$STRUCTFree + * when the variable goes out of scope. + * + * The vir$STRUCTFree function must take a pointer to a pointer + * to the struct. + */ +#define VIR_AUTOSTRUCT(type) __attribute__((cleanup(type ## Free))) type -- 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@redhat.com> --- 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

Introduce a virNetServerServiceNewSocket API that allows the various constructors to share more code. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- 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. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- 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 | 57 ++++++++++++++++++++--------------- src/rpc/virnetsocket.h | 1 + 6 files changed, 40 insertions(+), 24 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..a462c3eb05 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -81,6 +81,7 @@ struct _virNetSocket { bool client; bool ownsFd; bool quietEOF; + bool unlinkUNIX; /* Event callback fields */ virNetSocketIOFunc func; @@ -216,10 +217,13 @@ 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, + int fd, + int errfd, + pid_t pid, + bool unlinkUNIX) { virNetSocketPtr sock; int no_slow_start = 1; @@ -254,6 +258,7 @@ static virNetSocketPtr virNetSocketNew(virSocketAddrPtr localAddr, sock->pid = pid; sock->watch = -1; sock->ownsFd = true; + sock->unlinkUNIX = unlinkUNIX; /* Disable nagle for TCP sockets */ if (sock->localAddr.data.sa.sa_family == AF_INET || @@ -280,8 +285,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 +430,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, fd, -1, 0, false))) goto error; runp = runp->ai_next; fd = -1; @@ -513,7 +516,7 @@ int virNetSocketNewListenUNIX(const char *path, goto error; } - if (!(*retsock = virNetSocketNew(&addr, NULL, false, fd, -1, 0))) + if (!(*retsock = virNetSocketNew(&addr, NULL, fd, -1, 0, true))) goto error; return 0; @@ -538,6 +541,7 @@ int virNetSocketNewListenUNIX(const char *path ATTRIBUTE_UNUSED, #endif int virNetSocketNewListenFD(int fd, + bool unlinkUNIX, virNetSocketPtr *retsock) { virSocketAddr addr; @@ -551,7 +555,7 @@ int virNetSocketNewListenFD(int fd, return -1; } - if (!(*retsock = virNetSocketNew(&addr, NULL, false, fd, -1, 0))) + if (!(*retsock = virNetSocketNew(&addr, NULL, fd, -1, 0, unlinkUNIX))) return -1; return 0; @@ -627,7 +631,7 @@ int virNetSocketNewConnectTCP(const char *nodename, goto error; } - if (!(*retsock = virNetSocketNew(&localAddr, &remoteAddr, true, fd, -1, 0))) + if (!(*retsock = virNetSocketNew(&localAddr, &remoteAddr, fd, -1, 0, false))) goto error; freeaddrinfo(ai); @@ -752,7 +756,7 @@ int virNetSocketNewConnectUNIX(const char *path, goto cleanup; } - if (!(*retsock = virNetSocketNew(&localAddr, &remoteAddr, true, fd, -1, 0))) + if (!(*retsock = virNetSocketNew(&localAddr, &remoteAddr, fd, -1, 0, false))) goto cleanup; ret = 0; @@ -820,7 +824,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, sv[0], errfd[0], pid, false))) goto error; virCommandFree(cmd); @@ -1219,7 +1223,7 @@ int virNetSocketNewConnectSockFD(int sockfd, return -1; } - if (!(*retsock = virNetSocketNew(&localAddr, NULL, true, sockfd, -1, -1))) + if (!(*retsock = virNetSocketNew(&localAddr, NULL, sockfd, -1, -1, false))) return -1; return 0; @@ -1231,7 +1235,7 @@ virNetSocketPtr virNetSocketNewPostExecRestart(virJSONValuePtr object) virSocketAddr localAddr; virSocketAddr remoteAddr; int fd, thepid, errfd; - bool isClient; + bool unlinkUNIX; if (virJSONValueObjectGetNumberInt(object, "fd", &fd) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -1250,10 +1254,15 @@ 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) { + bool isClient; + if (virJSONValueObjectGetBoolean(object, "isClient", &isClient) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing unlinkUNIX/isClient data in JSON document")); + return NULL; + } + unlinkUNIX = !isClient; } memset(&localAddr, 0, sizeof(localAddr)); @@ -1272,7 +1281,7 @@ virNetSocketPtr virNetSocketNewPostExecRestart(virJSONValuePtr object) } return virNetSocketNew(&localAddr, &remoteAddr, - isClient, fd, errfd, thepid); + fd, errfd, thepid, unlinkUNIX); } @@ -1309,7 +1318,7 @@ virJSONValuePtr virNetSocketPreExecRestart(virNetSocketPtr sock) if (virJSONValueObjectAppendNumberInt(object, "pid", sock->pid) < 0) goto error; - if (virJSONValueObjectAppendBoolean(object, "isClient", sock->client) < 0) + if (virJSONValueObjectAppendBoolean(object, "unlinkUNIX", sock->unlinkUNIX) < 0) goto error; if (virSetInherit(sock->fd, true) < 0) { @@ -1350,7 +1359,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); @@ -2140,8 +2149,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 +2281,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, -- 2.21.0

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- 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 892091dd83..f4a62491e5 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 3205dde78f..5e42c7f23b 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@redhat.com> --- 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..cbdad69014 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_AUTOSTRUCT(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@redhat.com> --- 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 06714249b4..0479c2ddd1 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) @@ -276,6 +281,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

Certain libvirtd.conf settings are not honoured when using systemd socket activation. Certain systemd unit file settings must match those defined in libvirtd.conf for systemd socket activation to work with systemd version < 227, otherwise libvirtd cannot determine which inherited FD to use for which service. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/remote/libvirtd-admin.socket.in | 2 ++ src/remote/libvirtd-ro.socket.in | 2 ++ src/remote/libvirtd-tcp.socket.in | 2 ++ src/remote/libvirtd-tls.socket.in | 2 ++ src/remote/libvirtd.conf | 31 +++++++++++++++++++++++++++++ src/remote/libvirtd.socket.in | 2 ++ src/remote/libvirtd.sysconf | 3 ++- 7 files changed, 43 insertions(+), 1 deletion(-) diff --git a/src/remote/libvirtd-admin.socket.in b/src/remote/libvirtd-admin.socket.in index b791a2eb1b..307c9ba24b 100644 --- a/src/remote/libvirtd-admin.socket.in +++ b/src/remote/libvirtd-admin.socket.in @@ -5,6 +5,8 @@ BindsTo=libvirtd.socket After=libvirtd.socket [Socket] +# The directory must match the /etc/libvirt/libvirtd.conf unix_sock_dir setting +# when using systemd version < 227 ListenStream=@localstatedir@/run/libvirt/libvirt-admin-sock Service=libvirtd.service SocketMode=0600 diff --git a/src/remote/libvirtd-ro.socket.in b/src/remote/libvirtd-ro.socket.in index 55c44944b4..876daf0c9c 100644 --- a/src/remote/libvirtd-ro.socket.in +++ b/src/remote/libvirtd-ro.socket.in @@ -5,6 +5,8 @@ BindsTo=libvirtd.socket After=libvirtd.socket [Socket] +# The directory must match the /etc/libvirt/libvirtd.conf unix_sock_dir setting +# when using systemd version < 227 ListenStream=@localstatedir@/run/libvirt/libvirt-sock-ro Service=libvirtd.service SocketMode=0666 diff --git a/src/remote/libvirtd-tcp.socket.in b/src/remote/libvirtd-tcp.socket.in index 09d5d3d67a..16a4764283 100644 --- a/src/remote/libvirtd-tcp.socket.in +++ b/src/remote/libvirtd-tcp.socket.in @@ -5,6 +5,8 @@ BindsTo=libvirtd.socket After=libvirtd.socket [Socket] +# This must match the /etc/libvirt/libvirtd.conf tcp_port setting +# when using systemd version < 227 ListenStream=16509 Service=libvirtd.service diff --git a/src/remote/libvirtd-tls.socket.in b/src/remote/libvirtd-tls.socket.in index c60f0c9c77..e904583cf9 100644 --- a/src/remote/libvirtd-tls.socket.in +++ b/src/remote/libvirtd-tls.socket.in @@ -5,6 +5,8 @@ BindsTo=libvirtd.socket After=libvirtd.socket [Socket] +# This must match the /etc/libvirt/libvirtd.conf tls_port setting +# when using systemd version < 227 ListenStream=16514 Service=libvirtd.service diff --git a/src/remote/libvirtd.conf b/src/remote/libvirtd.conf index bbeb053495..b63b8d61b7 100644 --- a/src/remote/libvirtd.conf +++ b/src/remote/libvirtd.conf @@ -10,6 +10,9 @@ # NB, must pass the --listen flag to the libvirtd process for this to # have any effect. # +# This setting is not required or honoured if using systemd socket +# activation. +# # It is necessary to setup a CA and issue server certificates before # using this capability. # @@ -20,6 +23,9 @@ # NB, must pass the --listen flag to the libvirtd process for this to # have any effect. # +# This setting is not required or honoured if using systemd socket +# activation. +# # Using the TCP socket requires SASL authentication by default. Only # SASL mechanisms which support data encryption are allowed. This is # DIGEST_MD5 and GSSAPI (Kerberos5) @@ -32,17 +38,26 @@ # Override the port for accepting secure TLS connections # This can be a port number, or service name # +# This setting is not required or honoured if using systemd socket +# activation with systemd version >= 227 +# #tls_port = "16514" # Override the port for accepting insecure TCP connections # This can be a port number, or service name # +# This setting is not required or honoured if using systemd socket +# activation with systemd version >= 227 +# #tcp_port = "16509" # Override the default configuration which binds to all network # interfaces. This can be a numeric IPv4/6 address, or hostname # +# This setting is not required or honoured if using systemd socket +# activation. +# # If the libvirtd service is started in parallel with network # startup (e.g. with systemd), binding to addresses other than # the wildcards (0.0.0.0/::) might not be available yet. @@ -59,12 +74,18 @@ # allow a 'trusted' set of users access to management capabilities # without becoming root. # +# This setting is not required or honoured if using systemd socket +# activation. +# # This is restricted to 'root' by default. #unix_sock_group = "libvirt" # Set the UNIX socket permissions for the R/O socket. This is used # for monitoring VM status only # +# This setting is not required or honoured if using systemd socket +# activation. +# # Default allows any user. If setting group ownership, you may want to # restrict this too. #unix_sock_ro_perms = "0777" @@ -72,6 +93,9 @@ # Set the UNIX socket permissions for the R/W socket. This is used # for full management of VMs # +# This setting is not required or honoured if using systemd socket +# activation. +# # Default allows only root. If PolicyKit is enabled on the socket, # the default will change to allow everyone (eg, 0777) # @@ -81,11 +105,18 @@ # Set the UNIX socket permissions for the admin interface socket. # +# This setting is not required or honoured if using systemd socket +# activation. +# # Default allows only owner (root), do not change it unless you are # sure to whom you are exposing the access to. #unix_sock_admin_perms = "0700" # Set the name of the directory in which sockets will be found/created. +# +# This setting is not required or honoured if using systemd socket +# activation with systemd version >= 227 +# #unix_sock_dir = "/var/run/libvirt" diff --git a/src/remote/libvirtd.socket.in b/src/remote/libvirtd.socket.in index e194c6e76e..2ee4d7d7a2 100644 --- a/src/remote/libvirtd.socket.in +++ b/src/remote/libvirtd.socket.in @@ -3,6 +3,8 @@ Description=Libvirt local socket Before=libvirtd.service [Socket] +# The directory must match the /etc/libvirt/libvirtd.conf unix_sock_dir setting +# when using systemd version < 227 ListenStream=@localstatedir@/run/libvirt/libvirt-sock Service=libvirtd.service SocketMode=0666 diff --git a/src/remote/libvirtd.sysconf b/src/remote/libvirtd.sysconf index f15e5956eb..7af41c207f 100644 --- a/src/remote/libvirtd.sysconf +++ b/src/remote/libvirtd.sysconf @@ -4,7 +4,8 @@ # in LIBVIRTD_ARGS instead. #LIBVIRTD_CONFIG=/etc/libvirt/libvirtd.conf -# Listen for TCP/IP connections +# Listen for TCP/IP connections. This is not required if using systemd +# socket activation. # NB. must setup TLS/SSL keys prior to using this #LIBVIRTD_ARGS="--listen" -- 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@redhat.com> --- src/locking/lock_daemon.c | 125 +++++++++++--------------------------- 1 file changed, 37 insertions(+), 88 deletions(-) diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index 0f90606be6..92d6c717d2 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -582,78 +582,6 @@ virLockDaemonSetupSignals(virNetDaemonPtr dmn) } -static int -virLockDaemonSetupNetworkingSystemD(virNetServerPtr lockSrv, 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, "virtlockd-admin-sock")) { - srv = adminSrv; - } else if (strstr(path, "virtlockd-sock")) { - srv = lockSrv; - } 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 -virLockDaemonSetupNetworkingNative(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; -} - - struct virLockDaemonClientReleaseData { virLockDaemonClientPtr client; bool hadSomeLeases; @@ -1356,6 +1284,12 @@ int main(int argc, char **argv) { * (but still need to add @lockProgram into @srv). rv == 0 means that no * saved state is present, therefore initialize from scratch here. */ if (rv == 0) { + VIR_AUTOSTRUCT(virSystemdActivation) *act = NULL; + virSystemdActivationMap actmap[] = { + { .name = "virtlockd.socket", .family = AF_UNIX, .path = sock_file }, + { .name = "virtlockd-admin.socket", .family = AF_UNIX, .path = admin_sock_file }, + }; + if (godaemon) { char ebuf[1024]; @@ -1383,31 +1317,46 @@ int main(int argc, char **argv) { goto cleanup; } + if (virSystemdGetActivation(actmap, + ARRAY_CARDINALITY(actmap), + &act) < 0) { + ret = VIR_LOCK_DAEMON_ERR_NETWORK; + goto cleanup; + } + lockSrv = virNetDaemonGetServer(lockDaemon->dmn, "virtlockd"); adminSrv = virNetDaemonGetServer(lockDaemon->dmn, "admin"); - if ((rv = virLockDaemonSetupNetworkingSystemD(lockSrv, adminSrv)) < 0) { + + if (virNetServerAddServiceUNIX(lockSrv, + act, "virtlockd.socket", + sock_file, 0700, 0, 0, + NULL, + false, 0, 1) < 0) { + ret = VIR_LOCK_DAEMON_ERR_NETWORK; + goto cleanup; + } + if (virNetServerAddServiceUNIX(adminSrv, + act, "virtlockd-admin.socket", + admin_sock_file, 0700, 0, 0, + NULL, + false, 0, 1) < 0) { ret = VIR_LOCK_DAEMON_ERR_NETWORK; goto cleanup; } - /* Only do this, if systemd did not pass a FD */ - if (rv == 0) { - if (virLockDaemonSetupNetworkingNative(lockSrv, sock_file) < 0 || - virLockDaemonSetupNetworkingNative(adminSrv, admin_sock_file) < 0) { - ret = VIR_LOCK_DAEMON_ERR_NETWORK; - goto cleanup; - } + if (act && + virSystemdActivationComplete(act) < 0) { + ret = VIR_LOCK_DAEMON_ERR_NETWORK; + goto cleanup; } - virObjectUnref(lockSrv); - virObjectUnref(adminSrv); + } else { + lockSrv = virNetDaemonGetServer(lockDaemon->dmn, "virtlockd"); + /* If exec-restarting from old virtlockd, we won't have an + * admin server present */ + if (virNetDaemonHasServer(lockDaemon->dmn, "admin")) + adminSrv = virNetDaemonGetServer(lockDaemon->dmn, "admin"); } - lockSrv = virNetDaemonGetServer(lockDaemon->dmn, "virtlockd"); - /* If exec-restarting from old virtlockd, we won't have an - * admin server present */ - if (virNetDaemonHasServer(lockDaemon->dmn, "admin")) - adminSrv = virNetDaemonGetServer(lockDaemon->dmn, "admin"); - if (timeout != -1) { VIR_DEBUG("Registering shutdown timeout %d", timeout); virNetDaemonAutoShutdown(lockDaemon->dmn, -- 2.21.0

The virGetListenFDs method no longer needs to be called directly, so it can be a static function internal to the systemd code. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libvirt_private.syms | 1 - src/util/virsystemd.c | 74 +++++++++++++++++++++++++++++++++++++++- src/util/virutil.c | 72 -------------------------------------- src/util/virutil.h | 1 - 4 files changed, 73 insertions(+), 75 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c560dda5e8..37a8ff1285 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3247,7 +3247,6 @@ virGetGroupList; virGetGroupName; virGetHostname; virGetHostnameQuiet; -virGetListenFDs; virGetSelfLastChanged; virGetSystemPageSize; virGetSystemPageSizeKB; diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c index ae8401343d..f6c5adc5ef 100644 --- a/src/util/virsystemd.c +++ b/src/util/virsystemd.c @@ -756,6 +756,78 @@ virSystemdActivationInitFromMap(virSystemdActivationPtr act, return -1; } +#ifndef WIN32 + +/** + * virSystemdGetListenFDs: + * + * Parse LISTEN_PID and LISTEN_FDS passed from caller. + * + * Returns number of passed FDs. + */ +static unsigned int +virSystemdGetListenFDs(void) +{ + const char *pidstr; + const char *fdstr; + size_t i = 0; + unsigned long long procid; + unsigned int nfds; + + VIR_DEBUG("Setting up networking from caller"); + + if (!(pidstr = virGetEnvAllowSUID("LISTEN_PID"))) { + VIR_DEBUG("No LISTEN_PID from caller"); + return 0; + } + + if (virStrToLong_ull(pidstr, NULL, 10, &procid) < 0) { + VIR_DEBUG("Malformed LISTEN_PID from caller %s", pidstr); + return 0; + } + + if ((pid_t)procid != getpid()) { + VIR_DEBUG("LISTEN_PID %s is not for us %lld", + pidstr, (long long) getpid()); + return 0; + } + + if (!(fdstr = virGetEnvAllowSUID("LISTEN_FDS"))) { + VIR_DEBUG("No LISTEN_FDS from caller"); + return 0; + } + + if (virStrToLong_ui(fdstr, NULL, 10, &nfds) < 0) { + VIR_DEBUG("Malformed LISTEN_FDS from caller %s", fdstr); + return 0; + } + + unsetenv("LISTEN_PID"); + unsetenv("LISTEN_FDS"); + + VIR_DEBUG("Got %u file descriptors", nfds); + + for (i = 0; i < nfds; i++) { + int fd = STDERR_FILENO + i + 1; + + VIR_DEBUG("Disabling inheritance of passed FD %d", fd); + + if (virSetInherit(fd, false) < 0) + VIR_WARN("Couldn't disable inheritance of passed FD %d", fd); + } + + return nfds; +} + +#else /* WIN32 */ + +static unsigned int +virSystemdGetListenFDs(void) +{ + return 0; +} + +#endif /* WIN32 */ static virSystemdActivationPtr virSystemdActivationNew(virSystemdActivationMap *map, @@ -812,7 +884,7 @@ virSystemdGetActivation(virSystemdActivationMap *map, { int nfds = 0; - if ((nfds = virGetListenFDs()) < 0) + if ((nfds = virSystemdGetListenFDs()) < 0) return -1; if (nfds == 0) { diff --git a/src/util/virutil.c b/src/util/virutil.c index b85769d936..4bd719127b 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1784,78 +1784,6 @@ void virUpdateSelfLastChanged(const char *path) } } -#ifndef WIN32 - -/** - * virGetListenFDs: - * - * Parse LISTEN_PID and LISTEN_FDS passed from caller. - * - * Returns number of passed FDs. - */ -unsigned int -virGetListenFDs(void) -{ - const char *pidstr; - const char *fdstr; - size_t i = 0; - unsigned long long procid; - unsigned int nfds; - - VIR_DEBUG("Setting up networking from caller"); - - if (!(pidstr = virGetEnvAllowSUID("LISTEN_PID"))) { - VIR_DEBUG("No LISTEN_PID from caller"); - return 0; - } - - if (virStrToLong_ull(pidstr, NULL, 10, &procid) < 0) { - VIR_DEBUG("Malformed LISTEN_PID from caller %s", pidstr); - return 0; - } - - if ((pid_t)procid != getpid()) { - VIR_DEBUG("LISTEN_PID %s is not for us %lld", - pidstr, (long long) getpid()); - return 0; - } - - if (!(fdstr = virGetEnvAllowSUID("LISTEN_FDS"))) { - VIR_DEBUG("No LISTEN_FDS from caller"); - return 0; - } - - if (virStrToLong_ui(fdstr, NULL, 10, &nfds) < 0) { - VIR_DEBUG("Malformed LISTEN_FDS from caller %s", fdstr); - return 0; - } - - unsetenv("LISTEN_PID"); - unsetenv("LISTEN_FDS"); - - VIR_DEBUG("Got %u file descriptors", nfds); - - for (i = 0; i < nfds; i++) { - int fd = STDERR_FILENO + i + 1; - - VIR_DEBUG("Disabling inheritance of passed FD %d", fd); - - if (virSetInherit(fd, false) < 0) - VIR_WARN("Couldn't disable inheritance of passed FD %d", fd); - } - - return nfds; -} - -#else /* WIN32 */ - -unsigned int -virGetListenFDs(void) -{ - return 0; -} - -#endif /* WIN32 */ #ifdef HAVE_SYS_UN_H char *virGetUNIXSocketPath(int fd) diff --git a/src/util/virutil.h b/src/util/virutil.h index 67a21c86bd..b9715e5e66 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -149,7 +149,6 @@ bool virIsSUID(void); time_t virGetSelfLastChanged(void); void virUpdateSelfLastChanged(const char *path); -unsigned int virGetListenFDs(void); char *virGetUNIXSocketPath(int fd); long virGetSystemPageSize(void) ATTRIBUTE_NOINLINE; -- 2.21.0

On Thu, Jun 27, 2019 at 10:54:31AM +0100, Daniel P. Berrangé wrote:

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virsocketaddr.c | 42 ++++++++++++++++++++++++++++++++++++++++ src/util/virsocketaddr.h | 2 ++ 3 files changed, 45 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 34937adc5d..ce614e04bd 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2903,6 +2903,7 @@ virSocketAddrFormat; virSocketAddrFormatFull; virSocketAddrGetIPPrefix; virSocketAddrGetNumNetmaskBits; +virSocketAddrGetPath; virSocketAddrGetPort; virSocketAddrGetRange; virSocketAddrIsNetmask; diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index 182e18aa8c..7a50cbe040 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -500,6 +500,7 @@ virSocketAddrSetPort(virSocketAddrPtr addr, int port) return 0; }

+

Unrelated whitespace.

/* * virSocketGetPort: * @addr: an initialized virSocketAddrPtr

Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

On Thu, Jun 27, 2019 at 10:54:33AM +0100, Daniel P. Berrangé wrote:

The current VIR_AUTOPTR macro assumes that the struct needs to have a auto-free function auto-generated to call the real free function.

The new VIR_AUTOSTRUCT macro allows for structs which already have a free function which takes a pointer to a pointer to the struct and can thus be used directly for auto-cleanup.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/viralloc.h | 13 +++++++++++++ 1 file changed, 13 insertions(+)

diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 2b82096fde..5de18b9ea0 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -614,3 +614,16 @@ void virAllocTestHook(void (*func)(int, void*), void *data); * when the variable goes out of scope. */ #define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type + +/** + * VIR_AUTOSTRUCT: + * @type: type of the struct variable to be freed automatically + * + * Macro to automatically free the memory allocated to + * the struct variable declared with it by calling vir$STRUCTFree + * when the variable goes out of scope. + * + * The vir$STRUCTFree function must take a pointer to a pointer + * to the struct. + */ +#define VIR_AUTOSTRUCT(type) __attribute__((cleanup(type ## Free))) type

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@redhat.com> Jano

On Thu, Jun 27, 2019 at 10:54:35AM +0100, Daniel P. Berrangé wrote:

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@redhat.com> --- 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;

This hunk is not sorted properly. Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano