Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
---
 src/conf/node_device_conf.c | 34 +++++++++++++---------------------
 1 file changed, 13 insertions(+), 21 deletions(-)

diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 4477a8d9d2..5ac046f768 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -2059,21 +2059,19 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt,
                          int create,
                          const char *virt_type)
 {
-    virNodeDeviceDef *def;
+    g_autoptr(virNodeDeviceDef) def = g_new0(virNodeDeviceDef, 1);
     virNodeDevCapsDef **next_cap;
-    xmlNodePtr *nodes = NULL;
+    g_autofree xmlNodePtr *nodes = NULL;
     int n, m;
     size_t i;
 
-    def = g_new0(virNodeDeviceDef, 1);
-
     /* Extract device name */
     if (create == EXISTING_DEVICE) {
         def->name = virXPathString("string(./name[1])", ctxt);
 
         if (!def->name) {
             virReportError(VIR_ERR_NO_NAME, NULL);
-            goto error;
+            return NULL;
         }
     } else {
         def->name = g_strdup("new device");
@@ -2083,7 +2081,7 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt,
 
     /* Parse devnodes */
     if ((n = virXPathNodeSet("./devnode", ctxt, &nodes)) < 0)
-        goto error;
+        return NULL;
 
     def->devlinks = g_new0(char *, n + 1);
 
@@ -2093,16 +2091,16 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt,
 
         if (virXMLPropEnum(node, "type", virNodeDevDevnodeTypeFromString,
                            VIR_XML_PROP_REQUIRED, &val) < 0)
-            goto error;
+            return NULL;
 
         switch (val) {
         case VIR_NODE_DEV_DEVNODE_DEV:
             if (!(def->devnode = virXMLNodeContentString(node)))
-                goto error;
+                return NULL;
             break;
         case VIR_NODE_DEV_DEVNODE_LINK:
             if (!(def->devlinks[m++] = virXMLNodeContentString(node)))
-                goto error;
+                return NULL;
             break;
         case VIR_NODE_DEV_DEVNODE_LAST:
             break;
@@ -2118,7 +2116,7 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt,
                        _("when providing parent wwnn='%s', the "
                          "wwpn must also be provided"),
                        def->parent_wwnn);
-        goto error;
+        return NULL;
     }
 
     if (!def->parent_wwnn && def->parent_wwpn) {
@@ -2126,7 +2124,7 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt,
                        _("when providing parent wwpn='%s', the "
                          "wwnn must also be provided"),
                        def->parent_wwpn);
-        goto error;
+        return NULL;
     }
     def->parent_fabric_wwn = virXPathString("string(./parent[1]/@fabric_wwn)",
                                             ctxt);
@@ -2134,13 +2132,13 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt,
     /* Parse device capabilities */
     VIR_FREE(nodes);
     if ((n = virXPathNodeSet("./capability", ctxt, &nodes)) < 0)
-        goto error;
+        return NULL;
 
     if (n == 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("no device capabilities for '%s'"),
                        def->name);
-        goto error;
+        return NULL;
     }
 
     next_cap = &def->caps;
@@ -2150,18 +2148,12 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt,
                                               create,
                                               virt_type);
         if (!*next_cap)
-            goto error;
+            return NULL;
 
         next_cap = &(*next_cap)->next;
     }
-    VIR_FREE(nodes);
 
-    return def;
-
- error:
-    virNodeDeviceDefFree(def);
-    VIR_FREE(nodes);
-    return NULL;
+    return g_steal_pointer(&def);
 }
 
 
-- 
2.26.3

