[PATCH 00/11] Clean up arguments for qemuBuildCommandLine
This series cleans up the arguments. Note that patches 10,11 can be dropped if we decide that we want to keep the '-S' argument off the command line bulit for 'virsh domxml-to-native' for qemu definitions. Peter Krempa (11): virConnectDomainXMLToNative: Add note about dynamically configured features docs: drvqemu: Decrease expectations about command line from 'virsh domxml-to-native' qemu: command: Don't hide 'vhost' fds from 'standalone' command line qemuBuildCommandLine: Convert 'standalone' flag to use 'flags' qemu: Store state of FIPS in virQEMUDriver qemuBuildCommandLine: Sanitize debug logging qemuConnectDomainXMLToNative: Refactor cleanup qemuBuildCommandLine: Remove 'driver' argument qemuBuildCommandLine: Inline qemuCheckFips qemuBuildCommandLine: Don't avoid '-S' flag for 'domxml-to-native' conversion qemuBuildCommandLine: Remove 'flags' argument docs/drvqemu.rst | 14 +++++-- src/libvirt-domain.c | 7 ++++ src/qemu/qemu_command.c | 85 ++++++++++++---------------------------- src/qemu/qemu_command.h | 23 ++++------- src/qemu/qemu_conf.h | 1 + src/qemu/qemu_driver.c | 37 +++++++++-------- src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_process.c | 24 +++--------- src/qemu/qemu_process.h | 7 +--- tests/qemuxml2argvtest.c | 10 ++--- 10 files changed, 82 insertions(+), 128 deletions(-) -- 2.35.3
In the qemu driver certain configs such as disk throttling or CPU hotplug is configured by interacting with the monitor at the startup phase of the hypervisor and thus is not part of the "native config" as returned by 'virConnectDomainXMLToNative'. Similarly at least the commandline for qemu contains resources passed via file descriptors which are obviously not part of the returned "native config". Add a paragraph into the documentation outlining that the native configuration might not be completely usable. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt-domain.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index a32630a6e9..e3ced700b8 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -2909,6 +2909,13 @@ virConnectDomainXMLFromNative(virConnectPtr conn, * a native configuration file describing the domain. * The format of the native data is hypervisor dependent. * + * Note that certain hypervisor drivers such as the QEMU driver configure many + * aspects of the domain dynamically via hypervisor APIs and that may not be + * part of the returned native configuration format. Similarly certain resources + * are passed to the hypervisor via file descriptors, which are not part of the + * native configuration returned by this API. Such configuration is thus not + * trivially usable outside of libvirt. + * * Returns a 0 terminated UTF-8 encoded native config datafile, or * NULL in case of error. The caller must free() the returned value. * -- 2.35.3
In the qemu driver certain configs such as disk throttling or CPU hotplug is configured by interacting with the monitor at the startup phase of the hypervisor and thus is not part of the "native config". Add a paragraph into the documentation outlining that the native configuration might not be completely usable. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/drvqemu.rst | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/docs/drvqemu.rst b/docs/drvqemu.rst index 6368a91fb9..e08a60c705 100644 --- a/docs/drvqemu.rst +++ b/docs/drvqemu.rst @@ -443,10 +443,16 @@ Converting from domain XML to QEMU args ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ The ``virsh domxml-to-native`` provides a way to convert a guest description -using libvirt Domain XML, into a set of QEMU args that can be run manually. Note -that currently the command line formatted by libvirt is no longer suited for -manually running qemu as the configuration expects various resources and open -file descriptors passed to the process which are usually prepared by libvirtd. +using libvirt Domain XML, into a set of QEMU args that would be used by libvirt +to start the qemu process. + +Note that currently the command line formatted by libvirt is no longer suited +for manually running qemu as the configuration expects various resources and +open file descriptors passed to the process which are usually prepared by +libvirtd as well as certain features being configured via the monitor. + +The qemu arguments as returned by ``virsh domxml-to-native`` thus are not +trivially usable outside of libvirt. Pass-through of arbitrary qemu commands --------------------------------------- -- 2.35.3
We already format a commandline using FD passing for the tap devices so formatting the 'vhost' file descriptors won't make it any less usable directly. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 15 +++++---------- src/qemu/qemu_command.h | 3 +-- src/qemu/qemu_hotplug.c | 2 +- 3 files changed, 7 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a52ba70066..f0d92a2a10 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8676,8 +8676,7 @@ qemuInterfaceVhostuserConnect(virCommand *cmd, int qemuBuildInterfaceConnect(virDomainObj *vm, virDomainNetDef *net, - virNetDevVPortProfileOp vmop, - bool standalone) + virNetDevVPortProfileOp vmop) { qemuDomainObjPrivate *priv = vm->privateData; @@ -8759,7 +8758,7 @@ qemuBuildInterfaceConnect(virDomainObj *vm, } } - if (vhostfd && !standalone) { + if (vhostfd) { if (qemuInterfaceOpenVhostNet(vm, net) < 0) return -1; } @@ -8775,7 +8774,6 @@ qemuBuildInterfaceCommandLine(virQEMUDriver *driver, virDomainNetDef *net, virQEMUCaps *qemuCaps, virNetDevVPortProfileOp vmop, - bool standalone, size_t *nnicindexes, int **nicindexes) { @@ -8793,7 +8791,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriver *driver, if (qemuDomainValidateActualNetDef(net, qemuCaps) < 0) return -1; - if (qemuBuildInterfaceConnect(vm, net, vmop, standalone) < 0) + if (qemuBuildInterfaceConnect(vm, net, vmop) < 0) return -1; switch (actualType) { @@ -8983,7 +8981,6 @@ qemuBuildNetCommandLine(virQEMUDriver *driver, virCommand *cmd, virQEMUCaps *qemuCaps, virNetDevVPortProfileOp vmop, - bool standalone, size_t *nnicindexes, int **nicindexes) { @@ -8997,8 +8994,7 @@ qemuBuildNetCommandLine(virQEMUDriver *driver, if (qemuBuildInterfaceCommandLine(driver, vm, cmd, net, qemuCaps, vmop, - standalone, nnicindexes, - nicindexes) < 0) + nnicindexes, nicindexes) < 0) goto error; last_good_net = i; @@ -10588,8 +10584,7 @@ qemuBuildCommandLine(virQEMUDriver *driver, if (qemuBuildFilesystemCommandLine(cmd, def, qemuCaps, priv) < 0) return NULL; - if (qemuBuildNetCommandLine(driver, vm, cmd, - qemuCaps, vmop, standalone, + if (qemuBuildNetCommandLine(driver, vm, cmd, qemuCaps, vmop, nnicindexes, nicindexes) < 0) return NULL; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 84877b3d90..9e8eef1e29 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -91,8 +91,7 @@ qemuBuildHostNetProps(virDomainNetDef *net); int qemuBuildInterfaceConnect(virDomainObj *vm, virDomainNetDef *net, - virNetDevVPortProfileOp vmop, - bool standalone); + virNetDevVPortProfileOp vmop); /* Current, best practice */ virJSONValue * diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index cae7b0dd3b..f7fcd9d3f7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1268,7 +1268,7 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver, */ VIR_APPEND_ELEMENT_COPY(vm->def->nets, vm->def->nnets, net); - if (qemuBuildInterfaceConnect(vm, net, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, false) < 0) + if (qemuBuildInterfaceConnect(vm, net, VIR_NETDEV_VPORT_PROFILE_OP_CREATE) < 0) return -1; iface_connected = true; -- 2.35.3
Introduce 'qemuBuildCommandLineFlags' and use it instead of specific flag booleans. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 3 +-- src/qemu/qemu_command.h | 5 ++++- src/qemu/qemu_driver.c | 4 +++- src/qemu/qemu_process.c | 6 ++---- src/qemu/qemu_process.h | 8 ++++---- tests/qemuxml2argvtest.c | 2 +- 6 files changed, 15 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f0d92a2a10..d3b3603fbe 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -10428,7 +10428,6 @@ qemuBuildCommandLine(virQEMUDriver *driver, const char *migrateURI, virDomainMomentObj *snapshot, virNetDevVPortProfileOp vmop, - bool standalone, bool enableFips, size_t *nnicindexes, int **nicindexes, @@ -10476,7 +10475,7 @@ qemuBuildCommandLine(virQEMUDriver *driver, qemuBuildCompatDeprecatedCommandLine(cmd, cfg, def, qemuCaps); - if (!standalone) + if (!(flags & QEMU_BUILD_COMMAND_LINE_CPUS_RUNNING)) virCommandAddArg(cmd, "-S"); /* freeze CPU */ if (qemuBuildMasterKeyCommandLine(cmd, priv) < 0) diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 9e8eef1e29..df46e80067 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -43,12 +43,15 @@ VIR_ENUM_DECL(qemuVideo); VIR_ENUM_DECL(qemuSoundCodec); +typedef enum { + QEMU_BUILD_COMMAND_LINE_CPUS_RUNNING = 1 << 0, +} qemuBuildCommandLineFlags; + virCommand *qemuBuildCommandLine(virQEMUDriver *driver, virDomainObj *vm, const char *migrateURI, virDomainMomentObj *snapshot, virNetDevVPortProfileOp vmop, - bool standalone, bool enableFips, size_t *nnicindexes, int **nicindexes, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 702fd0239c..4f6b295859 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6330,6 +6330,7 @@ static char *qemuConnectDomainXMLToNative(virConnectPtr conn, virQEMUDriver *driver = conn->privateData; virDomainObj *vm = NULL; g_autoptr(virCommand) cmd = NULL; + unsigned int commandlineflags = QEMU_BUILD_COMMAND_LINE_CPUS_RUNNING; char *ret = NULL; size_t i; @@ -6383,7 +6384,8 @@ static char *qemuConnectDomainXMLToNative(virConnectPtr conn, goto cleanup; if (!(cmd = qemuProcessCreatePretendCmdBuild(driver, vm, NULL, - qemuCheckFips(vm), true))) + qemuCheckFips(vm), + commandlineflags))) goto cleanup; ret = virCommandToString(cmd, false); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 07e467d01e..38079ce159 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7450,7 +7450,6 @@ qemuProcessLaunch(virConnectPtr conn, vm, incoming ? "defer" : NULL, snapshot, vmop, - false, qemuCheckFips(vm), &nnicindexes, &nicindexes, 0))) goto cleanup; @@ -7952,7 +7951,7 @@ qemuProcessCreatePretendCmdBuild(virQEMUDriver *driver, virDomainObj *vm, const char *migrateURI, bool enableFips, - bool standalone) + unsigned int flags) { VIR_DEBUG("Building emulator command line"); return qemuBuildCommandLine(driver, @@ -7960,11 +7959,10 @@ qemuProcessCreatePretendCmdBuild(virQEMUDriver *driver, migrateURI, NULL, VIR_NETDEV_VPORT_PROFILE_OP_NO_OP, - standalone, enableFips, NULL, NULL, - 0); + flags); } diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index f81bfd930a..fa3634c64f 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -98,10 +98,10 @@ int qemuProcessCreatePretendCmdPrepare(virQEMUDriver *driver, unsigned int flags); virCommand *qemuProcessCreatePretendCmdBuild(virQEMUDriver *driver, - virDomainObj *vm, - const char *migrateURI, - bool enableFips, - bool standalone); + virDomainObj *vm, + const char *migrateURI, + bool enableFips, + unsigned int flags); int qemuProcessInit(virQEMUDriver *driver, virDomainObj *vm, diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 7708e3ba3e..8d0d4acca9 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -488,7 +488,7 @@ testCompareXMLToArgvCreateArgs(virQEMUDriver *drv, enableFips = false; return qemuProcessCreatePretendCmdBuild(drv, vm, migrateURI, - enableFips, false); + enableFips, 0); } -- 2.35.3
Rather than re-query all the time we can cache the state of FIPS of the host as it will not change during the runtime of the guest. Introduce a 'hostFips' flag to 'virQEMUDriver' and move the code checking the state from 'qemuCheckFips' to 'qemuStateInitialize' and also populate 'hostFips' in qemuxml2argvtest. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 14 ++------------ src/qemu/qemu_conf.h | 1 + src/qemu/qemu_driver.c | 9 +++++++++ tests/qemuxml2argvtest.c | 5 ++++- 4 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d3b3603fbe..3e9db271b1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1787,21 +1787,11 @@ bool qemuCheckFips(virDomainObj *vm) { qemuDomainObjPrivate *priv = vm->privateData; - virQEMUCaps *qemuCaps = priv->qemuCaps; - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_ENABLE_FIPS)) + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_ENABLE_FIPS)) return false; - if (virFileExists("/proc/sys/crypto/fips_enabled")) { - g_autofree char *buf = NULL; - - if (virFileReadAll("/proc/sys/crypto/fips_enabled", 10, &buf) < 0) - return false; - if (STREQ(buf, "1\n")) - return true; - } - - return false; + return priv->driver->hostFips; } diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index c71a666aea..5e752d075e 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -251,6 +251,7 @@ struct _virQEMUDriver { /* Immutable values */ bool privileged; char *embeddedRoot; + bool hostFips; /* FIPS mode is enabled on the host */ /* Immutable pointers. Caller must provide locking */ virStateInhibitCallback inhibitCallback; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4f6b295859..96ca67dfca 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -735,6 +735,15 @@ qemuStateInitialize(bool privileged, if (qemuMigrationDstErrorInit(qemu_driver) < 0) goto error; + /* qemu-5.1 and older requires use of '-enable-fips' flag when the host + * is in FIPS mode. We store whether FIPS is enabled */ + if (virFileExists("/proc/sys/crypto/fips_enabled")) { + g_autofree char *buf = NULL; + + if (virFileReadAll("/proc/sys/crypto/fips_enabled", 10, &buf) > 0) + qemu_driver->hostFips = STREQ(buf, "1\n"); + } + if (privileged) { g_autofree char *channeldir = NULL; diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 8d0d4acca9..385448b57a 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -386,9 +386,12 @@ testCompareXMLToArgvCreateArgs(virQEMUDriver *drv, unsigned int flags) { qemuDomainObjPrivate *priv = vm->privateData; - bool enableFips = !!(flags & FLAG_FIPS_HOST); + bool enableFips; size_t i; + drv->hostFips = flags & FLAG_FIPS_HOST; + enableFips = drv->hostFips; + if (qemuProcessCreatePretendCmdPrepare(drv, vm, migrateURI, VIR_QEMU_PROCESS_START_COLD) < 0) return NULL; -- 2.35.3
Improve the debug log inside 'qemuBuildCommandLine' to include the name from the definition and remove useless data such as the pointer to the qemuDriver object or qemuCaps. Additionally remove the non-specific debug statements: VIR_DEBUG("Building emulator command line"); from the two callers of qemuBuildCommandLine. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 6 ++---- src/qemu/qemu_process.c | 2 -- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3e9db271b1..35ed8ac0ed 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -10431,10 +10431,8 @@ qemuBuildCommandLine(virQEMUDriver *driver, virDomainDef *def = vm->def; virQEMUCaps *qemuCaps = priv->qemuCaps; - VIR_DEBUG("driver=%p def=%p mon=%p " - "qemuCaps=%p migrateURI=%s snapshot=%p vmop=%d flags=0x%x", - driver, def, priv->monConfig, - qemuCaps, migrateURI, snapshot, vmop, flags); + VIR_DEBUG("Building qemu commandline for def=%s(%p) migrateURI=%s snapshot=%p vmop=%d flags=0x%x", + def->name, def, migrateURI, snapshot, vmop, flags); if (qemuBuildCommandLineValidate(driver, def) < 0) return NULL; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 38079ce159..1098c3bf93 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7445,7 +7445,6 @@ qemuProcessLaunch(virConnectPtr conn, if (qemuExtDevicesStart(driver, vm, incoming != NULL) < 0) goto cleanup; - VIR_DEBUG("Building emulator command line"); if (!(cmd = qemuBuildCommandLine(driver, vm, incoming ? "defer" : NULL, @@ -7953,7 +7952,6 @@ qemuProcessCreatePretendCmdBuild(virQEMUDriver *driver, bool enableFips, unsigned int flags) { - VIR_DEBUG("Building emulator command line"); return qemuBuildCommandLine(driver, vm, migrateURI, -- 2.35.3
Automatically free the 'vm' temporary domain object and remove the 'cleanup' label and 'ret' helper variable. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 96ca67dfca..b862208327 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6337,30 +6337,29 @@ static char *qemuConnectDomainXMLToNative(virConnectPtr conn, unsigned int flags) { virQEMUDriver *driver = conn->privateData; - virDomainObj *vm = NULL; + g_autoptr(virDomainObj) vm = NULL; g_autoptr(virCommand) cmd = NULL; unsigned int commandlineflags = QEMU_BUILD_COMMAND_LINE_CPUS_RUNNING; - char *ret = NULL; size_t i; virCheckFlags(0, NULL); if (virConnectDomainXMLToNativeEnsureACL(conn) < 0) - goto cleanup; + return NULL; if (STRNEQ(format, QEMU_CONFIG_FORMAT_ARGV)) { virReportError(VIR_ERR_INVALID_ARG, _("unsupported config type %s"), format); - goto cleanup; + return NULL; } if (!(vm = virDomainObjNew(driver->xmlopt))) - goto cleanup; + return NULL; if (!(vm->def = virDomainDefParseString(xmlData, driver->xmlopt, NULL, VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_ABI_UPDATE))) - goto cleanup; + return NULL; /* Since we're just exporting args, we can't do bridge/network/direct * setups, since libvirt will normally create TAP/macvtap devices @@ -6372,7 +6371,7 @@ static char *qemuConnectDomainXMLToNative(virConnectPtr conn, virDomainNetDef *newNet = virDomainNetDefNew(driver->xmlopt); if (!newNet) - goto cleanup; + return NULL; newNet->type = VIR_DOMAIN_NET_TYPE_ETHERNET; newNet->info.bootIndex = net->info.bootIndex; @@ -6387,21 +6386,17 @@ static char *qemuConnectDomainXMLToNative(virConnectPtr conn, if (qemuProcessCreatePretendCmdPrepare(driver, vm, NULL, VIR_QEMU_PROCESS_START_COLD) < 0) - goto cleanup; + return NULL; if (qemuConnectDomainXMLToNativePrepareHost(vm) < 0) - goto cleanup; + return NULL; if (!(cmd = qemuProcessCreatePretendCmdBuild(driver, vm, NULL, qemuCheckFips(vm), commandlineflags))) - goto cleanup; - - ret = virCommandToString(cmd, false); + return NULL; - cleanup: - virObjectUnref(vm); - return ret; + return virCommandToString(cmd, false); } -- 2.35.3
We pass 'vm' which already contains it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 6 +++--- src/qemu/qemu_command.h | 17 ++++++++--------- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_process.c | 9 +++------ src/qemu/qemu_process.h | 3 +-- tests/qemuxml2argvtest.c | 2 +- 6 files changed, 17 insertions(+), 22 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 35ed8ac0ed..c48575f78c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -10413,8 +10413,7 @@ qemuBuildCompatDeprecatedCommandLine(virCommand *cmd, * for a given virtual machine. */ virCommand * -qemuBuildCommandLine(virQEMUDriver *driver, - virDomainObj *vm, +qemuBuildCommandLine(virDomainObj *vm, const char *migrateURI, virDomainMomentObj *snapshot, virNetDevVPortProfileOp vmop, @@ -10426,8 +10425,9 @@ qemuBuildCommandLine(virQEMUDriver *driver, size_t i; char uuid[VIR_UUID_STRING_BUFLEN]; g_autoptr(virCommand) cmd = NULL; - g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); qemuDomainObjPrivate *priv = vm->privateData; + virQEMUDriver *driver = priv->driver; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); virDomainDef *def = vm->def; virQEMUCaps *qemuCaps = priv->qemuCaps; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index df46e80067..db5b532cb8 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -47,15 +47,14 @@ typedef enum { QEMU_BUILD_COMMAND_LINE_CPUS_RUNNING = 1 << 0, } qemuBuildCommandLineFlags; -virCommand *qemuBuildCommandLine(virQEMUDriver *driver, - virDomainObj *vm, - const char *migrateURI, - virDomainMomentObj *snapshot, - virNetDevVPortProfileOp vmop, - bool enableFips, - size_t *nnicindexes, - int **nicindexes, - unsigned int flags); +virCommand *qemuBuildCommandLine(virDomainObj *vm, + const char *migrateURI, + virDomainMomentObj *snapshot, + virNetDevVPortProfileOp vmop, + bool enableFips, + size_t *nnicindexes, + int **nicindexes, + unsigned int flags); /* Generate the object properties for pr-manager */ virJSONValue *qemuBuildPRManagerInfoProps(virStorageSource *src); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b862208327..8097dcf144 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6391,7 +6391,7 @@ static char *qemuConnectDomainXMLToNative(virConnectPtr conn, if (qemuConnectDomainXMLToNativePrepareHost(vm) < 0) return NULL; - if (!(cmd = qemuProcessCreatePretendCmdBuild(driver, vm, NULL, + if (!(cmd = qemuProcessCreatePretendCmdBuild(vm, NULL, qemuCheckFips(vm), commandlineflags))) return NULL; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1098c3bf93..fbad1254a0 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7445,8 +7445,7 @@ qemuProcessLaunch(virConnectPtr conn, if (qemuExtDevicesStart(driver, vm, incoming != NULL) < 0) goto cleanup; - if (!(cmd = qemuBuildCommandLine(driver, - vm, + if (!(cmd = qemuBuildCommandLine(vm, incoming ? "defer" : NULL, snapshot, vmop, qemuCheckFips(vm), @@ -7946,14 +7945,12 @@ qemuProcessCreatePretendCmdPrepare(virQEMUDriver *driver, virCommand * -qemuProcessCreatePretendCmdBuild(virQEMUDriver *driver, - virDomainObj *vm, +qemuProcessCreatePretendCmdBuild(virDomainObj *vm, const char *migrateURI, bool enableFips, unsigned int flags) { - return qemuBuildCommandLine(driver, - vm, + return qemuBuildCommandLine(vm, migrateURI, NULL, VIR_NETDEV_VPORT_PROFILE_OP_NO_OP, diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index fa3634c64f..9856da3bb5 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -97,8 +97,7 @@ int qemuProcessCreatePretendCmdPrepare(virQEMUDriver *driver, const char *migrateURI, unsigned int flags); -virCommand *qemuProcessCreatePretendCmdBuild(virQEMUDriver *driver, - virDomainObj *vm, +virCommand *qemuProcessCreatePretendCmdBuild(virDomainObj *vm, const char *migrateURI, bool enableFips, unsigned int flags); diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 385448b57a..2385fa1209 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -490,7 +490,7 @@ testCompareXMLToArgvCreateArgs(virQEMUDriver *drv, if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_ENABLE_FIPS)) enableFips = false; - return qemuProcessCreatePretendCmdBuild(drv, vm, migrateURI, + return qemuProcessCreatePretendCmdBuild(vm, migrateURI, enableFips, 0); } -- 2.35.3
Now that we store the state of the host FIPS mode setting in the qemu driver object, we don't need to outsource the logic into 'qemuCheckFips'. Additionally since we no longer support very old qemu's which would not yet have --enable-fips we can drop the part of the comment about very old qemus. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 41 +++++++++++++--------------------------- src/qemu/qemu_command.h | 5 ----- src/qemu/qemu_driver.c | 4 +--- src/qemu/qemu_process.c | 3 --- src/qemu/qemu_process.h | 1 - tests/qemuxml2argvtest.c | 9 +-------- 6 files changed, 15 insertions(+), 48 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c48575f78c..8705f0018c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1769,32 +1769,6 @@ qemuDiskConfigBlkdeviotuneEnabled(virDomainDiskDef *disk) } -/* QEMU 1.2 and later have a binary flag -enable-fips that must be - * used for VNC auth to obey FIPS settings; but the flag only - * exists on Linux, and with no way to probe for it via QMP. Our - * solution: if FIPS mode is required, then unconditionally use - * the flag, regardless of qemu version, for the following matrix: - * - * old QEMU new QEMU - * FIPS enabled doesn't start VNC auth disabled - * FIPS disabled/missing VNC auth enabled VNC auth enabled - * - * In QEMU 5.2.0, use of -enable-fips was deprecated. In scenarios - * where FIPS is required, QEMU must be built against libgcrypt - * which automatically enforces FIPS compliance. - */ -bool -qemuCheckFips(virDomainObj *vm) -{ - qemuDomainObjPrivate *priv = vm->privateData; - - if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_ENABLE_FIPS)) - return false; - - return priv->driver->hostFips; -} - - /** * qemuDiskBusIsSD: * @bus: disk bus @@ -10417,7 +10391,6 @@ qemuBuildCommandLine(virDomainObj *vm, const char *migrateURI, virDomainMomentObj *snapshot, virNetDevVPortProfileOp vmop, - bool enableFips, size_t *nnicindexes, int **nicindexes, unsigned int flags) @@ -10478,7 +10451,19 @@ qemuBuildCommandLine(virDomainObj *vm, if (qemuBuildPflashBlockdevCommandLine(cmd, priv) < 0) return NULL; - if (enableFips) + /* QEMU 1.2 and later have a binary flag -enable-fips that must be + * used for VNC auth to obey FIPS settings; but the flag only + * exists on Linux, and with no way to probe for it via QMP. Our + * solution: if FIPS mode is required, then unconditionally use the flag. + * + * In QEMU 5.2.0, use of -enable-fips was deprecated. In scenarios + * where FIPS is required, QEMU must be built against libgcrypt + * which automatically enforces FIPS compliance. + * + * Note this is the only use of driver->hostFips. + */ + if (driver->hostFips && + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_ENABLE_FIPS)) virCommandAddArg(cmd, "-enable-fips"); if (qemuBuildMachineCommandLine(cmd, cfg, def, qemuCaps, priv) < 0) diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index db5b532cb8..72b0401c7b 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -51,7 +51,6 @@ virCommand *qemuBuildCommandLine(virDomainObj *vm, const char *migrateURI, virDomainMomentObj *snapshot, virNetDevVPortProfileOp vmop, - bool enableFips, size_t *nnicindexes, int **nicindexes, unsigned int flags); @@ -214,10 +213,6 @@ int qemuGetDriveSourceString(virStorageSource *src, bool qemuDiskConfigBlkdeviotuneEnabled(virDomainDiskDef *disk); - -bool -qemuCheckFips(virDomainObj *vm); - virJSONValue *qemuBuildHotpluggableCPUProps(const virDomainVcpuDef *vcpu) ATTRIBUTE_NONNULL(1); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8097dcf144..2ca264d9f9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6391,9 +6391,7 @@ static char *qemuConnectDomainXMLToNative(virConnectPtr conn, if (qemuConnectDomainXMLToNativePrepareHost(vm) < 0) return NULL; - if (!(cmd = qemuProcessCreatePretendCmdBuild(vm, NULL, - qemuCheckFips(vm), - commandlineflags))) + if (!(cmd = qemuProcessCreatePretendCmdBuild(vm, NULL, commandlineflags))) return NULL; return virCommandToString(cmd, false); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index fbad1254a0..d50cf2e6be 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7448,7 +7448,6 @@ qemuProcessLaunch(virConnectPtr conn, if (!(cmd = qemuBuildCommandLine(vm, incoming ? "defer" : NULL, snapshot, vmop, - qemuCheckFips(vm), &nnicindexes, &nicindexes, 0))) goto cleanup; @@ -7947,14 +7946,12 @@ qemuProcessCreatePretendCmdPrepare(virQEMUDriver *driver, virCommand * qemuProcessCreatePretendCmdBuild(virDomainObj *vm, const char *migrateURI, - bool enableFips, unsigned int flags) { return qemuBuildCommandLine(vm, migrateURI, NULL, VIR_NETDEV_VPORT_PROFILE_OP_NO_OP, - enableFips, NULL, NULL, flags); diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 9856da3bb5..2387fcdcdc 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -99,7 +99,6 @@ int qemuProcessCreatePretendCmdPrepare(virQEMUDriver *driver, virCommand *qemuProcessCreatePretendCmdBuild(virDomainObj *vm, const char *migrateURI, - bool enableFips, unsigned int flags); int qemuProcessInit(virQEMUDriver *driver, diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 2385fa1209..50aea47a68 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -386,11 +386,9 @@ testCompareXMLToArgvCreateArgs(virQEMUDriver *drv, unsigned int flags) { qemuDomainObjPrivate *priv = vm->privateData; - bool enableFips; size_t i; drv->hostFips = flags & FLAG_FIPS_HOST; - enableFips = drv->hostFips; if (qemuProcessCreatePretendCmdPrepare(drv, vm, migrateURI, VIR_QEMU_PROCESS_START_COLD) < 0) @@ -486,12 +484,7 @@ testCompareXMLToArgvCreateArgs(virQEMUDriver *drv, } } - /* we can't use qemuCheckFips() directly as it queries host state */ - if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_ENABLE_FIPS)) - enableFips = false; - - return qemuProcessCreatePretendCmdBuild(vm, migrateURI, - enableFips, 0); + return qemuProcessCreatePretendCmdBuild(vm, migrateURI, 0); } -- 2.35.3
The commandline generated from our XML->native convertor is the majority of cases not usable without libvirt anyways and the situation will not improve any more. As of such there's no much utility of avoiding the use of stopped CPUs flag in such case. Remove the QEMU_BUILD_COMMAND_LINE_CPUS_RUNNING flag and the associated logic. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 3 +-- src/qemu/qemu_command.h | 4 ---- src/qemu/qemu_driver.c | 3 +-- 3 files changed, 2 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8705f0018c..25b8dcb10a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -10436,8 +10436,7 @@ qemuBuildCommandLine(virDomainObj *vm, qemuBuildCompatDeprecatedCommandLine(cmd, cfg, def, qemuCaps); - if (!(flags & QEMU_BUILD_COMMAND_LINE_CPUS_RUNNING)) - virCommandAddArg(cmd, "-S"); /* freeze CPU */ + virCommandAddArg(cmd, "-S"); /* freeze CPUs during startup */ if (qemuBuildMasterKeyCommandLine(cmd, priv) < 0) return NULL; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 72b0401c7b..087ab52ee5 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -43,10 +43,6 @@ VIR_ENUM_DECL(qemuVideo); VIR_ENUM_DECL(qemuSoundCodec); -typedef enum { - QEMU_BUILD_COMMAND_LINE_CPUS_RUNNING = 1 << 0, -} qemuBuildCommandLineFlags; - virCommand *qemuBuildCommandLine(virDomainObj *vm, const char *migrateURI, virDomainMomentObj *snapshot, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2ca264d9f9..5f4990b09c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6339,7 +6339,6 @@ static char *qemuConnectDomainXMLToNative(virConnectPtr conn, virQEMUDriver *driver = conn->privateData; g_autoptr(virDomainObj) vm = NULL; g_autoptr(virCommand) cmd = NULL; - unsigned int commandlineflags = QEMU_BUILD_COMMAND_LINE_CPUS_RUNNING; size_t i; virCheckFlags(0, NULL); @@ -6391,7 +6390,7 @@ static char *qemuConnectDomainXMLToNative(virConnectPtr conn, if (qemuConnectDomainXMLToNativePrepareHost(vm) < 0) return NULL; - if (!(cmd = qemuProcessCreatePretendCmdBuild(vm, NULL, commandlineflags))) + if (!(cmd = qemuProcessCreatePretendCmdBuild(vm, NULL, 0))) return NULL; return virCommandToString(cmd, false); -- 2.35.3
The flags are not used for anything. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 7 +++---- src/qemu/qemu_command.h | 3 +-- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_process.c | 8 +++----- src/qemu/qemu_process.h | 3 +-- tests/qemuxml2argvtest.c | 2 +- 6 files changed, 10 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 25b8dcb10a..38bf4ebea5 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -10392,8 +10392,7 @@ qemuBuildCommandLine(virDomainObj *vm, virDomainMomentObj *snapshot, virNetDevVPortProfileOp vmop, size_t *nnicindexes, - int **nicindexes, - unsigned int flags) + int **nicindexes) { size_t i; char uuid[VIR_UUID_STRING_BUFLEN]; @@ -10404,8 +10403,8 @@ qemuBuildCommandLine(virDomainObj *vm, virDomainDef *def = vm->def; virQEMUCaps *qemuCaps = priv->qemuCaps; - VIR_DEBUG("Building qemu commandline for def=%s(%p) migrateURI=%s snapshot=%p vmop=%d flags=0x%x", - def->name, def, migrateURI, snapshot, vmop, flags); + VIR_DEBUG("Building qemu commandline for def=%s(%p) migrateURI=%s snapshot=%p vmop=%d", + def->name, def, migrateURI, snapshot, vmop); if (qemuBuildCommandLineValidate(driver, def) < 0) return NULL; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 087ab52ee5..2bb4984656 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -48,8 +48,7 @@ virCommand *qemuBuildCommandLine(virDomainObj *vm, virDomainMomentObj *snapshot, virNetDevVPortProfileOp vmop, size_t *nnicindexes, - int **nicindexes, - unsigned int flags); + int **nicindexes); /* Generate the object properties for pr-manager */ virJSONValue *qemuBuildPRManagerInfoProps(virStorageSource *src); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5f4990b09c..2533e22c38 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6390,7 +6390,7 @@ static char *qemuConnectDomainXMLToNative(virConnectPtr conn, if (qemuConnectDomainXMLToNativePrepareHost(vm) < 0) return NULL; - if (!(cmd = qemuProcessCreatePretendCmdBuild(vm, NULL, 0))) + if (!(cmd = qemuProcessCreatePretendCmdBuild(vm, NULL))) return NULL; return virCommandToString(cmd, false); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index d50cf2e6be..cabec71c1e 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7448,7 +7448,7 @@ qemuProcessLaunch(virConnectPtr conn, if (!(cmd = qemuBuildCommandLine(vm, incoming ? "defer" : NULL, snapshot, vmop, - &nnicindexes, &nicindexes, 0))) + &nnicindexes, &nicindexes))) goto cleanup; if (incoming && incoming->fd != -1) @@ -7945,16 +7945,14 @@ qemuProcessCreatePretendCmdPrepare(virQEMUDriver *driver, virCommand * qemuProcessCreatePretendCmdBuild(virDomainObj *vm, - const char *migrateURI, - unsigned int flags) + const char *migrateURI) { return qemuBuildCommandLine(vm, migrateURI, NULL, VIR_NETDEV_VPORT_PROFILE_OP_NO_OP, NULL, - NULL, - flags); + NULL); } diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 2387fcdcdc..9866d2bf35 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -98,8 +98,7 @@ int qemuProcessCreatePretendCmdPrepare(virQEMUDriver *driver, unsigned int flags); virCommand *qemuProcessCreatePretendCmdBuild(virDomainObj *vm, - const char *migrateURI, - unsigned int flags); + const char *migrateURI); int qemuProcessInit(virQEMUDriver *driver, virDomainObj *vm, diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 50aea47a68..afb8aa1aea 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -484,7 +484,7 @@ testCompareXMLToArgvCreateArgs(virQEMUDriver *drv, } } - return qemuProcessCreatePretendCmdBuild(vm, migrateURI, 0); + return qemuProcessCreatePretendCmdBuild(vm, migrateURI); } -- 2.35.3
On Mon, May 16, 2022 at 05:12:30PM +0200, Peter Krempa wrote:
This series cleans up the arguments.
Note that patches 10,11 can be dropped if we decide that we want to keep the '-S' argument off the command line bulit for 'virsh domxml-to-native' for qemu definitions.
Peter Krempa (11): virConnectDomainXMLToNative: Add note about dynamically configured features docs: drvqemu: Decrease expectations about command line from 'virsh domxml-to-native' qemu: command: Don't hide 'vhost' fds from 'standalone' command line qemuBuildCommandLine: Convert 'standalone' flag to use 'flags' qemu: Store state of FIPS in virQEMUDriver qemuBuildCommandLine: Sanitize debug logging qemuConnectDomainXMLToNative: Refactor cleanup qemuBuildCommandLine: Remove 'driver' argument qemuBuildCommandLine: Inline qemuCheckFips qemuBuildCommandLine: Don't avoid '-S' flag for 'domxml-to-native' conversion qemuBuildCommandLine: Remove 'flags' argument
Reviewed-by: Pavel Hrdina <phrdina@redhat.com> I'm OK with patches 10,11 as well so unless somebody else objects apply the RB to these patches too.
participants (2)
-
Pavel Hrdina -
Peter Krempa