[libvirt] [PATCH v3 2/7] cpu: add function to get the models for an arch

[Jiri Denemark]

On Sat, Sep 14, 2013 at 15:07:49 +0200, Giuseppe Scrivano wrote:
> Eric Blake <eblake at redhat.com> writes:
> 
> > NACK, needs a v4; this is where we need to fix things to do the right
> > subdriver mapping. Quoting IRC:
> >
> > and I know which one to change
> > [15:30]	jdenemar	eblake: cpu-models should work for both x86 and x86_64 imho
> > [15:31]	eblake	yep - where the map file lists x86, we need the
> > cpu-models to support it for both i686 and x86_64
> > [15:32]	jdenemar	yeah, the x86 in cpu_map.xml is actually a cpu driver
> > name and the driver has a list of archs it supports
> > [15:34]	eblake	jdenemar: giuseppe_s used cpuMapLoad(arch, ...) - which
> > is only doing a literal string match
> > [15:34]	eblake	so where do we reverse map the driver names in the map
> > file into actual arch names?
> > [15:35]	eblake	or, where SHOULD we be doing that mapping?
> > [15:38]	jdenemar	every cpu api in cpu.c calls cpuGetSubDriver to get the
> > driver from a real arch
> > [15:41]	jdenemar	so there should be a high level cpu api that takes a
> > real arch and gives model names, that should look up the appropriate sub
> > driver, call its api and it should load its section of cpu_map.xml and
> > return the list
> 
> Jiri, would something like this suffice or am I missing some other
> details?
> 
> Thanks,
> Giuseppe
> 
> 
> +int
> +cpuGetModels(const char *archName, char ***models)
> +{
> +    struct cpuGetModelsData data;
> +    virArch arch;
> +    struct cpuArchDriver *driver;
> +    data.data = NULL;
> +    data.len = 1;
> +
> +    arch = virArchFromString(archName);
> +    if (arch == VIR_ARCH_NONE) {
> +        virReportError(VIR_ERR_INVALID_ARG,
> +                       _("cannot find architecture %s"),
> +                       archName);
> +        goto error;
> +    }

I'm not sure if the API should take a string and call virArchFromString
or if that part should be done by the caller and this API would just
take virArch. I guess it doesn't really matter until we need to reuse
this API in a context where we don't have a string arch.

> +
> +    driver = cpuGetSubDriver(arch);
> +    if (driver == NULL) {
> +        virReportError(VIR_ERR_INVALID_ARG,
> +                       _("cannot find a driver for the architecture %s"),
> +                       archName);
> +        goto error;
> +    }

I was rather thinking about adding a new driver API so that you'd just
do

    return driver->getModels(models);

here, similarly to all other cpu APIs. And the per-arch API would just
use it's existing functions to load the CPU map and take the list of
models from the parsed map. But the way you do it will work too. It's
just that I'd feel better if all arch specific things were located in
sub drivers. Even though this API is somewhere between arch specific and
arch agnostic :-)

> +
> +    if (models && VIR_ALLOC_N(data.data, data.len) < 0)
> +        goto error;
> +
> +    if (cpuGetArchModels(driver->name, &data) < 0)
> +        goto error;
> +
> +    if (models)
> +        *models = data.data;
> +
> +    return data.len - 1;
> +
> +error:
> +    virStringFreeList(data.data);
> +    return -1;
> +}

Jirka