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