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

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, > }; >