Re-arrange the code so that the returned bitmap is always initialized to NULL even on early failures and return an error message as some callers are already expecting it. --- Notes: Version 2: Was already ACKed in v1, but: * fixed bracing of arguments of the _() macro * added src/uti/virbitmap.c to po/POTFILES.in (noticed by doing a full build ... ) po/POTFILES.in | 1 + src/util/virbitmap.c | 18 +++++++++--------- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/po/POTFILES.in b/po/POTFILES.in index 36d027a..9a83069 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -147,6 +147,7 @@ src/util/viralloc.c src/util/viraudit.c src/util/virauth.c src/util/virauthconfig.c +src/util/virbitmap.c src/util/vircgroup.c src/util/virclosecallbacks.c src/util/vircommand.c diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index 47c678e..289a7b9 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -298,23 +298,21 @@ virBitmapParse(const char *str, size_t bitmapSize) { bool neg = false; - const char *cur; + const char *cur = str; char *tmp; size_t i; int start, last; - if (!str) + if (!(*bitmap = virBitmapNew(bitmapSize))) return -1; - cur = str; - virSkipSpaces(&cur); + if (!str) + goto error; - if (*cur == 0) - return -1; + virSkipSpaces(&cur); - *bitmap = virBitmapNew(bitmapSize); - if (!*bitmap) - return -1; + if (*cur == '\0') + goto error; while (*cur != 0 && *cur != terminator) { /* @@ -384,6 +382,8 @@ virBitmapParse(const char *str, return virBitmapCountBits(*bitmap); error: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to parse bitmap '%s'"), str); virBitmapFree(*bitmap); *bitmap = NULL; return -1; -- 1.8.3.2

A few of the callers of virBitmapParse shadow the returned error. --- Notes: I'm kind of worried that we are making some error messages worse compared to what they were before. If you don't like the way this will turn out, I'll prepare a inverse fix where all callers will be fixed to report proper errors and virBitmapParse will remain silent. src/conf/domain_conf.c | 5 +---- src/conf/network_conf.c | 3 --- src/nodeinfo.c | 5 +---- src/qemu/qemu_driver.c | 2 -- src/xenxs/xen_sxpr.c | 5 +---- tests/cpuset | 2 +- 6 files changed, 4 insertions(+), 18 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 12b68ea..3fa4f00 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10971,11 +10971,8 @@ virDomainDefParseXML(xmlDocPtr xml, tmp = virXPathString("string(./vcpu[1]/@cpuset)", ctxt); if (tmp) { if (virBitmapParse(tmp, 0, &def->cpumask, - VIR_DOMAIN_CPUMASK_LEN) < 0) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("topology cpuset syntax error")); + VIR_DOMAIN_CPUMASK_LEN) < 0) goto error; - } VIR_FREE(tmp); } } diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index bbc980b..8aef609 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2897,9 +2897,6 @@ virNetworkLoadState(virNetworkObjListPtr nets, if ((class_id = virXPathString("string(./class_id[1]/@bitmap)", ctxt))) { if (virBitmapParse(class_id, 0, &class_id_map, CLASS_ID_BITMAP_SIZE) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Malformed 'class_id' attribute: %s"), - class_id); VIR_FREE(class_id); goto error; } diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 4df4851..33a79b7 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -1547,11 +1547,8 @@ virNodeGetSiblingsList(const char *dir, int cpu_id) if (virFileReadAll(path, SYSFS_THREAD_SIBLINGS_LIST_LENGTH_MAX, &buf) < 0) goto cleanup; - if (virBitmapParse(buf, 0, &ret, NUMA_MAX_N_CPUS) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Failed to parse thread siblings")); + if (virBitmapParse(buf, 0, &ret, NUMA_MAX_N_CPUS) < 0) goto cleanup; - } cleanup: VIR_FREE(buf); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2ad236e..5124f27 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8357,8 +8357,6 @@ qemuDomainSetNumaParameters(virDomainPtr dom, if (virBitmapParse(params[i].value.s, 0, &nodeset, VIR_DOMAIN_CPUMASK_LEN) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Failed to parse nodeset")); ret = -1; continue; } diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index fbbbaa9..6209c68 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -1160,11 +1160,8 @@ xenParseSxpr(const struct sexpr *root, if (cpus != NULL) { if (virBitmapParse(cpus, 0, &def->cpumask, - VIR_DOMAIN_CPUMASK_LEN) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("invalid CPU mask %s"), cpus); + VIR_DOMAIN_CPUMASK_LEN) < 0) goto error; - } } def->maxvcpus = sexpr_int(root, "domain/vcpus"); diff --git a/tests/cpuset b/tests/cpuset index b617d6f..8bcaae9 100755 --- a/tests/cpuset +++ b/tests/cpuset @@ -42,7 +42,7 @@ sed "s/vcpu placement='static'>/vcpu cpuset='aaa'>/" xml > xml-invalid || fail=1 $abs_top_builddir/tools/virsh --connect test:///default define xml-invalid > out 2>&1 && fail=1 cat <<\EOF > exp || fail=1 error: Failed to define domain from xml-invalid -error: XML error: topology cpuset syntax error +error: internal error: Failed to parse bitmap 'aaa' EOF compare exp out || fail=1 -- 1.8.3.2