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@linux.ibm.com> --- 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