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@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
  *
  * 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) {
diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h
index 83d5bcb63f..2d81927a0b 100644
--- a/src/cpu/cpu.h
+++ b/src/cpu/cpu.h
@@ -227,7 +227,8 @@ virCPUDataCheckFeature(const virCPUData *data,
 
 bool
 virCPUModelIsAllowed(const char *model,
-                     virDomainCapsCPUModelsPtr models)
+                     virDomainCapsCPUModelsPtr models,
+                     virDomainCapsCPUModelPtr *hvModel)
     ATTRIBUTE_NONNULL(1);
 
 int
diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c
index 7eb27c59bd..9f990a3fb5 100644
--- a/src/cpu/cpu_ppc64.c
+++ b/src/cpu/cpu_ppc64.c
@@ -679,7 +679,7 @@ ppc64DriverDecode(virCPUDefPtr cpu,
         goto cleanup;
     }
 
-    if (!virCPUModelIsAllowed(model->name, models)) {
+    if (!virCPUModelIsAllowed(model->name, models, NULL)) {
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                        _("CPU model %s is not supported by hypervisor"),
                        model->name);
diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
index 84ec878d1b..198e80a5c2 100644
--- a/src/cpu/cpu_x86.c
+++ b/src/cpu/cpu_x86.c
@@ -1854,7 +1854,7 @@ x86Decode(virCPUDefPtr cpu,
      */
     for (i = map->nmodels - 1; i >= 0; i--) {
         candidate = map->models[i];
-        if (!virCPUModelIsAllowed(candidate->name, models)) {
+        if (!virCPUModelIsAllowed(candidate->name, models, NULL)) {
             if (preferred && STREQ(candidate->name, preferred)) {
                 if (cpu->fallback != VIR_CPU_FALLBACK_ALLOW) {
                     virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-- 
2.14.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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@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) {
[...]

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, Oct 12, 2017 at 15:50:29 -0400, John Ferlan wrote:
> 
> 
> 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@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.

Hmm, I have a suggestion. I should just create a separate function
virDomainCapsCPUModelsGet or *Find which would just return a pointer to
the CPU model structure from the models array and change
virCPUModelIsAllowed into a simple wrapper around the new function. The
callers which would pass non-NULL hvModel pointer would just call the
underlying virDomainCapsCPUModelsGet function directly.

> 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?

No. If there were any concerns like this even going through the array
would be dangerous. Anyway, virDomainCapsCPUModelsPtr is a reference
counted object which never changes once it's created. It can only go
away and be replaced with another one.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 10/13/2017 11:31 AM, Jiri Denemark wrote:
> On Thu, Oct 12, 2017 at 15:50:29 -0400, John Ferlan wrote:
>>
>>
>> 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@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.
> 
> Hmm, I have a suggestion. I should just create a separate function
> virDomainCapsCPUModelsGet or *Find which would just return a pointer to
> the CPU model structure from the models array and change
> virCPUModelIsAllowed into a simple wrapper around the new function. The
> callers which would pass non-NULL hvModel pointer would just call the
> underlying virDomainCapsCPUModelsGet function directly.
> 

That would seem to work and Get would perhaps also imply it's not a copy
but rather a pointer to something existing.

John

>> 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?
> 
> No. If there were any concerns like this even going through the array
> would be dangerous. Anyway, virDomainCapsCPUModelsPtr is a reference
> counted object which never changes once it's created. It can only go
> away and be replaced with another one.
> 
> Jirka
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, Oct 13, 2017 at 14:45:05 -0400, John Ferlan wrote:
> >> 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.
> > 
> > Hmm, I have a suggestion. I should just create a separate function
> > virDomainCapsCPUModelsGet or *Find which would just return a pointer to
> > the CPU model structure from the models array and change
> > virCPUModelIsAllowed into a simple wrapper around the new function. The
> > callers which would pass non-NULL hvModel pointer would just call the
> > underlying virDomainCapsCPUModelsGet function directly.
> > 
> 
> That would seem to work and Get would perhaps also imply it's not a copy
> but rather a pointer to something existing.

Yeah, I think it looks better. See v2 of this series.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list