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@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>

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,
+qemuBuildChrChardevStr(virLogManager *logManager G_GNUC_UNUSED,
-                       virSecurityManager *secManager,
+                       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 ?
+        if (chrSourcePriv->fd != -1) {
-                                       logManager : NULL,
+            g_autofree char *fdset = NULL;
-                                       secManager, cfg, qemuCaps, def, cmd, &buf,
+            size_t idx;
-                                       "path", dev->data.file.path,
+
-                                       "append", dev->data.file.append) < 0)
+            virCommandPassFDIndex(cmd, chrSourcePriv->fd,
-            return NULL;
+                                  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 &&
+        if (chrSourcePriv->fd != -1) {
-            (cdevflags & QEMU_BUILD_CHARDEV_UNIX_FD_PASS) &&
+            virBufferAsprintf(&buf, ",fd=%d", chrSourcePriv->fd);
-            virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS_COMMANDLINE)) {
-            int fd;
 
-            if (qemuSecuritySetSocketLabel(secManager, (virDomainDef *)def) < 0)
+            virCommandPassFD(cmd, chrSourcePriv->fd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);
-                return NULL;
+            chrSourcePriv->fd = -1;
-            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);
-
-            virCommandPassFD(cmd, fd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);
         } 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,
+        if (chrSourcePriv->logfd != -1) {
-                                       qemuCaps, def, cmd, &buf,
+            g_autofree char *fdset = NULL;
-                                       "logfile", dev->logfile,
+            size_t idx;
-                                       "logappend", dev->logappend) < 0)
+
-            return NULL;
+            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);

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);
 }
 

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 */
 };
 
 

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,
@@ -6802,6 +6782,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
@@ -6848,11 +7022,10 @@ qemuProcessPrepareHost(virQEMUDriver *driver,
                                         hostdev_flags) < 0)
         return -1;
 
-    VIR_DEBUG("Preparing chr devices");
+    VIR_DEBUG("Preparing chr device backends");
-    if (virDomainChrDefForeach(vm->def,
+    if (qemuProcessPrepareHostBackendChardev(vm,
-                               true,
+                                             priv->qemuCaps,
-                               qemuProcessPrepareChardevDevice,
+                                             driver->securityManager) < 0)
-                               NULL) < 0)
         return -1;
 
     if (qemuProcessBuildDestroyMemoryPaths(driver, vm, NULL, true) < 0)

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];