On 5/11/21 11:01 AM, Tim Wiederhake wrote:
> Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
> ---
>   src/conf/node_device_conf.c | 34 +++++++++++++---------------------
>   1 file changed, 13 insertions(+), 21 deletions(-)
> 
> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
> index 4477a8d9d2..5ac046f768 100644
> --- a/src/conf/node_device_conf.c
> +++ b/src/conf/node_device_conf.c
> @@ -2059,21 +2059,19 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt,
>                            int create,
>                            const char *virt_type)
>   {
> -    virNodeDeviceDef *def;
> +    g_autoptr(virNodeDeviceDef) def = g_new0(virNodeDeviceDef, 1);
>       virNodeDevCapsDef **next_cap;
> -    xmlNodePtr *nodes = NULL;
> +    g_autofree xmlNodePtr *nodes = NULL;
>       int n, m;
>       size_t i;
>   
> -    def = g_new0(virNodeDeviceDef, 1);
> -
>       /* Extract device name */
>       if (create == EXISTING_DEVICE) {
>           def->name = virXPathString("string(./name[1])", ctxt);
>   
>           if (!def->name) {
>               virReportError(VIR_ERR_NO_NAME, NULL);
> -            goto error;
> +            return NULL;
>           }
>       } else {
>           def->name = g_strdup("new device");
> @@ -2083,7 +2081,7 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt,
>   
>       /* Parse devnodes */
>       if ((n = virXPathNodeSet("./devnode", ctxt, &nodes)) < 0)
> -        goto error;
> +        return NULL;
>   
>       def->devlinks = g_new0(char *, n + 1);
>   
> @@ -2093,16 +2091,16 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt,
>   
>           if (virXMLPropEnum(node, "type", virNodeDevDevnodeTypeFromString,
>                              VIR_XML_PROP_REQUIRED, &val) < 0)
> -            goto error;
> +            return NULL;
>   
>           switch (val) {
>           case VIR_NODE_DEV_DEVNODE_DEV:
>               if (!(def->devnode = virXMLNodeContentString(node)))
> -                goto error;
> +                return NULL;
>               break;
>           case VIR_NODE_DEV_DEVNODE_LINK:
>               if (!(def->devlinks[m++] = virXMLNodeContentString(node)))
> -                goto error;
> +                return NULL;
>               break;
>           case VIR_NODE_DEV_DEVNODE_LAST:
>               break;
> @@ -2118,7 +2116,7 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt,
>                          _("when providing parent wwnn='%s', the "
>                            "wwpn must also be provided"),
>                          def->parent_wwnn);
> -        goto error;
> +        return NULL;
>       }
>   
>       if (!def->parent_wwnn && def->parent_wwpn) {
> @@ -2126,7 +2124,7 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt,
>                          _("when providing parent wwpn='%s', the "
>                            "wwnn must also be provided"),
>                          def->parent_wwpn);
> -        goto error;
> +        return NULL;
>       }
>       def->parent_fabric_wwn = virXPathString("string(./parent[1]/@fabric_wwn)",
>                                               ctxt);
> @@ -2134,13 +2132,13 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt,
>       /* Parse device capabilities */
>       VIR_FREE(nodes);

The presence of a VIR_FREE() of a pointer that was declared g_autofree 
points out that it's being re-used, which we had decided is a bad idea 
(unless more discussion has happened on the topic that I missed). You 
should instead define two separate g_autofree pointers, one for each use.

>       if ((n = virXPathNodeSet("./capability", ctxt, &nodes)) < 0)
> -        goto error;
> +        return NULL;
>   
>       if (n == 0) {
>           virReportError(VIR_ERR_INTERNAL_ERROR,
>                          _("no device capabilities for '%s'"),
>                          def->name);
> -        goto error;
> +        return NULL;
>       }
>   
>       next_cap = &def->caps;
> @@ -2150,18 +2148,12 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt,
>                                                 create,
>                                                 virt_type);
>           if (!*next_cap)
> -            goto error;
> +            return NULL;
>   
>           next_cap = &(*next_cap)->next;
>       }
> -    VIR_FREE(nodes);
>   
> -    return def;
> -
> - error:
> -    virNodeDeviceDefFree(def);
> -    VIR_FREE(nodes);
> -    return NULL;
> +    return g_steal_pointer(&def);
>   }
>   
>   
>