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 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

> +        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
> 

Attachment: signature.asc
Description: PGP signature


[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