[PATCH 00/27] qemu: Fix chardev hot(un)plug issues and sanitize use of /dev/fdset/N
This series addresses the following issues: - hot-unplug of chardevs leaks the FDset in qemu - hot-plug of chardev doesn't use virtlogd even when configured - the name of the FD used for vdpa doesn't really tie it to the device There's also improvement in the common code handling the FD passing and few other drive-by fixes. Peter Krempa (27): scripts/moc-noinline: Use full name of the required annotation in error message qemuProcessPrepareHostBackendChardevFileHelper: Always use FD passing qemuProcessPrepareHostBackendChardev: Drop unneeded arguments qemuMonitorJSONQueryFdsetsParse: Don't check value passed to g_strdup qemuMonitorRemoveFdset: Convert @fdset to unsigned int to avoid error qemu: monitor: Make 'id' in 'struct _qemuMonitorFdsetInfo' unsigned qemu: domain: Move and unexport 'qemuDomainStorageIdReset' qemu: domain: Add helper for generating 'fdset' ids for VM startup qemu: Introduce helper functions for passing FDs to qemu qemuBuildInterfaceCommandLine: Use qemuFDPass for the vdpa fd qemu: hotplug: Extract code for unplugging fdsets qemuHotplugRemoveFDSet: Prepare for proper FD unplug handling qemuBuildInterfaceCommandLine: Use new pattern for naming the VDPA fdset qemu: Rewrite chardev startup code to use qemuFDPass qemuDomainRemoveChrDevice: Detach fdset after chardev hot-unplug qemumonitorjsontest: chardev: Remove need to allow unused commands qemumonitorjsontest: Refactor chardev hotplug testing qemuMonitorJSONTestAttachChardev: Add test for TLS-secured TCP chardev qemuMonitorJSONTestAttachChardev: Add logfile to some tests qemuMonitorJSONAttachCharDevGetProps: Properly handle private data tests: Move testPrepareHostBackendChardevOne into test utils qemuMonitorJSONTestAttachChardev: Add tests for FD passing of file backend qemu: process: Add a hotplug version of qemuProcessPrepareHostBackendChardev qemu: Honour 'virtlogd' use when hotplugging chardevs virTPMCreateCancelPath: Refactor value returning qemuBuildTPMOpenBackendFDs: Construct 'cancel_path' internally qemuBuildTPMCommandLine: Use 'qemuPassFD' infrastructure po/POTFILES.in | 1 + scripts/mock-noinline.py | 2 +- src/qemu/meson.build | 1 + src/qemu/qemu_command.c | 260 +++++---------- src/qemu/qemu_command.h | 4 +- src/qemu/qemu_domain.c | 94 +++--- src/qemu/qemu_domain.h | 13 +- src/qemu/qemu_driver.c | 3 +- src/qemu/qemu_fd.c | 296 ++++++++++++++++++ src/qemu/qemu_fd.h | 56 ++++ src/qemu/qemu_hotplug.c | 100 ++++-- src/qemu/qemu_hotplug.h | 2 +- src/qemu/qemu_monitor.c | 14 +- src/qemu/qemu_monitor.h | 4 +- src/qemu/qemu_monitor_json.c | 82 +++-- src/qemu/qemu_monitor_json.h | 2 +- src/qemu/qemu_process.c | 102 ++++-- src/qemu/qemu_process.h | 6 + src/util/virtpm.c | 13 +- tests/qemuhotplugmock.c | 11 + tests/qemuhotplugtest.c | 6 +- tests/qemumonitorjsontest.c | 253 +++++++++------ .../qemuxml2argvdata/aarch64-pci-serial.args | 3 +- .../name-escape.x86_64-2.11.0.args | 3 +- .../name-escape.x86_64-latest.args | 4 +- .../net-vdpa.x86_64-latest.args | 4 +- .../qemuxml2argvdata/serial-file-chardev.args | 3 +- .../serial-file-chardev.x86_64-latest.args | 4 +- tests/qemuxml2argvdata/serial-file-log.args | 4 +- .../serial-file-log.x86_64-latest.args | 6 +- .../qemuxml2argvdata/serial-many-chardev.args | 3 +- .../serial-many-chardev.x86_64-latest.args | 4 +- .../tpm-passthrough-crb.x86_64-latest.args | 6 +- .../tpm-passthrough.x86_64-latest.args | 6 +- tests/qemuxml2argvmock.c | 1 - tests/qemuxml2argvtest.c | 87 +---- tests/testutilsqemu.c | 106 +++++++ tests/testutilsqemu.h | 3 + 38 files changed, 1011 insertions(+), 561 deletions(-) create mode 100644 src/qemu/qemu_fd.c create mode 100644 src/qemu/qemu_fd.h -- 2.34.1
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- scripts/mock-noinline.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/mock-noinline.py b/scripts/mock-noinline.py index 69cf0b4b99..712550cb7a 100644 --- a/scripts/mock-noinline.py +++ b/scripts/mock-noinline.py @@ -73,7 +73,7 @@ warned = False for func in mocked.keys(): if func not in noninlined: warned = True - print("%s is mocked at %s but missing noinline annotation" % + print("%s is mocked at %s but missing 'G_GNUC_NO_INLINE' annotation" % (func, mocked[func]), file=sys.stderr) if warned: -- 2.34.1
s/moc/mock/ On a Wednesday in 2022, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- scripts/mock-noinline.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Code paths which don't wish to use FD passing are supposed to not call the function which sets up the chardev for FD passing. This is ensured by calling it only in the host prepare step. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 11 ----------- tests/qemuxml2argvdata/aarch64-pci-serial.args | 3 ++- .../name-escape.x86_64-2.11.0.args | 3 ++- tests/qemuxml2argvdata/serial-file-chardev.args | 3 ++- tests/qemuxml2argvdata/serial-file-log.args | 4 +++- tests/qemuxml2argvdata/serial-many-chardev.args | 3 ++- tests/qemuxml2argvtest.c | 16 ++++++---------- 7 files changed, 17 insertions(+), 26 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 24873f6fb7..169d9d1755 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6758,18 +6758,9 @@ qemuProcessPrepareHostBackendChardevFileHelper(const char *path, 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; @@ -6867,7 +6858,6 @@ qemuProcessPrepareHostBackendChardevOne(virDomainDeviceDef *dev, &charpriv->fd, data->logManager, data->secManager, - data->qemuCaps, data->cfg, data->def) < 0) return -1; @@ -6908,7 +6898,6 @@ qemuProcessPrepareHostBackendChardevOne(virDomainDeviceDef *dev, &charpriv->logfd, data->logManager, data->secManager, - data->qemuCaps, data->cfg, data->def) < 0) return -1; diff --git a/tests/qemuxml2argvdata/aarch64-pci-serial.args b/tests/qemuxml2argvdata/aarch64-pci-serial.args index b4f84cf778..aca88e8e60 100644 --- a/tests/qemuxml2argvdata/aarch64-pci-serial.args +++ b/tests/qemuxml2argvdata/aarch64-pci-serial.args @@ -29,6 +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 \ --chardev pty,id=charserial0,logfile=/tmp/log,logappend=on \ +-add-fd set=0,fd=1751 \ +-chardev pty,id=charserial0,logfile=/dev/fdset/0,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 a246c08b12..6d4c5bff6e 100644 --- a/tests/qemuxml2argvdata/name-escape.x86_64-2.11.0.args +++ b/tests/qemuxml2argvdata/name-escape.x86_64-2.11.0.args @@ -33,7 +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 \ --chardev file,id=charserial1,path=/tmp/serial.log,,foo,append=on \ +-add-fd set=0,fd=1750 \ +-chardev file,id=charserial1,path=/dev/fdset/0,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/serial-file-chardev.args b/tests/qemuxml2argvdata/serial-file-chardev.args index 4ec07d9b1f..7df5c2a115 100644 --- a/tests/qemuxml2argvdata/serial-file-chardev.args +++ b/tests/qemuxml2argvdata/serial-file-chardev.args @@ -29,7 +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 \ --chardev file,id=charserial0,path=/tmp/serial.log,append=on \ +-add-fd set=0,fd=1750 \ +-chardev file,id=charserial0,path=/dev/fdset/0,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-log.args b/tests/qemuxml2argvdata/serial-file-log.args index 1ffa30d207..72d7c49298 100644 --- a/tests/qemuxml2argvdata/serial-file-log.args +++ b/tests/qemuxml2argvdata/serial-file-log.args @@ -29,6 +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 \ --chardev file,id=charserial0,path=/tmp/serial.log,logfile=/var/lib/libvirt/qemu/demo-serial.log,logappend=off \ +-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 \ -device isa-serial,chardev=charserial0,id=serial0,index=0 \ -msg timestamp=on diff --git a/tests/qemuxml2argvdata/serial-many-chardev.args b/tests/qemuxml2argvdata/serial-many-chardev.args index e0005656ea..5e80348d11 100644 --- a/tests/qemuxml2argvdata/serial-many-chardev.args +++ b/tests/qemuxml2argvdata/serial-many-chardev.args @@ -31,7 +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 \ --chardev file,id=charserial1,path=/tmp/serial.log \ +-add-fd set=0,fd=1750 \ +-chardev file,id=charserial1,path=/dev/fdset/0,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/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 6cf35a0ebf..b2d6606d6e 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -416,11 +416,9 @@ testPrepareHostBackendChardevOne(virDomainDeviceDef *dev, 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; - } + if (fcntl(1750, F_GETFD) != -1) + abort(); + charpriv->fd = 1750; break; case VIR_DOMAIN_CHR_TYPE_UNIX: @@ -440,11 +438,9 @@ testPrepareHostBackendChardevOne(virDomainDeviceDef *dev, } if (chardev->logfile) { - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS_COMMANDLINE)) { - if (fcntl(1751, F_GETFD) != -1) - abort(); - charpriv->logfd = 1751; - } + if (fcntl(1751, F_GETFD) != -1) + abort(); + charpriv->logfd = 1751; } return 0; -- 2.34.1
On a Wednesday in 2022, Peter Krempa wrote:
Code paths which don't wish to use FD passing are supposed to not call the function which sets up the chardev for FD passing.
This is ensured by calling it only in the host prepare step.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 11 ----------- tests/qemuxml2argvdata/aarch64-pci-serial.args | 3 ++- .../name-escape.x86_64-2.11.0.args | 3 ++- tests/qemuxml2argvdata/serial-file-chardev.args | 3 ++- tests/qemuxml2argvdata/serial-file-log.args | 4 +++- tests/qemuxml2argvdata/serial-many-chardev.args | 3 ++- tests/qemuxml2argvtest.c | 16 ++++++---------- 7 files changed, 17 insertions(+), 26 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Caller passes 'driver->securityManager', and 'priv->qemuCaps' as arguments along with 'vm', but both aforementioned objects are accessible directly from 'vm'. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 169d9d1755..0e17e1bf94 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6805,9 +6805,8 @@ qemuProcessPrepareHostBackendChardevFileHelper(const char *path, struct qemuProcessPrepareHostBackendChardevData { - virQEMUCaps *qemuCaps; + qemuDomainObjPrivate *priv; virLogManager *logManager; - virSecurityManager *secManager; virQEMUDriverConfig *cfg; virDomainDef *def; }; @@ -6857,7 +6856,7 @@ qemuProcessPrepareHostBackendChardevOne(virDomainDeviceDef *dev, chardev->data.file.append, &charpriv->fd, data->logManager, - data->secManager, + data->priv->driver->securityManager, data->cfg, data->def) < 0) return -1; @@ -6866,14 +6865,14 @@ qemuProcessPrepareHostBackendChardevOne(virDomainDeviceDef *dev, case VIR_DOMAIN_CHR_TYPE_UNIX: if (chardev->data.nix.listen && - virQEMUCapsGet(data->qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS_COMMANDLINE)) { + virQEMUCapsGet(data->priv->qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS_COMMANDLINE)) { - if (qemuSecuritySetSocketLabel(data->secManager, data->def) < 0) + if (qemuSecuritySetSocketLabel(data->priv->driver->securityManager, data->def) < 0) return -1; charpriv->fd = qemuOpenChrChardevUNIXSocket(chardev); - if (qemuSecurityClearSocketLabel(data->secManager, data->def) < 0) { + if (qemuSecurityClearSocketLabel(data->priv->driver->securityManager, data->def) < 0) { VIR_FORCE_CLOSE(charpriv->fd); return -1; } @@ -6897,7 +6896,7 @@ qemuProcessPrepareHostBackendChardevOne(virDomainDeviceDef *dev, chardev->logappend, &charpriv->logfd, data->logManager, - data->secManager, + data->priv->driver->securityManager, data->cfg, data->def) < 0) return -1; @@ -6911,16 +6910,13 @@ qemuProcessPrepareHostBackendChardevOne(virDomainDeviceDef *dev, * 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) +qemuProcessPrepareHostBackendChardev(virDomainObj *vm) { qemuDomainObjPrivate *priv = vm->privateData; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver); struct qemuProcessPrepareHostBackendChardevData data = { - .qemuCaps = qemuCaps, + .priv = priv, .logManager = NULL, - .secManager = secManager, .cfg = cfg, .def = vm->def, }; @@ -6998,9 +6994,7 @@ qemuProcessPrepareHost(virQEMUDriver *driver, return -1; VIR_DEBUG("Preparing chr device backends"); - if (qemuProcessPrepareHostBackendChardev(vm, - priv->qemuCaps, - driver->securityManager) < 0) + if (qemuProcessPrepareHostBackendChardev(vm) < 0) return -1; if (qemuProcessBuildDestroyMemoryPaths(driver, vm, NULL, true) < 0) -- 2.34.1
On a Wednesday in 2022, Peter Krempa wrote:
Caller passes 'driver->securityManager', and 'priv->qemuCaps' as arguments along with 'vm', but both aforementioned objects are accessible directly from 'vm'.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
'g_strdup()' is NULL-tolerant. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 8e1501d91f..69ecc8a2d1 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3695,7 +3695,6 @@ qemuMonitorJSONQueryFdsetsParse(virJSONValue *msg, for (i = 0; i < ninfo; i++) { size_t j; - const char *tmp; virJSONValue *fdarray; qemuMonitorFdsetInfo *fdsetinfo = &sets->fdsets[i]; @@ -3734,9 +3733,7 @@ qemuMonitorJSONQueryFdsetsParse(virJSONValue *msg, } /* opaque is optional and may be missing */ - tmp = virJSONValueObjectGetString(fdentry, "opaque"); - if (tmp) - fdinfo->opaque = g_strdup(tmp); + fdinfo->opaque = g_strdup(virJSONValueObjectGetString(fdentry, "opaque")); } } -- 2.34.1
On a Wednesday in 2022, Peter Krempa wrote:
'g_strdup()' is NULL-tolerant.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
'qemuMonitorRemoveFdset' validatest that the 'fdset' argument isn't less than 0. We can turn it to unsigned and thus avoid the error message completely. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor.c | 14 ++++---------- src/qemu/qemu_monitor.h | 2 +- src/qemu/qemu_monitor_json.c | 4 ++-- src/qemu/qemu_monitor_json.h | 2 +- 4 files changed, 8 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index fcd39b80c6..7915729cbb 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2556,24 +2556,18 @@ qemuMonitorAddFileHandleToSet(qemuMonitor *mon, /** * qemuMonitorRemoveFdset: * @mon: monitor object - * @fdset: the fdset to remove + * @fdset: id of the fdset to remove * - * Attempts to remove a fdset from qemu and close associated file descriptors + * Attempts to remove @fdset from qemu and close associated file descriptors * Returns 0 if ok, and -1 on failure */ int qemuMonitorRemoveFdset(qemuMonitor *mon, - int fdset) + unsigned int fdset) { - VIR_DEBUG("fdset=%d", fdset); + VIR_DEBUG("fdset=%u", fdset); QEMU_CHECK_MONITOR(mon); - if (fdset < 0) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("fdset must be valid")); - return -1; - } - return qemuMonitorJSONRemoveFdset(mon, fdset); } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index efc4721ea9..27e288c724 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -974,7 +974,7 @@ qemuMonitorAddFileHandleToSet(qemuMonitor *mon, int qemuMonitorRemoveFdset(qemuMonitor *mon, - int fdset); + unsigned int fdset); typedef struct _qemuMonitorFdsetFdInfo qemuMonitorFdsetFdInfo; struct _qemuMonitorFdsetFdInfo { diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 69ecc8a2d1..acecdc3943 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3766,11 +3766,11 @@ int qemuMonitorJSONQueryFdsets(qemuMonitor *mon, int qemuMonitorJSONRemoveFdset(qemuMonitor *mon, - int fdset) + unsigned int fdset) { g_autoptr(virJSONValue) reply = NULL; g_autoptr(virJSONValue) cmd = qemuMonitorJSONMakeCommand("remove-fd", - "i:fdset-id", fdset, + "u:fdset-id", fdset, NULL); if (!cmd) diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 309d1fb409..eea3478af0 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -257,7 +257,7 @@ qemuMonitorJSONAddFileHandleToSet(qemuMonitor *mon, int qemuMonitorJSONRemoveFdset(qemuMonitor *mon, - int fdset); + unsigned int fdset); int qemuMonitorJSONQueryFdsets(qemuMonitor *mon, -- 2.34.1
On a Wednesday in 2022, Peter Krempa wrote:
'qemuMonitorRemoveFdset' validatest that the 'fdset' argument isn't less
validates
than 0. We can turn it to unsigned and thus avoid the error message completely.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor.c | 14 ++++---------- src/qemu/qemu_monitor.h | 2 +- src/qemu/qemu_monitor_json.c | 4 ++-- src/qemu/qemu_monitor_json.h | 2 +- 4 files changed, 8 insertions(+), 14 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Similarly to the 'qemuMonitorRemoveFdset', it doesn't make sense to store it as signed when only unsigned values are expected. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor.h | 2 +- src/qemu/qemu_monitor_json.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 27e288c724..4c22394972 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -983,7 +983,7 @@ struct _qemuMonitorFdsetFdInfo { }; typedef struct _qemuMonitorFdsetInfo qemuMonitorFdsetInfo; struct _qemuMonitorFdsetInfo { - int id; + unsigned int id; qemuMonitorFdsetFdInfo *fds; int nfds; }; diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index acecdc3943..e5425daf05 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3704,7 +3704,7 @@ qemuMonitorJSONQueryFdsetsParse(virJSONValue *msg, return -1; } - if (virJSONValueObjectGetNumberInt(entry, "fdset-id", &fdsetinfo->id) < 0) { + if (virJSONValueObjectGetNumberUint(entry, "fdset-id", &fdsetinfo->id) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("query-fdsets reply was missing 'fdset-id'")); return -1; -- 2.34.1
On a Wednesday in 2022, Peter Krempa wrote:
Similarly to the 'qemuMonitorRemoveFdset', it doesn't make sense to store it as signed when only unsigned values are expected.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor.h | 2 +- src/qemu/qemu_monitor_json.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
It's used only inside qemu_domain.c. Move it before it's usage, along with the qemuDomainStorageIdNew counterpart and unexport it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 54 +++++++++++++++++++++--------------------- src/qemu/qemu_domain.h | 1 - 2 files changed, 27 insertions(+), 28 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c764f6296c..64be39d49f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -656,6 +656,33 @@ qemuDomainMasterKeyCreate(virDomainObj *vm) } +/** + * qemuDomainStorageIdNew: + * @priv: qemu VM private data object. + * + * Generate a new unique id for a storage object. Useful for node name generation. + */ +unsigned int +qemuDomainStorageIdNew(qemuDomainObjPrivate *priv) +{ + return ++priv->nodenameindex; +} + + +/** + * qemuDomainStorageIdReset: + * @priv: qemu VM private data object. + * + * Resets the data for the node name generator. The node names need to be unique + * for a single instance, so can be reset on VM shutdown. + */ +static void +qemuDomainStorageIdReset(qemuDomainObjPrivate *priv) +{ + priv->nodenameindex = 0; +} + + static void qemuDomainSecretInfoClear(qemuDomainSecretInfo *secinfo, bool keepAlias) @@ -10889,33 +10916,6 @@ qemuDomainGetManagedPRSocketPath(qemuDomainObjPrivate *priv) } -/** - * qemuDomainStorageIdNew: - * @priv: qemu VM private data object. - * - * Generate a new unique id for a storage object. Useful for node name generation. - */ -unsigned int -qemuDomainStorageIdNew(qemuDomainObjPrivate *priv) -{ - return ++priv->nodenameindex; -} - - -/** - * qemuDomainStorageIdReset: - * @priv: qemu VM private data object. - * - * Resets the data for the node name generator. The node names need to be unique - * for a single instance, so can be reset on VM shutdown. - */ -void -qemuDomainStorageIdReset(qemuDomainObjPrivate *priv) -{ - priv->nodenameindex = 0; -} - - virDomainEventResumedDetailType qemuDomainRunningReasonToResumeEvent(virDomainRunningReason reason) { diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 8bf1c91049..7ec337e9ae 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -970,7 +970,6 @@ char * qemuDomainGetManagedPRSocketPath(qemuDomainObjPrivate *priv); bool qemuDomainDefHasManagedPR(virDomainObj *vm); unsigned int qemuDomainStorageIdNew(qemuDomainObjPrivate *priv); -void qemuDomainStorageIdReset(qemuDomainObjPrivate *priv); virDomainEventResumedDetailType qemuDomainRunningReasonToResumeEvent(virDomainRunningReason reason); -- 2.34.1
On a Wednesday in 2022, Peter Krempa wrote:
It's used only inside qemu_domain.c. Move it before it's usage, along with the qemuDomainStorageIdNew counterpart and unexport it.
s/it's/its/ (It's is correct)
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 54 +++++++++++++++++++++--------------------- src/qemu/qemu_domain.h | 1 - 2 files changed, 27 insertions(+), 28 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
When starting a VM we must assign unique IDs for fdsets we add via '-add-fd'. For now it was done by using the index of the filedescriptor passed to the virCommand. That approach is not very flexible, because you need to have already passed the 'fd' to virCommand before generating the fdset path, and also won't nicely work with fdsets containing two or more fds. This patch introduces a counter into the private data of a qemu domain so that we can allocate unique ids without relying on virCommand. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 29 +++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 4 ++++ 2 files changed, 33 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 64be39d49f..d0abf76e4a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -683,6 +683,33 @@ qemuDomainStorageIdReset(qemuDomainObjPrivate *priv) } +/** + * qemuDomainFDSetIdNew: + * @priv: qemu VM private data object. + * + * Generate a new unique id for a fdset. Note that this is necessary only for + * startup. When running qemu auto-assigns id for added fdset. + */ +unsigned int +qemuDomainFDSetIdNew(qemuDomainObjPrivate *priv) +{ + return ++priv->fdsetindex; +} + + +/** + * qemuDomainFDSetIdReset: + * @priv: qemu VM private data object. + * + * Resets the data for the fdset ID generator. + */ +static void +qemuDomainFDSetIdReset(qemuDomainObjPrivate *priv) +{ + priv->fdsetindex = 0; +} + + static void qemuDomainSecretInfoClear(qemuDomainSecretInfo *secinfo, bool keepAlias) @@ -1683,6 +1710,8 @@ qemuDomainObjPrivateDataClear(qemuDomainObjPrivate *priv) /* reset node name allocator */ qemuDomainStorageIdReset(priv); + qemuDomainFDSetIdReset(priv); + priv->dbusDaemonRunning = false; if (priv->dbusVMStateIds) diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 7ec337e9ae..b7d0f5d2d5 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -207,6 +207,9 @@ struct _qemuDomainObjPrivate { /* counter for generating node names for qemu disks */ unsigned long long nodenameindex; + /* counter for generating IDs of fdsets - only relevant during startup */ + unsigned int fdsetindex; + /* qemuProcessStartCPUs stores the reason for starting vCPUs here for the * RESUME event handler to use it */ virDomainRunningReason runningReason; @@ -970,6 +973,7 @@ char * qemuDomainGetManagedPRSocketPath(qemuDomainObjPrivate *priv); bool qemuDomainDefHasManagedPR(virDomainObj *vm); unsigned int qemuDomainStorageIdNew(qemuDomainObjPrivate *priv); +unsigned int qemuDomainFDSetIdNew(qemuDomainObjPrivate *priv); virDomainEventResumedDetailType qemuDomainRunningReasonToResumeEvent(virDomainRunningReason reason); -- 2.34.1
On a Wednesday in 2022, Peter Krempa wrote:
When starting a VM we must assign unique IDs for fdsets we add via '-add-fd'. For now it was done by using the index of the filedescriptor passed to the virCommand. That approach is not very flexible, because you need to have already passed the 'fd' to virCommand before generating the fdset path, and also won't nicely work with fdsets containing two or more fds.
This patch introduces a counter into the private data of a qemu domain so that we can allocate unique ids without relying on virCommand.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 29 +++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 4 ++++ 2 files changed, 33 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On a Wednesday in 2022, Peter Krempa wrote:
When starting a VM we must assign unique IDs for fdsets we add via '-add-fd'. For now it was done by using the index of the filedescriptor passed to the virCommand. That approach is not very flexible, because you need to have already passed the 'fd' to virCommand before generating the fdset path, and also won't nicely work with fdsets containing two or more fds.
This patch introduces a counter into the private data of a qemu domain so that we can allocate unique ids without relying on virCommand.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 29 +++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 4 ++++ 2 files changed, 33 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 64be39d49f..d0abf76e4a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -683,6 +683,33 @@ qemuDomainStorageIdReset(qemuDomainObjPrivate *priv) }
+/** + * qemuDomainFDSetIdNew: + * @priv: qemu VM private data object. + * + * Generate a new unique id for a fdset. Note that this is necessary only for + * startup. When running qemu auto-assigns id for added fdset. + */ +unsigned int +qemuDomainFDSetIdNew(qemuDomainObjPrivate *priv)
The spelling of 'ID' would look nicer.
+{ + return ++priv->fdsetindex;
Is there a reason why we shouldn't allow '0' as a valid fdset index? It would reduce test churn. Jano
+} + +
On Thu, Feb 10, 2022 at 18:53:43 +0100, Ján Tomko wrote:
On a Wednesday in 2022, Peter Krempa wrote:
When starting a VM we must assign unique IDs for fdsets we add via '-add-fd'. For now it was done by using the index of the filedescriptor passed to the virCommand. That approach is not very flexible, because you need to have already passed the 'fd' to virCommand before generating the fdset path, and also won't nicely work with fdsets containing two or more fds.
This patch introduces a counter into the private data of a qemu domain so that we can allocate unique ids without relying on virCommand.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 29 +++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 4 ++++ 2 files changed, 33 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 64be39d49f..d0abf76e4a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -683,6 +683,33 @@ qemuDomainStorageIdReset(qemuDomainObjPrivate *priv) }
+/** + * qemuDomainFDSetIdNew: + * @priv: qemu VM private data object. + * + * Generate a new unique id for a fdset. Note that this is necessary only for + * startup. When running qemu auto-assigns id for added fdset. + */ +unsigned int +qemuDomainFDSetIdNew(qemuDomainObjPrivate *priv)
The spelling of 'ID' would look nicer.
Hmm, okay, I'll thus also fix qemuDomainStorageIdNew/qemuDomainStorageIdReset separately.
+{ + return ++priv->fdsetindex;
Is there a reason why we shouldn't allow '0' as a valid fdset index?
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.
The existing helpers we have are very clumsy and there's no integration with the monitor. This patch introduces new helpers to bridge the gap and simplify handing of fdsets and classic FD passing when generating commandline/hotplug arguments. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- po/POTFILES.in | 1 + src/qemu/meson.build | 1 + src/qemu/qemu_fd.c | 296 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_fd.h | 56 ++++++++ 4 files changed, 354 insertions(+) create mode 100644 src/qemu/qemu_fd.c create mode 100644 src/qemu/qemu_fd.h diff --git a/po/POTFILES.in b/po/POTFILES.in index bf0a3b3529..1fd3afdd6f 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -164,6 +164,7 @@ @SRCDIR@src/qemu/qemu_domainjob.c @SRCDIR@src/qemu/qemu_driver.c @SRCDIR@src/qemu/qemu_extdevice.c +@SRCDIR@src/qemu/qemu_fd.c @SRCDIR@src/qemu/qemu_firmware.c @SRCDIR@src/qemu/qemu_hostdev.c @SRCDIR@src/qemu/qemu_hotplug.c diff --git a/src/qemu/meson.build b/src/qemu/meson.build index 3ea084cff8..39f0f615cc 100644 --- a/src/qemu/meson.build +++ b/src/qemu/meson.build @@ -15,6 +15,7 @@ qemu_driver_sources = [ 'qemu_domainjob.c', 'qemu_driver.c', 'qemu_extdevice.c', + 'qemu_fd.c', 'qemu_firmware.c', 'qemu_hostdev.c', 'qemu_hotplug.c', diff --git a/src/qemu/qemu_fd.c b/src/qemu/qemu_fd.c new file mode 100644 index 0000000000..4052323cb0 --- /dev/null +++ b/src/qemu/qemu_fd.c @@ -0,0 +1,296 @@ +/* + * qemu_fd.c: QEMU fd and fdpass passing helpers + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#include <config.h> + +#include "qemu_fd.h" +#include "qemu_domain.h" + +#include "viralloc.h" +#include "virfile.h" +#include "virlog.h" + +#define VIR_FROM_THIS VIR_FROM_QEMU +VIR_LOG_INIT("qemu.qemu_fd"); + +struct qemuFDPassFD { + int fd; + char *opaque; +}; + +struct _qemuFDPass { + bool useFDSet; + unsigned int fdSetID; + size_t nfds; + struct qemuFDPassFD *fds; + char *prefix; + char *path; + + bool passed; /* passed to qemu via monitor */ +}; + + +void +qemuFDPassFree(qemuFDPass *fdpass) +{ + size_t i; + + if (!fdpass) + return; + + for (i = 0; i < fdpass->nfds; i++) { + VIR_FORCE_CLOSE(fdpass->fds[i].fd); + g_free(fdpass->fds[i].opaque); + } + + g_free(fdpass->fds); + g_free(fdpass->prefix); + g_free(fdpass->path); + g_free(fdpass); +} + + +/** + * qemuFDPassNew: + * @prefix: prefix used for naming the passed FDs + * @dompriv: qemu domain private data + * @useFDSet: Always use FD sets (/dev/fdset/N) for this instance + * + * Create a new helper object for passing FDs to qemu. If @useFDSet is false + * the older approach of directly using FD number on the commandline and 'getfd' + * QMP command is used. Otherwise '-add-fd' and /dev/fdset/N is used to pass the + * FD. + * + * Non-test uses must pass a valid @dompriv. + * + * @prefix is used for naming the FD if needed and is later referenced when + * removing the FDSet via monitor. + */ +qemuFDPass * +qemuFDPassNew(const char *prefix, + void *dompriv, + bool useFDSet) +{ + qemuDomainObjPrivate *priv = dompriv; + qemuFDPass *fdpass = g_new0(qemuFDPass, 1); + + fdpass->prefix = g_strdup(prefix); + fdpass->useFDSet = useFDSet; + + if (fdpass->useFDSet && priv) + fdpass->fdSetID = qemuDomainFDSetIdNew(priv); + + return fdpass; +} + + +/** + * qemuFDPassAddFD: + * @fdpass: The fd passing helper struct + * @fd: File descriptor to pass + * @suffix: Name suffix for the file descriptor name + * + * Adds @fd to be passed to qemu when transferring @fdpass to qemu. When @fdpass + * is configured to use FD set mode, multiple file descriptors can be passed by + * calling this function repeatedly. + * + * @suffix is used to build the name of the file descriptor by concatenating + * it with @prefix passed to qemuFDPassNew. @suffix may be NULL, in which case + * it's considered to be an empty string. + * + * Returns 0 on success, -1 on error (when attempting to pass multiple FDs) using + * the 'direct' method. + */ +int +qemuFDPassAddFD(qemuFDPass *fdpass, + int *fd, + const char *suffix) +{ + struct qemuFDPassFD newfd = { .fd = *fd }; + + if (newfd.fd < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid file descriptor")); + return -1; + } + + if (fdpass->nfds >= 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("direct FD passing supports only 1 file descriptor")); + return -1; + } + + *fd = -1; + + newfd.opaque = g_strdup_printf("%s%s", fdpass->prefix, NULLSTR_EMPTY(suffix)); + + VIR_APPEND_ELEMENT(fdpass->fds, fdpass->nfds, newfd); + + return 0; +} + + +/** + * qemuFDPassTransferCommand: + * @fdpass: The fd passing helper struct + * @cmd: Command to pass the filedescriptors to + * + * Pass the fds in @fdpass to a commandline object @cmd. @fdpass may be NULL + * in which case this is a no-op. + */ +void +qemuFDPassTransferCommand(qemuFDPass *fdpass, + virCommand *cmd) +{ + size_t i; + + if (!fdpass) + return; + + for (i = 0; i < fdpass->nfds; i++) { + virCommandPassFD(cmd, fdpass->fds[i].fd, VIR_COMMAND_PASS_FD_CLOSE_PARENT); + + if (fdpass->useFDSet) { + g_autofree char *arg = NULL; + + arg = g_strdup_printf("set=%u,fd=%d,opaque=%s", + fdpass->fdSetID, + fdpass->fds[i].fd, + fdpass->fds[i].opaque); + + virCommandAddArgList(cmd, "-add-fd", arg, NULL); + + fdpass->path = g_strdup_printf("/dev/fdset/%u", fdpass->fdSetID); + } else { + fdpass->path = g_strdup_printf("%u", fdpass->fds[i].fd); + } + + fdpass->fds[i].fd = -1; + } +} + + +/** + * qemuFDPassTransferMonitor: + * @fdpass: The fd passing helper struct + * @mon: monitor object + * + * Pass the fds in @fdpass to qemu via the monitor. @fdpass may be NULL + * in which case this is a no-op. Caller needs to enter the monitor context. + */ +int +qemuFDPassTransferMonitor(qemuFDPass *fdpass, + qemuMonitor *mon) +{ + int fdsetid = -1; + size_t i; + + if (!fdpass) + return 0; + + for (i = 0; i < fdpass->nfds; i++) { + if (fdpass->useFDSet) { + qemuMonitorAddFdInfo fdsetinfo; + + if (qemuMonitorAddFileHandleToSet(mon, + fdpass->fds[i].fd, + fdsetid, + fdpass->fds[i].opaque, + &fdsetinfo) < 0) + return -1; + + if (fdsetid == -1) { + fdpass->fdSetID = fdsetid = fdsetinfo.fdset; + fdpass->path = g_strdup_printf("/dev/fdset/%u", fdsetid); + } + } else { + if (qemuMonitorSendFileHandle(mon, + fdpass->fds[i].opaque, + fdpass->fds[i].fd) < 0) + return -1; + + fdpass->path = g_strdup(fdpass->fds[i].opaque); + } + + fdpass->passed = true; + } + + return 0; +} + + +/** + * qemuFDPassTransferMonitorFake: + * @fdpass: The fd passing helper struct + * + * Simulate as if @fdpass was passed via monitor for callers which don't + * actually wish to test that code path. + */ +void +qemuFDPassTransferMonitorFake(qemuFDPass *fdpass) +{ + + if (!fdpass) + return; + + if (fdpass->useFDSet) { + fdpass->path = g_strdup_printf("/dev/fdset/monitor-fake"); + } else { + fdpass->path = g_strdup(fdpass->fds[0].opaque); + } +} + + +/** + * qemuFDPassTransferMonitorRollback: + * @fdpass: The fd passing helper struct + * @mon: monitor object + * + * Rolls back the addition of @fdpass to @mon if it was added originally. + */ +void +qemuFDPassTransferMonitorRollback(qemuFDPass *fdpass, + qemuMonitor *mon) +{ + if (!fdpass || !fdpass->passed) + return; + + if (fdpass->useFDSet) { + ignore_value(qemuMonitorRemoveFdset(mon, fdpass->fdSetID)); + } else { + ignore_value(qemuMonitorCloseFileHandle(mon, fdpass->fds[0].opaque)); + } +} + + +/** + * qemuFDPassGetPath: + * @fdpass: The fd passing helper struct + * + * Returns the path/fd name that is used in qemu to refer to the passed FD. + * Note that it's only valid to call this function after @fdpass was already + * transferred to the command or monitor. + */ +const char * +qemuFDPassGetPath(qemuFDPass *fdpass) +{ + if (!fdpass) + return NULL; + + return fdpass->path; +} diff --git a/src/qemu/qemu_fd.h b/src/qemu/qemu_fd.h new file mode 100644 index 0000000000..775d18fe2c --- /dev/null +++ b/src/qemu/qemu_fd.h @@ -0,0 +1,56 @@ +/* + * qemu_fd.h: QEMU fd and fdpass passing helpers + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#pragma once + +#include "vircommand.h" +#include "qemu_monitor.h" + +typedef struct _qemuFDPass qemuFDPass; + +void +qemuFDPassFree(qemuFDPass *fdpass); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuFDPass, qemuFDPassFree); + +qemuFDPass * +qemuFDPassNew(const char *prefix, + void *dompriv, + bool useFDSet); + +int +qemuFDPassAddFD(qemuFDPass *fdpass, + int *fd, + const char *suffix); + +void +qemuFDPassTransferCommand(qemuFDPass *fdpass, + virCommand *cmd); + +int +qemuFDPassTransferMonitor(qemuFDPass *fdpass, + qemuMonitor *mon); + +void +qemuFDPassTransferMonitorFake(qemuFDPass *fdpass); + +void +qemuFDPassTransferMonitorRollback(qemuFDPass *fdpass, + qemuMonitor *mon); + +const char * +qemuFDPassGetPath(qemuFDPass *fdpass); -- 2.34.1
On a Wednesday in 2022, Peter Krempa wrote:
The existing helpers we have are very clumsy and there's no integration with the monitor.
This patch introduces new helpers to bridge the gap and simplify handing
*handling
of fdsets and classic FD passing when generating commandline/hotplug arguments.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- po/POTFILES.in | 1 + src/qemu/meson.build | 1 + src/qemu/qemu_fd.c | 296 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_fd.h | 56 ++++++++ 4 files changed, 354 insertions(+) create mode 100644 src/qemu/qemu_fd.c create mode 100644 src/qemu/qemu_fd.h
diff --git a/po/POTFILES.in b/po/POTFILES.in index bf0a3b3529..1fd3afdd6f 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -164,6 +164,7 @@ @SRCDIR@src/qemu/qemu_domainjob.c @SRCDIR@src/qemu/qemu_driver.c @SRCDIR@src/qemu/qemu_extdevice.c +@SRCDIR@src/qemu/qemu_fd.c @SRCDIR@src/qemu/qemu_firmware.c @SRCDIR@src/qemu/qemu_hostdev.c @SRCDIR@src/qemu/qemu_hotplug.c diff --git a/src/qemu/meson.build b/src/qemu/meson.build index 3ea084cff8..39f0f615cc 100644 --- a/src/qemu/meson.build +++ b/src/qemu/meson.build @@ -15,6 +15,7 @@ qemu_driver_sources = [ 'qemu_domainjob.c', 'qemu_driver.c', 'qemu_extdevice.c', + 'qemu_fd.c', 'qemu_firmware.c', 'qemu_hostdev.c', 'qemu_hotplug.c', diff --git a/src/qemu/qemu_fd.c b/src/qemu/qemu_fd.c new file mode 100644 index 0000000000..4052323cb0 --- /dev/null +++ b/src/qemu/qemu_fd.c @@ -0,0 +1,296 @@ +/* + * qemu_fd.c: QEMU fd and fdpass passing helpers + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>.
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)
+ */ + +#include <config.h> + +#include "qemu_fd.h" +#include "qemu_domain.h" + +#include "viralloc.h" +#include "virfile.h" +#include "virlog.h"
+/** + * qemuFDPassNew: + * @prefix: prefix used for naming the passed FDs + * @dompriv: qemu domain private data + * @useFDSet: Always use FD sets (/dev/fdset/N) for this instance + * + * Create a new helper object for passing FDs to qemu. If @useFDSet is false + * the older approach of directly using FD number on the commandline and 'getfd' + * QMP command is used. Otherwise '-add-fd' and /dev/fdset/N is used to pass the + * FD. + *
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?)
+ * Non-test uses must pass a valid @dompriv. + * + * @prefix is used for naming the FD if needed and is later referenced when + * removing the FDSet via monitor. + */ +qemuFDPass * +qemuFDPassNew(const char *prefix, + void *dompriv, + bool useFDSet) +{ + qemuDomainObjPrivate *priv = dompriv; + qemuFDPass *fdpass = g_new0(qemuFDPass, 1); + + fdpass->prefix = g_strdup(prefix); + fdpass->useFDSet = useFDSet; + + if (fdpass->useFDSet && priv) + fdpass->fdSetID = qemuDomainFDSetIdNew(priv); + + return fdpass; +} + + +/** + * qemuFDPassAddFD: + * @fdpass: The fd passing helper struct + * @fd: File descriptor to pass + * @suffix: Name suffix for the file descriptor name + * + * Adds @fd to be passed to qemu when transferring @fdpass to qemu.
QEMU
When @fdpass + * is configured to use FD set mode, multiple file descriptors can be passed by + * calling this function repeatedly. + * + * @suffix is used to build the name of the file descriptor by concatenating + * it with @prefix passed to qemuFDPassNew. @suffix may be NULL, in which case + * it's considered to be an empty string. + * + * Returns 0 on success, -1 on error (when attempting to pass multiple FDs) using + * the 'direct' method. + */ +int +qemuFDPassAddFD(qemuFDPass *fdpass, + int *fd, + const char *suffix) +{ + struct qemuFDPassFD newfd = { .fd = *fd }; + + if (newfd.fd < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid file descriptor")); + return -1; + } + + if (fdpass->nfds >= 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("direct FD passing supports only 1 file descriptor"));
This check conflicts with the comment above saying multiple FDs are allowed with fdsets. Also, please s/1/one/
+ return -1; + } + + *fd = -1; + + newfd.opaque = g_strdup_printf("%s%s", fdpass->prefix, NULLSTR_EMPTY(suffix)); + + VIR_APPEND_ELEMENT(fdpass->fds, fdpass->nfds, newfd); + + return 0; +}
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Thu, Feb 10, 2022 at 18:35:44 +0100, Ján Tomko wrote:
On a Wednesday in 2022, Peter Krempa wrote:
The existing helpers we have are very clumsy and there's no integration with the monitor.
This patch introduces new helpers to bridge the gap and simplify handing
[...]
+/** + * qemuFDPassNew: + * @prefix: prefix used for naming the passed FDs + * @dompriv: qemu domain private data + * @useFDSet: Always use FD sets (/dev/fdset/N) for this instance + * + * Create a new helper object for passing FDs to qemu. If @useFDSet is false + * the older approach of directly using FD number on the commandline and 'getfd' + * QMP command is used. Otherwise '-add-fd' and /dev/fdset/N is used to pass the + * FD. + *
I dislike using bools in helpers, can this be turned into a flag?
Well I dislike flags because it makes the code of the function harder to read as opposed to making the call itself easier to read, so I try to avoid them if it's feasible in a given situation.
Alternatively, to discourage the older approach you can create a thin qemuFDPassNewLegacy wrapper and leave qemuFDPassNew without the comment.
I think I'll go with this one.
(Can we even convince QEMU to use fdset for sockets? Or is that not worth pursuing?)
I actually have a qemu patch prepared which makes the code paths using direct FD passing or the 'getfd' QMP command to accept fdsets too, but I still have to test it. The idea is to hide the complexity in these wrappers and then just use fdsets.
+ * Non-test uses must pass a valid @dompriv. + * + * @prefix is used for naming the FD if needed and is later referenced when + * removing the FDSet via monitor. + */ +qemuFDPass * +qemuFDPassNew(const char *prefix, + void *dompriv, + bool useFDSet) +{ + qemuDomainObjPrivate *priv = dompriv; + qemuFDPass *fdpass = g_new0(qemuFDPass, 1); + + fdpass->prefix = g_strdup(prefix); + fdpass->useFDSet = useFDSet; + + if (fdpass->useFDSet && priv) + fdpass->fdSetID = qemuDomainFDSetIdNew(priv); + + return fdpass; +} + + +/** + * qemuFDPassAddFD: + * @fdpass: The fd passing helper struct + * @fd: File descriptor to pass + * @suffix: Name suffix for the file descriptor name + * + * Adds @fd to be passed to qemu when transferring @fdpass to qemu.
QEMU
When @fdpass + * is configured to use FD set mode, multiple file descriptors can be passed by + * calling this function repeatedly. + * + * @suffix is used to build the name of the file descriptor by concatenating + * it with @prefix passed to qemuFDPassNew. @suffix may be NULL, in which case + * it's considered to be an empty string. + * + * Returns 0 on success, -1 on error (when attempting to pass multiple FDs) using + * the 'direct' method. + */ +int +qemuFDPassAddFD(qemuFDPass *fdpass, + int *fd, + const char *suffix) +{ + struct qemuFDPassFD newfd = { .fd = *fd }; + + if (newfd.fd < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid file descriptor")); + return -1; + } + + if (fdpass->nfds >= 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("direct FD passing supports only 1 file descriptor"));
This check conflicts with the comment above saying multiple FDs are allowed with fdsets. Also, please s/1/one/
oops, the idea was to do that check only when the legacy mode is used. Changes in this series actually don't use the multi-fd mode yet.
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@redhat.com> --- 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
On a Wednesday in 2022, Peter Krempa wrote:
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@redhat.com> --- src/qemu/qemu_command.c | 35 ++++++++----------- .../net-vdpa.x86_64-latest.args | 4 +-- 2 files changed, 17 insertions(+), 22 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The code unplugging the fdset for a 'vdpa' network device can be later reused. Extract it into 'qemuHotplugRemoveFDSet'. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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
On a Wednesday in 2022, Peter Krempa wrote:
The code unplugging the fdset for a 'vdpa' network device can be later reused. Extract it into 'qemuHotplugRemoveFDSet'.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 69 +++++++++++++++++++++++------------------ 1 file changed, 39 insertions(+), 30 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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@redhat.com> --- 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
On a Wednesday in 2022, Peter Krempa wrote:
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
QEMUs
VDPA interface to use the path.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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
*field
+ * @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;
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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@redhat.com> --- 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
On a Wednesday in 2022, Peter Krempa wrote:
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@redhat.com> --- src/qemu/qemu_command.c | 4 ++-- tests/qemuxml2argvdata/net-vdpa.x86_64-latest.args | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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@redhat.com> --- 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
On a Wednesday in 2022, Peter Krempa wrote:
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@redhat.com> --- 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(-)
[..]
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 */
*sockets
+ charpriv->sourcefd = qemuFDPassNew(devalias, data->priv, false); + + if (qemuFDPassAddFD(charpriv->sourcefd, &sourcefd, "-source") < 0) return -1; } break;
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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@redhat.com> --- 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
On a Wednesday in 2022, Peter Krempa wrote:
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@redhat.com> --- src/qemu/qemu_hotplug.c | 1 + tests/qemuhotplugtest.c | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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@redhat.com> --- 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
On a Wednesday in 2022, Peter Krempa wrote:
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@redhat.com> --- tests/qemumonitorjsontest.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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@redhat.com> --- 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
On a Wednesday in 2022, Peter Krempa wrote:
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@redhat.com> --- tests/qemumonitorjsontest.c | 214 +++++++++++++++++++----------------- 1 file changed, 113 insertions(+), 101 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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
On a Wednesday in 2022, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitorjsontest.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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
On a Wednesday in 2022, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitorjsontest.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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@redhat.com> --- 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 a Wednesday in 2022, Peter Krempa wrote:
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@redhat.com> --- 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)
What does the 'i' stand for?
+{ + 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)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Thu, Feb 10, 2022 at 18:46:30 +0100, Ján Tomko wrote:
On a Wednesday in 2022, Peter Krempa wrote:
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@redhat.com> --- 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)
What does the 'i' stand for?
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 :)
+{ + 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)
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Jano
Move the function doing the fake setup of chardev backend for FD passing into the collection of qemu test helpers so that it can be used in qemumonitorjsontest. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuxml2argvtest.c | 93 +--------------------------------- tests/testutilsqemu.c | 106 +++++++++++++++++++++++++++++++++++++++ tests/testutilsqemu.h | 3 ++ 3 files changed, 111 insertions(+), 91 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index d4e518e3a1..1e1d9ee9a1 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -378,95 +378,6 @@ testCheckExclusiveFlags(int flags) } -static int -testPrepareHostBackendChardevOne(virDomainDeviceDef *dev, - virDomainChrSourceDef *chardev, - void *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; - - 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; - } - - /* 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) { - 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: - fakesourcefd = 1750; - break; - - case VIR_DOMAIN_CHR_TYPE_UNIX: - if (chardev->data.nix.listen && - virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS_COMMANDLINE)) - fakesourcefd = 1729; - - usefdset = false; - break; - - case VIR_DOMAIN_CHR_TYPE_NMDM: - case VIR_DOMAIN_CHR_TYPE_LAST: - 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) { - int fd = 1751; - - if (fcntl(fd, F_GETFD) != -1) - abort(); - - charpriv->logfd = qemuFDPassNew(devalias, priv, true); - - if (qemuFDPassAddFD(charpriv->logfd, &fd, "-log") < 0) - return -1; - } - - return 0; -} - - static virCommand * testCompareXMLToArgvCreateArgs(virQEMUDriver *drv, virDomainObj *vm, @@ -483,11 +394,11 @@ testCompareXMLToArgvCreateArgs(virQEMUDriver *drv, return NULL; if (qemuDomainDeviceBackendChardevForeach(vm->def, - testPrepareHostBackendChardevOne, + testQemuPrepareHostBackendChardevOne, vm) < 0) return NULL; - if (testPrepareHostBackendChardevOne(NULL, priv->monConfig, vm) < 0) + if (testQemuPrepareHostBackendChardevOne(NULL, priv->monConfig, vm) < 0) return NULL; for (i = 0; i < vm->def->ndisks; i++) { diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 646ef415d1..c156a89413 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -13,6 +13,9 @@ # include "virstring.h" # include "virfilecache.h" +# include <sys/types.h> +# include <fcntl.h> + # define VIR_FROM_THIS VIR_FROM_QEMU virCPUDef *cpuDefault; @@ -979,3 +982,106 @@ testQemuInfoClear(struct testQemuInfo *info) virObjectUnref(info->qemuCaps); g_clear_pointer(&info->args.fakeCaps, virObjectUnref); } + + +/** + * testQemuPrepareHostBackendChardevOne: + * @dev: device definition object + * @chardev: chardev source object + * @opaque: Caller is expected to pass pointer to virDomainObj or NULL + * + * This helper sets up a chardev source backend for FD passing with fake + * file descriptros. It's expected to be used as callback for + * 'qemuDomainDeviceBackendChardevForeach', thus the VM object is passed via + * @opaque. Callers may pass NULL if the test scope is limited. + */ +int +testQemuPrepareHostBackendChardevOne(virDomainDeviceDef *dev, + virDomainChrSourceDef *chardev, + void *opaque) +{ + virDomainObj *vm = opaque; + qemuDomainObjPrivate *priv = NULL; + qemuDomainChrSourcePrivate *charpriv = QEMU_DOMAIN_CHR_SOURCE_PRIVATE(chardev); + int fakesourcefd = -1; + const char *devalias = NULL; + bool usefdset = true; + + if (vm) + priv = vm->privateData; + + 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; + + 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; + } + + /* 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) { + 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: + fakesourcefd = 1750; + break; + + case VIR_DOMAIN_CHR_TYPE_UNIX: + if (chardev->data.nix.listen && + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS_COMMANDLINE)) + fakesourcefd = 1729; + + usefdset = false; + break; + + case VIR_DOMAIN_CHR_TYPE_NMDM: + case VIR_DOMAIN_CHR_TYPE_LAST: + 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) { + int fd = 1751; + + if (fcntl(fd, F_GETFD) != -1) + abort(); + + charpriv->logfd = qemuFDPassNew(devalias, priv, true); + + if (qemuFDPassAddFD(charpriv->logfd, &fd, "-log") < 0) + return -1; + } + + return 0; +} diff --git a/tests/testutilsqemu.h b/tests/testutilsqemu.h index 51139e6a97..187f9b7cd3 100644 --- a/tests/testutilsqemu.h +++ b/tests/testutilsqemu.h @@ -143,4 +143,7 @@ void testQemuInfoSetArgs(struct testQemuInfo *info, int testQemuInfoInitArgs(struct testQemuInfo *info); void testQemuInfoClear(struct testQemuInfo *info); +int testQemuPrepareHostBackendChardevOne(virDomainDeviceDef *dev, + virDomainChrSourceDef *chardev, + void *opaque); #endif -- 2.34.1
On a Wednesday in 2022, Peter Krempa wrote:
Move the function doing the fake setup of chardev backend for FD passing into the collection of qemu test helpers so that it can be used in qemumonitorjsontest.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuxml2argvtest.c | 93 +--------------------------------- tests/testutilsqemu.c | 106 +++++++++++++++++++++++++++++++++++++++ tests/testutilsqemu.h | 3 ++ 3 files changed, 111 insertions(+), 91 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitorjsontest.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 8fbb199a59..278d7ba765 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -680,6 +680,8 @@ static int qemuMonitorJSONTestAttachChardev(virDomainXMLOption *xmlopt, GHashTable *schema) { + virDomainChrDef chrdev = { .info = { .alias = (char *) "alias" }}; + virDomainDeviceDef dev = { .type = VIR_DOMAIN_DEVICE_CHR, .data.chr = &chrdev }; int ret = 0; #define CHECK(label, fail, expectargs) \ @@ -723,6 +725,7 @@ qemuMonitorJSONTestAttachChardev(virDomainXMLOption *xmlopt, { g_autoptr(virDomainChrSourceDef) chr = virDomainChrSourceDefNew(xmlopt); + qemuDomainChrSourcePrivate *charpriv = QEMU_DOMAIN_CHR_SOURCE_PRIVATE(chr); chr->data.file.path = g_strdup("/test/path"); @@ -737,6 +740,16 @@ qemuMonitorJSONTestAttachChardev(virDomainXMLOption *xmlopt, "{'id':'alias','backend':{'type':'file','data':{'out':'/test/path'," "'logfile':'/test/logfile'," "'logappend':false}}}"); + + chrdev.source = chr; + ignore_value(testQemuPrepareHostBackendChardevOne(&dev, chr, NULL)); + qemuFDPassTransferMonitorFake(charpriv->sourcefd); + qemuFDPassTransferMonitorFake(charpriv->logfd); + CHECK("file", false, + "{'id':'alias','backend':{'type':'file','data':{'out':'/dev/fdset/monitor-fake'," + "'append':true," + "'logfile':'/dev/fdset/monitor-fake'," + "'logappend':true}}}"); } { -- 2.34.1
On a Wednesday in 2022, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitorjsontest.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
When hotplugging a chardev we need the same form of setup for the character device. Export a version which takes a 'virDomainDeviceDef'. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 27 +++++++++++++++++++++++++++ src/qemu/qemu_process.h | 6 ++++++ 2 files changed, 33 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index b44e684589..9fea72960f 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6965,6 +6965,33 @@ qemuProcessPrepareHostBackendChardev(virDomainObj *vm) } +int +qemuProcessPrepareHostBackendChardevHotplug(virDomainObj *vm, + virDomainDeviceDef *dev) +{ + qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver); + struct qemuProcessPrepareHostBackendChardevData data = { + .priv = priv, + .logManager = NULL, + .cfg = cfg, + .def = vm->def, + }; + g_autoptr(virLogManager) logManager = NULL; + + if (cfg->stdioLogD) { + if (!(logManager = data.logManager = virLogManagerNew(priv->driver->privileged))) + return -1; + } + + if (qemuDomainDeviceBackendChardevForeachOne(dev, + qemuProcessPrepareHostBackendChardevOne, + &data) < 0) + return -1; + + return 0; +} + /** * qemuProcessPrepareHost: * @driver: qemu driver diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index f6dd3f5104..289cd74eb7 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -120,6 +120,12 @@ int qemuProcessOpenVhostVsock(virDomainVsockDef *vsock); int qemuProcessPrepareHostHostdev(virDomainHostdevDef *hostdev); + +int qemuProcessPrepareHostBackendChardevHotplug(virDomainObj *vm, + virDomainDeviceDef *dev) + G_GNUC_NO_INLINE; + + int qemuProcessPrepareHost(virQEMUDriver *driver, virDomainObj *vm, unsigned int flags); -- 2.34.1
On a Wednesday in 2022, Peter Krempa wrote:
When hotplugging a chardev we need the same form of setup for the character device. Export a version which takes a 'virDomainDeviceDef'.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 27 +++++++++++++++++++++++++++ src/qemu/qemu_process.h | 6 ++++++ 2 files changed, 33 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Setup the chardev similarly to how we do it on startup so that virtlogd is properly used with chardevs which are hotplugged to a VM. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 3 +-- src/qemu/qemu_hotplug.c | 25 ++++++++++++++++++++++--- src/qemu/qemu_hotplug.h | 2 +- tests/qemuhotplugmock.c | 11 +++++++++++ tests/qemuhotplugtest.c | 2 +- 5 files changed, 36 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9279eaf811..d6876baa34 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6730,8 +6730,7 @@ qemuDomainAttachDeviceLive(virDomainObj *vm, break; case VIR_DOMAIN_DEVICE_CHR: - ret = qemuDomainAttachChrDevice(driver, vm, - dev->data.chr); + ret = qemuDomainAttachChrDevice(driver, vm, dev); if (!ret) { alias = dev->data.chr->info.alias; dev->data.chr = NULL; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e05edffae2..01523ce508 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2180,10 +2180,14 @@ qemuDomainAttachChrDeviceAssignAddr(virDomainObj *vm, return 0; } -int qemuDomainAttachChrDevice(virQEMUDriver *driver, - virDomainObj *vm, - virDomainChrDef *chr) + +int +qemuDomainAttachChrDevice(virQEMUDriver *driver, + virDomainObj *vm, + virDomainDeviceDef *dev) { + virDomainChrDef *chr = dev->data.chr; + qemuDomainChrSourcePrivate *charpriv = QEMU_DOMAIN_CHR_SOURCE_PRIVATE(chr->source); int ret = -1; qemuDomainObjPrivate *priv = vm->privateData; virErrorPtr orig_err; @@ -2224,6 +2228,19 @@ int qemuDomainAttachChrDevice(virQEMUDriver *driver, goto cleanup; teardowncgroup = true; + if (qemuProcessPrepareHostBackendChardevHotplug(vm, dev) < 0) + goto cleanup; + + if (charpriv->sourcefd || charpriv->logfd) { + qemuDomainObjEnterMonitor(driver, vm); + + if (qemuFDPassTransferMonitor(charpriv->sourcefd, priv->mon) < 0 || + qemuFDPassTransferMonitor(charpriv->logfd, priv->mon) < 0) + goto exit_monitor; + + qemuDomainObjExitMonitor(driver, vm); + } + if (guestfwd) { if (!(netdevprops = qemuBuildChannelGuestfwdNetdevProps(chr))) goto cleanup; @@ -2286,6 +2303,8 @@ int qemuDomainAttachChrDevice(virQEMUDriver *driver, /* detach associated chardev on error */ if (chardevAttached) qemuMonitorDetachCharDev(priv->mon, charAlias); + qemuFDPassTransferMonitorRollback(charpriv->sourcefd, priv->mon); + qemuFDPassTransferMonitorRollback(charpriv->logfd, priv->mon); qemuDomainObjExitMonitor(driver, vm); virErrorRestore(&orig_err); diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index 244dd5278d..19c07497b5 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -117,7 +117,7 @@ int qemuDomainAttachLease(virQEMUDriver *driver, virDomainLeaseDef *lease); int qemuDomainAttachChrDevice(virQEMUDriver *driver, virDomainObj *vm, - virDomainChrDef *chr); + virDomainDeviceDef *dev); int qemuDomainAttachRNGDevice(virQEMUDriver *driver, virDomainObj *vm, virDomainRNGDef *rng); diff --git a/tests/qemuhotplugmock.c b/tests/qemuhotplugmock.c index 051f2b20dc..e3f0715058 100644 --- a/tests/qemuhotplugmock.c +++ b/tests/qemuhotplugmock.c @@ -21,6 +21,7 @@ #include "qemu/qemu_hotplug.h" #include "qemu/qemu_interface.h" #include "qemu/qemu_process.h" +#include "testutilsqemu.h" #include "conf/domain_conf.h" #include "virdevmapper.h" #include "virutil.h" @@ -96,3 +97,13 @@ qemuInterfaceVDPAConnect(virDomainNetDef *net G_GNUC_UNUSED) /* need a valid fd or sendmsg won't work. Just open /dev/null */ return open("/dev/null", O_RDONLY); } + + +int +qemuProcessPrepareHostBackendChardevHotplug(virDomainObj *vm, + virDomainDeviceDef *dev) +{ + return qemuDomainDeviceBackendChardevForeachOne(dev, + testQemuPrepareHostBackendChardevOne, + vm); +} diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index fe1b227b4e..ce215da099 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -135,7 +135,7 @@ testQemuHotplugAttach(virDomainObj *vm, ret = qemuDomainAttachDeviceDiskLive(&driver, vm, dev); break; case VIR_DOMAIN_DEVICE_CHR: - ret = qemuDomainAttachChrDevice(&driver, vm, dev->data.chr); + ret = qemuDomainAttachChrDevice(&driver, vm, dev); break; case VIR_DOMAIN_DEVICE_SHMEM: ret = qemuDomainAttachShmemDevice(&driver, vm, dev->data.shmem); -- 2.34.1
On a Wednesday in 2022, Peter Krempa wrote:
Setup the chardev similarly to how we do it on startup so that virtlogd is properly used with chardevs which are hotplugged to a VM.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 3 +-- src/qemu/qemu_hotplug.c | 25 ++++++++++++++++++++++--- src/qemu/qemu_hotplug.h | 2 +- tests/qemuhotplugmock.c | 11 +++++++++++ tests/qemuhotplugtest.c | 2 +- 5 files changed, 36 insertions(+), 7 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Automatically free 'path' inside the loop which fills it and return the values directly. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virtpm.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/util/virtpm.c b/src/util/virtpm.c index c02b42f948..8005708522 100644 --- a/src/util/virtpm.c +++ b/src/util/virtpm.c @@ -59,7 +59,6 @@ VIR_ENUM_IMPL(virTPMSwtpmSetupFeature, char * virTPMCreateCancelPath(const char *devpath) { - char *path = NULL; const char *dev; const char *prefix[] = {"misc/", "tpm/"}; size_t i; @@ -77,18 +76,14 @@ virTPMCreateCancelPath(const char *devpath) dev++; for (i = 0; i < G_N_ELEMENTS(prefix); i++) { - path = g_strdup_printf("/sys/class/%s%s/device/cancel", prefix[i], - dev); + g_autofree char *path = g_strdup_printf("/sys/class/%s%s/device/cancel", + prefix[i], dev); if (virFileExists(path)) - break; - - VIR_FREE(path); + return g_steal_pointer(&path); } - if (!path) - path = g_strdup("/dev/null"); - return path; + return g_strdup("/dev/null"); } /* -- 2.34.1
On a Wednesday in 2022, Peter Krempa wrote:
Automatically free 'path' inside the loop which fills it and return the values directly.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virtpm.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Since 'cancel_path' is constructed from the 'tpmdev' argument, we can push it down into the function opening the FDs. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 15 +++++++-------- src/qemu/qemu_command.h | 4 +--- tests/qemuxml2argvmock.c | 1 - 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 9a51a373b0..7169b8ff90 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9737,10 +9737,14 @@ qemuBuildTPMDevCmd(virCommand *cmd, /* this function is exported so that tests can mock the FDs */ int qemuBuildTPMOpenBackendFDs(const char *tpmdev, - const char *cancel_path, int *tpmfd, int *cancelfd) { + g_autofree char *cancel_path = NULL; + + if (!(cancel_path = virTPMCreateCancelPath(tpmdev))) + return -1; + if ((*tpmfd = open(tpmdev, O_RDWR)) < 0) { virReportSystemError(errno, _("Could not open TPM device %s"), tpmdev); @@ -9766,10 +9770,8 @@ qemuBuildTPMBackendStr(virCommand *cmd, int *cancelfd) { g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; - g_autofree char *cancel_path = NULL; g_autofree char *devset = NULL; g_autofree char *cancelset = NULL; - const char *tpmdev; *tpmfd = -1; *cancelfd = -1; @@ -9779,11 +9781,8 @@ qemuBuildTPMBackendStr(virCommand *cmd, switch (tpm->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: - tpmdev = tpm->data.passthrough.source->data.file.path; - if (!(cancel_path = virTPMCreateCancelPath(tpmdev))) - return NULL; - - if (qemuBuildTPMOpenBackendFDs(tpmdev, cancel_path, tpmfd, cancelfd) < 0) + if (qemuBuildTPMOpenBackendFDs(tpm->data.passthrough.source->data.file.path, + tpmfd, cancelfd) < 0) return NULL; virCommandPassFD(cmd, *tpmfd, VIR_COMMAND_PASS_FD_CLOSE_PARENT); diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index b7fcf15a1e..d84de3f093 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -251,11 +251,9 @@ qemuBuildVsockDevProps(virDomainDef *def, /* this function is exported so that tests can mock the FDs */ int qemuBuildTPMOpenBackendFDs(const char *tpmdev, - const char *cancel_path, int *tpmfd, int *cancelfd) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) - ATTRIBUTE_NONNULL(4) G_GNUC_NO_INLINE; + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) G_GNUC_NO_INLINE; const char * qemuAudioDriverTypeToString(virDomainAudioType type); virDomainAudioType qemuAudioDriverTypeFromString(const char *str); diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c index f4e2f52680..08f176667d 100644 --- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c @@ -263,7 +263,6 @@ qemuOpenChrChardevUNIXSocket(const virDomainChrSourceDef *dev G_GNUC_UNUSED) int qemuBuildTPMOpenBackendFDs(const char *tpmdev G_GNUC_UNUSED, - const char *cancel_path G_GNUC_UNUSED, int *tpmfd, int *cancelfd) { -- 2.34.1
On a Wednesday in 2022, Peter Krempa wrote:
Since 'cancel_path' is constructed from the 'tpmdev' argument, we can push it down into the function opening the FDs.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 15 +++++++-------- src/qemu/qemu_command.h | 4 +--- tests/qemuxml2argvmock.c | 1 - 3 files changed, 8 insertions(+), 12 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Remove the last code path using hardcoded fdsets. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 140 +++++------------- .../tpm-passthrough-crb.x86_64-latest.args | 6 +- .../tpm-passthrough.x86_64-latest.args | 6 +- 3 files changed, 45 insertions(+), 107 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 7169b8ff90..9649ba3701 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -301,56 +301,6 @@ qemuBuildMasterKeyCommandLine(virCommand *cmd, } -/** - * qemuVirCommandGetFDSet: - * @cmd: the command to modify - * @fd: fd to reassign to the child - * - * Get the parameters for the QEMU -add-fd command line option - * for the given file descriptor. The file descriptor must previously - * have been 'transferred' in a virCommandPassFD() call. - * This function for example returns "set=10,fd=20". - */ -static char * -qemuVirCommandGetFDSet(virCommand *cmd, int fd) -{ - int idx = virCommandPassFDGetFDIndex(cmd, fd); - - if (idx < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("file descriptor %d has not been transferred"), fd); - return NULL; - } - - return g_strdup_printf("set=%d,fd=%d", idx, fd); -} - - -/** - * qemuVirCommandGetDevSet: - * @cmd: the command to modify - * @fd: fd to reassign to the child - * - * Get the parameters for the QEMU path= parameter where a file - * descriptor is accessed via a file descriptor set, for example - * /dev/fdset/10. The file descriptor must previously have been - * 'transferred' in a virCommandPassFD() call. - */ -static char * -qemuVirCommandGetDevSet(virCommand *cmd, int fd) -{ - int idx = virCommandPassFDGetFDIndex(cmd, fd); - - if (idx < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("file descriptor %d has not been transferred"), fd); - return NULL; - } - - return g_strdup_printf("/dev/fdset/%d", idx); -} - - static char * qemuBuildDeviceAddressPCIGetBus(const virDomainDef *domainDef, const virDomainDeviceInfo *info) @@ -9764,40 +9714,22 @@ qemuBuildTPMOpenBackendFDs(const char *tpmdev, static char * -qemuBuildTPMBackendStr(virCommand *cmd, - virDomainTPMDef *tpm, - int *tpmfd, - int *cancelfd) +qemuBuildTPMBackendStr(virDomainTPMDef *tpm, + qemuFDPass *passtpm, + qemuFDPass *passcancel) { g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; - g_autofree char *devset = NULL; - g_autofree char *cancelset = NULL; - - *tpmfd = -1; - *cancelfd = -1; virBufferAsprintf(&buf, "%s", virDomainTPMBackendTypeToString(tpm->type)); virBufferAsprintf(&buf, ",id=tpm-%s", tpm->info.alias); switch (tpm->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: - if (qemuBuildTPMOpenBackendFDs(tpm->data.passthrough.source->data.file.path, - tpmfd, cancelfd) < 0) - return NULL; - - virCommandPassFD(cmd, *tpmfd, VIR_COMMAND_PASS_FD_CLOSE_PARENT); - virCommandPassFD(cmd, *cancelfd, VIR_COMMAND_PASS_FD_CLOSE_PARENT); - - if (!(devset = qemuVirCommandGetDevSet(cmd, *tpmfd)) || - !(cancelset = qemuVirCommandGetDevSet(cmd, *cancelfd))) - return NULL; - virBufferAddLit(&buf, ",path="); - virQEMUBuildBufferEscapeComma(&buf, devset); + virQEMUBuildBufferEscapeComma(&buf, qemuFDPassGetPath(passtpm)); virBufferAddLit(&buf, ",cancel-path="); - virQEMUBuildBufferEscapeComma(&buf, cancelset); - + virQEMUBuildBufferEscapeComma(&buf, qemuFDPassGetPath(passcancel)); break; case VIR_DOMAIN_TPM_TYPE_EMULATOR: virBufferAddLit(&buf, ",chardev=chrtpm"); @@ -9814,42 +9746,49 @@ static int qemuBuildTPMCommandLine(virCommand *cmd, const virDomainDef *def, virDomainTPMDef *tpm, - virQEMUCaps *qemuCaps) + qemuDomainObjPrivate *priv) { g_autofree char *tpmdevstr = NULL; - int tpmfd = -1; - int cancelfd = -1; - char *fdset; + g_autoptr(qemuFDPass) passtpm = NULL; + g_autoptr(qemuFDPass) passcancel = NULL; - if (tpm->type == VIR_DOMAIN_TPM_TYPE_EMULATOR) { - if (qemuBuildChardevCommand(cmd, tpm->data.emulator.source, "chrtpm", qemuCaps) < 0) - return -1; - } + switch ((virDomainTPMBackendType) tpm->type) { + case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: { + VIR_AUTOCLOSE fdtpm = -1; + VIR_AUTOCLOSE fdcancel = -1; - if (!(tpmdevstr = qemuBuildTPMBackendStr(cmd, tpm, &tpmfd, &cancelfd))) - return -1; + if (qemuBuildTPMOpenBackendFDs(tpm->data.passthrough.source->data.file.path, + &fdtpm, &fdcancel) < 0) + return -1; - virCommandAddArgList(cmd, "-tpmdev", tpmdevstr, NULL); + passtpm = qemuFDPassNew(tpm->info.alias, priv, true); + passcancel = qemuFDPassNew(tpm->info.alias, priv, true); - if (tpmfd >= 0) { - fdset = qemuVirCommandGetFDSet(cmd, tpmfd); - if (!fdset) + if (qemuFDPassAddFD(passtpm, &fdtpm, "-tpm") < 0 || + qemuFDPassAddFD(passcancel, &fdcancel, "-cancel") < 0) return -1; - - virCommandAddArgList(cmd, "-add-fd", fdset, NULL); - VIR_FREE(fdset); } + break; - if (cancelfd >= 0) { - fdset = qemuVirCommandGetFDSet(cmd, cancelfd); - if (!fdset) + case VIR_DOMAIN_TPM_TYPE_EMULATOR: + if (qemuBuildChardevCommand(cmd, tpm->data.emulator.source, "chrtpm", priv->qemuCaps) < 0) return -1; + break; - virCommandAddArgList(cmd, "-add-fd", fdset, NULL); - VIR_FREE(fdset); + case VIR_DOMAIN_TPM_TYPE_LAST: + virReportEnumRangeError(virDomainTPMBackendType, tpm->type); + return -1; } - if (qemuBuildTPMDevCmd(cmd, def, tpm, qemuCaps) < 0) + qemuFDPassTransferCommand(passtpm, cmd); + qemuFDPassTransferCommand(passcancel, cmd); + + if (!(tpmdevstr = qemuBuildTPMBackendStr(tpm, passtpm, passcancel))) + return -1; + + virCommandAddArgList(cmd, "-tpmdev", tpmdevstr, NULL); + + if (qemuBuildTPMDevCmd(cmd, def, tpm, priv->qemuCaps) < 0) return -1; return 0; @@ -9880,16 +9819,15 @@ qemuBuildTPMProxyCommandLine(virCommand *cmd, static int qemuBuildTPMsCommandLine(virCommand *cmd, const virDomainDef *def, - virQEMUCaps *qemuCaps) + qemuDomainObjPrivate *priv) { size_t i; for (i = 0; i < def->ntpms; i++) { if (def->tpms[i]->model == VIR_DOMAIN_TPM_MODEL_SPAPR_PROXY) { - if (qemuBuildTPMProxyCommandLine(cmd, def->tpms[i], qemuCaps) < 0) + if (qemuBuildTPMProxyCommandLine(cmd, def->tpms[i], priv->qemuCaps) < 0) return -1; - } else if (qemuBuildTPMCommandLine(cmd, def, - def->tpms[i], qemuCaps) < 0) { + } else if (qemuBuildTPMCommandLine(cmd, def, def->tpms[i], priv) < 0) { return -1; } } @@ -10633,7 +10571,7 @@ qemuBuildCommandLine(virQEMUDriver *driver, if (qemuBuildConsoleCommandLine(cmd, def, qemuCaps) < 0) return NULL; - if (qemuBuildTPMsCommandLine(cmd, def, qemuCaps) < 0) + if (qemuBuildTPMsCommandLine(cmd, def, priv) < 0) return NULL; if (qemuBuildInputCommandLine(cmd, def, qemuCaps) < 0) diff --git a/tests/qemuxml2argvdata/tpm-passthrough-crb.x86_64-latest.args b/tests/qemuxml2argvdata/tpm-passthrough-crb.x86_64-latest.args index e099cf0fa1..b9f22188c2 100644 --- a/tests/qemuxml2argvdata/tpm-passthrough-crb.x86_64-latest.args +++ b/tests/qemuxml2argvdata/tpm-passthrough-crb.x86_64-latest.args @@ -27,9 +27,9 @@ XDG_CONFIG_HOME=/tmp/lib/domain--1-TPM-VM/.config \ -no-shutdown \ -boot menu=on,strict=on \ -device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \ --tpmdev passthrough,id=tpm-tpm0,path=/dev/fdset/0,cancel-path=/dev/fdset/1 \ --add-fd set=0,fd=1730 \ --add-fd set=1,fd=1731 \ +-add-fd set=1,fd=1730,opaque=tpm0-tpm \ +-add-fd set=2,fd=1731,opaque=tpm0-cancel \ +-tpmdev passthrough,id=tpm-tpm0,path=/dev/fdset/1,cancel-path=/dev/fdset/2 \ -device '{"driver":"tpm-crb","tpmdev":"tpm-tpm0","id":"tpm0"}' \ -audiodev '{"id":"audio1","driver":"none"}' \ -device '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.0","addr":"0x2"}' \ diff --git a/tests/qemuxml2argvdata/tpm-passthrough.x86_64-latest.args b/tests/qemuxml2argvdata/tpm-passthrough.x86_64-latest.args index beb6a307cb..da042d3b2b 100644 --- a/tests/qemuxml2argvdata/tpm-passthrough.x86_64-latest.args +++ b/tests/qemuxml2argvdata/tpm-passthrough.x86_64-latest.args @@ -27,9 +27,9 @@ XDG_CONFIG_HOME=/tmp/lib/domain--1-TPM-VM/.config \ -no-shutdown \ -boot menu=on,strict=on \ -device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \ --tpmdev passthrough,id=tpm-tpm0,path=/dev/fdset/0,cancel-path=/dev/fdset/1 \ --add-fd set=0,fd=1730 \ --add-fd set=1,fd=1731 \ +-add-fd set=1,fd=1730,opaque=tpm0-tpm \ +-add-fd set=2,fd=1731,opaque=tpm0-cancel \ +-tpmdev passthrough,id=tpm-tpm0,path=/dev/fdset/1,cancel-path=/dev/fdset/2 \ -device '{"driver":"tpm-tis","tpmdev":"tpm-tpm0","id":"tpm0"}' \ -audiodev '{"id":"audio1","driver":"none"}' \ -device '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.0","addr":"0x2"}' \ -- 2.34.1
On a Wednesday in 2022, Peter Krempa wrote:
Remove the last code path using hardcoded fdsets.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 140 +++++------------- .../tpm-passthrough-crb.x86_64-latest.args | 6 +- .../tpm-passthrough.x86_64-latest.args | 6 +- 3 files changed, 45 insertions(+), 107 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa