[PATCH 03/24] qemu: Move creation and opening of chardev backend FDs to host prepare step

The opening of files for FD passing for a chardev backend was historically done in the function which is formatting the commandline. This has multiple problems. Firstly the function takes a lot of parameters which need to be passed through the commandline formatters. This made the 'qemuBuildChrChardevStr' extremely unappealing to the extent that we have multiple other custom formatters in places which didn't really want to use the function. Additionally the function is also creating files in the host in certain configurations which is wrong for a commandline formatter to do. This meant that e.g. not all chardev test cases can be converted to use DO_TEST_CAPS_LATEST as we attempt to use such code path and attempt to create files outside of the test directory. This patch moves the opening of the filedescriptors from 'qemuBuildChrChardevFileStr' into a new helper 'qemuProcessPrepareHostBackendChardevOne' which is called using 'qemuDomainDeviceBackendChardevForeach'. To preserve test behaviour we also have another instance 'testPrepareHostBackendChardevOne' which is populating mock filedescriptors. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_command.c | 160 +++++++++------------------ src/qemu/qemu_domain.c | 6 + src/qemu/qemu_domain.h | 3 + src/qemu/qemu_process.c | 229 ++++++++++++++++++++++++++++++++++----- tests/qemuxml2argvtest.c | 79 ++++++++++++++ 5 files changed, 338 insertions(+), 139 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c47998aabd..01388b44ef 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4967,86 +4967,6 @@ qemuBuildSCSIHostdevDevProps(const virDomainDef *def, return g_steal_pointer(&props); } -static int -qemuBuildChrChardevFileStr(virLogManager *logManager, - virSecurityManager *secManager, - virQEMUDriverConfig *cfg, - virQEMUCaps *qemuCaps, - const virDomainDef *def, - virCommand *cmd, - virBuffer *buf, - const char *filearg, const char *fileval, - const char *appendarg, int appendval) -{ - /* Technically, to pass an FD via /dev/fdset we don't need - * any capability check because X_QEMU_CAPS_ADD_FD is already - * assumed. But keeping the old style is still handy when - * building a standalone command line (e.g. for tests). */ - if (logManager || - virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS_COMMANDLINE)) { - g_autofree char *fdset = NULL; - int logfd; - size_t idx; - - if (logManager) { - int flags = 0; - - if (appendval == VIR_TRISTATE_SWITCH_ABSENT || - appendval == VIR_TRISTATE_SWITCH_OFF) - flags |= VIR_LOG_MANAGER_PROTOCOL_DOMAIN_OPEN_LOG_FILE_TRUNCATE; - - if ((logfd = virLogManagerDomainOpenLogFile(logManager, - "qemu", - def->uuid, - def->name, - fileval, - flags, - NULL, NULL)) < 0) - return -1; - } else { - int oflags = O_CREAT | O_WRONLY; - - switch (appendval) { - case VIR_TRISTATE_SWITCH_ABSENT: - case VIR_TRISTATE_SWITCH_OFF: - oflags |= O_TRUNC; - break; - case VIR_TRISTATE_SWITCH_ON: - oflags |= O_APPEND; - break; - case VIR_TRISTATE_SWITCH_LAST: - break; - } - - if ((logfd = qemuDomainOpenFile(cfg, def, fileval, oflags, NULL)) < 0) - return -1; - - if (qemuSecuritySetImageFDLabel(secManager, (virDomainDef*)def, logfd) < 0) { - VIR_FORCE_CLOSE(logfd); - return -1; - } - } - - virCommandPassFDIndex(cmd, logfd, VIR_COMMAND_PASS_FD_CLOSE_PARENT, &idx); - fdset = qemuBuildFDSet(logfd, idx); - - virCommandAddArg(cmd, "-add-fd"); - virCommandAddArg(cmd, fdset); - - virBufferAsprintf(buf, ",%s=/dev/fdset/%zu,%s=on", filearg, idx, appendarg); - } else { - virBufferAsprintf(buf, ",%s=", filearg); - virQEMUBuildBufferEscapeComma(buf, fileval); - if (appendval != VIR_TRISTATE_SWITCH_ABSENT) { - virBufferAsprintf(buf, ",%s=%s", appendarg, - virTristateSwitchTypeToString(appendval)); - } - } - - return 0; -} - - static void qemuBuildChrChardevReconnectStr(virBuffer *buf, const virDomainChrSourceReconnectDef *def) @@ -5125,11 +5045,11 @@ enum { /* This function outputs a -chardev command line option which describes only the * host side of the character device */ static char * -qemuBuildChrChardevStr(virLogManager *logManager, - virSecurityManager *secManager, +qemuBuildChrChardevStr(virLogManager *logManager G_GNUC_UNUSED, + virSecurityManager *secManager G_GNUC_UNUSED, virCommand *cmd, virQEMUDriverConfig *cfg, - const virDomainDef *def, + const virDomainDef *def G_GNUC_UNUSED, const virDomainChrSourceDef *dev, const char *alias, virQEMUCaps *qemuCaps, @@ -5166,12 +5086,27 @@ qemuBuildChrChardevStr(virLogManager *logManager, case VIR_DOMAIN_CHR_TYPE_FILE: virBufferAsprintf(&buf, "file,id=%s", charAlias); - if (qemuBuildChrChardevFileStr(cdevflags & QEMU_BUILD_CHARDEV_FILE_LOGD ? - logManager : NULL, - secManager, cfg, qemuCaps, def, cmd, &buf, - "path", dev->data.file.path, - "append", dev->data.file.append) < 0) - return NULL; + if (chrSourcePriv->fd != -1) { + g_autofree char *fdset = NULL; + size_t idx; + + virCommandPassFDIndex(cmd, chrSourcePriv->fd, + VIR_COMMAND_PASS_FD_CLOSE_PARENT, &idx); + fdset = qemuBuildFDSet(chrSourcePriv->fd, idx); + chrSourcePriv->fd = -1; + + virCommandAddArg(cmd, "-add-fd"); + virCommandAddArg(cmd, fdset); + + virBufferAsprintf(&buf, ",path=/dev/fdset/%zu,append=on", idx); + } else { + virBufferAddLit(&buf, ",path="); + virQEMUBuildBufferEscapeComma(&buf, dev->data.file.path); + if (dev->data.file.append != VIR_TRISTATE_SWITCH_ABSENT) { + virBufferAsprintf(&buf, ",append=%s", + virTristateSwitchTypeToString(dev->data.file.append)); + } + } break; case VIR_DOMAIN_CHR_TYPE_PIPE: @@ -5255,24 +5190,11 @@ qemuBuildChrChardevStr(virLogManager *logManager, case VIR_DOMAIN_CHR_TYPE_UNIX: virBufferAsprintf(&buf, "socket,id=%s", charAlias); - if (dev->data.nix.listen && - (cdevflags & QEMU_BUILD_CHARDEV_UNIX_FD_PASS) && - virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS_COMMANDLINE)) { - int fd; - - if (qemuSecuritySetSocketLabel(secManager, (virDomainDef *)def) < 0) - return NULL; - fd = qemuOpenChrChardevUNIXSocket(dev); - if (qemuSecurityClearSocketLabel(secManager, (virDomainDef *)def) < 0) { - VIR_FORCE_CLOSE(fd); - return NULL; - } - if (fd < 0) - return NULL; - - virBufferAsprintf(&buf, ",fd=%d", fd); + if (chrSourcePriv->fd != -1) { + virBufferAsprintf(&buf, ",fd=%d", chrSourcePriv->fd); - virCommandPassFD(cmd, fd, VIR_COMMAND_PASS_FD_CLOSE_PARENT); + virCommandPassFD(cmd, chrSourcePriv->fd, VIR_COMMAND_PASS_FD_CLOSE_PARENT); + chrSourcePriv->fd = -1; } else { virBufferAddLit(&buf, ",path="); virQEMUBuildBufferEscapeComma(&buf, dev->data.nix.path); @@ -5306,11 +5228,27 @@ qemuBuildChrChardevStr(virLogManager *logManager, } if (dev->logfile) { - if (qemuBuildChrChardevFileStr(logManager, secManager, cfg, - qemuCaps, def, cmd, &buf, - "logfile", dev->logfile, - "logappend", dev->logappend) < 0) - return NULL; + if (chrSourcePriv->logfd != -1) { + g_autofree char *fdset = NULL; + size_t idx; + + virCommandPassFDIndex(cmd, chrSourcePriv->logfd, + VIR_COMMAND_PASS_FD_CLOSE_PARENT, &idx); + fdset = qemuBuildFDSet(chrSourcePriv->logfd, idx); + chrSourcePriv->logfd = -1; + + virCommandAddArg(cmd, "-add-fd"); + virCommandAddArg(cmd, fdset); + + virBufferAsprintf(&buf, ",logfile=/dev/fdset/%zu,logappend=on", idx); + } else { + virBufferAddLit(&buf, ",logfile="); + virQEMUBuildBufferEscapeComma(&buf, dev->logfile); + if (dev->logappend != VIR_TRISTATE_SWITCH_ABSENT) { + virBufferAsprintf(&buf, ",logappend=%s", + virTristateSwitchTypeToString(dev->logappend)); + } + } } return virBufferContentAndReset(&buf); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 4f0d266c73..cbaa39b618 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -850,6 +850,9 @@ qemuDomainChrSourcePrivateNew(void) if (!(priv = virObjectNew(qemuDomainChrSourcePrivateClass))) return NULL; + priv->fd = -1; + priv->logfd = -1; + return (virObject *) priv; } @@ -859,6 +862,9 @@ qemuDomainChrSourcePrivateDispose(void *obj) { qemuDomainChrSourcePrivate *priv = obj; + VIR_FORCE_CLOSE(priv->fd); + VIR_FORCE_CLOSE(priv->logfd); + g_clear_pointer(&priv->secinfo, qemuDomainSecretInfoFree); } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 3de3f70b94..61704fdae7 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -341,6 +341,9 @@ struct _qemuDomainChrSourcePrivate { /* for char devices using secret * NB: *not* to be written to qemu domain object XML */ qemuDomainSecretInfo *secinfo; + + int fd; /* file descriptor of the chardev source */ + int logfd; /* file descriptor of the logging source */ }; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 6b83a571b9..866b6b9ed6 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -98,6 +98,9 @@ #include "storage_source.h" #include "backup_conf.h" +#include "logging/log_manager.h" +#include "logging/log_protocol.h" + #define VIR_FROM_THIS VIR_FROM_QEMU VIR_LOG_INIT("qemu.qemu_process"); @@ -3069,29 +3072,6 @@ qemuProcessInitPasswords(virQEMUDriver *driver, } -static int -qemuProcessPrepareChardevDevice(virDomainDef *def G_GNUC_UNUSED, - virDomainChrDef *dev, - void *opaque G_GNUC_UNUSED) -{ - int fd; - if (dev->source->type != VIR_DOMAIN_CHR_TYPE_FILE) - return 0; - - if ((fd = open(dev->source->data.file.path, - O_CREAT | O_APPEND, S_IRUSR|S_IWUSR)) < 0) { - virReportSystemError(errno, - _("Unable to pre-create chardev file '%s'"), - dev->source->data.file.path); - return -1; - } - - VIR_FORCE_CLOSE(fd); - - return 0; -} - - static int qemuProcessCleanupChardevDevice(virDomainDef *def G_GNUC_UNUSED, virDomainChrDef *dev, @@ -6806,6 +6786,200 @@ qemuProcessOpenVhostVsock(virDomainVsockDef *vsock) } +static int +qemuProcessPrepareHostBackendChardevFileHelper(const char *path, + virTristateSwitch append, + int *fd, + virLogManager *logManager, + virSecurityManager *secManager, + virQEMUCaps *qemuCaps, + virQEMUDriverConfig *cfg, + const virDomainDef *def) +{ + /* Technically, to pass an FD via /dev/fdset we don't need + * any capability check because X_QEMU_CAPS_ADD_FD is already + * assumed. But keeping the old style is still handy when + * building a standalone command line (e.g. for tests). */ + if (!logManager && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS_COMMANDLINE)) + return 0; + + if (logManager) { + int flags = 0; + + if (append == VIR_TRISTATE_SWITCH_ABSENT || + append == VIR_TRISTATE_SWITCH_OFF) + flags |= VIR_LOG_MANAGER_PROTOCOL_DOMAIN_OPEN_LOG_FILE_TRUNCATE; + + if ((*fd = virLogManagerDomainOpenLogFile(logManager, + "qemu", + def->uuid, + def->name, + path, + flags, + NULL, NULL)) < 0) + return -1; + } else { + int oflags = O_CREAT | O_WRONLY; + + switch (append) { + case VIR_TRISTATE_SWITCH_ABSENT: + case VIR_TRISTATE_SWITCH_OFF: + oflags |= O_TRUNC; + break; + case VIR_TRISTATE_SWITCH_ON: + oflags |= O_APPEND; + break; + case VIR_TRISTATE_SWITCH_LAST: + break; + } + + if ((*fd = qemuDomainOpenFile(cfg, def, path, oflags, NULL)) < 0) + return -1; + + if (qemuSecuritySetImageFDLabel(secManager, (virDomainDef*)def, *fd) < 0) { + VIR_FORCE_CLOSE(*fd); + return -1; + } + } + + return 0; +} + + +struct qemuProcessPrepareHostBackendChardevData { + virQEMUCaps *qemuCaps; + virLogManager *logManager; + virSecurityManager *secManager; + virQEMUDriverConfig *cfg; + virDomainDef *def; +}; + + +static int +qemuProcessPrepareHostBackendChardevOne(virDomainDeviceDef *dev, + virDomainChrSourceDef *chardev, + void *opaque) +{ + struct qemuProcessPrepareHostBackendChardevData *data = opaque; + qemuDomainChrSourcePrivate *charpriv = QEMU_DOMAIN_CHR_SOURCE_PRIVATE(chardev); + + /* this function is also called for the monitor backend which doesn't have + * a 'dev' */ + if (dev) { + if (dev->type == VIR_DOMAIN_DEVICE_NET) { + /* due to a historical bug in qemu we don't use FD passtrhough for + * vhost-sockets for network devices */ + return 0; + } + } + + switch ((virDomainChrType) chardev->type) { + case VIR_DOMAIN_CHR_TYPE_NULL: + case VIR_DOMAIN_CHR_TYPE_VC: + case VIR_DOMAIN_CHR_TYPE_PTY: + case VIR_DOMAIN_CHR_TYPE_DEV: + case VIR_DOMAIN_CHR_TYPE_PIPE: + case VIR_DOMAIN_CHR_TYPE_STDIO: + case VIR_DOMAIN_CHR_TYPE_UDP: + case VIR_DOMAIN_CHR_TYPE_TCP: + case VIR_DOMAIN_CHR_TYPE_SPICEVMC: + case VIR_DOMAIN_CHR_TYPE_SPICEPORT: + break; + + case VIR_DOMAIN_CHR_TYPE_FILE: + if (qemuProcessPrepareHostBackendChardevFileHelper(chardev->data.file.path, + chardev->data.file.append, + &charpriv->fd, + data->logManager, + data->secManager, + data->qemuCaps, + data->cfg, + data->def) < 0) + return -1; + + break; + + case VIR_DOMAIN_CHR_TYPE_UNIX: + if (chardev->data.nix.listen && + virQEMUCapsGet(data->qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS_COMMANDLINE)) { + + if (qemuSecuritySetSocketLabel(data->secManager, data->def) < 0) + return -1; + + charpriv->fd = qemuOpenChrChardevUNIXSocket(chardev); + + if (qemuSecurityClearSocketLabel(data->secManager, data->def) < 0) { + VIR_FORCE_CLOSE(charpriv->fd); + return -1; + } + + if (charpriv->fd < 0) + return -1; + } + break; + + case VIR_DOMAIN_CHR_TYPE_NMDM: + case VIR_DOMAIN_CHR_TYPE_LAST: + default: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported chardev '%s'"), + virDomainChrTypeToString(chardev->type)); + return -1; + } + + if (chardev->logfile) { + if (qemuProcessPrepareHostBackendChardevFileHelper(chardev->logfile, + chardev->logappend, + &charpriv->logfd, + data->logManager, + data->secManager, + data->qemuCaps, + data->cfg, + data->def) < 0) + return -1; + } + + return 0; +} + + +/* prepare the chardev backends for various devices: + * serial/parallel/channel chardevs, vhost-user disks, vhost-user network + * interfaces, smartcards, shared memory, and redirdevs */ +static int +qemuProcessPrepareHostBackendChardev(virDomainObj *vm, + virQEMUCaps *qemuCaps, + virSecurityManager *secManager) +{ + qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver); + struct qemuProcessPrepareHostBackendChardevData data = { + .qemuCaps = qemuCaps, + .logManager = NULL, + .secManager = secManager, + .cfg = cfg, + .def = vm->def, + }; + g_autoptr(virLogManager) logManager = NULL; + + if (cfg->stdioLogD) { + if (!(logManager = data.logManager = virLogManagerNew(priv->driver->privileged))) + return -1; + } + + if (qemuDomainDeviceBackendChardevForeach(vm->def, + qemuProcessPrepareHostBackendChardevOne, + &data) < 0) + return -1; + + if (qemuProcessPrepareHostBackendChardevOne(NULL, priv->monConfig, &data) < 0) + return -1; + + return 0; +} + + /** * qemuProcessPrepareHost: * @driver: qemu driver @@ -6852,11 +7026,10 @@ qemuProcessPrepareHost(virQEMUDriver *driver, hostdev_flags) < 0) return -1; - VIR_DEBUG("Preparing chr devices"); - if (virDomainChrDefForeach(vm->def, - true, - qemuProcessPrepareChardevDevice, - NULL) < 0) + VIR_DEBUG("Preparing chr device backends"); + if (qemuProcessPrepareHostBackendChardev(vm, + priv->qemuCaps, + driver->securityManager) < 0) return -1; if (qemuProcessBuildDestroyMemoryPaths(driver, vm, NULL, true) < 0) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 5e4cd7389c..9cece1df91 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -376,6 +376,71 @@ testCheckExclusiveFlags(int flags) } +static int +testPrepareHostBackendChardevOne(virDomainDeviceDef *dev, + virDomainChrSourceDef *chardev, + void *opaque) +{ + virQEMUCaps *qemuCaps = opaque; + qemuDomainChrSourcePrivate *charpriv = QEMU_DOMAIN_CHR_SOURCE_PRIVATE(chardev); + + if (dev) { + if (dev->type == VIR_DOMAIN_DEVICE_NET) { + /* due to a historical bug in qemu we don't use FD passtrhough for + * vhost-sockets for network devices */ + return 0; + } + } + + switch ((virDomainChrType) chardev->type) { + case VIR_DOMAIN_CHR_TYPE_NULL: + case VIR_DOMAIN_CHR_TYPE_VC: + case VIR_DOMAIN_CHR_TYPE_PTY: + case VIR_DOMAIN_CHR_TYPE_DEV: + case VIR_DOMAIN_CHR_TYPE_PIPE: + case VIR_DOMAIN_CHR_TYPE_STDIO: + case VIR_DOMAIN_CHR_TYPE_UDP: + case VIR_DOMAIN_CHR_TYPE_TCP: + case VIR_DOMAIN_CHR_TYPE_SPICEVMC: + case VIR_DOMAIN_CHR_TYPE_SPICEPORT: + break; + + case VIR_DOMAIN_CHR_TYPE_FILE: + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS_COMMANDLINE)) { + if (fcntl(1750, F_GETFD) != -1) + abort(); + charpriv->fd = 1750; + } + break; + + case VIR_DOMAIN_CHR_TYPE_UNIX: + if (chardev->data.nix.listen && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS_COMMANDLINE)) { + + if (fcntl(1729, F_GETFD) != -1) + abort(); + + charpriv->fd = 1729; + } + break; + + case VIR_DOMAIN_CHR_TYPE_NMDM: + case VIR_DOMAIN_CHR_TYPE_LAST: + break; + } + + if (chardev->logfile) { + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS_COMMANDLINE)) { + if (fcntl(1751, F_GETFD) != -1) + abort(); + charpriv->logfd = 1751; + } + } + + return 0; +} + + static virCommand * testCompareXMLToArgvCreateArgs(virQEMUDriver *drv, virDomainObj *vm, @@ -391,6 +456,20 @@ testCompareXMLToArgvCreateArgs(virQEMUDriver *drv, VIR_QEMU_PROCESS_START_COLD) < 0) return NULL; + if (qemuDomainDeviceBackendChardevForeach(vm->def, + testPrepareHostBackendChardevOne, + info->qemuCaps) < 0) + return NULL; + + if (virQEMUCapsGet(info->qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS_COMMANDLINE)) { + qemuDomainChrSourcePrivate *monpriv = QEMU_DOMAIN_CHR_SOURCE_PRIVATE(priv->monConfig); + + if (fcntl(1729, F_GETFD) != -1) + abort(); + + monpriv->fd = 1729; + } + for (i = 0; i < vm->def->ndisks; i++) { virDomainDiskDef *disk = vm->def->disks[i]; -- 2.31.1