[libvirt] [PATCH 2/4] cpu: push more parsing logic into common code
John Ferlan
jferlan at redhat.com
Mon Aug 13 22:27:06 UTC 2018
On 08/01/2018 01:02 PM, Daniel P. Berrangé wrote:
> The x86 and ppc impls both duplicate some logic when parsing CPU
> features. Change the callback signature so that this duplication can be
> pushed up a level to common code.
>
> Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> ---
> src/cpu/cpu_map.c | 106 +++++++++++++++---------
> src/cpu/cpu_map.h | 22 ++---
> src/cpu/cpu_ppc64.c | 112 ++++++-------------------
> src/cpu/cpu_x86.c | 196 +++++++++++++-------------------------------
> 4 files changed, 155 insertions(+), 281 deletions(-)
>
> diff --git a/src/cpu/cpu_map.c b/src/cpu/cpu_map.c
> index 9e090919ed..17ed53fda6 100644
> --- a/src/cpu/cpu_map.c
> +++ b/src/cpu/cpu_map.c
> @@ -35,31 +35,51 @@
>
> VIR_LOG_INIT("cpu.cpu_map");
>
> -VIR_ENUM_IMPL(cpuMapElement, CPU_MAP_ELEMENT_LAST,
> - "vendor",
> - "feature",
> - "model")
> -
> -
> -static int load(xmlXPathContextPtr ctxt,
> - cpuMapElement element,
> - cpuMapLoadCallback callback,
> - void *data)
> +static int
> +loadData(const char *mapfile,
> + xmlXPathContextPtr ctxt,
> + const char *xpath,
> + cpuMapLoadCallback callback,
> + void *data)
> {
> int ret = -1;
> xmlNodePtr ctxt_node;
> xmlNodePtr *nodes = NULL;
> int n;
> + size_t i;
> + int rv;
>
> ctxt_node = ctxt->node;
>
> - n = virXPathNodeSet(cpuMapElementTypeToString(element), ctxt, &nodes);
> - if (n < 0)
> + n = virXPathNodeSet(xpath, ctxt, &nodes);
> + if (n < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("cannot find '%s' in CPU map '%s'"), xpath, mapfile);
> goto cleanup;
> + }
>
> - if (n > 0 &&
> - callback(element, ctxt, nodes, n, data) < 0)
> + if (n > 0 && !callback) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unexpected %s in CPU map '%s'"), xpath, mapfile);
How about "Unexpected element %s..."
to be fair it's a callback function isn't provided for a specific arch,
but adding that level of detail would be a bit more painful for the low
level of benefit at least.
> goto cleanup;
> + }
> +
[...]
>
> int cpuMapLoad(const char *arch,
> - cpuMapLoadCallback cb,
> + cpuMapLoadCallback vendorCB,
> + cpuMapLoadCallback featureCB,
> + cpuMapLoadCallback modelCB,
> void *data)
> {
> xmlDocPtr xml = NULL;
> @@ -157,7 +183,6 @@ int cpuMapLoad(const char *arch,
> virBuffer buf = VIR_BUFFER_INITIALIZER;
> char *xpath = NULL;
> int ret = -1;
> - int element;
> char *mapfile;
>
> if (!(mapfile = virFileFindResource("cpu_map.xml",
> @@ -173,9 +198,15 @@ int cpuMapLoad(const char *arch,
> goto cleanup;
> }
>
> - if (cb == NULL) {
> + if (vendorCB == NULL) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + "%s", _("no vendor callback provided"));
> + goto cleanup;
> + }
To be fair, loadData does check "if (n > 0 && !callback)", so these
checks perhaps aren't necessary as if they were NULL we'd fail a bit
later on (if I'm reading things properly ;-)).
I suppose it doesn't matter if they stay or not until someone, some day
wonders why featureCB isn't checked.
> +
> + if (modelCB == NULL) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> - "%s", _("no callback provided"));
> + "%s", _("no model callback provided"));
> goto cleanup;
> }
>
[...]
> diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c
> index d562677fa3..75da5b77d8 100644
> --- a/src/cpu/cpu_ppc64.c
> +++ b/src/cpu/cpu_ppc64.c
> @@ -281,21 +281,19 @@ ppc64MapFree(struct ppc64_map *map)
> VIR_FREE(map);
> }
>
[...]
> ppc64ModelParse(xmlXPathContextPtr ctxt,
> - struct ppc64_map *map)
> + const char *name,
> + void *data)
> {
> + struct ppc64_map *map = data;
> struct ppc64_model *model;
> xmlNodePtr *nodes = NULL;
> char *vendor = NULL;
> unsigned long pvr;
> size_t i;
> int n;
> + int ret = -1;
>
> if (VIR_ALLOC(model) < 0)
> goto error;
>
> - model->name = virXPathString("string(@name)", ctxt);
> - if (!model->name) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - "%s", _("Missing CPU model name"));
> + if (VIR_STRDUP(model->name, name) < 0)
> goto error;
> - }
>
> if (ppc64ModelFind(map, model->name)) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -410,63 +387,22 @@ ppc64ModelParse(xmlXPathContextPtr ctxt,
> model->data.pvr[i].mask = pvr;
> }
>
> + if (VIR_APPEND_ELEMENT(map->models, map->nmodels, model) < 0)
> + goto error;
> +
Since VIR_APPEND_ELEMENT would clear @model on success, we don't
necessarily need the error and cleanup labels. More churn on the changes
though with the ppc64ModelFree moved to cleanup.
> + ret = 0;
> +
> cleanup:
> VIR_FREE(vendor);
> VIR_FREE(nodes);
> - return model;
> + return ret;
>
> error:
> ppc64ModelFree(model);
> - model = NULL;
> goto cleanup;
> }
>
>
[...]
> diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
> index 809da94117..76f1d417c1 100644
> --- a/src/cpu/cpu_x86.c
> +++ b/src/cpu/cpu_x86.c
> @@ -712,22 +712,21 @@ x86VendorFind(virCPUx86MapPtr map,
> }
>
>
> -static virCPUx86VendorPtr
> +static int
> x86VendorParse(xmlXPathContextPtr ctxt,
> - virCPUx86MapPtr map)
> + const char *name,
> + void *data)
> {
> + virCPUx86MapPtr map = data;
> virCPUx86VendorPtr vendor = NULL;
> char *string = NULL;
> + int ret = -1;
>
> if (VIR_ALLOC(vendor) < 0)
> goto error;
>
> - vendor->name = virXPathString("string(@name)", ctxt);
> - if (!vendor->name) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("Missing CPU vendor name"));
> + if (VIR_STRDUP(vendor->name, name) < 0)
> goto error;
> - }
>
> if (x86VendorFind(map, vendor->name)) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -746,40 +745,21 @@ x86VendorParse(xmlXPathContextPtr ctxt,
> if (virCPUx86VendorToCPUID(string, &vendor->cpuid) < 0)
> goto error;
>
> + if (VIR_APPEND_ELEMENT(map->vendors, map->nvendors, vendor) < 0)
> + goto error;
Similar comment here regarding macro and cleanup/error labels
> +
> + ret = 0;
> +
> cleanup:
> VIR_FREE(string);
> - return vendor;
> + return ret;
>
> error:
> x86VendorFree(vendor);
> - vendor = NULL;
> goto cleanup;
> }
>
>
[...]
> -static virCPUx86FeaturePtr
> +static int
> x86FeatureParse(xmlXPathContextPtr ctxt,
> - virCPUx86MapPtr map)
> + const char *name,
> + void *data)
> {
> + virCPUx86MapPtr map = data;
> xmlNodePtr *nodes = NULL;
> virCPUx86FeaturePtr feature;
> virCPUx86CPUID cpuid;
> size_t i;
> int n;
> char *str = NULL;
> + int ret = -1;
>
> if (!(feature = x86FeatureNew()))
> goto error;
>
> feature->migratable = true;
> - feature->name = virXPathString("string(@name)", ctxt);
> - if (!feature->name) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - "%s", _("Missing CPU feature name"));
> +
> + if (VIR_STRDUP(feature->name, name) < 0)
> goto error;
> - }
>
> if (x86FeatureFind(map, feature->name)) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -949,46 +929,28 @@ x86FeatureParse(xmlXPathContextPtr ctxt,
> goto error;
> }
>
> + if (!feature->migratable &&
> + VIR_APPEND_ELEMENT_COPY(map->migrate_blockers,
> + map->nblockers,
> + feature) < 0)
> + goto error;
> +
> + if (VIR_APPEND_ELEMENT(map->features, map->nfeatures, feature) < 0)
> + goto error;
> +
Same here too.
> + ret = 0;
> +
> cleanup:
> VIR_FREE(nodes);
> VIR_FREE(str);
> - return feature;
> + return ret;
>
> error:
> x86FeatureFree(feature);
> - feature = NULL;
> goto cleanup;
> }
>
>
[...]
> +static int
> x86ModelParse(xmlXPathContextPtr ctxt,
> - virCPUx86MapPtr map)
> + const char *name,
> + void *data)
> {
[...]
>
> + if (VIR_APPEND_ELEMENT(map->models, map->nmodels, model) < 0)
> + goto error;
Similar regarding cleanup/error; however, ...
> +
> + ret = 0;
> +
> cleanup:
I ran the changes through Coverity and it complains here because one can
goto cleanup if the signature code fails, return -1, but the @model
wouldn't be free'd.
Prior to this change it seems failing in that code wasn't necessarily an
error, but it would generate the error, not go through the vendor and
feature processing, and return a somewhat empty @model. Perhaps a bug in
existing code, but uncovered in this refactoring.
> VIR_FREE(vendor);
> VIR_FREE(nodes);
> - return model;
> + return ret;
>
> error:
> x86ModelFree(model);
> - model = NULL;
> goto cleanup;
> }
>
>
[...]
Whether the VIR_APPEND_ELEMENT cleanup/error changes are made is your
call. The code is fine as is.
I think the signature return issue noted in x86ModelParse should
probably be it's own patch, but I'm not 100% clear what would happen if
you now error instead of just returning a mostly empty model. Since
it'd be developer error generating the file, perhaps we should just
error. Hopefully Jirka chimes in!
Reviewed-by: John Ferlan <jferlan at redhat.com>
John
More information about the libvir-list
mailing list