Re: [PATCH] cpu-driver: Fix the cross driver function call

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi John,

thank you for your comment.
As you mentioned this problem is solved (and now pushed) by Pavel. Thanks Pavel for fixing this issue.

Kind regards,
Daniel

On 03.12.2014 15:29, John Ferlan wrote:
> 
> 
> On 11/20/2014 05:08 AM, Daniel Hansel wrote:
>> For Intel and PowerPC the implementation is calling a cpu driver
>> function across driver layers (i.e. from qemu driver directly to cpu
>> driver).
>> The correct behavior is to use libvirt API functionality to perform such
>> a inter-driver call.
>>
>> This patch introduces a new cpu driver API function getModels() to
>> retrieve the cpu models. The currect implementation to process the
>> cpu_map XML content is transferred to the INTEL and PowerPC cpu driver
>> specific API functions.
>> Additionally processing the cpu_map XML file is not safe due to the fact
>> that the cpu map does not exist for all architectures. Therefore it is
>> better to encapsulate the processing in the architecture specific cpu
>> drivers.
>>
>> Signed-off-by: Daniel Hansel <daniel.hansel@xxxxxxxxxxxxxxxxxx>
>> Reviewed-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxxxxxxx>
>> Reviewed-by: Viktor Mihajlovski <mihajlov@xxxxxxxxxxxxxxxxxx>
>> ---
>>  src/cpu/cpu.c         | 68 +++++++++------------------------------------------
>>  src/cpu/cpu.h         |  4 +++
>>  src/cpu/cpu_powerpc.c | 37 ++++++++++++++++++++++++++++
>>  src/cpu/cpu_x86.c     | 33 +++++++++++++++++++++++++
>>  4 files changed, 86 insertions(+), 56 deletions(-)
>>
> 
> The changes triggered a Coverity FORWARD_NULL...  Which uncovered a
> regression when 'models' is passed as NULL...
> 
>> diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c
>> index 08bec5e..788f688 100644
>> --- a/src/cpu/cpu.c
>> +++ b/src/cpu/cpu.c
>> @@ -724,43 +724,6 @@ cpuModelIsAllowed(const char *model,
>>      return false;
>>  }
>>  
>> -struct cpuGetModelsData
>> -{
>> -    char **data;
>> -    size_t len;  /* It includes the last element of DATA, which is NULL. */
>> -};
>> -
>> -static int
>> -cpuGetArchModelsCb(cpuMapElement element,
>> -                   xmlXPathContextPtr ctxt,
>> -                   void *cbdata)
>> -{
>> -    char *name;
>> -    struct cpuGetModelsData *data = cbdata;
>> -    if (element != CPU_MAP_ELEMENT_MODEL)
>> -        return 0;
>> -
>> -    name = virXPathString("string(@name)", ctxt);
>> -    if (name == NULL)
>> -        return -1;
>> -
>> -    if (!data->data) {
>> -        VIR_FREE(name);
>> -        data->len++;
>> -        return 0;
>> -    }
>> -
>> -    return VIR_INSERT_ELEMENT(data->data, data->len - 1, data->len, name);
>> -}
>> -
>> -
>> -static int
>> -cpuGetArchModels(const char *arch, struct cpuGetModelsData *data)
>> -{
>> -    return cpuMapLoad(arch, cpuGetArchModelsCb, data);
>> -}
>> -
>> -
>>  /**
>>   * cpuGetModels:
>>   *
>> @@ -774,18 +737,17 @@ cpuGetArchModels(const char *arch, struct cpuGetModelsData *data)
>>  int
>>  cpuGetModels(const char *archName, char ***models)
>>  {
>> -    struct cpuGetModelsData data;
>> -    virArch arch;
>>      struct cpuArchDriver *driver;
>> -    data.data = NULL;
>> -    data.len = 1;
>> +    virArch arch;
>> +
>> +    VIR_DEBUG("arch=%s", archName);
>>  
>>      arch = virArchFromString(archName);
>>      if (arch == VIR_ARCH_NONE) {
>>          virReportError(VIR_ERR_INVALID_ARG,
>>                         _("cannot find architecture %s"),
>>                         archName);
>> -        goto error;
>> +        return -1;
>>      }
>>  
>>      driver = cpuGetSubDriver(arch);
>> @@ -793,21 +755,15 @@ cpuGetModels(const char *archName, char ***models)
>>          virReportError(VIR_ERR_INVALID_ARG,
>>                         _("cannot find a driver for the architecture %s"),
>>                         archName);
>> -        goto error;
>> +        return -1;
>>      }
>>  
>> -    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;
>> +    if (!driver->getModels) {
>> +        virReportError(VIR_ERR_NO_SUPPORT,
>> +                       _("CPU driver for %s has no CPU model support"),
>> +                       virArchToString(arch));
>> +        return -1;
>> +    }
>>  
>> - error:
>> -    virStringFreeList(data.data);
>> -    return -1;
>> +    return driver->getModels(models);
>>  }
>> diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h
>> index 339964c..09e9538 100644
>> --- a/src/cpu/cpu.h
>> +++ b/src/cpu/cpu.h
>> @@ -100,6 +100,9 @@ typedef char *
>>  typedef virCPUDataPtr
>>  (*cpuArchDataParse) (const char *xmlStr);
>>  
>> +typedef int
>> +(*cpuArchGetModels) (char ***models);
>> +
>>  struct cpuArchDriver {
>>      const char *name;
>>      const virArch *arch;
>> @@ -115,6 +118,7 @@ struct cpuArchDriver {
>>      cpuArchHasFeature    hasFeature;
>>      cpuArchDataFormat   dataFormat;
>>      cpuArchDataParse    dataParse;
>> +    cpuArchGetModels    getModels;
>>  };
>>  
>>  
>> diff --git a/src/cpu/cpu_powerpc.c b/src/cpu/cpu_powerpc.c
>> index 67cb9ff..155d93e 100644
>> --- a/src/cpu/cpu_powerpc.c
>> +++ b/src/cpu/cpu_powerpc.c
>> @@ -649,6 +649,42 @@ ppcBaseline(virCPUDefPtr *cpus,
>>      goto cleanup;
>>  }
>>  
>> +static int
>> +ppcGetModels(char ***models)
>> +{
>> +    struct ppc_map *map;
>> +    struct ppc_model *model;
>> +    char *name;
>> +    size_t nmodels = 0;
>> +
>> +    if (!(map = ppcLoadMap()))
>> +        goto error;
>> +
>> +    if (models && VIR_ALLOC_N(*models, 0) < 0)
>> +        goto error;
>> +
>> +    model = map->models;
>> +    while (model != NULL) {
>> +        if (VIR_STRDUP(name, model->name) < 0)
>> +            goto error;
>> +
>> +        if (VIR_INSERT_ELEMENT(*models, 0, nmodels, name) < 0)
>> +            goto error;
>> +
>> +        model = model->next;
>> +    }
>> +
>> + cleanup:
>> +    ppcMapFree(map);
>> +
>> +    return nmodels;
>> +
>> + error:
>> +    virStringFreeList(*models);
>> +    nmodels = -1;
>> +    goto cleanup;
>> +}
>> +
>>  struct cpuArchDriver cpuDriverPowerPC = {
>>      .name = "ppc64",
>>      .arch = archs,
>> @@ -662,4 +698,5 @@ struct cpuArchDriver cpuDriverPowerPC = {
>>      .baseline   = ppcBaseline,
>>      .update     = ppcUpdate,
>>      .hasFeature = NULL,
>> +    .getModels  = ppcGetModels,
>>  };
>> diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
>> index 026b54e..f6dcba4 100644
>> --- a/src/cpu/cpu_x86.c
>> +++ b/src/cpu/cpu_x86.c
>> @@ -2160,6 +2160,38 @@ x86HasFeature(const virCPUData *data,
>>      return ret;
>>  }
>>  
>> +static int
>> +x86GetModels(char ***models)
>> +{
>> +    const struct x86_map *map;
>> +    struct x86_model *model;
>> +    char *name;
>> +    size_t nmodels = 0;
>> +
>> +    if (!(map = virCPUx86GetMap()))
>> +        return -1;
>> +
> 
> (5) Event var_compare_op: 	Comparing "models" to null implies that
> "models" might be null.
> 
>> +    if (models && VIR_ALLOC_N(*models, 0) < 0)
>> +        goto error;
>> +
>> +    model = map->models;
>> +    while (model != NULL) {
>> +        if (VIR_STRDUP(name, model->name) < 0)
>> +            goto error;
>> +
> 
> (9) Event var_deref_model: 	Passing null pointer "models" to
> "virInsertElementsN", which dereferences it. (The dereference is assumed
> on the basis of the 'nonnull' parameter attribute.)
> Also see events: 	[var_compare_op]
> 
> 
>> +        if (VIR_INSERT_ELEMENT(*models, 0, nmodels, name) < 0)
>> +            goto error;
> 
> So, what Coverity is saying is that you check for models before doing
> the VIR_ALLOC() above; however, here if models == NULL, then this piece
> of code is going to be very unhappy.
> 
> The previous code didn't have this issue because of the use of the local
> data and the assignment to *models only when models was non-null:
> 
> -    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;
> 
> 
> Since the external API's can be called with a NULL 'models' :
> 
>  * virConnectGetCPUModelNames:
>  *
>  * @conn: virConnect connection
>  * @arch: Architecture
>  * @models: Pointer to a variable to store the NULL-terminated array of the
>  *          CPU models supported for the specified architecture.  Each
> element
>  *          and the array itself must be freed by the caller with free.
>  Pass
>  *          NULL if only the list length is needed.
>  * @flags: extra flags; not used yet, so callers should always pass 0.
> 
> This and the ppc counterpart code needs some further adjustment to
> handle that case properly.  IOW: A way to count the number of map->models.
> 
> John
>> +
>> +        model = model->next;
>> +    }
>> +
>> +    return nmodels;
>> +
>> + error:
>> +    virStringFreeList(*models);
>> +    return -1;
>> +}
>> +
>>  
>>  struct cpuArchDriver cpuDriverX86 = {
>>      .name = "x86",
>> @@ -2180,4 +2212,5 @@ struct cpuArchDriver cpuDriverX86 = {
>>      .hasFeature = x86HasFeature,
>>      .dataFormat = x86CPUDataFormat,
>>      .dataParse  = x86CPUDataParse,
>> +    .getModels  = x86GetModels,
>>  };
>>
> 

-- 

Mit freundlichen Grüßen / Kind regards
Daniel Hansel

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

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





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]