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

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(a)redhat.com&gt; >> --- >> 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