On Thu, May 24, 2018 at 13:24:25 -0400, Collin Walling wrote:
> 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?
Yeah, hv-* is definitely shorter, but I don't know if it's better. What
do others think?
> > +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".
Definitely, I forgot to fix it here.
>
> > + {.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".
It doesn't really matter what libvirt currently checks for which
architecture. The API takes a CPU definition XML and libvirt will use
anything it needs from that.
...
> > @@ -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."
Yeah, your version looks better.
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
