On Thu, Feb 10, 2022 at 18:53:43 +0100, Ján Tomko wrote: Hmm, okay, I'll thus also fix qemuDomainStorageIdNew/qemuDomainStorageIdReset separately. Not really. FDset ID '0' is valid in qemu. Starting from 1 has the nice side effect of seeing that this counter was used, but that's not very helpful and we have few tests adding multiple fdsets where it's possible to see too, so I'll change it.

On a Wednesday in 2022, Peter Krempa wrote: *handling I believe that for new code, all we need is: SPDX-License-Identifier: LGPL-2.1-or-later (which is shorter and easier to read, but will be inconsistent) I dislike using bools in helpers, can this be turned into a flag? Alternatively, to discourage the older approach you can create a thin qemuFDPassNewLegacy wrapper and leave qemuFDPassNew without the comment. (Can we even convince QEMU to use fdset for sockets? Or is that not worth pursuing?) QEMU This check conflicts with the comment above saying multiple FDs are allowed with fdsets. Also, please s/1/one/ Reviewed-by: Ján Tomko <jtomko(a)redhat.com&gt; Jano

Use the new helpers for passing of the file descriptor needed for 'vdpa' interfaces. Apart from the simplification in this case it will allow further changes to unify all fdset handling. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_command.c | 35 ++++++++----------- .../net-vdpa.x86_64-latest.args | 4 +-- 2 files changed, 17 insertions(+), 22 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 9713467aa8..839cd05e23 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -29,6 +29,7 @@ #include "qemu_security.h" #include "qemu_slirp.h" #include "qemu_block.h" +#include "qemu_fd.h" #include "viralloc.h" #include "virlog.h" #include "virarch.h" @@ -8732,6 +8733,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriver *driver, size_t *nnicindexes, int **nicindexes) { + qemuDomainObjPrivate *priv = vm->privateData; virDomainDef *def = vm->def; int ret = -1; g_autoptr(virJSONValue) nicprops = NULL; @@ -8743,8 +8745,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriver *driver, char **tapfdName = NULL; char **vhostfdName = NULL; g_autofree char *slirpfdName = NULL; - g_autofree char *vdpafdName = NULL; - int vdpafd = -1; + g_autoptr(qemuFDPass) vdpa = NULL; virDomainNetType actualType = virDomainNetGetActualType(net); const virNetDevBandwidth *actualBandwidth; bool requireNicdev = false; @@ -8823,9 +8824,17 @@ qemuBuildInterfaceCommandLine(virQEMUDriver *driver, break; - case VIR_DOMAIN_NET_TYPE_VDPA: + case VIR_DOMAIN_NET_TYPE_VDPA: { + VIR_AUTOCLOSE vdpafd = -1; + if ((vdpafd = qemuInterfaceVDPAConnect(net)) < 0) goto cleanup; + + vdpa = qemuFDPassNew(net->data.vdpa.devicepath, priv, true); + + if (qemuFDPassAddFD(vdpa, &vdpafd, NULL) < 0) + return -1; + } break; case VIR_DOMAIN_NET_TYPE_USER: @@ -8958,26 +8967,13 @@ qemuBuildInterfaceCommandLine(virQEMUDriver *driver, vhostfd[i] = -1; } - if (vdpafd > 0) { - g_autofree char *fdset = NULL; - g_autofree char *addfdarg = NULL; - size_t idx; - - virCommandPassFDIndex(cmd, vdpafd, VIR_COMMAND_PASS_FD_CLOSE_PARENT, &idx); - fdset = qemuBuildFDSet(vdpafd, idx); - vdpafdName = g_strdup_printf("/dev/fdset/%zu", idx); - /* set opaque to the devicepath so that we can look up the fdset later - * if necessary */ - addfdarg = g_strdup_printf("%s,opaque=%s", fdset, - net->data.vdpa.devicepath); - virCommandAddArgList(cmd, "-add-fd", addfdarg, NULL); - vdpafd = -1; - } + qemuFDPassTransferCommand(vdpa, cmd); if (!(hostnetprops = qemuBuildHostNetProps(net, tapfdName, tapfdSize, vhostfdName, vhostfdSize, - slirpfdName, vdpafdName))) + slirpfdName, + qemuFDPassGetPath(vdpa)))) goto cleanup; if (qemuBuildNetdevCommandlineFromJSON(cmd, hostnetprops, qemuCaps) < 0) @@ -9035,7 +9031,6 @@ qemuBuildInterfaceCommandLine(virQEMUDriver *driver, VIR_FREE(tapfdName); VIR_FREE(vhostfd); VIR_FREE(tapfd); - VIR_FORCE_CLOSE(vdpafd); return ret; } diff --git a/tests/qemuxml2argvdata/net-vdpa.x86_64-latest.args b/tests/qemuxml2argvdata/net-vdpa.x86_64-latest.args index a56e08a4f1..0300f36314 100644 --- a/tests/qemuxml2argvdata/net-vdpa.x86_64-latest.args +++ b/tests/qemuxml2argvdata/net-vdpa.x86_64-latest.args @@ -28,8 +28,8 @@ XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \ -no-acpi \ -boot strict=on \ -device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \ --add-fd set=0,fd=1732,opaque=/dev/vhost-vdpa-0 \ --netdev vhost-vdpa,vhostdev=/dev/fdset/0,id=hostnet0 \ +-add-fd set=1,fd=1732,opaque=/dev/vhost-vdpa-0 \ +-netdev vhost-vdpa,vhostdev=/dev/fdset/1,id=hostnet0 \ -device '{"driver":"virtio-net-pci","netdev":"hostnet0","id":"net0","mac":"52:54:00:95:db:c0","bus":"pci.0","addr":"0x2"}' \ -audiodev '{"id":"audio1","driver":"none"}' \ -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ -- 2.34.1

The code unplugging the fdset for a 'vdpa' network device can be later reused. Extract it into 'qemuHotplugRemoveFDSet'. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_hotplug.c | 69 +++++++++++++++++++++++------------------ 1 file changed, 39 insertions(+), 30 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index ac2b07d940..3264f287c3 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -137,6 +137,44 @@ qemuDomainDeleteDevice(virDomainObj *vm, } +/** + * qemuHotplugRemoveFDSet: + * @mon: monitor object + * @fdname: the 'opaque' string used as a name for the FD + * + * Looks up the 'fdset' by looking for a fd inside one of the fdsets which + * has the opaque string set to @fdname. Removes the whole fdset which contains + * the fd. + * + * Errors are logged, but this is a best-effort hot-unplug cleanup helper so it's + * pointless to return a value. + */ +static void +qemuHotplugRemoveFDSet(qemuMonitor *mon, + const char *fdname) +{ + g_autoptr(qemuMonitorFdsets) fdsets = NULL; + size_t i; + + if (qemuMonitorQueryFdsets(mon, &fdsets) < 0) + return; + + for (i = 0; i < fdsets->nfdsets; i++) { + qemuMonitorFdsetInfo *set = &fdsets->fdsets[i]; + size_t j; + + for (j = 0; j < set->nfds; j++) { + qemuMonitorFdsetFdInfo *fdinfo = &set->fds[j]; + + if (STREQ_NULLABLE(fdinfo->opaque, fdname)) { + ignore_value(qemuMonitorRemoveFdset(mon, set->id)); + return; + } + } + } +} + + static int qemuDomainDetachZPCIDevice(qemuMonitor *mon, virDomainDeviceInfo *info) @@ -4761,38 +4799,9 @@ qemuDomainRemoveNetDevice(virQEMUDriver *driver, */ } } else if (actualType == VIR_DOMAIN_NET_TYPE_VDPA) { - int vdpafdset = -1; - g_autoptr(qemuMonitorFdsets) fdsets = NULL; - - /* query qemu for which fdset is associated with the fd that we passed - * to qemu via 'add-fd' for this vdpa device. If we don't remove the - * fd, qemu will keep it open */ - if (qemuMonitorQueryFdsets(priv->mon, &fdsets) == 0) { - for (i = 0; i < fdsets->nfdsets && vdpafdset < 0; i++) { - size_t j; - qemuMonitorFdsetInfo *set = &fdsets->fdsets[i]; - - for (j = 0; j < set->nfds; j++) { - qemuMonitorFdsetFdInfo *fdinfo = &set->fds[j]; - if (STREQ_NULLABLE(fdinfo->opaque, net->data.vdpa.devicepath)) { - vdpafdset = set->id; - break; - } - } - } - } - - if (vdpafdset < 0) { - VIR_WARN("Cannot determine fdset for vdpa device"); - } else { - if (qemuMonitorRemoveFdset(priv->mon, vdpafdset) < 0) { - /* if it fails, there's not much we can do... just carry on */ - VIR_WARN("failed to close vdpa device"); - } - } + qemuHotplugRemoveFDSet(priv->mon, net->data.vdpa.devicepath); } - qemuDomainObjExitMonitor(driver, vm); if (QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp) -- 2.34.1

