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(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; Reviewed-by: Viktor Mihajlovski <mihajlov(a)linux.vnet.ibm.com&gt; --- 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(-) 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; + + 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; + } + + 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, };

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(a)linux.vnet.ibm.com&gt; > Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; > Reviewed-by: Viktor Mihajlovski <mihajlov(a)linux.vnet.ibm.com&gt; > --- > 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, > }; >