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