For now we have only one code path ('vdpa' interface) which actually cleans up the fdset after it's done, but there are more device types using fdsets. In order to unify the handling of fdsets the removal code will now be able to remove fdsets based on a prefix of the 'opaque' field, which we'll always prefix with a device alias or e.g. node name once fdsets are also used for disk backing. To keep compatibility with old qemus, retain the possibility for the VDPA interface to use the path. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_hotplug.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 3264f287c3..03b7ca30de 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -140,18 +140,21 @@ qemuDomainDeleteDevice(virDomainObj *vm, /** * qemuHotplugRemoveFDSet: * @mon: monitor object - * @fdname: the 'opaque' string used as a name for the FD + * @prefix: the prefix of FD names ('opaque' filed) to delete + * @alternate: alternate name for FD, for historical usage (may be NULL) * * Looks up the 'fdset' by looking for a fd inside one of the fdsets which - * has the opaque string set to @fdname. Removes the whole fdset which contains - * the fd. + * has the opaque string starting with @prefix. Removes the whole fdset which + * contains the fd. Alternatively if @alternate is specified fdsets having a fd + * with that exact 'opaque' string is removed too. * * Errors are logged, but this is a best-effort hot-unplug cleanup helper so it's * pointless to return a value. */ static void qemuHotplugRemoveFDSet(qemuMonitor *mon, - const char *fdname) + const char *prefix, + const char *alternate) { g_autoptr(qemuMonitorFdsets) fdsets = NULL; size_t i; @@ -166,9 +169,11 @@ qemuHotplugRemoveFDSet(qemuMonitor *mon, for (j = 0; j < set->nfds; j++) { qemuMonitorFdsetFdInfo *fdinfo = &set->fds[j]; - if (STREQ_NULLABLE(fdinfo->opaque, fdname)) { + if (fdinfo->opaque && + (STRPREFIX(fdinfo->opaque, prefix) || + STREQ_NULLABLE(fdinfo->opaque, alternate))) { ignore_value(qemuMonitorRemoveFdset(mon, set->id)); - return; + break; } } } @@ -4799,7 +4804,7 @@ qemuDomainRemoveNetDevice(virQEMUDriver *driver, */ } } else if (actualType == VIR_DOMAIN_NET_TYPE_VDPA) { - qemuHotplugRemoveFDSet(priv->mon, net->data.vdpa.devicepath); + qemuHotplugRemoveFDSet(priv->mon, net->info.alias, net->data.vdpa.devicepath); } qemuDomainObjExitMonitor(driver, vm); -- 2.34.1

Prefix the file descriptor name with the alias of the network device so that it's similar to other upcoming use. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_command.c | 4 ++-- tests/qemuxml2argvdata/net-vdpa.x86_64-latest.args | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 839cd05e23..6667433616 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8830,9 +8830,9 @@ qemuBuildInterfaceCommandLine(virQEMUDriver *driver, if ((vdpafd = qemuInterfaceVDPAConnect(net)) < 0) goto cleanup; - vdpa = qemuFDPassNew(net->data.vdpa.devicepath, priv, true); + vdpa = qemuFDPassNew(net->info.alias, priv, true); - if (qemuFDPassAddFD(vdpa, &vdpafd, NULL) < 0) + if (qemuFDPassAddFD(vdpa, &vdpafd, "-vdpa") < 0) return -1; } break; diff --git a/tests/qemuxml2argvdata/net-vdpa.x86_64-latest.args b/tests/qemuxml2argvdata/net-vdpa.x86_64-latest.args index 0300f36314..aff024af2f 100644 --- a/tests/qemuxml2argvdata/net-vdpa.x86_64-latest.args +++ b/tests/qemuxml2argvdata/net-vdpa.x86_64-latest.args @@ -28,7 +28,7 @@ XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \ -no-acpi \ -boot strict=on \ -device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \ --add-fd set=1,fd=1732,opaque=/dev/vhost-vdpa-0 \ +-add-fd set=1,fd=1732,opaque=net0-vdpa \ -netdev vhost-vdpa,vhostdev=/dev/fdset/1,id=hostnet0 \ -device '{"driver":"virtio-net-pci","netdev":"hostnet0","id":"net0","mac":"52:54:00:95:db:c0","bus":"pci.0","addr":"0x2"}' \ -audiodev '{"id":"audio1","driver":"none"}' \ -- 2.34.1

Rewrite the parts which already pass FDs via fdset or directly to use the new infrastructure. Apart from simpler code this also adds the appropriate names to the fds in the fdsets which will allow us to properly remove the fdsets won hot-unplug of chardevs, which we didn't do for now and resulted in leaking the FDs. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_command.c | 74 ++++--------------- src/qemu/qemu_domain.c | 11 +-- src/qemu/qemu_domain.h | 8 +- src/qemu/qemu_process.c | 42 +++++++++-- .../qemuxml2argvdata/aarch64-pci-serial.args | 4 +- .../name-escape.x86_64-2.11.0.args | 4 +- .../name-escape.x86_64-latest.args | 4 +- .../qemuxml2argvdata/serial-file-chardev.args | 4 +- .../serial-file-chardev.x86_64-latest.args | 4 +- tests/qemuxml2argvdata/serial-file-log.args | 6 +- .../serial-file-log.x86_64-latest.args | 6 +- .../qemuxml2argvdata/serial-many-chardev.args | 4 +- .../serial-many-chardev.x86_64-latest.args | 4 +- tests/qemuxml2argvtest.c | 56 ++++++++------ 14 files changed, 108 insertions(+), 123 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6667433616..9a51a373b0 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -301,23 +301,6 @@ qemuBuildMasterKeyCommandLine(virCommand *cmd, } -/** - * qemuBuildFDSet: - * @fd: fd to reassign to the child - * @idx: index in the fd set - * - * Format the parameters for the -add-fd command line option - * for the given file descriptor. The file descriptor must previously - * have been 'transferred' in a virCommandPassFDIndex() call, - * and @idx is the value returned by that call. - */ -static char * -qemuBuildFDSet(int fd, size_t idx) -{ - return g_strdup_printf("set=%zu,fd=%d", idx, fd); -} - - /** * qemuVirCommandGetFDSet: * @cmd: the command to modify @@ -1362,8 +1345,8 @@ qemuBuildChardevStr(const virDomainChrSourceDef *dev, path = dev->data.file.path; append = dev->data.file.append; - if (chrSourcePriv->fdset) { - path = chrSourcePriv->fdset; + if (chrSourcePriv->sourcefd) { + path = qemuFDPassGetPath(chrSourcePriv->sourcefd); append = VIR_TRISTATE_SWITCH_ON; } @@ -1430,8 +1413,8 @@ qemuBuildChardevStr(const virDomainChrSourceDef *dev, case VIR_DOMAIN_CHR_TYPE_UNIX: virBufferAsprintf(&buf, "socket,id=%s", charAlias); - if (chrSourcePriv->passedFD != -1) { - virBufferAsprintf(&buf, ",fd=%d", chrSourcePriv->passedFD); + if (chrSourcePriv->sourcefd) { + virBufferAsprintf(&buf, ",fd=%s", qemuFDPassGetPath(chrSourcePriv->sourcefd)); } else { virBufferAddLit(&buf, ",path="); virQEMUBuildBufferEscapeComma(&buf, dev->data.nix.path); @@ -1466,8 +1449,8 @@ qemuBuildChardevStr(const virDomainChrSourceDef *dev, path = dev->logfile; append = dev->logappend; - if (chrSourcePriv->logFdset) { - path = chrSourcePriv->logFdset; + if (chrSourcePriv->logfd) { + path = qemuFDPassGetPath(chrSourcePriv->logfd); append = VIR_TRISTATE_SWITCH_ON; } @@ -1527,28 +1510,8 @@ qemuBuildChardevCommand(virCommand *cmd, break; case VIR_DOMAIN_CHR_TYPE_FILE: - 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); - - chrSourcePriv->fdset = g_strdup_printf("/dev/fdset/%zu", idx); - } - break; - case VIR_DOMAIN_CHR_TYPE_UNIX: - if (chrSourcePriv->fd != -1) { - virCommandPassFD(cmd, chrSourcePriv->fd, VIR_COMMAND_PASS_FD_CLOSE_PARENT); - chrSourcePriv->passedFD = chrSourcePriv->fd; - chrSourcePriv->fd = -1; - } + qemuFDPassTransferCommand(chrSourcePriv->sourcefd, cmd); break; case VIR_DOMAIN_CHR_TYPE_NULL: @@ -1571,20 +1534,7 @@ qemuBuildChardevCommand(virCommand *cmd, return -1; } - 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); - - chrSourcePriv->logFdset = g_strdup_printf("/dev/fdset/%zu", idx); - } + qemuFDPassTransferCommand(chrSourcePriv->logfd, cmd); if (!(charstr = qemuBuildChardevStr(dev, charAlias))) return -1; @@ -5001,8 +4951,12 @@ qemuBuildVideoCommandLine(virCommand *cmd, qemuDomainChrSourcePrivate *chrsrcpriv = QEMU_DOMAIN_CHR_SOURCE_PRIVATE(chrsrc); chrsrc->type = VIR_DOMAIN_CHR_TYPE_UNIX; - chrsrcpriv->fd = videopriv->vhost_user_fd; - videopriv->vhost_user_fd = -1; + chrsrcpriv->sourcefd = qemuFDPassNew(video->info.alias, priv, false); + + if (qemuFDPassAddFD(chrsrcpriv->sourcefd, + &videopriv->vhost_user_fd, + "-vhost-user") < 0) + return -1; if (qemuBuildChardevCommand(cmd, chrsrc, chrAlias, priv->qemuCaps) < 0) return -1; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d0abf76e4a..5483e0720f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -904,11 +904,6 @@ qemuDomainChrSourcePrivateNew(void) if (!(priv = virObjectNew(qemuDomainChrSourcePrivateClass))) return NULL; - priv->fd = -1; - priv->logfd = -1; - - priv->passedFD = -1; - return (virObject *) priv; } @@ -918,13 +913,11 @@ qemuDomainChrSourcePrivateDispose(void *obj) { qemuDomainChrSourcePrivate *priv = obj; - VIR_FORCE_CLOSE(priv->fd); - VIR_FORCE_CLOSE(priv->logfd); + qemuFDPassFree(priv->sourcefd); + qemuFDPassFree(priv->logfd); g_free(priv->tlsCertPath); - g_free(priv->fdset); - g_free(priv->logFdset); g_free(priv->tlsCredsAlias); g_clear_pointer(&priv->secinfo, qemuDomainSecretInfoFree); diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index b7d0f5d2d5..e9901cb5c3 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -36,6 +36,7 @@ #include "qemu_capabilities.h" #include "qemu_migration_params.h" #include "qemu_slirp.h" +#include "qemu_fd.h" #include "virmdev.h" #include "virchrdev.h" #include "virobject.h" @@ -347,16 +348,13 @@ struct _qemuDomainChrSourcePrivate { * 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 */ + qemuFDPass *sourcefd; + qemuFDPass *logfd; bool wait; /* wait for incoming connections on chardev */ char *tlsCertPath; /* path to certificates if TLS is requested */ bool tlsVerify; /* whether server should verify client certificates */ - char *fdset; /* fdset path corresponding to the passed filedescriptor */ - char *logFdset; /* fdset path corresponding to the passed filedescriptor for logfile */ - int passedFD; /* filedescriptor number when fdset passing it directly */ char *tlsCredsAlias; /* alias of the x509 tls credentials object */ }; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 0e17e1bf94..b44e684589 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6809,6 +6809,7 @@ struct qemuProcessPrepareHostBackendChardevData { virLogManager *logManager; virQEMUDriverConfig *cfg; virDomainDef *def; + const char *fdprefix; }; @@ -6819,10 +6820,14 @@ qemuProcessPrepareHostBackendChardevOne(virDomainDeviceDef *dev, { struct qemuProcessPrepareHostBackendChardevData *data = opaque; qemuDomainChrSourcePrivate *charpriv = QEMU_DOMAIN_CHR_SOURCE_PRIVATE(chardev); + const char *devalias = NULL; /* this function is also called for the monitor backend which doesn't have * a 'dev' */ if (dev) { + virDomainDeviceInfo *info = virDomainDeviceGetInfo(dev); + devalias = info->alias; + /* vhost-user disk doesn't use FD passing */ if (dev->type == VIR_DOMAIN_DEVICE_DISK) return 0; @@ -6836,6 +6841,8 @@ qemuProcessPrepareHostBackendChardevOne(virDomainDeviceDef *dev, /* TPMs FD passing setup is special and handled separately */ if (dev->type == VIR_DOMAIN_DEVICE_TPM) return 0; + } else { + devalias = data->fdprefix; } switch ((virDomainChrType) chardev->type) { @@ -6851,33 +6858,43 @@ qemuProcessPrepareHostBackendChardevOne(virDomainDeviceDef *dev, case VIR_DOMAIN_CHR_TYPE_SPICEPORT: break; - case VIR_DOMAIN_CHR_TYPE_FILE: + case VIR_DOMAIN_CHR_TYPE_FILE: { + VIR_AUTOCLOSE sourcefd = -1; + if (qemuProcessPrepareHostBackendChardevFileHelper(chardev->data.file.path, chardev->data.file.append, - &charpriv->fd, + &sourcefd, data->logManager, data->priv->driver->securityManager, data->cfg, data->def) < 0) return -1; + charpriv->sourcefd = qemuFDPassNew(devalias, data->priv, true); + + if (qemuFDPassAddFD(charpriv->sourcefd, &sourcefd, "-source") < 0) + return -1; + } break; case VIR_DOMAIN_CHR_TYPE_UNIX: if (chardev->data.nix.listen && virQEMUCapsGet(data->priv->qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS_COMMANDLINE)) { + VIR_AUTOCLOSE sourcefd = -1; if (qemuSecuritySetSocketLabel(data->priv->driver->securityManager, data->def) < 0) return -1; - charpriv->fd = qemuOpenChrChardevUNIXSocket(chardev); + sourcefd = qemuOpenChrChardevUNIXSocket(chardev); - if (qemuSecurityClearSocketLabel(data->priv->driver->securityManager, data->def) < 0) { - VIR_FORCE_CLOSE(charpriv->fd); + if (qemuSecurityClearSocketLabel(data->priv->driver->securityManager, data->def) < 0 || + sourcefd < 0) return -1; - } - if (charpriv->fd < 0) + /* UNIX socket use direct FD passing */ + charpriv->sourcefd = qemuFDPassNew(devalias, data->priv, false); + + if (qemuFDPassAddFD(charpriv->sourcefd, &sourcefd, "-source") < 0) return -1; } break; @@ -6892,14 +6909,21 @@ qemuProcessPrepareHostBackendChardevOne(virDomainDeviceDef *dev, } if (chardev->logfile) { + VIR_AUTOCLOSE logfd = -1; + if (qemuProcessPrepareHostBackendChardevFileHelper(chardev->logfile, chardev->logappend, - &charpriv->logfd, + &logfd, data->logManager, data->priv->driver->securityManager, data->cfg, data->def) < 0) return -1; + + charpriv->logfd = qemuFDPassNew(devalias, data->priv, true); + + if (qemuFDPassAddFD(charpriv->logfd, &logfd, "-log") < 0) + return -1; } return 0; @@ -6932,6 +6956,8 @@ qemuProcessPrepareHostBackendChardev(virDomainObj *vm) &data) < 0) return -1; + data.fdprefix = "monitor"; + if (qemuProcessPrepareHostBackendChardevOne(NULL, priv->monConfig, &data) < 0) return -1; diff --git a/tests/qemuxml2argvdata/aarch64-pci-serial.args b/tests/qemuxml2argvdata/aarch64-pci-serial.args index aca88e8e60..e012ea24dd 100644 --- a/tests/qemuxml2argvdata/aarch64-pci-serial.args +++ b/tests/qemuxml2argvdata/aarch64-pci-serial.args @@ -29,7 +29,7 @@ QEMU_AUDIO_DRV=none \ -device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1 \ -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \ -device pcie-root-port,port=16,chassis=3,id=pci.3,bus=pcie.0,addr=0x2 \ --add-fd set=0,fd=1751 \ --chardev pty,id=charserial0,logfile=/dev/fdset/0,logappend=on \ +-add-fd set=1,fd=1751,opaque=serial0-log \ +-chardev pty,id=charserial0,logfile=/dev/fdset/1,logappend=on \ -device pci-serial,chardev=charserial0,id=serial0,bus=pci.2,addr=0x1 \ -msg timestamp=on diff --git a/tests/qemuxml2argvdata/name-escape.x86_64-2.11.0.args b/tests/qemuxml2argvdata/name-escape.x86_64-2.11.0.args index 6d4c5bff6e..7f9677abb6 100644 --- a/tests/qemuxml2argvdata/name-escape.x86_64-2.11.0.args +++ b/tests/qemuxml2argvdata/name-escape.x86_64-2.11.0.args @@ -33,8 +33,8 @@ QEMU_AUDIO_DRV=spice \ -device ccid-card-emulated,backend=certificates,cert1=cert1,,foo,cert2=cert2,cert3=cert3,db=/etc/pki/nssdb,,foo,id=smartcard0,bus=ccid0.0 \ -chardev tty,id=charserial0,path=/dev/ttyS2,,foo \ -device isa-serial,chardev=charserial0,id=serial0,index=1 \ --add-fd set=0,fd=1750 \ --chardev file,id=charserial1,path=/dev/fdset/0,append=on \ +-add-fd set=1,fd=1750,opaque=serial1-source \ +-chardev file,id=charserial1,path=/dev/fdset/1,append=on \ -device isa-serial,chardev=charserial1,id=serial1,index=0 \ -chardev pipe,id=charchannel0,path=/tmp/guestfwd,,foo \ -netdev user,guestfwd=tcp:10.0.2.1:4600-chardev:charchannel0,id=channel0 \ diff --git a/tests/qemuxml2argvdata/name-escape.x86_64-latest.args b/tests/qemuxml2argvdata/name-escape.x86_64-latest.args index 0b3247b6bd..665a016a40 100644 --- a/tests/qemuxml2argvdata/name-escape.x86_64-latest.args +++ b/tests/qemuxml2argvdata/name-escape.x86_64-latest.args @@ -35,8 +35,8 @@ XDG_CONFIG_HOME=/tmp/lib/domain--1-foo=1,bar=2/.config \ -device '{"driver":"ccid-card-emulated","backend":"certificates","cert1":"cert1,foo","cert2":"cert2","cert3":"cert3","db":"/etc/pki/nssdb,foo","id":"smartcard0","bus":"ccid0.0"}' \ -chardev tty,id=charserial0,path=/dev/ttyS2,,foo \ -device '{"driver":"isa-serial","chardev":"charserial0","id":"serial0","index":1}' \ --add-fd set=0,fd=1750 \ --chardev file,id=charserial1,path=/dev/fdset/0,append=on \ +-add-fd set=1,fd=1750,opaque=serial1-source \ +-chardev file,id=charserial1,path=/dev/fdset/1,append=on \ -device '{"driver":"isa-serial","chardev":"charserial1","id":"serial1","index":0}' \ -chardev pipe,id=charchannel0,path=/tmp/guestfwd,,foo \ -netdev user,guestfwd=tcp:10.0.2.1:4600-chardev:charchannel0,id=channel0 \ diff --git a/tests/qemuxml2argvdata/serial-file-chardev.args b/tests/qemuxml2argvdata/serial-file-chardev.args index 7df5c2a115..e4dee0033c 100644 --- a/tests/qemuxml2argvdata/serial-file-chardev.args +++ b/tests/qemuxml2argvdata/serial-file-chardev.args @@ -29,8 +29,8 @@ QEMU_AUDIO_DRV=none \ -usb \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ -device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \ --add-fd set=0,fd=1750 \ --chardev file,id=charserial0,path=/dev/fdset/0,append=on \ +-add-fd set=1,fd=1750,opaque=serial0-source \ +-chardev file,id=charserial0,path=/dev/fdset/1,append=on \ -device isa-serial,chardev=charserial0,id=serial0,index=0 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 \ -msg timestamp=on diff --git a/tests/qemuxml2argvdata/serial-file-chardev.x86_64-latest.args b/tests/qemuxml2argvdata/serial-file-chardev.x86_64-latest.args index 6a28170503..568a1aaf76 100644 --- a/tests/qemuxml2argvdata/serial-file-chardev.x86_64-latest.args +++ b/tests/qemuxml2argvdata/serial-file-chardev.x86_64-latest.args @@ -31,8 +31,8 @@ XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \ -blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest1","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \ -blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"raw","file":"libvirt-1-storage"}' \ -device '{"driver":"ide-hd","bus":"ide.0","unit":0,"drive":"libvirt-1-format","id":"ide0-0-0","bootindex":1}' \ --add-fd set=0,fd=1750 \ --chardev file,id=charserial0,path=/dev/fdset/0,append=on \ +-add-fd set=1,fd=1750,opaque=serial0-source \ +-chardev file,id=charserial0,path=/dev/fdset/1,append=on \ -device '{"driver":"isa-serial","chardev":"charserial0","id":"serial0","index":0}' \ -audiodev '{"id":"audio1","driver":"none"}' \ -device '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.0","addr":"0x2"}' \ diff --git a/tests/qemuxml2argvdata/serial-file-log.args b/tests/qemuxml2argvdata/serial-file-log.args index 72d7c49298..1d237ca7c7 100644 --- a/tests/qemuxml2argvdata/serial-file-log.args +++ b/tests/qemuxml2argvdata/serial-file-log.args @@ -29,8 +29,8 @@ QEMU_AUDIO_DRV=none \ -usb \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ -device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \ --add-fd set=0,fd=1750 \ --add-fd set=1,fd=1751 \ --chardev file,id=charserial0,path=/dev/fdset/0,append=on,logfile=/dev/fdset/1,logappend=on \ +-add-fd set=1,fd=1750,opaque=serial0-source \ +-add-fd set=2,fd=1751,opaque=serial0-log \ +-chardev file,id=charserial0,path=/dev/fdset/1,append=on,logfile=/dev/fdset/2,logappend=on \ -device isa-serial,chardev=charserial0,id=serial0,index=0 \ -msg timestamp=on diff --git a/tests/qemuxml2argvdata/serial-file-log.x86_64-latest.args b/tests/qemuxml2argvdata/serial-file-log.x86_64-latest.args index c12b4b3a60..de47bad812 100644 --- a/tests/qemuxml2argvdata/serial-file-log.x86_64-latest.args +++ b/tests/qemuxml2argvdata/serial-file-log.x86_64-latest.args @@ -31,9 +31,9 @@ XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \ -blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest1","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \ -blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"raw","file":"libvirt-1-storage"}' \ -device '{"driver":"ide-hd","bus":"ide.0","unit":0,"drive":"libvirt-1-format","id":"ide0-0-0","bootindex":1}' \ --add-fd set=0,fd=1750 \ --add-fd set=1,fd=1751 \ --chardev file,id=charserial0,path=/dev/fdset/0,append=on,logfile=/dev/fdset/1,logappend=on \ +-add-fd set=1,fd=1750,opaque=serial0-source \ +-add-fd set=2,fd=1751,opaque=serial0-log \ +-chardev file,id=charserial0,path=/dev/fdset/1,append=on,logfile=/dev/fdset/2,logappend=on \ -device '{"driver":"isa-serial","chardev":"charserial0","id":"serial0","index":0}' \ -audiodev '{"id":"audio1","driver":"none"}' \ -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ diff --git a/tests/qemuxml2argvdata/serial-many-chardev.args b/tests/qemuxml2argvdata/serial-many-chardev.args index 5e80348d11..95a828e606 100644 --- a/tests/qemuxml2argvdata/serial-many-chardev.args +++ b/tests/qemuxml2argvdata/serial-many-chardev.args @@ -31,8 +31,8 @@ QEMU_AUDIO_DRV=none \ -device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \ -chardev pty,id=charserial0 \ -device isa-serial,chardev=charserial0,id=serial0,index=0 \ --add-fd set=0,fd=1750 \ --chardev file,id=charserial1,path=/dev/fdset/0,append=on \ +-add-fd set=1,fd=1750,opaque=serial1-source \ +-chardev file,id=charserial1,path=/dev/fdset/1,append=on \ -device isa-serial,chardev=charserial1,id=serial1,index=1 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 \ -msg timestamp=on diff --git a/tests/qemuxml2argvdata/serial-many-chardev.x86_64-latest.args b/tests/qemuxml2argvdata/serial-many-chardev.x86_64-latest.args index cbdcc82052..0e5ebaf9e9 100644 --- a/tests/qemuxml2argvdata/serial-many-chardev.x86_64-latest.args +++ b/tests/qemuxml2argvdata/serial-many-chardev.x86_64-latest.args @@ -33,8 +33,8 @@ XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \ -device '{"driver":"ide-hd","bus":"ide.0","unit":0,"drive":"libvirt-1-format","id":"ide0-0-0","bootindex":1}' \ -chardev pty,id=charserial0 \ -device '{"driver":"isa-serial","chardev":"charserial0","id":"serial0","index":0}' \ --add-fd set=0,fd=1750 \ --chardev file,id=charserial1,path=/dev/fdset/0,append=on \ +-add-fd set=1,fd=1750,opaque=serial1-source \ +-chardev file,id=charserial1,path=/dev/fdset/1,append=on \ -device '{"driver":"isa-serial","chardev":"charserial1","id":"serial1","index":1}' \ -audiodev '{"id":"audio1","driver":"none"}' \ -device '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.0","addr":"0x2"}' \ diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index b2d6606d6e..d4e518e3a1 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -383,10 +383,17 @@ testPrepareHostBackendChardevOne(virDomainDeviceDef *dev, virDomainChrSourceDef *chardev, void *opaque) { - virQEMUCaps *qemuCaps = opaque; + virDomainObj *vm = opaque; + qemuDomainObjPrivate *priv = vm->privateData; qemuDomainChrSourcePrivate *charpriv = QEMU_DOMAIN_CHR_SOURCE_PRIVATE(chardev); + int fakesourcefd = -1; + const char *devalias = NULL; + bool usefdset = true; if (dev) { + virDomainDeviceInfo *info = virDomainDeviceGetInfo(dev); + devalias = info->alias; + /* vhost-user disk doesn't use FD passing */ if (dev->type == VIR_DOMAIN_DEVICE_DISK) return 0; @@ -400,6 +407,8 @@ testPrepareHostBackendChardevOne(virDomainDeviceDef *dev, /* TPMs FD passing setup is special and handled separately */ if (dev->type == VIR_DOMAIN_DEVICE_TPM) return 0; + } else { + devalias = "monitor"; } switch ((virDomainChrType) chardev->type) { @@ -416,20 +425,15 @@ testPrepareHostBackendChardevOne(virDomainDeviceDef *dev, break; case VIR_DOMAIN_CHR_TYPE_FILE: - if (fcntl(1750, F_GETFD) != -1) - abort(); - charpriv->fd = 1750; + fakesourcefd = 1750; break; case VIR_DOMAIN_CHR_TYPE_UNIX: if (chardev->data.nix.listen && - virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS_COMMANDLINE)) { + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS_COMMANDLINE)) + fakesourcefd = 1729; - if (fcntl(1729, F_GETFD) != -1) - abort(); - - charpriv->fd = 1729; - } + usefdset = false; break; case VIR_DOMAIN_CHR_TYPE_NMDM: @@ -437,10 +441,26 @@ testPrepareHostBackendChardevOne(virDomainDeviceDef *dev, break; } + if (fakesourcefd != -1) { + if (fcntl(fakesourcefd, F_GETFD) != -1) + abort(); + + charpriv->sourcefd = qemuFDPassNew(devalias, priv, usefdset); + + if (qemuFDPassAddFD(charpriv->sourcefd, &fakesourcefd, "-source") < 0) + return -1; + } + if (chardev->logfile) { - if (fcntl(1751, F_GETFD) != -1) + int fd = 1751; + + if (fcntl(fd, F_GETFD) != -1) abort(); - charpriv->logfd = 1751; + + charpriv->logfd = qemuFDPassNew(devalias, priv, true); + + if (qemuFDPassAddFD(charpriv->logfd, &fd, "-log") < 0) + return -1; } return 0; @@ -464,17 +484,11 @@ testCompareXMLToArgvCreateArgs(virQEMUDriver *drv, if (qemuDomainDeviceBackendChardevForeach(vm->def, testPrepareHostBackendChardevOne, - info->qemuCaps) < 0) + vm) < 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; - } + if (testPrepareHostBackendChardevOne(NULL, priv->monConfig, vm) < 0) + return NULL; for (i = 0; i < vm->def->ndisks; i++) { virDomainDiskDef *disk = vm->def->disks[i]; -- 2.34.1

Our code uses fdsets for the pipe passed from virtlogd to qemu, but the chardev hot-unplug code neglected to detach the fdset after the chardev was removed. This kept the FDs open by qemu even after they were not used any more. After the refactor to use qemuFDPass for chardevs we now configure the 'opaque' field for fdsets used for chardevs so we can use qemuHotplugRemoveFDSet to remove the unused fdset. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_hotplug.c | 1 + tests/qemuhotplugtest.c | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 03b7ca30de..e05edffae2 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4881,6 +4881,7 @@ qemuDomainRemoveChrDevice(virQEMUDriver *driver, if (monitor) { qemuDomainObjEnterMonitor(driver, vm); rc = qemuMonitorDetachCharDev(priv->mon, charAlias); + qemuHotplugRemoveFDSet(priv->mon, chr->info.alias, NULL); qemuDomainObjExitMonitor(driver, vm); } diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 263a92425c..fe1b227b4e 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -708,7 +708,7 @@ mymain(void) DO_TEST_DETACH("console-compat-2-live", "console-virtio", false, false, "device_del", QMP_DEVICE_DELETED("console1") QMP_OK, - "chardev-remove", QMP_OK); + "chardev-remove", QMP_OK, "query-fdsets", "{\"return\": []}"); DO_TEST_ATTACH("base-live", "disk-virtio", false, true, "human-monitor-command", HMP("OK\\r\\n"), @@ -768,7 +768,7 @@ mymain(void) "device_add", QMP_OK); DO_TEST_DETACH("base-live", "qemu-agent-detach", false, false, "device_del", QMP_DEVICE_DELETED("channel0") QMP_OK, - "chardev-remove", QMP_OK); + "chardev-remove", QMP_OK, "query-fdsets", "{\"return\": []}"); DO_TEST_ATTACH("base-ccw-live", "ccw-virtio", false, true, "human-monitor-command", HMP("OK\\r\\n"), -- 2.34.1

Don't add the command to the test monitor when we don't expect to invoke it rather than bypassing the test monitor. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- tests/qemumonitorjsontest.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 7987182a82..b7937a6046 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -659,9 +659,8 @@ qemuMonitorJSONTestAttachOneChardev(virDomainXMLOption *xmlopt, fulllabel = g_strdup_printf("qemuMonitorJSONTestAttachChardev(%s)", label); - qemuMonitorTestAllowUnusedCommands(test); - - if (qemuMonitorTestAddItemExpect(test, "chardev-add", + if (expectargs && + qemuMonitorTestAddItemExpect(test, "chardev-add", expectargs, true, jsonreply) < 0) return -1; @@ -784,10 +783,10 @@ qemuMonitorJSONTestAttachChardev(virDomainXMLOption *xmlopt, "data':{'type':'vdagent'}}}"); chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_PIPE }; - CHECK("pipe", true, ""); + CHECK("pipe", true, NULL); chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_STDIO }; - CHECK("stdio", true, ""); + CHECK("stdio", true, NULL); #undef CHECK return ret; -- 2.34.1

The main objective of this patch is to use a proper instance of virDomainChrSourceDef allocated with the private data. To achieve this the test cases are grouped into blocks by how much they fill in the chardev definition. Some test cases are moved around so that the resulting sequence doesn't need extra clearing. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- tests/qemumonitorjsontest.c | 214 +++++++++++++++++++----------------- 1 file changed, 113 insertions(+), 101 deletions(-) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index b7937a6046..d42ccefece 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -679,114 +679,126 @@ static int qemuMonitorJSONTestAttachChardev(virDomainXMLOption *xmlopt, GHashTable *schema) { - virDomainChrSourceDef chr; int ret = 0; #define CHECK(label, fail, expectargs) \ - if (qemuMonitorJSONTestAttachOneChardev(xmlopt, schema, label, &chr, \ + if (qemuMonitorJSONTestAttachOneChardev(xmlopt, schema, label, chr, \ expectargs, NULL, NULL, fail) < 0) \ ret = -1 - chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_NULL }; - CHECK("null", false, - "{'id':'alias','backend':{'type':'null','data':{}}}"); - - chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_VC }; - CHECK("vc", false, - "{'id':'alias','backend':{'type':'vc','data':{}}}"); - - chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_PTY }; - if (qemuMonitorJSONTestAttachOneChardev(xmlopt, schema, "pty", &chr, - "{'id':'alias'," - "'backend':{'type':'pty'," - "'data':{}}}", - "\"pty\" : \"/dev/pts/0\"", - "/dev/pts/0", false) < 0) - ret = -1; + { + g_autoptr(virDomainChrSourceDef) chr = virDomainChrSourceDefNew(xmlopt); + + chr->type = VIR_DOMAIN_CHR_TYPE_NULL; + CHECK("null", false, + "{'id':'alias','backend':{'type':'null','data':{}}}"); + + chr->type = VIR_DOMAIN_CHR_TYPE_VC; + CHECK("vc", false, + "{'id':'alias','backend':{'type':'vc','data':{}}}"); + + chr->type = VIR_DOMAIN_CHR_TYPE_SPICEVMC; + CHECK("spicevmc", false, + "{'id':'alias','backend':{'type':'spicevmc'," + "'data':{'type':'vdagent'}}}"); + + chr->type = VIR_DOMAIN_CHR_TYPE_PIPE; + CHECK("pipe", true, NULL); + + chr->type = VIR_DOMAIN_CHR_TYPE_STDIO; + CHECK("stdio", true, NULL); + + chr->type = VIR_DOMAIN_CHR_TYPE_PTY; + CHECK("pty missing path", true, + "{'id':'alias','backend':{'type':'pty','data':{}}}"); + if (qemuMonitorJSONTestAttachOneChardev(xmlopt, schema, "pty", chr, + "{'id':'alias'," + "'backend':{'type':'pty'," + "'data':{}}}", + "\"pty\" : \"/dev/pts/0\"", + "/dev/pts/0", false) < 0) + ret = -1; + } + + { + g_autoptr(virDomainChrSourceDef) chr = virDomainChrSourceDefNew(xmlopt); + + chr->data.file.path = g_strdup("/test/path"); + + chr->type = VIR_DOMAIN_CHR_TYPE_FILE; + CHECK("file", false, + "{'id':'alias','backend':{'type':'file','data':{'out':'/test/path'}}}"); + + chr->type = VIR_DOMAIN_CHR_TYPE_DEV; + CHECK("device", false, + "{'id':'alias','backend':{'type':'serial','data':{'device':'/test/path'}}}"); + } + + { + g_autoptr(virDomainChrSourceDef) chr = virDomainChrSourceDefNew(xmlopt); + + chr->type = VIR_DOMAIN_CHR_TYPE_TCP; + chr->data.tcp.host = g_strdup("example.com"); + chr->data.tcp.service = g_strdup("1234"); + CHECK("tcp", false, + "{'id':'alias'," + "'backend':{'type':'socket'," + "'data':{'addr':{'type':'inet'," + "'data':{'host':'example.com'," + "'port':'1234'}}," + "'telnet':false," + "'server':false}}}"); + } + + { + g_autoptr(virDomainChrSourceDef) chr = virDomainChrSourceDefNew(xmlopt); + + chr->type = VIR_DOMAIN_CHR_TYPE_UDP; + chr->data.udp.connectHost = g_strdup("example.com"); + chr->data.udp.connectService = g_strdup("1234"); + CHECK("udp", false, + "{'id':'alias'," + "'backend':{'type':'udp'," + "'data':{'remote':{'type':'inet'," + "'data':{'host':'example.com'," + "'port':'1234'}}}}}"); + + chr->data.udp.bindService = g_strdup("4321"); + CHECK("udp", false, + "{'id':'alias'," + "'backend':{'type':'udp'," + "'data':{'remote':{'type':'inet'," + "'data':{'host':'example.com'," + "'port':'1234'}}," + "'local':{'type':'inet'," + "'data':{'host':''," + "'port':'4321'}}}}}"); + + chr->data.udp.bindHost = g_strdup("localhost"); + CHECK("udp", false, + "{'id':'alias'," + "'backend':{'type':'udp'," + "'data':{'remote':{'type':'inet'," + "'data':{'host':'example.com'," + "'port':'1234'}}," + "'local':{'type':'inet'," + "'data':{'host':'localhost'," + "'port':'4321'}}}}}"); + } + + { + g_autoptr(virDomainChrSourceDef) chr = virDomainChrSourceDefNew(xmlopt); + + chr->type = VIR_DOMAIN_CHR_TYPE_UNIX; + chr->data.nix.path = g_strdup("/path/to/socket"); + CHECK("unix", false, + "{'id':'alias'," + "'backend':{'type':'socket'," + "'data':{'addr':{'type':'unix'," + "'data':{'path':'/path/to/socket'}}," + "'server':false}}}"); + } - chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_PTY }; - CHECK("pty missing path", true, - "{'id':'alias','backend':{'type':'pty','data':{}}}"); - - memset(&chr, 0, sizeof(chr)); - chr.type = VIR_DOMAIN_CHR_TYPE_FILE; - chr.data.file.path = (char *) "/test/path"; - CHECK("file", false, - "{'id':'alias','backend':{'type':'file','data':{'out':'/test/path'}}}"); - - memset(&chr, 0, sizeof(chr)); - chr.type = VIR_DOMAIN_CHR_TYPE_DEV; - chr.data.file.path = (char *) "/test/path"; - CHECK("device", false, - "{'id':'alias','backend':{'type':'serial','data':{'device':'/test/path'}}}"); - - memset(&chr, 0, sizeof(chr)); - chr.type = VIR_DOMAIN_CHR_TYPE_TCP; - chr.data.tcp.host = (char *) "example.com"; - chr.data.tcp.service = (char *) "1234"; - CHECK("tcp", false, - "{'id':'alias'," - "'backend':{'type':'socket'," - "'data':{'addr':{'type':'inet'," - "'data':{'host':'example.com'," - "'port':'1234'}}," - "'telnet':false," - "'server':false}}}"); - - memset(&chr, 0, sizeof(chr)); - chr.type = VIR_DOMAIN_CHR_TYPE_UDP; - chr.data.udp.connectHost = (char *) "example.com"; - chr.data.udp.connectService = (char *) "1234"; - CHECK("udp", false, - "{'id':'alias'," - "'backend':{'type':'udp'," - "'data':{'remote':{'type':'inet'," - "'data':{'host':'example.com'," - "'port':'1234'}}}}}"); - - chr.data.udp.bindHost = (char *) "localhost"; - chr.data.udp.bindService = (char *) "4321"; - CHECK("udp", false, - "{'id':'alias'," - "'backend':{'type':'udp'," - "'data':{'remote':{'type':'inet'," - "'data':{'host':'example.com'," - "'port':'1234'}}," - "'local':{'type':'inet'," - "'data':{'host':'localhost'," - "'port':'4321'}}}}}"); - - chr.data.udp.bindHost = NULL; - chr.data.udp.bindService = (char *) "4321"; - CHECK("udp", false, - "{'id':'alias'," - "'backend':{'type':'udp'," - "'data':{'remote':{'type':'inet'," - "'data':{'host':'example.com'," - "'port':'1234'}}," - "'local':{'type':'inet'," - "'data':{'host':''," - "'port':'4321'}}}}}"); - memset(&chr, 0, sizeof(chr)); - chr.type = VIR_DOMAIN_CHR_TYPE_UNIX; - chr.data.nix.path = (char *) "/path/to/socket"; - CHECK("unix", false, - "{'id':'alias'," - "'backend':{'type':'socket'," - "'data':{'addr':{'type':'unix'," - "'data':{'path':'/path/to/socket'}}," - "'server':false}}}"); - - chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_SPICEVMC }; - CHECK("spicevmc", false, - "{'id':'alias','backend':{'type':'spicevmc','" - "data':{'type':'vdagent'}}}"); - - chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_PIPE }; - CHECK("pipe", true, NULL); - - chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_STDIO }; - CHECK("stdio", true, NULL); #undef CHECK return ret; -- 2.34.1

Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- tests/qemumonitorjsontest.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index d42ccefece..434677f25f 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -27,6 +27,7 @@ #include "qemu/qemu_block.h" #include "qemu/qemu_monitor_json.h" #include "qemu/qemu_qapi.h" +#include "qemu/qemu_alias.h" #include "virthread.h" #include "virerror.h" #include "virstring.h" @@ -736,6 +737,7 @@ qemuMonitorJSONTestAttachChardev(virDomainXMLOption *xmlopt, { g_autoptr(virDomainChrSourceDef) chr = virDomainChrSourceDefNew(xmlopt); + qemuDomainChrSourcePrivate *chrSourcePriv = QEMU_DOMAIN_CHR_SOURCE_PRIVATE(chr); chr->type = VIR_DOMAIN_CHR_TYPE_TCP; chr->data.tcp.host = g_strdup("example.com"); @@ -748,6 +750,19 @@ qemuMonitorJSONTestAttachChardev(virDomainXMLOption *xmlopt, "'port':'1234'}}," "'telnet':false," "'server':false}}}"); + + chr->data.tcp.tlscreds = true; + chrSourcePriv->tlsCredsAlias = qemuAliasTLSObjFromSrcAlias("alias"); + CHECK("tcp", false, + "{'id':'alias'," + "'backend':{'type':'socket'," + "'data':{'addr':{'type':'inet'," + "'data':{'host':'example.com'," + "'port':'1234'}}," + "'telnet':false," + "'server':false," + "'tls-creds':'objalias_tls0'}}}"); + } { -- 2.34.1

Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- tests/qemumonitorjsontest.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 434677f25f..8fbb199a59 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -726,13 +726,17 @@ qemuMonitorJSONTestAttachChardev(virDomainXMLOption *xmlopt, chr->data.file.path = g_strdup("/test/path"); - chr->type = VIR_DOMAIN_CHR_TYPE_FILE; - CHECK("file", false, - "{'id':'alias','backend':{'type':'file','data':{'out':'/test/path'}}}"); - chr->type = VIR_DOMAIN_CHR_TYPE_DEV; CHECK("device", false, "{'id':'alias','backend':{'type':'serial','data':{'device':'/test/path'}}}"); + + chr->type = VIR_DOMAIN_CHR_TYPE_FILE; + chr->logfile = g_strdup("/test/logfile"); + chr->logappend = VIR_TRISTATE_SWITCH_OFF; + CHECK("file", false, + "{'id':'alias','backend':{'type':'file','data':{'out':'/test/path'," + "'logfile':'/test/logfile'," + "'logappend':false}}}"); } { @@ -753,6 +757,7 @@ qemuMonitorJSONTestAttachChardev(virDomainXMLOption *xmlopt, chr->data.tcp.tlscreds = true; chrSourcePriv->tlsCredsAlias = qemuAliasTLSObjFromSrcAlias("alias"); + chr->logfile = g_strdup("/test/log"); CHECK("tcp", false, "{'id':'alias'," "'backend':{'type':'socket'," @@ -761,7 +766,8 @@ qemuMonitorJSONTestAttachChardev(virDomainXMLOption *xmlopt, "'port':'1234'}}," "'telnet':false," "'server':false," - "'tls-creds':'objalias_tls0'}}}"); + "'tls-creds':'objalias_tls0'," + "'logfile':'/test/log'}}}"); } -- 2.34.1

FD passing and TLS is normally setup via private data for the chardev source. The monitor implementation didn't support it. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_monitor_json.c | 71 +++++++++++++++++++++++++++--------- 1 file changed, 54 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e5425daf05..aac0ace7ff 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -6527,6 +6527,23 @@ qemuMonitorJSONBuildUnixSocketAddress(const char *path) return g_steal_pointer(&addr); } + +static virJSONValue * +qemuMonitorJSONBuildiFDSocketAddress(const char *fdname) +{ + g_autoptr(virJSONValue) addr = NULL; + g_autoptr(virJSONValue) data = NULL; + + if (virJSONValueObjectAdd(&data, "s:str", fdname, NULL) < 0) + return NULL; + + if (virJSONValueObjectAdd(&addr, + "s:type", "fd", + "a:data", &data, NULL) < 0) + return NULL; + + return g_steal_pointer(&addr); +} int qemuMonitorJSONNBDServerStart(qemuMonitor *mon, const virStorageNetHostDef *server, @@ -6683,6 +6700,7 @@ static virJSONValue * qemuMonitorJSONAttachCharDevGetProps(const char *chrID, const virDomainChrSourceDef *chr) { + qemuDomainChrSourcePrivate *chrSourcePriv = QEMU_DOMAIN_CHR_SOURCE_PRIVATE(chr); g_autoptr(virJSONValue) props = NULL; g_autoptr(virJSONValue) backend = NULL; g_autoptr(virJSONValue) backendData = virJSONValueNewObject(); @@ -6695,14 +6713,22 @@ qemuMonitorJSONAttachCharDevGetProps(const char *chrID, backendType = virDomainChrTypeToString(chr->type); break; - case VIR_DOMAIN_CHR_TYPE_FILE: + case VIR_DOMAIN_CHR_TYPE_FILE: { + const char *path = chr->data.file.path; + virTristateSwitch append = chr->data.file.append; backendType = "file"; + + if (chrSourcePriv->sourcefd) { + path = qemuFDPassGetPath(chrSourcePriv->sourcefd); + append = VIR_TRISTATE_SWITCH_ON; + } + if (virJSONValueObjectAdd(&backendData, - "s:out", chr->data.file.path, - "T:append", chr->data.file.append, + "s:out", path, + "T:append", append, NULL) < 0) return NULL; - + } break; case VIR_DOMAIN_CHR_TYPE_DEV: @@ -6720,7 +6746,7 @@ qemuMonitorJSONAttachCharDevGetProps(const char *chrID, case VIR_DOMAIN_CHR_TYPE_UNIX: case VIR_DOMAIN_CHR_TYPE_TCP: { - g_autofree char *tlsalias = NULL; + const char *tlsalias = NULL; g_autoptr(virJSONValue) addr = NULL; virTristateBool waitval = VIR_TRISTATE_BOOL_ABSENT; virTristateBool telnet = VIR_TRISTATE_BOOL_ABSENT; @@ -6737,9 +6763,7 @@ qemuMonitorJSONAttachCharDevGetProps(const char *chrID, waitval = VIR_TRISTATE_BOOL_NO; } - if (chr->data.tcp.tlscreds && - !(tlsalias = qemuAliasTLSObjFromSrcAlias(chrID))) - return NULL; + tlsalias = chrSourcePriv->tlsCredsAlias; if (!(addr = qemuMonitorJSONBuildInetSocketAddress(chr->data.tcp.host, chr->data.tcp.service))) @@ -6755,13 +6779,18 @@ qemuMonitorJSONAttachCharDevGetProps(const char *chrID, waitval = VIR_TRISTATE_BOOL_NO; } - if (!(addr = qemuMonitorJSONBuildUnixSocketAddress(chr->data.nix.path))) - return NULL; - - if (chr->data.nix.reconnect.enabled == VIR_TRISTATE_BOOL_YES) - reconnect = chr->data.tcp.reconnect.timeout; - else if (chr->data.nix.reconnect.enabled == VIR_TRISTATE_BOOL_NO) - reconnect = 0; + if (chrSourcePriv->sourcefd) { + if (!(addr = qemuMonitorJSONBuildiFDSocketAddress(qemuFDPassGetPath(chrSourcePriv->sourcefd)))) + return NULL; + } else { + if (!(addr = qemuMonitorJSONBuildUnixSocketAddress(chr->data.nix.path))) + return NULL; + + if (chr->data.nix.reconnect.enabled == VIR_TRISTATE_BOOL_YES) + reconnect = chr->data.tcp.reconnect.timeout; + else if (chr->data.nix.reconnect.enabled == VIR_TRISTATE_BOOL_NO) + reconnect = 0; + } } if (virJSONValueObjectAdd(&backendData, @@ -6826,9 +6855,17 @@ qemuMonitorJSONAttachCharDevGetProps(const char *chrID, } if (chr->logfile) { + const char *path = chr->logfile; + virTristateSwitch append = chr->logappend; + + if (chrSourcePriv->logfd) { + path = qemuFDPassGetPath(chrSourcePriv->logfd); + append = VIR_TRISTATE_SWITCH_ON; + } + if (virJSONValueObjectAdd(&backendData, - "s:logfile", chr->logfile, - "T:logappend", chr->logappend, + "s:logfile", path, + "T:logappend", append, NULL) < 0) return NULL; } -- 2.34.1

On Thu, Feb 10, 2022 at 18:46:30 +0100, Ján Tomko wrote: For 'insert' mode in vim, which I managed to fat-finger into the function name somehow :D, obviously cut&paste engineering was used from that point on :)