Re: [PATCH v3 2/6] conf: modernize SEV XML parse and format methods

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 6/25/21 10:39 AM, Pavel Hrdina wrote:
On Tue, Jun 22, 2021 at 03:10:45PM +0200, Boris Fiuczynski wrote:
Make use of virDomainLaunchSecurity enum and automatic memory freeing.

Signed-off-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx>
---
  src/conf/domain_conf.c | 123 +++++++++++++++++++++--------------------
  src/conf/domain_conf.h |   2 +
  2 files changed, 64 insertions(+), 61 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index af2fd03d3c..93ec50ff5d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3490,7 +3490,7 @@ virDomainResctrlDefFree(virDomainResctrlDef *resctrl)
  }
-static void
+void
  virDomainSEVDefFree(virDomainSEVDef *def)
  {
      if (!def)
@@ -14719,73 +14719,66 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode,
                          xmlXPathContextPtr ctxt)
  {
      VIR_XPATH_NODE_AUTORESTORE(ctxt)
-    virDomainSEVDef *def;
      unsigned long policy;
      g_autofree char *type = NULL;
      int rc = -1;

No need for this empty line.

-    def = g_new0(virDomainSEVDef, 1);
+    g_autoptr(virDomainSEVDef) def = g_new0(virDomainSEVDef, 1);
ctxt->node = sevNode; if (!(type = virXMLPropString(sevNode, "type"))) {
          virReportError(VIR_ERR_XML_ERROR, "%s",
                         _("missing launch security type"));
-        goto error;
+        return NULL;
      }
def->sectype = virDomainLaunchSecurityTypeFromString(type);
      switch ((virDomainLaunchSecurity) def->sectype) {
      case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
-        break;
+        if (virXPathULongHex("string(./policy)", ctxt, &policy) < 0) {
+            virReportError(VIR_ERR_XML_ERROR, "%s",
+                           _("failed to get launch security policy for "
+                             "launch security type SEV"));
+            return NULL;
+        }
+
+        /* the following attributes are platform dependent and if missing,
+         * we can autofill them from domain capabilities later
+         */
+        rc = virXPathUInt("string(./cbitpos)", ctxt, &def->cbitpos);
+        if (rc == 0) {
+            def->haveCbitpos = true;
+        } else if (rc == -2) {
+            virReportError(VIR_ERR_XML_ERROR, "%s",
+                           _("Invalid format for launch security cbitpos"));
+            return NULL;
+        }
+
+        rc = virXPathUInt("string(./reducedPhysBits)", ctxt,
+                          &def->reduced_phys_bits);
+        if (rc == 0) {
+            def->haveReducedPhysBits = true;
+        } else if (rc == -2) {
+            virReportError(VIR_ERR_XML_ERROR, "%s",
+                           _("Invalid format for launch security "
+                             "reduced-phys-bits"));
+            return NULL;
+        }
+
+        def->policy = policy;
+        def->dh_cert = virXPathString("string(./dhCert)", ctxt);
+        def->session = virXPathString("string(./session)", ctxt);
+
+        return g_steal_pointer(&def);
      case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
      case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
      default:
          virReportError(VIR_ERR_XML_ERROR,
                         _("unsupported launch security type '%s'"),
                         type);
-        goto error;
-    }
-
-    if (virXPathULongHex("string(./policy)", ctxt, &policy) < 0) {
-        virReportError(VIR_ERR_XML_ERROR, "%s",
-                       _("failed to get launch security policy for "
-                         "launch security type SEV"));
-        goto error;
-    }
-
-    /* the following attributes are platform dependent and if missing, we can
-     * autofill them from domain capabilities later
-     */
-    rc = virXPathUInt("string(./cbitpos)", ctxt, &def->cbitpos);
-    if (rc == 0) {
-        def->haveCbitpos = true;
-    } else if (rc == -2) {
-        virReportError(VIR_ERR_XML_ERROR, "%s",
-                       _("Invalid format for launch security cbitpos"));
-        goto error;
-    }
-
-    rc = virXPathUInt("string(./reducedPhysBits)", ctxt,
-                      &def->reduced_phys_bits);
-    if (rc == 0) {
-        def->haveReducedPhysBits = true;
-    } else if (rc == -2) {
-        virReportError(VIR_ERR_XML_ERROR, "%s",
-                       _("Invalid format for launch security "
-                         "reduced-phys-bits"));
-        goto error;
+        return NULL;
      }
-
-    def->policy = policy;
-    def->dh_cert = virXPathString("string(./dhCert)", ctxt);
-    def->session = virXPathString("string(./session)", ctxt);
-
-    return def;
-
- error:
-    virDomainSEVDefFree(def);
-    return NULL;
  }
