virDomainBlkioDeviceParseXML() has multiple cases of sending the
return from xmlNodeGetContent() directly to virStrToLong_xx() without
checking for NULL. Although it is *very* rare for xmlNodeGetContent()
to return NULL (possibly it only happens in an OOM condition? The
documentation is unclear), it could happen, and the refactor in this
patch manages to eliminate several lines of repeated code while adding
in a (single) check for NULL.

Signed-off-by: Laine Stump <laine@redhat.com>
---
 src/conf/domain_conf.c | 39 +++++++++++++++------------------------
 1 file changed, 15 insertions(+), 24 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1916b51d38..8cde1cd0e8 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1628,73 +1628,64 @@ virDomainBlkioDeviceParseXML(xmlNodePtr root,
                              virBlkioDevicePtr dev)
 {
     xmlNodePtr node;
-    g_autofree char *c = NULL;
+    g_autofree char *path = NULL;
 
     node = root->children;
     while (node) {
-        if (node->type == XML_ELEMENT_NODE) {
-            if (virXMLNodeNameEqual(node, "path") && !dev->path) {
-                dev->path = (char *)xmlNodeGetContent(node);
+        g_autofree char *c = NULL;
+
+        if (node->type == XML_ELEMENT_NODE &&
+            (c = (char *)xmlNodeGetContent(node))) {
+            if (virXMLNodeNameEqual(node, "path") && !path) {
+                path = g_steal_pointer(&c);
             } else if (virXMLNodeNameEqual(node, "weight")) {
-                c = (char *)xmlNodeGetContent(node);
                 if (virStrToLong_ui(c, NULL, 10, &dev->weight) < 0) {
                     virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                                    _("could not parse weight %s"),
                                    c);
-                        goto error;
+                    return -1;
                 }
-                VIR_FREE(c);
             } else if (virXMLNodeNameEqual(node, "read_bytes_sec")) {
-                c = (char *)xmlNodeGetContent(node);
                 if (virStrToLong_ull(c, NULL, 10, &dev->rbps) < 0) {
                     virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                                    _("could not parse read bytes sec %s"),
                                    c);
-                    goto error;
+                    return -1;
                 }
-                VIR_FREE(c);
             } else if (virXMLNodeNameEqual(node, "write_bytes_sec")) {
-                c = (char *)xmlNodeGetContent(node);
                 if (virStrToLong_ull(c, NULL, 10, &dev->wbps) < 0) {
                     virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                                    _("could not parse write bytes sec %s"),
                                    c);
-                    goto error;
+                    return -1;
                 }
-                VIR_FREE(c);
             } else if (virXMLNodeNameEqual(node, "read_iops_sec")) {
-                c = (char *)xmlNodeGetContent(node);
                 if (virStrToLong_ui(c, NULL, 10, &dev->riops) < 0) {
                     virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                                    _("could not parse read iops sec %s"),
                                    c);
-                    goto error;
+                    return -1;
                 }
-                VIR_FREE(c);
             } else if (virXMLNodeNameEqual(node, "write_iops_sec")) {
-                c = (char *)xmlNodeGetContent(node);
                 if (virStrToLong_ui(c, NULL, 10, &dev->wiops) < 0) {
                     virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                                    _("could not parse write iops sec %s"),
                                    c);
-                    goto error;
+                    return -1;
                 }
-                VIR_FREE(c);
             }
         }
         node = node->next;
     }
-    if (!dev->path) {
+
+    if (!path) {
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                        _("missing per-device path"));
         return -1;
     }
 
+    dev->path = g_steal_pointer(&path);
     return 0;
-
- error:
-    VIR_FREE(dev->path);
-    return -1;
 }
 
 
-- 
2.25.4

