[PATCH v2 4/8] domain_conf.c: XML parsing for VIR_DOMAIN_TPM_MODEL_SPAPR_PROXY

[Daniel Henrique Barboza]

On 5/13/20 11:35 AM, Stefan Berger wrote:
> On 5/13/20 10:10 AM, Daniel Henrique Barboza wrote:
>> Aside from trivial XML parsing/format changes, this patch adds
>> additional rules for TPM device support to better accomodate
>> all the available scenarios with the new TPM Proxy.
>>
>> The changes make no impact to existing domains. This means that
>> the scenario of a domain with a single TPM device is still
>> supported in the same way.  The restriction of multiple TPM devices
>> got alleviated to allow a TPM Proxy device to be added together
>> with a TPM device in the same domain. All other combinations
>> are still forbidden.
>>
>> To summarize, after this patch, the following combinations in the same
>> domain are valid:
>>
>> - a single TPM device
>> - a single TPM Proxy device
>> - a single TPM + single TPM Proxy devices
>>
>> These combinations in the same domain are NOT allowed:
>>
>> - 2 or more TPM devices
>> - 2 or more TPM Proxy devices
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413 at gmail.com>
>> ---
>>   src/conf/domain_conf.c | 47 ++++++++++++++++++++++++++++++++++++++----
>>   1 file changed, 43 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 01a32f62d1..33b7d69318 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -13730,6 +13730,14 @@ virDomainTPMDefParseXML(virDomainXMLOptionPtr xmlopt,
>>           goto error;
>>       }
>> +    /* TPM Proxy devices have 'passthrough' backend */
>> +    if (def->model == VIR_DOMAIN_TPM_MODEL_SPAPR_PROXY &&
>> +        def->type != VIR_DOMAIN_TPM_TYPE_PASSTHROUGH) {
>> +        virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                       _("'Passthrough' backend is required for TPM Proxy devices"));
>> +        goto error;
>> +    }
>> +
>>       if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0)
>>           goto error;
>> @@ -21972,15 +21980,41 @@ virDomainDefParseXML(xmlDocPtr xml,
>>       if ((n = virXPathNodeSet("./devices/tpm", ctxt, &nodes)) < 0)
>>           goto error;
>> -    if (n > 1) {
>> +    if (n > 2) {
>>           virReportError(VIR_ERR_XML_ERROR, "%s",
>> -                       _("only a single TPM device is supported"));
>> +                       _("a maximum of two TPM devices is supported, one of "
>> +                         "them being a TPM Proxy device"));
>>           goto error;
>>       }
>>       if (n > 0) {
>> -        if (!(def->tpm = virDomainTPMDefParseXML(xmlopt, nodes[0], ctxt, flags)))
>> -            goto error;
>> +        for (i = 0; i < n; i++) {
>> +            virDomainTPMDefPtr dev = virDomainTPMDefParseXML(xmlopt, nodes[i], ctxt, flags);
>> +
>> +            if (!dev)
>> +                goto error;
>> +
>> +            /* TPM Proxy devices must be held in def->tpmproxy. Error
>> +             * out if there's a TPM Proxy declared already */
>> +            if (dev->model == VIR_DOMAIN_TPM_MODEL_SPAPR_PROXY) {
>> +                if (def->tpmproxy) {
>> +                    virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                                   _("only a single TPM Proxy device is supported"));
>> +                    VIR_FREE(dev);
>> +                    goto error;
>> +                }
>> +                def->tpmproxy = g_steal_pointer(&dev);
> 
> 
> Is g_steal_pointer necessary ?


Not in this case, since the VIR_FREE() call is being done before the jump
and I'm not using g_autopt() with this pointer. Thing is that we should
use g_autoptr() in these scenarios to avoid the manual cleanup. Also, I
don't think I can use VIR_FREE() with this pointer - I should've used
virDomainTPMDefFree().

I'll change the code to using g_autoptr() and g_steal_pointer().


Thanks,


DHB



> 
> 
>> +            } else {
>> +                /* all other TPM devices goes to def->tpm */
>> +                if (def->tpm) {
>> +                    virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                                   _("only a single TPM non-proxy device is supported"));
>> +                    VIR_FREE(dev);
>> +                    goto error;
>> +                }
>> +                def->tpm = g_steal_pointer(&dev);
>> +            }
>> +        }
>>       }
>>       VIR_FREE(nodes);
>> @@ -29807,6 +29841,11 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr def,
>>               goto error;
>>       }
>> +    if (def->tpmproxy) {
>> +        if (virDomainTPMDefFormat(buf, def->tpmproxy, flags) < 0)
>> +            goto error;
>> +    }
>> +
>>       for (n = 0; n < def->ngraphics; n++) {
>>           if (virDomainGraphicsDefFormat(buf, def->graphics[n], flags) < 0)
>>               goto error;
> 
>