Re: [libvirt] [PATCH 10/22] virsh: Introduce new hypervisor-cpu-compare command

On 05/16/2018 04:39 AM, Jiri Denemark wrote: > This command is a virsh wrapper for virConnectCompareHypervisorCPU. > > Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; > --- > 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".

> @@ -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."