[libvirt] [PATCHv2] virBitmapParse: Fix behavior in case of error and fix up callers

[Peter Krempa]

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. Fix up the rest not to shadow the error.
---

Version 2:
 - squashed cleanup of callers into this patch

 po/POTFILES.in          |  1 +
 src/conf/domain_conf.c  |  5 +----
 src/conf/network_conf.c |  3 ---
 src/nodeinfo.c          |  5 +----
 src/qemu/qemu_driver.c  |  2 --
 src/util/virbitmap.c    | 18 +++++++++---------
 src/xenxs/xen_sxpr.c    |  5 +----
 tests/cpuset            |  2 +-
 8 files changed, 14 insertions(+), 27 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/conf/domain_conf.c b/src/conf/domain_conf.c
index ea49d2c..8a187a6 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -10981,11 +10981,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/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;
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