[libvirt] [PATCH v2 1/5] cpu_models: add new public API

[Eric Blake]

On 09/06/2013 05:11 PM, Giuseppe Scrivano wrote:
> The new function virConnectGetCPUModelNames allows to retrieve the list
> of CPU models known by the hypervisor for a specific architecture.
> 
> Signed-off-by: Giuseppe Scrivano <gscrivan at redhat.com>
> ---
>  include/libvirt/libvirt.h.in | 18 +++++++++++++
>  python/generator.py          |  1 +
>  src/cpu/cpu.c                | 64 ++++++++++++++++++++++++++++++++++++++++++++
>  src/cpu/cpu.h                |  3 +++
>  src/driver.h                 |  7 +++++
>  src/libvirt.c                | 47 ++++++++++++++++++++++++++++++++
>  src/libvirt_private.syms     |  1 +
>  src/libvirt_public.syms      |  5 ++++
>  tools/virsh-host.c           | 48 +++++++++++++++++++++++++++++++++
>  tools/virsh.pod              |  5 ++++
>  10 files changed, 199 insertions(+)

In addition to Daniel's review,

> +++ b/include/libvirt/libvirt.h.in
> @@ -4006,6 +4006,24 @@ int virConnectCompareCPU(virConnectPtr conn,
>                           const char *xmlDesc,
>                           unsigned int flags);
>  
> +/**
> + * virConnectGetCPUModelNames:
> + *
> + * @conn: virConnect connection
> + * @arch: Architecture
> + * @models: NULL terminated array of the CPU models supported for the specified
> + * architecture.  Each element and the array itself must be freed by the caller
> + * with free.
> + * @flags: extra flags; not used yet, so callers should always pass 0.
> + *
> + * Get the list of supported CPU models for a specific architecture.
> + *
> + * Returns -1 on error, 0 on success.
> + */
> +int virConnectGetCPUModelNames(virConnectPtr conn,
> +                               const char *arch,
> +                               char ***models,
> +                               unsigned int flags);

Typically we have not documented functions in the .h, just in src/libvirt.c.


> +static int
> +cpuGetArchModelsCb(enum cpuMapElement element,
> +                   xmlXPathContextPtr ctxt,
> +                   void *cbdata)
> +{
> +    char *name;
> +    struct cpuGetModelsData *data = cbdata;
> +    if (element != CPU_MAP_ELEMENT_MODEL)
> +        return 0;
> +
> +    name = virXPathString("string(@name)", ctxt);
> +    if (name == NULL)
> +        return -1;
> +
> +    if (VIR_EXPAND_N(data->data, data->len, 1) < 0)
> +        return -1;
> +
> +    data->data[data->len - 2] = name;
> +    data->data[data->len - 1] = NULL;
> +    return 0;

VIR_INSERT_ELEMENT may be simpler to use than VIR_EXPAND_N.
Furthermore, VIR_EXPAND_N guarantees zero-initialization of the growth,
so the data->data[data->len - 1] = NULL; line is redundant.


> +int
> +virConnectGetCPUModelNames(virConnectPtr conn, const char *arch, char ***models,
> +                           unsigned int flags)
> +{
> +    VIR_DEBUG("conn=%p, arch=%s, flags=%x", conn, arch, flags);
> +    virResetLastError();
> +
> +    if (!VIR_IS_CONNECT(conn)) {
> +        virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virDispatchError(NULL);
> +        return -1;
> +    }
> +
> +    if (arch == NULL) {
> +        virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        goto error;
> +    }

virCheckNonNullArgReturn(arch, -1)

> +
> +    if (conn->driver->connectGetCPUModelNames) {
> +        if (conn->driver->connectGetCPUModelNames(conn, arch, models, flags) < 0)
> +            goto error;
> +
> +        return 0;

I agree with Dan that this should return the number of non-NULL entries
(so setting the array to { "x86_64", "i686", NULL } would return 2, even
though the array has allocated room for [at least] 3 pointers).

> +++ b/tools/virsh-host.c
> @@ -92,6 +92,25 @@ static const vshCmdOptDef opts_freecell[] = {
>      {.name = NULL}
>  };
>  
> +static const vshCmdInfo info_cpu_models[] = {
> +    {.name = "help",
> +     .data = N_("CPU models.")

We generally don't have trailing '.' in the short help.

> +    },
> +    {.name = "desc",
> +     .data = N_("Get the CPU models for an arch.")
> +    },
> +    {.name = NULL}
> +};
> +
> +static const vshCmdOptDef opts_cpu_models[] = {
> +    {.name = "arch",
> +     .type = VSH_OT_DATA,
> +     .flags = VSH_OFLAG_REQ,
> +     .help = N_("architecture")
> +    },
> +    {.name = NULL}
> +};
> +

These new [] data should be closer...

>  static bool
>  cmdFreecell(vshControl *ctl, const vshCmd *cmd)
>  {
> @@ -626,6 +645,29 @@ cmdURI(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED)
>      return true;
>  }
>  
> +static bool
> +cmdCPUModelNames(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED)

...to the function they are associated with.  (That is, don't split
cmdFreecell from its options).

> +{
> +    char **models, **it;
> +    const char *arch = NULL;
> +
> +    if (vshCommandOptStringReq(ctl, cmd, "arch", &arch) < 0)
> +        return false;
> +
> +    if (virConnectGetCPUModelNames(ctl->conn, arch, &models, 0) < 0) {
> +        vshError(ctl, "%s", _("failed to get CPU Models Names"));

Sounds awkward; maybe "failed to get CPU model names"

> @@ -851,6 +893,12 @@ const vshCmdDef hostAndHypervisorCmds[] = {
>       .info = info_capabilities,
>       .flags = 0
>      },
> +    {.name = "cpu-models",
> +     .handler = cmdCPUModelNames,
> +     .opts = opts_cpu_models,
> +     .info = info_cpu_models,
> +     .flags = 0

Not your fault, but I'd like trailing commas in all of these
initializers (as a separate cleanup), so that future additions can be
strict additions (thinking forward to Tomas' work on tab-completers).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130910/7d00ad18/attachment-0001.sig>