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

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(a)linux.ibm.com&gt; > --- > 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 >