@@ -26844,25 +26837,33 @@ virDomainSEVDefFormat(virBuffer *buf, virDomainSEVDef *sev)
      if (!sev)
          return;
- virBufferAsprintf(buf, "<launchSecurity type='%s'>\n",
-                      virDomainLaunchSecurityTypeToString(sev->sectype));
-    virBufferAdjustIndent(buf, 2);
+    switch ((virDomainLaunchSecurity) sev->sectype) {
+    case VIR_DOMAIN_LAUNCH_SECURITY_SEV: {
+        virBufferAsprintf(buf, "<launchSecurity type='%s'>\n",
+                          virDomainLaunchSecurityTypeToString(sev->sectype));

I would keep this line out out of the switch as it can be reused by
other launchSecurity types.

In the spirit of modernizing the parse/format code you can user the
following pattern witch will make addition of other types with
sub-elements easier:

     g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
     g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER;

     virBufferAsprintf(buf, " type='%s'",
                       virDomainLaunchSecurityTypeToString(sev->sectype));

     switch ((virDomainLaunchSecurity) sev->sectype) {
     case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
         if (sev->haveCbitpos)
             virBufferAsprintf(childBuf, "<cbitpos>%d</cbitpos>\n", sev->cbitpos);

         ...

     }

     virXMLFormatElement(buf, "launchSecurity", &attrBuf, &childBuf);


With this helper we don't have to duplicate the code formatting
launchSecurity type and it will also correctly handle if there are any
sub-elements or not.

Otherwise looks good.

Pavel


Good idea. I will change it. Thanks for the review.

+        virBufferAdjustIndent(buf, 2);
- if (sev->haveCbitpos)
-        virBufferAsprintf(buf, "<cbitpos>%d</cbitpos>\n", sev->cbitpos);
+        if (sev->haveCbitpos)
+            virBufferAsprintf(buf, "<cbitpos>%d</cbitpos>\n", sev->cbitpos);
- if (sev->haveReducedPhysBits)
-        virBufferAsprintf(buf, "<reducedPhysBits>%d</reducedPhysBits>\n",
-                          sev->reduced_phys_bits);
-    virBufferAsprintf(buf, "<policy>0x%04x</policy>\n", sev->policy);
-    if (sev->dh_cert)
-        virBufferEscapeString(buf, "<dhCert>%s</dhCert>\n", sev->dh_cert);
+        if (sev->haveReducedPhysBits)
+            virBufferAsprintf(buf, "<reducedPhysBits>%d</reducedPhysBits>\n",
+                              sev->reduced_phys_bits);
+        virBufferAsprintf(buf, "<policy>0x%04x</policy>\n", sev->policy);
+        if (sev->dh_cert)
+            virBufferEscapeString(buf, "<dhCert>%s</dhCert>\n", sev->dh_cert);
- if (sev->session)
-        virBufferEscapeString(buf, "<session>%s</session>\n", sev->session);
+        if (sev->session)
+            virBufferEscapeString(buf, "<session>%s</session>\n", sev->session);
- virBufferAdjustIndent(buf, -2);
-    virBufferAddLit(buf, "</launchSecurity>\n");
+        virBufferAdjustIndent(buf, -2);
+        virBufferAddLit(buf, "</launchSecurity>\n");
+        break;
+    }
+    case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
+    case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
+        break;
+    }
  }
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index f706c498ff..512c7c8bd7 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3285,6 +3285,8 @@ void virDomainRedirdevDefFree(virDomainRedirdevDef *def);
  void virDomainRedirFilterDefFree(virDomainRedirFilterDef *def);
  void virDomainShmemDefFree(virDomainShmemDef *def);
  G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainShmemDef, virDomainShmemDefFree);
+void virDomainSEVDefFree(virDomainSEVDef *def);
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainSEVDef, virDomainSEVDefFree);
  void virDomainDeviceDefFree(virDomainDeviceDef *def);
G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainDeviceDef, virDomainDeviceDefFree);
--
2.30.2



--
Mit freundlichen Grüßen/Kind regards
   Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux