Re: [libvirt] [PATCH 10/12] conf: Add support for choosing emulation of a TPM 2

On Wed, May 23, 2018 at 02:33:08PM -0400, Stefan Berger wrote: > On 05/23/2018 11:55 AM, Ján Tomko wrote: >> On Tue, May 22, 2018 at 04:44:51PM -0400, Stefan Berger wrote: >>> @@ -24941,6 +24963,9 @@ virDomainTPMDefFormat(virBufferPtr buf, >>>     virBufferAsprintf(buf, "<backend type='%s'", >>> virDomainTPMBackendTypeToString(def->type)); >>> >>> +    if (def->version == VIR_DOMAIN_TPM_VERSION_2) >>> +        virBufferAddLit(buf, " version='2'"); >>> + >> >> Any reason for not formatting version 1.2? >> We should format implicit defaults in the XML if possible. > > Basically I did it because the previous default 1.2 didn't have it. So I > though I'd keep it as is for 1.2 and only write out 2. > What previous default? <backend type='emulator'/> is introduced by this series. We should format the configurable attributes even when they are at their default values.

>> >>>     switch (def->type) { >>>     case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: >>>         virBufferAddLit(buf, ">\n"); >> >> Given that version 2 has to be requested in the XML and we don't try to >> use it automatically, I'd suggest just dropping this function. We don't >> need to parse another tool's --help output to make up for the removal >> of parsing --help of qemu and qemu-img. > > swtpm doesn't have all the bells and whistles of QEMU that we would have > a JSON interface to query the features from. With QEMU, we actually need to know some capabilities upfront, because different versions have had different ways of requesting the same functionality. Giving nicer errors for unsupported features is just a bonus. With qemu-img, we only care about one way of representing the functionality and let it print an error if something's compiled out.

> So if a bad command line > parameter is passed to swtpm, it will dump the help screen. Here's the > output I get from trying to run a VM with an attached TPM 2 but there's > no TPM 2 support compiled into swtpm (basically because it was created > from the master branch not from the preview branch): > > # virsh start testvm-tpm2 > Error: Failed to start domain testvm-tpm2 > error: internal error: Could not start 'swtpm'. exitstatus: 1, error: > socket: unrecognized option '--tpm2' > Usage: /usr/bin/swtpm socket [options] > > The following options are supported: > > -p|--port <port> : use the given port > -f|--fd <fd>     : use the given socket file descriptor > [...] > > > I think a more controlled error message would be better basically > stating 'Local swtpm installation does not support TPM 2'. > Yes, but not worth introducing all the code and extra exec() for all the successful starts. Jano >    Stefan > >> >> Jano > >