Re: [libvirt] [PATCH v2 2/6] tpm: Add support for external swtpm TPM emulator

On 04/10/2018 10:50 PM, Stefan Berger wrote: Wow that's a lot of stuff changing... Again, you should alter XML, then add the capability, and then alter qemu... Perhaps the util/virtpm.{c,h} files could be their own patch assuming they compile separately. The pieces of qemu code that would support the eventual ability to launch qemu with this emulator model should be in place before it's possible to launch - IOW conf, security, and cgroup changes before qemu_command can adds support. I think this and the next 4 patches needs some rearranging. It seems that this alteration should be in a separate patch series from the CRB series. I'll scan through the rest of this patch, but nothing to in depth. functionality I'm having a brain lock right now "UnixIO socket" - probably could just be a Unix socket I think. Thankfully unlike the persistent reservations work also on list waiting to be reviewed there isn't a configurable socket path option! Two blank lines between new functions and we like Free instead of Delete unless of course this is something more specific... use: void virDomainTPMFree(virDomainDefPtr def) I would expect the def->tpm to also be deleted, but all this is doing is freeing the storagepath tree which would say to me perhaps the callers of this should just call virTPMDeleteEmulatorStorage Yes, I see this doesn't follow the model outlined above. I'm fine with it changing as part of this. and this perhaps is where I'd think virTPMDeleteEmulatorStorage would be called. Eww... hard to read/follow Maybe the def->type switch should handle the <backend> printing completely for which ever type this is... Well this has less to do with running and more to do with persistence vs. transient Perhaps separate the above hunk and deal with all the switches that need a case in one patch with dummy code that just accepts it. Then subsequent patches add "real code" for dummy pieces. Just a thought - makes it easier to find places later on that care about the model type. This feels separable too All the qemu.conf related stuff should be its own patch. Capabilities changes in their own patch which also seems to pull in parts of patch3 (the caps_*.xml changes) If all we need @driver for is @cfg, then why not pass @cfg into qemuBuildTPMCommandLine and then into here? That way this code doesn't manage yet another reference to @cfg Passing @cfg from here should be fine just like other callers do... "...cmd, cfg, ..." Something doesn't feel right here especially for unprivileged mode. If there isn't a "tss" user, then are things configured correctly? Should we follow the other callers to virGetUserID and set to -1 instead? Still 0 is the default for @cfg members anyway. qemuExtDevicesInitPaths returns an int not a bool... Still strange to see on a Remove/Inactive path This one I get - it's a creation path... Again, confused over failure in an undefine path? Or is this just git diff fooling me? More recently we prefer two blank lines between functions. You're mixing function entry point formats, use: static int qemuExtTPMInitPaths(virQEMUDriverPtr driver, virDomainDefPtr def) This repeats throughout. You leak @cfg here. again @cfg is leaked. Wouldn't be running right? Leaking @cfg Why a separate qemuExtTPMStart? Why not: if (@def->tpm) return 0; <guts of qemuExtTPMStart> Similar... don't see the need for the extra indirection. In fact the whole module only really cares about TPM and less about any sort of external device. Could be qemu_tpm.c and qemuTPM* API's too. security changes should be their own patch after we have a TPM_TYPE_EMULATOR The following is definitely separable into its own patch "util: Introduce ..." virFileChownFiles ? Hope that @name isn't a file path on an NFS volume - there's some other consumers of chown in this module that you may want to have a look at. If uid/gid change from non root user X to user Y, then wouldn't this just fail anyway? Still using the -1, -1 logic like other chown/chmod consumers may actually be a benefit here at least w/r/t not failing. We could use virFileDeleteTree instead.... Using virFileRemove instead of remove... Be wary of adding some new call or something specific to Linux as we have non-Linux build variants... Separable I would think similar to virFileRemoveLastComponent - although that would clear out the '/' IIRC: We only checked if swtpm_user was a user - we did not check if it was also a gid... You cannot use the same value for both, can you? Still may want to consider using the -1, -1 because it is handled via the File API's. If we've created the directory properly then wouldn't this possibly change the mode on any existing file in the directory? We really shouldn't need to do this should we? This would essentially cause a failure to chown something we're not allowed to chown and I believe the DirCreate would have failed anyway since See virDomainDefGetShortName and it's consumers. Don't be stuck in the quagmire that caused nightmares for mkletzan... In fact - anywhere that I may have already missed or will miss subsequently that uses vmname should use the short name. Anyone in the same group (that hasn't been defined yet) can write? Or should they just Read, Execute? And the world can Read? Again, may want to consider a short name... So this makes sense 660 to allow RW for owner/group... And this differs w/ mode, doesn't it? Although I'll admit - I've lost a bit more context now... But user != gid ^^^ This would seem to need to be some sort of generic TPM.*Free type function... If you set this to NULL, then you can move the virCommandFree after cleanup: (doesn't really matter, just an option). goto cleanup.. I'm sure I missed a few things - this was all done by sight... Quite a few patches could be generated out of this single patch I would think.. It would allow at least you to make some progress to get some ACK's/R-b's and possibly pushes rather than continually looking at the same things. John