Firstly, this function changes node for relative XPaths but
doesn't restore the original one in case VIR_ALLOC(def) fails.
Secondly, @type is leaked. Thirdly, dh-cert and session
attributes are strdup()-ed needlessly, virXPathString already
does that so we can use the retval immediately.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/conf/domain_conf.c | 30 +++++++++---------------------
 1 file changed, 9 insertions(+), 21 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 85f07af46e..c788b525b5 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15849,17 +15849,16 @@ static virDomainSevDefPtr
 virDomainSEVDefParseXML(xmlNodePtr sevNode,
                         xmlXPathContextPtr ctxt)
 {
-    char *tmp = NULL;
     char *type = NULL;
     xmlNodePtr save = ctxt->node;
     virDomainSevDefPtr def;
     unsigned long policy;
 
+    if (VIR_ALLOC(def) < 0)
+        return NULL;
+
     ctxt->node = sevNode;
 
-    if (VIR_ALLOC(def) < 0)
-        return NULL;
-
     if (!(type = virXMLPropString(sevNode, "type"))) {
         virReportError(VIR_ERR_XML_ERROR, "%s",
                        _("missing launch-security type"));
@@ -15899,29 +15898,18 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode,
     }
 
     def->policy = policy;
+    def->dh_cert = virXPathString("string(./dh-cert)", ctxt);
+    def->session = virXPathString("string(./session)", ctxt);
 
-    if ((tmp = virXPathString("string(./dh-cert)", ctxt))) {
-        if (VIR_STRDUP(def->dh_cert, tmp) < 0)
-            goto error;
-
-        VIR_FREE(tmp);
-    }
-
-    if ((tmp = virXPathString("string(./session)", ctxt))) {
-        if (VIR_STRDUP(def->session, tmp) < 0)
-            goto error;
-
-        VIR_FREE(tmp);
-    }
-
+ cleanup:
+    VIR_FREE(type);
     ctxt->node = save;
     return def;
 
  error:
-    VIR_FREE(tmp);
     virDomainSEVDefFree(def);
-    ctxt->node = save;
-    return NULL;
+    def = NULL;
+    goto cleanup;
 }
 
 static virDomainMemoryDefPtr
-- 
2.16.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, Jun 13, 2018 at 12:51:59PM +0200, Michal Privoznik wrote:
>Firstly, this function changes node for relative XPaths but
>doesn't restore the original one in case VIR_ALLOC(def) fails.

This should not matter, since we're going to abort parsing anyway.

>Secondly, @type is leaked. Thirdly, dh-cert and session

s/dh-cert/dhCert

It has been renamed in the meantime.

>attributes are strdup()-ed needlessly, virXPathString already
>does that so we can use the retval immediately.
>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> src/conf/domain_conf.c | 30 +++++++++---------------------
> 1 file changed, 9 insertions(+), 21 deletions(-)
>
>diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>index 85f07af46e..c788b525b5 100644
>--- a/src/conf/domain_conf.c
>+++ b/src/conf/domain_conf.c
>@@ -15849,17 +15849,16 @@ static virDomainSevDefPtr
> virDomainSEVDefParseXML(xmlNodePtr sevNode,
>                         xmlXPathContextPtr ctxt)
> {
>-    char *tmp = NULL;
>     char *type = NULL;
>     xmlNodePtr save = ctxt->node;
>     virDomainSevDefPtr def;
>     unsigned long policy;
>
>+    if (VIR_ALLOC(def) < 0)
>+        return NULL;
>+
>     ctxt->node = sevNode;
>
>-    if (VIR_ALLOC(def) < 0)
>-        return NULL;
>-

Or just 'goto error' instead of moving the allocation.
Not sure which option is more future-proof.

>     if (!(type = virXMLPropString(sevNode, "type"))) {
>         virReportError(VIR_ERR_XML_ERROR, "%s",
>                        _("missing launch-security type"));
>@@ -15899,29 +15898,18 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode,
>     }
>
>     def->policy = policy;
>+    def->dh_cert = virXPathString("string(./dh-cert)", ctxt);

string(./dhCert)

>+    def->session = virXPathString("string(./session)", ctxt);
>
>-    if ((tmp = virXPathString("string(./dh-cert)", ctxt))) {
>-        if (VIR_STRDUP(def->dh_cert, tmp) < 0)
>-            goto error;
>-
>-        VIR_FREE(tmp);
>-    }
>-

With the dh-cert -> dhCert adjustments:

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

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list