[libvirt] [PATCH 1/8] cpu: Introduce cpuModelIsAllowed internal API

[Eric Blake]

On 12/20/2012 05:01 PM, Jiri Denemark wrote:
> The API can be used to check if the model is on the supported models
> list, which needs to be done in several places.
> ---
>  src/cpu/cpu.c         | 17 +++++++++++++++++
>  src/cpu/cpu.h         |  5 +++++
>  src/cpu/cpu_generic.c | 19 +++++--------------
>  src/cpu/cpu_x86.c     | 11 +----------
>  4 files changed, 28 insertions(+), 24 deletions(-)
> 
> diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c
> index 53c4cc3..b41fb38 100644
> --- a/src/cpu/cpu.c
> +++ b/src/cpu/cpu.c
> @@ -442,3 +442,20 @@ cpuHasFeature(virArch arch,
>  
>      return driver->hasFeature(data, feature);
>  }
> +
> +bool
> +cpuModelIsAllowed(const char *model,
> +                  const char **models,
> +                  unsigned int nmodels)

Is size_t any better than unsigned int for nmodels?

> +{
> +    unsigned int i;
> +
> +    if (!models || !nmodels)
> +        return true;

Should this case be false?  Or more specifically, in the old code...

> +
> +    for (i = 0; i < nmodels; i++) {
> +        if (models[i] && STREQ(models[i], model))
> +            return true;
> +    }
> +    return false;
> +}

> +++ b/src/cpu/cpu_generic.c
> @@ -123,20 +123,11 @@ genericBaseline(virCPUDefPtr *cpus,
>      unsigned int count;
>      unsigned int i, j;
>  
> -    if (models) {

!models skipped the error message, but allocated models with nmodels==0
errored out.  You have a silent change in behavior by not erroring where
you used to; meanwhile, if you return false instead of true for both
branches of the ||, you would have a change in behavior of erroring
where you previously did not.  I think that our current mix of
half-and-half erroring is not useful, but it's probably worth deciding
what we meant, and documenting in the commit message that the change in
error policy for (!models || !nmodels) is intentional.

> -        bool found = false;
> -        for (i = 0; i < nmodels; i++) {
> -            if (STREQ(cpus[0]->model, models[i])) {
> -                found = true;
> -                break;
> -            }
> -        }
> -        if (!found) {
> -            virReportError(VIR_ERR_INTERNAL_ERROR,
> -                           _("CPU model '%s' is not support by hypervisor"),
> -                           cpus[0]->model);
> -            goto error;
> -        }
> +    if (!cpuModelIsAllowed(cpus[0]->model, models, nmodels)) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("CPU model %s is not supported by hypervisor"),
> +                       cpus[0]->model);
> +        goto error;
>      }

> +++ b/src/cpu/cpu_x86.c
> @@ -1325,16 +1325,7 @@ x86Decode(virCPUDefPtr cpu,
>  
>      candidate = map->models;
>      while (candidate != NULL) {
> -        bool allowed = (models == NULL);
> -
> -        for (i = 0; i < nmodels; i++) {
> -            if (models && models[i] && STREQ(models[i], candidate->name)) {
> -                allowed = true;
> -                break;
> -            }
> -        }

Hmm, here the behavior was different for (!models || !nmodels).

> -
> -        if (!allowed) {
> +        if (!cpuModelIsAllowed(candidate->name, models, nmodels)) {
>              if (preferred && STREQ(candidate->name, preferred)) {
>                  if (cpu->fallback != VIR_CPU_FALLBACK_ALLOW) {
>                      virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> 

-- 
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: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20121221/d13c9dd9/attachment-0001.sig>