[libvirt] [PATCH 07/23] cpu: Return model from virCPUModelIsAllowed

[John Ferlan]

On 10/04/2017 10:58 AM, Jiri Denemark wrote:
> If a given CPU model is supported by the hypervisor, we want to know
> more about it, e.g., what features may block its usage on the current
> host and such details are stored in the virDomainCapsCPUModelsPtr list
> which virCPUModelIsAllowed uses to check whether the CPU model is
> supported. Thus if the CPU model is found in the list we can directly
> return a pointer to the corresponding virDomainCapsCPUModel if the
> caller needs to look at the details.
> 
> Signed-off-by: Jiri Denemark <jdenemar at redhat.com>
> ---
>  src/cpu/cpu.c       | 18 ++++++++++++++----
>  src/cpu/cpu.h       |  3 ++-
>  src/cpu/cpu_ppc64.c |  2 +-
>  src/cpu/cpu_x86.c   |  2 +-
>  4 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c
> index b815ed383a..48290a471b 100644
> --- a/src/cpu/cpu.c
> +++ b/src/cpu/cpu.c
> @@ -819,24 +819,34 @@ virCPUDataParse(const char *xmlStr)
>   *
>   * @model: CPU model to be checked
>   * @models: list of supported CPU models
> + * @hvModel: pointer to matching model from @models will be returned here

As later pointed out - 'hv' == hypervisor version.

In any case, the 'ModelIsAllowed" now feels overloaded returning more
than just true/false that a typical "Is" type function would return.
Sorry, I don't have suggestions, so unless someone else is looking and
has a better name, then just go with it.

The only other concern is that since you're returning a pointer in the
middle of some array, any concerns over some other thread changing
things and freeing what you're looking at?

e.g., should there be a way to deep copy the model information and force
the caller to free when it's done?

As long as you're comfortable with pointing in the middle of an array
(and I didn't see anything existing that would seem to cause a problem),
then fine go with what you have.


Reviewed-by: John Ferlan <jferlan at redhat.com>

John

>   *
>   * Checks whether @model can be found in the list of supported @models.
> - * If @models is NULL, all models are supported.
> + * If @models is NULL, all models are supported. If both @models and @hvModel
> + * are non-NULL and @model is found in the list of supported models, @hvModel
> + * will be filled with the pointer to the matching CPU model from @models.
>   *
>   * Returns true if @model is supported, false otherwise.
>   */
>  bool
>  virCPUModelIsAllowed(const char *model,
> -                     virDomainCapsCPUModelsPtr models)
> +                     virDomainCapsCPUModelsPtr models,
> +                     virDomainCapsCPUModelPtr *hvModel)
>  {
>      size_t i;
>  
> +    if (hvModel)
> +        *hvModel = NULL;
> +
>      if (!models)
>          return true;
>  
>      for (i = 0; i < models->nmodels; i++) {
> -        if (STREQ(models->models[i].name, model))
> +        if (STREQ(models->models[i].name, model)) {
> +            if (hvModel)
> +                *hvModel = models->models + i;
>              return true;
> +        }
>      }
>      return false;
>  }
> @@ -908,7 +918,7 @@ virCPUTranslate(virArch arch,
>          cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH)
>          return 0;
>  
> -    if (virCPUModelIsAllowed(cpu->model, models))
> +    if (virCPUModelIsAllowed(cpu->model, models, NULL))
>          return 0;
>  
>      if (cpu->fallback != VIR_CPU_FALLBACK_ALLOW) {
[...]