On a Wednesday in 2020, Laine Stump wrote:
>virDomainBlkioDeviceParseXML() has multiple cases of sending the
>return from xmlNodeGetContent() directly to virStrToLong_xx() without
>checking for NULL. Although it is *very* rare for xmlNodeGetContent()
>to return NULL (possibly it only happens in an OOM condition? The
>documentation is unclear), it could happen, and the refactor in this
>patch manages to eliminate several lines of repeated code while adding
>in a (single) check for NULL.
>
>Signed-off-by: Laine Stump <laine@redhat.com>
>---
> src/conf/domain_conf.c | 39 +++++++++++++++------------------------
> 1 file changed, 15 insertions(+), 24 deletions(-)
>
>diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>index 1916b51d38..8cde1cd0e8 100644
>--- a/src/conf/domain_conf.c
>+++ b/src/conf/domain_conf.c
>@@ -1628,73 +1628,64 @@ virDomainBlkioDeviceParseXML(xmlNodePtr root,
>                              virBlkioDevicePtr dev)
> {
>     xmlNodePtr node;
>-    g_autofree char *c = NULL;
>+    g_autofree char *path = NULL;
>
>     node = root->children;
>     while (node) {
>-        if (node->type == XML_ELEMENT_NODE) {
>-            if (virXMLNodeNameEqual(node, "path") && !dev->path) {
>-                dev->path = (char *)xmlNodeGetContent(node);
>+        g_autofree char *c = NULL;
>+
>+        if (node->type == XML_ELEMENT_NODE &&
>+            (c = (char *)xmlNodeGetContent(node))) {

Missing ErrorReport if xmlNodeGetContent fails.

Converting this open-coded for loop to an actual for loop would
grant us 'continue' privileges, which would make the checks
nicer and give a possibility of assigning the path directly
to 'path', without the extra steal_pointer.

Alternatively, the minimum diff where I'd consider this patch to be
a strict improvement is:

} else if (node->type == XML_ELEMENT_NODE && !c) {
     virReportOOMError();
     return -1;
}

With that: Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano

On 6/25/20 6:44 PM, Ján Tomko wrote:
> On a Wednesday in 2020, Laine Stump wrote:
>> virDomainBlkioDeviceParseXML() has multiple cases of sending the
>> return from xmlNodeGetContent() directly to virStrToLong_xx() without
>> checking for NULL. Although it is *very* rare for xmlNodeGetContent()
>> to return NULL (possibly it only happens in an OOM condition? The
>> documentation is unclear), it could happen, and the refactor in this
>> patch manages to eliminate several lines of repeated code while adding
>> in a (single) check for NULL.
>>
>> Signed-off-by: Laine Stump <laine@redhat.com>
>> ---
>> src/conf/domain_conf.c | 39 +++++++++++++++------------------------
>> 1 file changed, 15 insertions(+), 24 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 1916b51d38..8cde1cd0e8 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -1628,73 +1628,64 @@ virDomainBlkioDeviceParseXML(xmlNodePtr root,
>>                              virBlkioDevicePtr dev)
>> {
>>     xmlNodePtr node;
>> -    g_autofree char *c = NULL;
>> +    g_autofree char *path = NULL;
>>
>>     node = root->children;
>>     while (node) {
>> -        if (node->type == XML_ELEMENT_NODE) {
>> -            if (virXMLNodeNameEqual(node, "path") && !dev->path) {
>> -                dev->path = (char *)xmlNodeGetContent(node);
>> +        g_autofree char *c = NULL;
>> +
>> +        if (node->type == XML_ELEMENT_NODE &&
>> +            (c = (char *)xmlNodeGetContent(node))) {
>
> Missing ErrorReport if xmlNodeGetContent fails.


Well, I was uncertain whether or not it was *always* an error. The API 
docs don't specifically say, a google search revealed people asking the 
question, but nobody answering it definitively (I think there may have 
been some snarky condescending reply on stackexchange (par for the 
course), but no actual information), and I stopped trying to figure it 
out by looking at the libxml2 source after just a couple layers - ain't 
nobody got time for that!


But you apparently tried it out and determined that it will return "" 
rather than NULL as long as node->type == XML_ELEMENT_NODE, so I'll 
trust that and treat all NULL returns as OOM (including in a later patch).


>
>
> Converting this open-coded for loop to an actual for loop would
> grant us 'continue' privileges, which would make the checks
> nicer 


If you're averse to "else if" I guess.


> and give a possibility of assigning the path directly
> to 'path', without the extra steal_pointer.


I don't follow there - if you assign directly from xmlNodeGetContent() 
into path, then you'll need to duplicate the virReportOOMError().


Anyway, I'll turn it into a for() loop make the NULL return from 
xmlNodeGetContent() an error (rather than ignoring it) and resubmit, 
since it's too many changes to trust me on it.


>
> Alternatively, the minimum diff where I'd consider this patch to be
> a strict improvement is:
>
> } else if (node->type == XML_ELEMENT_NODE && !c) {
>     virReportOOMError();
>     return -1;
> }
>
> With that: Reviewed-by: Ján Tomko <jtomko@redhat.com>
>
> Jano

On 6/26/20 6:54 PM, Laine Stump wrote:
> On 6/25/20 6:44 PM, Ján Tomko wrote:
>
>> and give a possibility of assigning the path directly
>> to 'path', without the extra steal_pointer.
>
>
> I don't follow there - if you assign directly from xmlNodeGetContent() 
> into path, then you'll need to duplicate the virReportOOMError().


Sigh. Nevermind. It had already been several days since I wrote the 
code, so I'd completely forgotten it and hadn't looked back at the 
bottom yet (where I would have seen the "extra steal pointer" you mention).


Now I get it.