Re: [PATCH] virsh: Introduce new hypervisor-cpu-models command
On a Thursday in 2025, Collin Walling wrote: From: David Judkovics Add new virsh command 'hypervisor-cpu-models'. Command pulls from the existing domcapabilities XML and uses xpath to parse CPU model strings. By default, only models reported as usable by the hypervisor on the host system are printed. User may specify "--all" to also print models which are not supported on the host. Signed-off-by: David Judkovics Signed-off-by: Boris Fiuczynski Signed-off-by: Collin Walling --- This is a continuation of a previous series found here: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/I4YKUBDJDMLFJ6767ZPP2NP6Y4COEMBW/ --- docs/manpages/virsh.rst | 24 + tools/virsh-host.c | 74 + 2 files changed, 98 insertions(+) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index baced15dec..48b667736d 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -1034,6 +1034,30 @@ listed in the XML description. If *--migratable* is specified, features that block migration will not be included in the resulting CPU. +hypervisor-cpu-models +- + +**Syntax:** + +:: + + hypervisor-cpu-models [virttype] [emulator] [arch] [machine] [--all] + The actualy syntax for this (as reported by virsh hypervisor-cpu-models --help) is: hypervisor-cpu-models [--virttype ] [--emulator ] [--arch ] [--machine ] [--all] i.e. specifying the option name is mandatory +Print the list of CPU models known by the hypervisor for the specified architecture. +It is not guaranteed that a listed CPU will run on the host. To determine CPU +model compatibility with the host, see ``virsh hypervisor-cpu-baseline`` and +``virsh hypervisor-cpu-compare``. + +The *virttype* option specifies the virtualization type (usable in the 'type' +attribute of the top level element from the domain XML). *emulator* +specifies the path to the emulator, *arch* specifies the CPU architecture, and +*machine* specifies the machine type. + +By default, only the models that are claimed to be "usable" by the hypervisor +on the host are reported. The option *--all* will report every CPU model known +to the hypervisor, including ones that are not supported on the hypervisor (e.g. +newer generation models). + DOMAIN COMMANDS === diff --git a/tools/virsh-host.c b/tools/virsh-host.c index 9e8f542c96..2884067bbb 100644 --- a/tools/virsh-host.c +++ b/tools/virsh-host.c @@ -1766,6 +1766,74 @@ cmdHypervisorCPUBaseline(vshControl *ctl, } +/* + * "hypervisor-cpu-models" command + */ +static const vshCmdInfo info_hypervisor_cpu_models = { +.help = N_("Hypervisor reported CPU models"), +.desc = N_("Get the CPU models reported by the hypervisor."), +}; + +static const vshCmdOptDef opts_hypervisor_cpu_models[] = { +{.name = "virttype", + .type = VSH_OT_STRING, + .completer = virshDomainVirtTypeCompleter, + .help = N_("virtualization type (/domain/@type)"), +}, +{.name = "emulator", + .type = VSH_OT_STRING, + .unwanted_positional = true, The use of this option is discouraged, see: commit a67f737ddfc59b0f5b3905bd1c0935ced82de3d0 Author: Peter Krempa AuthorDate: 2024-03-14 17:17:19 +0100 Commit: Peter Krempa CommitDate: 2024-04-02 14:24:30 +0200 virt-admin: Annodate 'unwanted_positional' arguments https://gitlab.com/libvirt/libvirt/-/commit/a67f737ddfc59b0f5b3905bd1c0935ced82de3d0 + .help = N_("path to emulator binary (/domain/devices/emulator)"), +}, +{.name = "arch", + .type = VSH_OT_STRING, + .completer = virshArchCompleter, + .help = N_("CPU architecture (/domain/os/type/@arch)"), +}, +{.name = "machine", + .type = VSH_OT_STRING, + .help = N_("machine type (/domain/os/type/@machine)"), +}, +{.name = "all", + .type = VSH_OT_BOOL, + .help = N_("include all CPU models known to the hypervisor for the architecture") +}, +{.name = NULL} +}; + +static bool +cmdHypervisorCPUModelNames(vshControl *ctl, + const vshCmd *cmd) +{ +g_autofree char *caps_xml = NULL; +const char *virttype = NULL; +const char *emulator = NULL; +const char *arch = NULL; +const char *machine = NULL; +const char *xpath_all = "//cpu//model[@usable]/text()"; +const char *xpath_usable = "//cpu//model[@usable='yes']/text()"; +virshControl *priv = ctl->privData; + if (vshCommandOptBool(cmd, "all")) xpath = "//cpu//model[@usable]/text()"; else xpath = "//cpu//model[@usable='yes']/text()"; gets rid of the ternary operator and groups the command line processing together. +if (vshCommandOptString(ctl, cmd, "virttype", &virttype) < 0 || +vshCommandOptString(ctl, cmd, "emulator", &emulator) < 0 || +vshCommandOptString(ctl, cmd, "arch", &arch) < 0 || +vshCommandOptString(ctl, cmd, "machine", &machine) < 0) +return false; + +caps_xml = virConnectGetDo
[PATCH v9 16/17] virsh: Add support for throttle group operations
From: Chun Feng Wu Implement new throttle cmds * Add new virsh cmds: domthrottlegroupset, domthrottlegrouplist, domthrottlegroupinfo, domthrottlegroupdel * Add doc for new cmds at docs/manpages/virsh.rst * Add cmd helper "virshDomainThrottleGroupCompleter", which is used by domthrottlegroupset, domthrottlegroupinfo, domthrottlegroupdel Signed-off-by: Chun Feng Wu * Update of code documentation comments. * Reimplement Get throttle group from XML. Signed-off-by: Harikumar Rajkumar a * Fixed memleaks * Rewrote getter to avoid extra copies * Simplified name extractor Reviewed-by: Peter Krempa Signed-off-by: Peter Krempa --- docs/manpages/virsh.rst| 134 tools/virsh-completer-domain.c | 45 tools/virsh-completer-domain.h | 9 + tools/virsh-domain.c | 368 - 4 files changed, 555 insertions(+), 1 deletion(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index baced15dec..621c02fdbd 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -1139,6 +1139,140 @@ given, but *--current* is exclusive. For querying only one of *--live*, is different depending on hypervisor. +domthrottlegroupset +--- + +**Syntax:** + +:: + + domthrottlegroupset domain group-name [[--config] [--live] | [--current]] + [[total-bytes-sec] | [read-bytes-sec] [write-bytes-sec]] + [[total-iops-sec] | [read-iops-sec] [write-iops-sec]] + [[total-bytes-sec-max] | [read-bytes-sec-max] [write-bytes-sec-max]] + [[total-iops-sec-max] | [read-iops-sec-max] [write-iops-sec-max]] + [[total-bytes-sec-max-length] | + [read-bytes-sec-max-length] [write-bytes-sec-max-length]] + [[total-iops-sec-max-length] | + [read-iops-sec-max-length] [write-iops-sec-max-length]] + [size-iops-sec] + +Add or update a throttle group against specific *domain*. +*group-name* specifies a unique throttle group name, which defines limit, and +will be referenced by drives. + +If no limit is specified, default them as all zeros, which will fail, +Otherwise, set limits with these flags: +*--total-bytes-sec* specifies total throughput limit as a scaled integer, the +default being bytes per second if no suffix is specified. +*--read-bytes-sec* specifies read throughput limit as a scaled integer, the +default being bytes per second if no suffix is specified. +*--write-bytes-sec* specifies write throughput limit as a scaled integer, the +default being bytes per second if no suffix is specified. +*--total-iops-sec* specifies total I/O operations limit per second. +*--read-iops-sec* specifies read I/O operations limit per second. +*--write-iops-sec* specifies write I/O operations limit per second. +*--total-bytes-sec-max* specifies maximum total throughput limit as a scaled +integer, the default being bytes per second if no suffix is specified +*--read-bytes-sec-max* specifies maximum read throughput limit as a scaled +integer, the default being bytes per second if no suffix is specified. +*--write-bytes-sec-max* specifies maximum write throughput limit as a scaled +integer, the default being bytes per second if no suffix is specified. +*--total-iops-sec-max* specifies maximum total I/O operations limit per second. +*--read-iops-sec-max* specifies maximum read I/O operations limit per second. +*--write-iops-sec-max* specifies maximum write I/O operations limit per second. +*--total-bytes-sec-max-length* specifies duration in seconds to allow maximum +total throughput limit. +*--read-bytes-sec-max-length* specifies duration in seconds to allow maximum +read throughput limit. +*--write-bytes-sec-max-length* specifies duration in seconds to allow maximum +write throughput limit. +*--total-iops-sec-max-length* specifies duration in seconds to allow maximum +total I/O operations limit. +*--read-iops-sec-max-length* specifies duration in seconds to allow maximum +read I/O operations limit. +*--write-iops-sec-max-length* specifies duration in seconds to allow maximum +write I/O operations limit. +*--size-iops-sec* specifies size I/O operations limit per second. + +Bytes and iops values are independent, but setting only one value (such +as --read-bytes-sec) resets the other two in that category to unlimited. +An explicit 0 also clears any limit. A non-zero value for a given total +cannot be mixed with non-zero values for read or write. + +It is up to the hypervisor to determine how to handle the length values. +For the QEMU hypervisor, if an I/O limit value or maximum value is set, +then the default value of 1 second will be displayed. Supplying a 0 will +reset the value back to the default. + +If *--live* is specified, affect a running guest. +If *--config* is specified, affect the next start of a persistent guest. +If *--current* is specified, it is equivalent to either *--live* or +*--config*, depending on the current state of the guest. +When setting the disk io parameters both *--live* and *--config* +are sp
Re: [PATCH 0/5] qemu: Two block job fixes
On Mon, Mar 17, 2025 at 06:27:27PM +0100, Peter Krempa via Devel wrote: > Peter Krempa (5): > qemu: monitor: Wire up 'replaces' attribute for 'blockdev-mirror' > qemu: Do not replace filter nodes with virDomainBlockCopy > qemu: Remove return value from 'qemuHotplugRemoveManagedPR' > qemuDomainChangeEjectableMedia: Separate rollback and success code > paths > qemuHotplugRemoveManagedPR: Integrate check whether removal is needed Reviewed-by: Pavel Hrdina signature.asc Description: PGP signature
Re: [PATCH V4 11/18] qemu: Apply migration parameters in qemuMigrationDstRun
On 3/19/25 09:45, Daniel P. Berrangé wrote: On Wed, Mar 05, 2025 at 03:48:20PM -0700, Jim Fehlig via Devel wrote: Similar to qemuMigrationSrcRun, apply migration parameters in qemuMigrationDstRun. This allows callers to create customized migration parameters, but delegates their application to the function performing the migration. Signed-off-by: Jim Fehlig --- src/qemu/qemu_migration.c | 16 ++-- src/qemu/qemu_migration.h | 5 - src/qemu/qemu_process.c | 2 +- 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index f692986056..329b5f61d4 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2427,7 +2427,10 @@ qemuMigrationDstGetURI(const char *migrateFrom, int qemuMigrationDstRun(virDomainObj *vm, const char *uri, -virDomainAsyncJob asyncJob) +virDomainAsyncJob asyncJob, +qemuMigrationParams *migParams, +unsigned int flags) + { virTristateBool exitOnError = VIR_TRISTATE_BOOL_ABSENT; qemuDomainObjPrivate *priv = vm->privateData; @@ -2435,6 +2438,10 @@ qemuMigrationDstRun(virDomainObj *vm, VIR_DEBUG("Setting up incoming migration with URI %s", uri); +if (migParams && qemuMigrationParamsApply(vm, asyncJob, + migParams, flags) < 0) +return -1; + /* Ask QEMU not to exit on failure during incoming migration (if supported) * so that we can properly check and report error during Finish phase. */ @@ -3368,10 +3375,6 @@ qemuMigrationDstPrepareActive(virQEMUDriver *driver, goto error; } -if (qemuMigrationParamsApply(vm, VIR_ASYNC_JOB_MIGRATION_IN, - migParams, flags) < 0) -goto error; - So semantically, we now apply the dst migration parameters /after/ starting the NBD server, instead of before. This seems like it ought to be safe, as migration is independent of the NBD block layer. Good point. I took another look and agree it should be fine. The interesting scenario is failure to apply the migration params. Prior to this patch the NBD server would not have been started. After the patch it would be running. Shouldn't be a problem though, since the code already handles other misc failures after starting the NBD server. Regards, Jim
Re: [PATCH V4 08/18] qemu: Add helper function for creating save image fd
On 3/19/25 06:49, Daniel P. Berrangé wrote: On Wed, Mar 05, 2025 at 03:48:17PM -0700, Jim Fehlig via Devel wrote: Move the code in qemuSaveImageCreate that opens, labels, and wraps the save image fd to a helper function, providing more flexibility for upcoming mapped-ram support. Signed-off-by: Jim Fehlig --- src/qemu/qemu_saveimage.c | 65 +++ 1 file changed, 45 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index 29b4e39879..e3f6a0ad0f 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -425,6 +425,50 @@ qemuSaveImageDecompressionStop(virCommand *cmd, } +static int +qemuSaveImageCreateFd(virQEMUDriver *driver, + virDomainObj *vm, + const char *path, + virFileWrapperFd *wrapperFd, Doesn't this need to be virFileWrapperFd **, otherwise... + bool *needUnlink, + unsigned int flags) +{ +g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); +int ret = -1; +VIR_AUTOCLOSE fd = -1; +int directFlag = 0; +unsigned int wrapperFlags = VIR_FILE_WRAPPER_NON_BLOCKING; + +if (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) { +wrapperFlags |= VIR_FILE_WRAPPER_BYPASS_CACHE; +directFlag = virFileDirectFdFlag(); +if (directFlag < 0) { +virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("bypass cache unsupported by this system")); +return -1; +} +} + +fd = virQEMUFileOpenAs(cfg->user, cfg->group, false, path, + O_WRONLY | O_TRUNC | O_CREAT | directFlag, + needUnlink); + +if (fd < 0) +return -1; + +if (qemuSecuritySetImageFDLabel(driver->securityManager, vm->def, fd) < 0) +return -1; + +if (!(wrapperFd = virFileWrapperFdNew(&fd, path, wrapperFlags))) +return -1; this assignment won't be visible to the caller Opps, which makes qemuDomainFileWrapperFDClose a NOP in all cases. I've applied the below diff locally. Any need to respin this series? Regards, Jim diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index e3f6a0ad0f..7ab44edcdc 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -429,7 +429,7 @@ static int qemuSaveImageCreateFd(virQEMUDriver *driver, virDomainObj *vm, const char *path, - virFileWrapperFd *wrapperFd, + virFileWrapperFd **wrapperFd, bool *needUnlink, unsigned int flags) { @@ -459,7 +459,7 @@ qemuSaveImageCreateFd(virQEMUDriver *driver, if (qemuSecuritySetImageFDLabel(driver->securityManager, vm->def, fd) < 0) return -1; -if (!(wrapperFd = virFileWrapperFdNew(&fd, path, wrapperFlags))) +if (!(*wrapperFd = virFileWrapperFdNew(&fd, path, wrapperFlags))) return -1; ret = fd; @@ -488,7 +488,7 @@ qemuSaveImageCreate(virQEMUDriver *driver, virFileWrapperFd *wrapperFd = NULL; /* Obtain the file handle. */ -fd = qemuSaveImageCreateFd(driver, vm, path, wrapperFd, &needUnlink, flags); +fd = qemuSaveImageCreateFd(driver, vm, path, &wrapperFd, &needUnlink, flags); if (fd < 0) goto cleanup;
Re: [PATCH] virsh: Introduce new hypervisor-cpu-models command
On 3/18/25 1:13 PM, Ján Tomko wrote: > On a Thursday in 2025, Collin Walling wrote: >> From: David Judkovics >> >> Add new virsh command 'hypervisor-cpu-models'. Command pulls from the >> existing domcapabilities XML and uses xpath to parse CPU model strings. >> By default, only models reported as usable by the hypervisor on the >> host system are printed. User may specify "--all" to also print >> models which are not supported on the host. >> >> Signed-off-by: David Judkovics >> Signed-off-by: Boris Fiuczynski >> Signed-off-by: Collin Walling >> --- >> >> This is a continuation of a previous series found here: >> >> https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/I4YKUBDJDMLFJ6767ZPP2NP6Y4COEMBW/ >> >> --- >> docs/manpages/virsh.rst | 24 + >> tools/virsh-host.c | 74 + >> 2 files changed, 98 insertions(+) >> >> diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst >> index baced15dec..48b667736d 100644 >> --- a/docs/manpages/virsh.rst >> +++ b/docs/manpages/virsh.rst >> @@ -1034,6 +1034,30 @@ listed in the XML description. If *--migratable* is >> specified, features that >> block migration will not be included in the resulting CPU. >> >> >> +hypervisor-cpu-models >> +- >> + >> +**Syntax:** >> + >> +:: >> + >> + hypervisor-cpu-models [virttype] [emulator] [arch] [machine] [--all] >> + > > The actualy syntax for this (as reported by virsh hypervisor-cpu-models > --help) is: > > hypervisor-cpu-models [--virttype ] [--emulator ] > [--arch ] [--machine ] [--all] > > i.e. specifying the option name is mandatory > Okay, I'll change the doc in virsh.rst to: hypervisor-cpu-models [--virttype virttype] [--emulator emulator] [--arch arch] [--machine machine] [--all] >> +Print the list of CPU models known by the hypervisor for the specified >> architecture. >> +It is not guaranteed that a listed CPU will run on the host. To determine >> CPU >> +model compatibility with the host, see ``virsh hypervisor-cpu-baseline`` and >> +``virsh hypervisor-cpu-compare``. >> + >> +The *virttype* option specifies the virtualization type (usable in the >> 'type' >> +attribute of the top level element from the domain XML). *emulator* >> +specifies the path to the emulator, *arch* specifies the CPU architecture, >> and >> +*machine* specifies the machine type. >> + >> +By default, only the models that are claimed to be "usable" by the >> hypervisor >> +on the host are reported. The option *--all* will report every CPU model >> known >> +to the hypervisor, including ones that are not supported on the hypervisor >> (e.g. >> +newer generation models). >> + >> DOMAIN COMMANDS >> === >> >> diff --git a/tools/virsh-host.c b/tools/virsh-host.c >> index 9e8f542c96..2884067bbb 100644 >> --- a/tools/virsh-host.c >> +++ b/tools/virsh-host.c >> @@ -1766,6 +1766,74 @@ cmdHypervisorCPUBaseline(vshControl *ctl, >> } >> >> >> +/* >> + * "hypervisor-cpu-models" command >> + */ >> +static const vshCmdInfo info_hypervisor_cpu_models = { >> +.help = N_("Hypervisor reported CPU models"), >> +.desc = N_("Get the CPU models reported by the hypervisor."), >> +}; >> + >> +static const vshCmdOptDef opts_hypervisor_cpu_models[] = { >> +{.name = "virttype", >> + .type = VSH_OT_STRING, >> + .completer = virshDomainVirtTypeCompleter, >> + .help = N_("virtualization type (/domain/@type)"), >> +}, >> +{.name = "emulator", >> + .type = VSH_OT_STRING, >> + .unwanted_positional = true, > > The use of this option is discouraged, see: > > commit a67f737ddfc59b0f5b3905bd1c0935ced82de3d0 > Author: Peter Krempa > AuthorDate: 2024-03-14 17:17:19 +0100 > Commit: Peter Krempa > CommitDate: 2024-04-02 14:24:30 +0200 > > virt-admin: Annodate 'unwanted_positional' arguments > > https://gitlab.com/libvirt/libvirt/-/commit/a67f737ddfc59b0f5b3905bd1c0935ced82de3d0 > I'll remove it. >> + .help = N_("path to emulator binary (/domain/devices/emulator)"), >> +}, >> +{.name = "arch", >> + .type = VSH_OT_STRING, >> + .completer = virshArchCompleter, >> + .help = N_("CPU architecture (/domain/os/type/@arch)"), >> +}, >> +{.name = "machine", >> + .type = VSH_OT_STRING, >> + .help = N_("machine type (/domain/os/type/@machine)"), >> +}, >> +{.name = "all", >> + .type = VSH_OT_BOOL, >> + .help = N_("include all CPU models known to the hypervisor for the >> architecture") >> +}, >> +{.name = NULL} >> +}; >> + >> +static bool >> +cmdHypervisorCPUModelNames(vshControl *ctl, >> + const vshCmd *cmd) >> +{ >> +g_autofree char *caps_xml = NULL; >> +const char *virttype = NULL; >> +const char *emulator = NULL; >> +const char *arch = NULL; >> +const char *machine = NULL; >> +const char *xpath_all = "//cpu//model[@usable]/text()"; >> +const char *xpath_usable = "//cpu//model[@us
Re: [PATCH] virsh: Introduce new hypervisor-cpu-models command
On 3/19/25 4:15 AM, Peter Krempa via Devel wrote: > On Tue, Mar 18, 2025 at 18:13:57 +0100, Ján Tomko via Devel wrote: >> On a Thursday in 2025, Collin Walling wrote: >>> From: David Judkovics >>> >>> Add new virsh command 'hypervisor-cpu-models'. Command pulls from the >>> existing domcapabilities XML and uses xpath to parse CPU model strings. >>> By default, only models reported as usable by the hypervisor on the >>> host system are printed. User may specify "--all" to also print >>> models which are not supported on the host. >>> >>> Signed-off-by: David Judkovics >>> Signed-off-by: Boris Fiuczynski >>> Signed-off-by: Collin Walling >>> --- > > [...] > >>> diff --git a/tools/virsh-host.c b/tools/virsh-host.c >>> index 9e8f542c96..2884067bbb 100644 >>> --- a/tools/virsh-host.c >>> +++ b/tools/virsh-host.c >>> @@ -1766,6 +1766,74 @@ cmdHypervisorCPUBaseline(vshControl *ctl, >>> } >>> >>> >>> +/* >>> + * "hypervisor-cpu-models" command >>> + */ >>> +static const vshCmdInfo info_hypervisor_cpu_models = { >>> +.help = N_("Hypervisor reported CPU models"), >>> +.desc = N_("Get the CPU models reported by the hypervisor."), >>> +}; >>> + >>> +static const vshCmdOptDef opts_hypervisor_cpu_models[] = { >>> +{.name = "virttype", >>> + .type = VSH_OT_STRING, >>> + .completer = virshDomainVirtTypeCompleter, >>> + .help = N_("virtualization type (/domain/@type)"), >>> +}, >>> +{.name = "emulator", >>> + .type = VSH_OT_STRING, >>> + .unwanted_positional = true, >> >> The use of this option is discouraged, see: > > I'd say forbidden. The idea of the flag was that no new code would ever > use it. > > In fact in waranted cases, where the options were conflicting or new > options were added in between existing options I even changed old code > to require the options instead of filling them to unwanted fields. > >> commit a67f737ddfc59b0f5b3905bd1c0935ced82de3d0 >> Author: Peter Krempa >> AuthorDate: 2024-03-14 17:17:19 +0100 >> Commit: Peter Krempa >> CommitDate: 2024-04-02 14:24:30 +0200 >> >> virt-admin: Annodate 'unwanted_positional' arguments >> >> https://gitlab.com/libvirt/libvirt/-/commit/a67f737ddfc59b0f5b3905bd1c0935ced82de3d0 > > Commits 348010ac937fd9a71d81cd3d4154dd46bdbb6a87 and > a7b10919e7d371b9e76ccec2956ba74773de6943 illustrate the problem further: > > Specifically some of the arguments would be handled differently > regardless of documentation based on position. > > In case of some other commands new options which were parsed this way > were added before pre-existing options thus would break how the > arguments were filled. > > We do not want that to happen again. > > Commands now can have at most one positional optional option, any > further optional argument must use explicit option string. Thanks for the detailed explanation. I merely copied a the emulator parameter from the other virsh hypervisor-cpu-* commands, but did not understand the significance of this option. I will omit it in v2. -- Regards, Collin
[PATCH v2] virsh: Introduce new hypervisor-cpu-models command
From: David Judkovics Add new virsh command 'hypervisor-cpu-models'. Command pulls from the existing domcapabilities XML and uses xpath to parse CPU model strings. By default, only models reported as usable by the hypervisor on the host system are printed. User may specify "--all" to also print models which are not supported on the host. Signed-off-by: David Judkovics Signed-off-by: Boris Fiuczynski Signed-off-by: Collin Walling Reviewed-by: Ján Tomko --- Changelog: v2 - Corrected virsh.rst documentation - Removed unwanted_positional from emulator option - Adjusted xpath string based on feedback --- docs/manpages/virsh.rst | 25 ++ tools/virsh-host.c | 75 + 2 files changed, 100 insertions(+) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index baced15dec..612c567ff4 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -1034,6 +1034,31 @@ listed in the XML description. If *--migratable* is specified, features that block migration will not be included in the resulting CPU. +hypervisor-cpu-models +- + +**Syntax:** + +:: + + hypervisor-cpu-models [--virttype virttype] [--emulator emulator] + [--arch arch] [--machine machine] [--all] + +Print the list of CPU models known by the hypervisor for the specified architecture. +It is not guaranteed that a listed CPU will run on the host. To determine CPU +model compatibility with the host, see ``virsh hypervisor-cpu-baseline`` and +``virsh hypervisor-cpu-compare``. + +The *virttype* option specifies the virtualization type (usable in the 'type' +attribute of the top level element from the domain XML). *emulator* +specifies the path to the emulator, *arch* specifies the CPU architecture, and +*machine* specifies the machine type. + +By default, only the models that are claimed to be "usable" by the hypervisor +on the host are reported. The option *--all* will report every CPU model known +to the hypervisor, including ones that are not supported on the hypervisor (e.g. +newer generation models). + DOMAIN COMMANDS === diff --git a/tools/virsh-host.c b/tools/virsh-host.c index 9e8f542c96..f4e34fb146 100644 --- a/tools/virsh-host.c +++ b/tools/virsh-host.c @@ -1766,6 +1766,75 @@ cmdHypervisorCPUBaseline(vshControl *ctl, } +/* + * "hypervisor-cpu-models" command + */ +static const vshCmdInfo info_hypervisor_cpu_models = { +.help = N_("Hypervisor reported CPU models"), +.desc = N_("Get the CPU models reported by the hypervisor."), +}; + +static const vshCmdOptDef opts_hypervisor_cpu_models[] = { +{.name = "virttype", + .type = VSH_OT_STRING, + .completer = virshDomainVirtTypeCompleter, + .help = N_("virtualization type (/domain/@type)"), +}, +{.name = "emulator", + .type = VSH_OT_STRING, + .help = N_("path to emulator binary (/domain/devices/emulator)"), +}, +{.name = "arch", + .type = VSH_OT_STRING, + .completer = virshArchCompleter, + .help = N_("CPU architecture (/domain/os/type/@arch)"), +}, +{.name = "machine", + .type = VSH_OT_STRING, + .help = N_("machine type (/domain/os/type/@machine)"), +}, +{.name = "all", + .type = VSH_OT_BOOL, + .help = N_("include all CPU models known to the hypervisor for the architecture") +}, +{.name = NULL} +}; + +static bool +cmdHypervisorCPUModelNames(vshControl *ctl, + const vshCmd *cmd) +{ +g_autofree char *caps_xml = NULL; +const char *virttype = NULL; +const char *emulator = NULL; +const char *arch = NULL; +const char *machine = NULL; +const char *xpath = NULL; +virshControl *priv = ctl->privData; + +if (vshCommandOptString(ctl, cmd, "virttype", &virttype) < 0 || +vshCommandOptString(ctl, cmd, "emulator", &emulator) < 0 || +vshCommandOptString(ctl, cmd, "arch", &arch) < 0 || +vshCommandOptString(ctl, cmd, "machine", &machine) < 0) +return false; + +if (vshCommandOptBool(cmd, "all")) +xpath = "//cpu//model[@usable]/text()"; +else +xpath = "//cpu//model[@usable='yes']/text()"; + +caps_xml = virConnectGetDomainCapabilities(priv->conn, emulator, arch, + machine, virttype, 0); + +if (!caps_xml) { +vshError(ctl, "%s", _("failed to get hypervisor CPU model names")); +return false; +} + +return virshDumpXML(ctl, caps_xml, "domcapabilities", xpath, false); +} + + const vshCmdDef hostAndHypervisorCmds[] = { {.name = "allocpages", .handler = cmdAllocpages, @@ -1833,6 +1902,12 @@ const vshCmdDef hostAndHypervisorCmds[] = { .info = &info_hypervisor_cpu_compare, .flags = 0 }, +{.name = "hypervisor-cpu-models", + .handler = cmdHypervisorCPUModelNames, + .opts = opts_hypervisor_cpu_models, + .info = &info_hypervisor_cpu_models, + .flags =
Re: [PATCH] virsh: Introduce new hypervisor-cpu-models command
On Tue, Mar 18, 2025 at 18:13:57 +0100, Ján Tomko via Devel wrote: > On a Thursday in 2025, Collin Walling wrote: > > From: David Judkovics > > > > Add new virsh command 'hypervisor-cpu-models'. Command pulls from the > > existing domcapabilities XML and uses xpath to parse CPU model strings. > > By default, only models reported as usable by the hypervisor on the > > host system are printed. User may specify "--all" to also print > > models which are not supported on the host. > > > > Signed-off-by: David Judkovics > > Signed-off-by: Boris Fiuczynski > > Signed-off-by: Collin Walling > > --- [...] > > diff --git a/tools/virsh-host.c b/tools/virsh-host.c > > index 9e8f542c96..2884067bbb 100644 > > --- a/tools/virsh-host.c > > +++ b/tools/virsh-host.c > > @@ -1766,6 +1766,74 @@ cmdHypervisorCPUBaseline(vshControl *ctl, > > } > > > > > > +/* > > + * "hypervisor-cpu-models" command > > + */ > > +static const vshCmdInfo info_hypervisor_cpu_models = { > > +.help = N_("Hypervisor reported CPU models"), > > +.desc = N_("Get the CPU models reported by the hypervisor."), > > +}; > > + > > +static const vshCmdOptDef opts_hypervisor_cpu_models[] = { > > +{.name = "virttype", > > + .type = VSH_OT_STRING, > > + .completer = virshDomainVirtTypeCompleter, > > + .help = N_("virtualization type (/domain/@type)"), > > +}, > > +{.name = "emulator", > > + .type = VSH_OT_STRING, > > + .unwanted_positional = true, > > The use of this option is discouraged, see: I'd say forbidden. The idea of the flag was that no new code would ever use it. In fact in waranted cases, where the options were conflicting or new options were added in between existing options I even changed old code to require the options instead of filling them to unwanted fields. > commit a67f737ddfc59b0f5b3905bd1c0935ced82de3d0 > Author: Peter Krempa > AuthorDate: 2024-03-14 17:17:19 +0100 > Commit: Peter Krempa > CommitDate: 2024-04-02 14:24:30 +0200 > > virt-admin: Annodate 'unwanted_positional' arguments > > https://gitlab.com/libvirt/libvirt/-/commit/a67f737ddfc59b0f5b3905bd1c0935ced82de3d0 Commits 348010ac937fd9a71d81cd3d4154dd46bdbb6a87 and a7b10919e7d371b9e76ccec2956ba74773de6943 illustrate the problem further: Specifically some of the arguments would be handled differently regardless of documentation based on position. In case of some other commands new options which were parsed this way were added before pre-existing options thus would break how the arguments were filled. We do not want that to happen again. Commands now can have at most one positional optional option, any further optional argument must use explicit option string.
Re: [PATCH V4 01/18] lib: virDomain{Save,Restore}Params: Ensure absolute path
On Wed, Mar 05, 2025 at 03:48:10PM -0700, Jim Fehlig via Devel wrote: > When invoking virDomainSaveParams with a relative path, the image is > saved to the daemon's CWD. Similarly, when providing virDomainRestoreParams > with a relative path, it attempts to restore from the daemon's CWD. In most > configurations, the daemon's CWD is set to '/'. Ensure a relative path is > converted to absolute before invoking the driver domain{Save,Restore}Params > functions. Are you aware of any common usage of these APIs with a relative path ? Although we've not documented it, my general view is that all paths given to libvirt are expected to be absolute, everywhere - whether API parameters like these, or config file parmeters, or XML elements/attributes. In the perfect world we would canonicalize all relative paths on the client side, but doing that is such an enourmous & complex job it is not likely to be practical. We'd be better off just documenting use of relative paths as "out of scope" and / or "undefined behaviour" > > Signed-off-by: Jim Fehlig > --- > src/libvirt-domain.c | 89 > 1 file changed, 73 insertions(+), 16 deletions(-) > > diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c > index 18451ebdb9..0ee7c6f45b 100644 > --- a/src/libvirt-domain.c > +++ b/src/libvirt-domain.c > @@ -1023,6 +1023,11 @@ virDomainSaveParams(virDomainPtr domain, > unsigned int flags) > { > virConnectPtr conn; > +virTypedParameterPtr params_copy = NULL; > +int nparams_copy = 0; > +const char *to = NULL; > +g_autofree char *absolute_to = NULL; > +int ret = -1; > > VIR_DOMAIN_DEBUG(domain, "params=%p, nparams=%d, flags=0x%x", > params, nparams, flags); > @@ -1033,23 +1038,46 @@ virDomainSaveParams(virDomainPtr domain, > virCheckDomainReturn(domain, -1); > conn = domain->conn; > > -virCheckReadOnlyGoto(conn->flags, error); > +virCheckReadOnlyGoto(conn->flags, done); > > VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_SAVE_RUNNING, > VIR_DOMAIN_SAVE_PAUSED, > - error); > + done); > + > +/* We must absolutize the file path as the save is done out of process */ > +virTypedParamsCopy(¶ms_copy, params, nparams); > +nparams_copy = nparams; > +if (virTypedParamsGetString(params_copy, nparams_copy, > +VIR_DOMAIN_SAVE_PARAM_FILE, &to) < 0) > +goto done; > + > +if (to) { > +if (!(absolute_to = g_canonicalize_filename(to, NULL))) { > +virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("could not build absolute output file path")); > +goto done; > +} > + > +if (virTypedParamsReplaceString(¶ms_copy, &nparams_copy, > +VIR_DOMAIN_SAVE_PARAM_FILE, > +absolute_to) < 0) > +goto done; > +} > > if (conn->driver->domainSaveParams) { > -if (conn->driver->domainSaveParams(domain, params, nparams, flags) < > 0) > -goto error; > -return 0; > +if (conn->driver->domainSaveParams(domain, params_copy, > nparams_copy, flags) < 0) > +goto done; > +ret = 0; > +} else { > +virReportUnsupportedError(); > } > > -virReportUnsupportedError(); > + done: > +if (ret < 0) > +virDispatchError(domain->conn); > +virTypedParamsFree(params_copy, nparams_copy); > > - error: > -virDispatchError(domain->conn); > -return -1; > +return ret; > } > > > @@ -1206,6 +1234,12 @@ virDomainRestoreParams(virConnectPtr conn, > virTypedParameterPtr params, int nparams, > unsigned int flags) > { > +virTypedParameterPtr params_copy = NULL; > +int nparams_copy = 0; > +const char *from = NULL; > +g_autofree char *absolute_from = NULL; > +int ret = -1; > + > VIR_DEBUG("conn=%p, params=%p, nparams=%d, flags=0x%x", >conn, params, nparams, flags); > VIR_TYPED_PARAMS_DEBUG(params, nparams); > @@ -1213,23 +1247,46 @@ virDomainRestoreParams(virConnectPtr conn, > virResetLastError(); > > virCheckConnectReturn(conn, -1); > -virCheckReadOnlyGoto(conn->flags, error); > +virCheckReadOnlyGoto(conn->flags, done); > > VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_SAVE_RUNNING, > VIR_DOMAIN_SAVE_PAUSED, > - error); > + done); > > if (conn->driver->domainRestoreParams) { > +/* We must absolutize the file path as the save is done out of > process */ > +virTypedParamsCopy(¶ms_copy, params, nparams); > +nparams_copy = nparams; > +if (virTypedParamsGetString(params_copy, nparams_copy, > +
Re: [PATCH V4 08/18] qemu: Add helper function for creating save image fd
On Wed, Mar 05, 2025 at 03:48:17PM -0700, Jim Fehlig via Devel wrote: > Move the code in qemuSaveImageCreate that opens, labels, and wraps the > save image fd to a helper function, providing more flexibility for > upcoming mapped-ram support. > > Signed-off-by: Jim Fehlig > --- > src/qemu/qemu_saveimage.c | 65 +++ > 1 file changed, 45 insertions(+), 20 deletions(-) > > diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c > index 29b4e39879..e3f6a0ad0f 100644 > --- a/src/qemu/qemu_saveimage.c > +++ b/src/qemu/qemu_saveimage.c > @@ -425,6 +425,50 @@ qemuSaveImageDecompressionStop(virCommand *cmd, > } > > > +static int > +qemuSaveImageCreateFd(virQEMUDriver *driver, > + virDomainObj *vm, > + const char *path, > + virFileWrapperFd *wrapperFd, Doesn't this need to be virFileWrapperFd **, otherwise... > + bool *needUnlink, > + unsigned int flags) > +{ > +g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); > +int ret = -1; > +VIR_AUTOCLOSE fd = -1; > +int directFlag = 0; > +unsigned int wrapperFlags = VIR_FILE_WRAPPER_NON_BLOCKING; > + > +if (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) { > +wrapperFlags |= VIR_FILE_WRAPPER_BYPASS_CACHE; > +directFlag = virFileDirectFdFlag(); > +if (directFlag < 0) { > +virReportError(VIR_ERR_OPERATION_FAILED, "%s", > + _("bypass cache unsupported by this system")); > +return -1; > +} > +} > + > +fd = virQEMUFileOpenAs(cfg->user, cfg->group, false, path, > + O_WRONLY | O_TRUNC | O_CREAT | directFlag, > + needUnlink); > + > +if (fd < 0) > +return -1; > + > +if (qemuSecuritySetImageFDLabel(driver->securityManager, vm->def, fd) < > 0) > +return -1; > + > +if (!(wrapperFd = virFileWrapperFdNew(&fd, path, wrapperFlags))) > +return -1; this assignment won't be visible to the caller > + > +ret = fd; > +fd = -1; > + > +return ret; > +} > + > + > /* Helper function to execute a migration to file with a correct save header > * the caller needs to make sure that the processors are stopped and do all > other > * actions besides saving memory */ > @@ -441,33 +485,14 @@ qemuSaveImageCreate(virQEMUDriver *driver, > bool needUnlink = false; > int ret = -1; > int fd = -1; > -int directFlag = 0; > virFileWrapperFd *wrapperFd = NULL; > -unsigned int wrapperFlags = VIR_FILE_WRAPPER_NON_BLOCKING; ... > +fd = qemuSaveImageCreateFd(driver, vm, path, wrapperFd, &needUnlink, > flags); ..This would neeed to be &wrapperFd > > -fd = virQEMUFileOpenAs(cfg->user, cfg->group, false, path, > - O_WRONLY | O_TRUNC | O_CREAT | directFlag, > - &needUnlink); > if (fd < 0) > goto cleanup; > > -if (qemuSecuritySetImageFDLabel(driver->securityManager, vm->def, fd) < > 0) > -goto cleanup; > - > -if (!(wrapperFd = virFileWrapperFdNew(&fd, path, wrapperFlags))) > -goto cleanup; > - > if (virQEMUSaveDataWrite(data, fd, path) < 0) > goto cleanup; > > -- > 2.43.0 > With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH V4 07/18] qemu_saveimage: add "sparse" to supported save image formats
On Wed, Mar 05, 2025 at 03:48:16PM -0700, Jim Fehlig via Devel wrote: > Extend the list of formats to include "sparse", which uses QEMU's mapped-ram > stream format [1] to write guest memory blocks at fixed offsets in the save > image file. > > [1] > https://gitlab.com/qemu-project/qemu/-/blob/master/docs/devel/migration/mapped-ram.rst?ref_type=heads > > Signed-off-by: Jim Fehlig > --- > src/qemu/qemu.conf.in | 9 - > src/qemu/qemu_saveimage.c | 1 + > src/qemu/qemu_saveimage.h | 1 + > 3 files changed, 10 insertions(+), 1 deletion(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH V4 18/18] tools: add parallel parameter to virsh restore command
On Wed, Mar 05, 2025 at 03:48:27PM -0700, Jim Fehlig via Devel wrote: > From: Claudio Fontana > > Signed-off-by: Claudio Fontana > Signed-off-by: Jim Fehlig > --- > docs/manpages/virsh.rst | 9 +++-- > tools/virsh-domain.c| 39 +++ > 2 files changed, 42 insertions(+), 6 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH V4 11/18] qemu: Apply migration parameters in qemuMigrationDstRun
On Wed, Mar 05, 2025 at 03:48:20PM -0700, Jim Fehlig via Devel wrote: > Similar to qemuMigrationSrcRun, apply migration parameters in > qemuMigrationDstRun. This allows callers to create customized > migration parameters, but delegates their application to the > function performing the migration. > > Signed-off-by: Jim Fehlig > --- > src/qemu/qemu_migration.c | 16 ++-- > src/qemu/qemu_migration.h | 5 - > src/qemu/qemu_process.c | 2 +- > 3 files changed, 15 insertions(+), 8 deletions(-) > > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c > index f692986056..329b5f61d4 100644 > --- a/src/qemu/qemu_migration.c > +++ b/src/qemu/qemu_migration.c > @@ -2427,7 +2427,10 @@ qemuMigrationDstGetURI(const char *migrateFrom, > int > qemuMigrationDstRun(virDomainObj *vm, > const char *uri, > -virDomainAsyncJob asyncJob) > +virDomainAsyncJob asyncJob, > +qemuMigrationParams *migParams, > +unsigned int flags) > + > { > virTristateBool exitOnError = VIR_TRISTATE_BOOL_ABSENT; > qemuDomainObjPrivate *priv = vm->privateData; > @@ -2435,6 +2438,10 @@ qemuMigrationDstRun(virDomainObj *vm, > > VIR_DEBUG("Setting up incoming migration with URI %s", uri); > > +if (migParams && qemuMigrationParamsApply(vm, asyncJob, > + migParams, flags) < 0) > +return -1; > + > /* Ask QEMU not to exit on failure during incoming migration (if > supported) > * so that we can properly check and report error during Finish phase. > */ > @@ -3368,10 +3375,6 @@ qemuMigrationDstPrepareActive(virQEMUDriver *driver, > goto error; > } > > -if (qemuMigrationParamsApply(vm, VIR_ASYNC_JOB_MIGRATION_IN, > - migParams, flags) < 0) > -goto error; > - So semantically, we now apply the dst migration parameters /after/ starting the NBD server, instead of before. This seems like it ought to be safe, as migration is independent of the NBD block layer. > if (mig->nbd && > flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC)) { > const char *nbdTLSAlias = NULL; > @@ -3403,7 +3406,8 @@ qemuMigrationDstPrepareActive(virQEMUDriver *driver, > } > > if (qemuMigrationDstRun(vm, incoming->uri, > -VIR_ASYNC_JOB_MIGRATION_IN) < 0) > +VIR_ASYNC_JOB_MIGRATION_IN, > +migParams, flags) < 0) > goto error; > > if (qemuProcessFinishStartup(driver, vm, VIR_ASYNC_JOB_MIGRATION_IN, Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH V4 12/18] qemu: Add support for mapped-ram on restore
On Wed, Mar 05, 2025 at 03:48:21PM -0700, Jim Fehlig via Devel wrote: > Add support for the mapped-ram migration capability on restore. > > Signed-off-by: Jim Fehlig > --- > src/qemu/qemu_driver.c| 27 +++--- > src/qemu/qemu_migration.c | 12 ++-- > src/qemu/qemu_process.c | 41 --- > src/qemu/qemu_process.h | 15 +- > src/qemu/qemu_saveimage.c | 29 --- > src/qemu/qemu_saveimage.h | 2 ++ > src/qemu/qemu_snapshot.c | 8 > 7 files changed, 90 insertions(+), 44 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|