On 05/16/2018 04:39 AM, Jiri Denemark wrote:
> This command is a virsh wrapper for virConnectCompareHypervisorCPU.
>
> Signed-off-by: Jiri Denemark <jdenemar@xxxxxxxxxx>
> ---
> tools/virsh-host.c | 113 +++++++++++++++++++++++++++++++++++++++++++++
> tools/virsh.pod | 29 +++++++++++-
> 2 files changed, 141 insertions(+), 1 deletion(-)
>
> diff --git a/tools/virsh-host.c b/tools/virsh-host.c
> index ea2c411c02..1e7cfcbd5e 100644
> --- a/tools/virsh-host.c
> +++ b/tools/virsh-host.c
> @@ -1595,6 +1595,113 @@ cmdNodeMemoryTune(vshControl *ctl, const vshCmd *cmd)
> goto cleanup;
> }
>
> +
> +/*
> + * "hypervisor-cpu-compare" command
> + */
Really just a nit:
I'm somewhat torn by the verbose command name. "hypervisor-" is a bit cumbersome,
but hy<tab> will auto-complete it for you at this point. Maybe shorten it to hv-cpu-compare?
> +static const vshCmdInfo info_hypervisor_cpu_compare[] = {
> + {.name = "help",
> + .data = N_("compare a CPU with the CPU created by a hypervisor on the host")
> + },
> + {.name = "desc",
> + .data = N_("compare CPU with hypervisor CPU")
> + },
> + {.name = NULL}
> +};
> +
> +static const vshCmdOptDef opts_hypervisor_cpu_compare[] = {
> + VIRSH_COMMON_OPT_FILE(N_("file containing an XML CPU description")),
> + {.name = "virttype",
> + .type = VSH_OT_STRING,
> + .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,
> + .help = N_("domain architecture (/domain/os/type/@arch)"),
> + },
Your documentation states that this option is the "CPU architecture", which
I think I like more than "domain architecture".
> + {.name = "machine",
> + .type = VSH_OT_STRING,
> + .help = N_("machine type (/domain/os/type/@machine)"),
> + },
> + {.name = "error",
> + .type = VSH_OT_BOOL,
> + .help = N_("report error if CPUs are incompatible")
> + },
> + {.name = NULL}
> +};
> +
> +static bool
> +cmdHypervisorCPUCompare(vshControl *ctl,
> + const vshCmd *cmd)
> +{
> + const char *from = NULL;
> + const char *virttype = NULL;
> + const char *emulator = NULL;
> + const char *arch = NULL;
> + const char *machine = NULL;
> + bool ret = false;
> + int result;
> + char **cpus = NULL;
> + unsigned int flags = 0;
> + virshControlPtr priv = ctl->privData;
> +
> + if (vshCommandOptBool(cmd, "error"))
> + flags |= VIR_CONNECT_COMPARE_CPU_FAIL_INCOMPATIBLE;
> +
> + if (vshCommandOptStringReq(ctl, cmd, "file", &from) < 0 ||
> + vshCommandOptStringReq(ctl, cmd, "virttype", &virttype) < 0 ||
> + vshCommandOptStringReq(ctl, cmd, "emulator", &emulator) < 0 ||
> + vshCommandOptStringReq(ctl, cmd, "arch", &arch) < 0 ||
> + vshCommandOptStringReq(ctl, cmd, "machine", &machine) < 0)
> + return false;
> +
> + if (!(cpus = vshExtractCPUDefXMLs(ctl, from)))
> + return false;
> +
> + result = virConnectCompareHypervisorCPU(priv->conn, emulator, arch,
> + machine, virttype, cpus[0], flags);
> +
> + switch (result) {
> + case VIR_CPU_COMPARE_INCOMPATIBLE:
> + vshPrint(ctl,
> + _("CPU described in %s is incompatible with the CPU provided "
> + "by hypervisor on the host\n"),
> + from);
How much information regarding a CPU definition does libvirt consider when comparing CPU's
for x86 (and for other archs, if you happen to know)? On s390, we only take the cpu model
and features into consideration.
If the archs other than s390 will only use the cpu model and features as well -- or if this
API should explicitly work only with CPU models -- then perhaps it is more accurate for these
messages to state "CPU model" instead of "CPU"? This change would also have to be propagated
in to the documentation, replacing "CPU definition" with "CPU model".
> + goto cleanup;
> + break;
> +
> + case VIR_CPU_COMPARE_IDENTICAL:
> + vshPrint(ctl,
> + _("CPU described in %s is identical to the CPU provided by "
> + "hypervisor on the host\n"),
> + from);
> + break;
> +
> + case VIR_CPU_COMPARE_SUPERSET:
> + vshPrint(ctl,
> + _("The CPU provided by hypervisor on the host is a superset "
> + "of CPU described in %s\n"),
> + from);
> + break;
> +
> + case VIR_CPU_COMPARE_ERROR:
> + default:
> + vshError(ctl, _("Failed to compare hypervisor CPU with %s"), from);
> + goto cleanup;
> + }
> +
> + ret = true;
> +
> + cleanup:
> + virStringListFree(cpus);
> + return ret;
> +}
> +
> +
> const vshCmdDef hostAndHypervisorCmds[] = {
> {.name = "allocpages",
> .handler = cmdAllocpages,
> @@ -1650,6 +1757,12 @@ const vshCmdDef hostAndHypervisorCmds[] = {
> .info = info_hostname,
> .flags = 0
> },
> + {.name = "hypervisor-cpu-compare",
> + .handler = cmdHypervisorCPUCompare,
> + .opts = opts_hypervisor_cpu_compare,
> + .info = info_hypervisor_cpu_compare,
> + .flags = 0
> + },
> {.name = "maxvcpus",
> .handler = cmdMaxvcpus,
> .opts = opts_maxvcpus,
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index ea10e1ad43..1a55092efd 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -585,7 +585,9 @@ features that block migration will not be included in the resulting CPU.
>
> =item B<cpu-compare> I<FILE> [I<--error>]
>
> -Compare CPU definition from XML <file> with host CPU. The XML <file> may
> +Compare CPU definition from XML <file> with host CPU. (See
> +B<hypervisor-cpu-compare> command for comparing the CPU definition with the CPU
> +which a specific hypervisor is able to provide on the host.) The XML <file> may
> contain either host or guest CPU definition. The host CPU definition is the
> <cpu> element and its contents as printed by B<capabilities> command. The
> guest CPU definition is the <cpu> element and its contents from domain XML
> @@ -616,6 +618,31 @@ specified, then the output will be single-quoted where needed, so that
> it is suitable for reuse in a shell context. If I<--xml> is
> specified, then the output will be escaped for use in XML.
>
> +=item B<hypervisor-cpu-compare> I<FILE> [I<virttype>] [I<emulator>] [I<arch>]
> +[I<machine>] [I<--error>]
> +
> +Compare CPU definition from XML <file> with the CPU the specified hypervisor
What is "the specified hypervisor"? To me, this implies that the user would have
to provide the other options to specify a hypervisor for the command, but you can
simply provide the XML <file> and be done.
I think it reads better as:
"Compares the CPU definition from an XML <file> with the CPU the hypervisor is able
to provide on the host."
And then leave it up to your last paragraph (which defines the additional options)
to explain how to "specify" a hypervisor.
> +is able to provide on the host. (This is different from B<cpu-compare> which
> +compares the CPU definition with the host CPU without considering any specific
> +hypervisor and its abilities.)
> +
> +The XML I<FILE> may contain either host or guest CPU definition. The host CPU
"either _a_ host or guest CPU definition"
> +definition is the <cpu> element and its contents as printed by B<capabilities>
"by _the_ B<capabilities> command"
> +command. The guest CPU definition is the <cpu> element and its contents from
"from _the_ domain XML definition"
> +domain XML definition or the CPU definition created from the host CPU model
> +found in domain capabilities XML (printed by B<domcapabilities> command). In
"found in _the_ domain capabilities XML (printed by _the_ B<domcapabilities command)."
> +addition to the <cpu> element itself, this command accepts full domain XML,
> +capabilities XML, or domain capabilities XML containing the CPU definition. For
> +more information on guest CPU definition see:
> +L<https://libvirt.org/formatdomain.html#elementsCPU>.
> +
> +The I<virttype> option specifies the virtualization type (usable in the 'type'
> +attribute of the <domain> top level element from the domain XML). I<emulator>
> +specifies the path to the emulator, I<arch> specifies the CPU architecture, and
> +I<machine> specifies the machine type. If I<--error> is specified, the command
> +will return an error when the given CPU is incompatible with host CPU and a
"is incompatible with _the_ host CPU"
> +message providing more details about the incompatibility will be printed out.
> +
> =back
>
> =head1 DOMAIN COMMANDS
>
Functionally, everything looks good.
--
Respectfully,
- Collin Walling
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
