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

[Jiri Denemark]

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 at redhat.com>
> > ---
> >  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