[PATCH v3 2/6] conf: modernize SEV XML parse and format methods
Boris Fiuczynski
fiuczy at linux.ibm.com
Mon Jun 28 15:27:55 UTC 2021
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 at 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
More information about the libvir-list
mailing list