[...]
Two blank lines between new functions and we like Free instead of Delete unless of course this is something more specific...
This is something more specifc. The TPM emulator writes state into files in a dedicated directory. That state would normally be written into NVRAM of a TPM so that, after it is powered up again, it has that old state again (keys etc.) . We only delete this state and the per-VM directory tree when the domain is undefined.
Probably just an API naming thing as I was reading forward... Lot of context to plow through on the first pass for me!
[...]
@@ -27548,6 +27587,10 @@ virDomainDeleteConfig(const char *configDir, goto cleanup; } + /* in case domain is NOT running, remove any TPM storage */ + if (!dom->persistent) Well this has less to do with running and more to do with persistence vs. transient
Iirc this comes from the following: I ran into this case where I had a running domain and undefined it. The domain keeps running of course, the XML definition is gone. In this case we cannot just delete the TPM emulator's storage while it is running but we have to do it later once that VM terminates.
I wonder if this is something where using the /var/run/libvirt (or whatever the temporary running domain path is - my brain is fried from so many reviews)... Using this would cause/allow cleanup when the process eventually dies
+ virDomainTPMDelete(dom->def); +
[...]
virQEMUDriverConfigNew(bool privileged) &cfg->nfirmwares) < 0) goto error; + if (virGetUserID("tss", &cfg->swtpm_user) < 0) + cfg->swtpm_user = 0; /* root */ + Something doesn't feel right here especially for unprivileged mode.
I haven't tried it in unprivileged mode. Not sure how to invoke it even...
-c qemu:///session
If there isn't a "tss" user, then are things configured correctly?
There are only two choice, one is tss and the other one is root.
Should we follow the other callers to virGetUserID and set to -1 instead?
Set swtpm_user = -1; ? What happens when we need to actually use this variable then ? Report an error ?
When using the virFile* API's if the incoming uid/gid is -1, then it's not specifically and it IIRC it takes whatever the current default running mode is. If it is set, then it's set specifically to the value. Been a while since I traversed through all that code.
Still 0 is the default for @cfg members anyway.
[...]
@@ -7510,6 +7514,9 @@ qemuDomainUndefineFlags(virDomainPtr dom, if (!(vm = qemuDomObjFromDomain(dom))) return -1; + if (qemuExtDevicesInitPaths(driver, vm->def) < 0) + return -1; Again, confused over failure in an undefine path? Or is this just git diff fooling me?
This is for a domain that's being undefined but was never started (which would have caused the path initialization) since libvirtd was restarted. Again, we need to initialize the paths. If there was a better place to put these paths, I'd be curious where that is.
Perhaps "stateDir" - it just came to me - hopefully it's what I was thinking about ;-)
[...]
+{ + struct dirent *ent; + int ret; + DIR *dir; + char *path; + + if (virDirOpen(&dir, name) < 0) + return -1; Hope that @name isn't a file path on an NFS volume - there's some other
That would depend on the configuration of the host
consumers of chown in this module that you may want to have a look at.
The other consumers stat the file before and check whether chown() is necessary. Hm, is that the recommended way. I just try to change all of them without looking at the state.
The NFS thing is something that came to mind while thinking of uid/gid mgmt and rootsquash mode (or something like that) which absolutely causes nightmares w/r/t using root...
+ + while ((ret = virDirRead(dir, &ent, name)) > 0) { + if (ent->d_type != DT_REG) + continue; + + if (virAsprintf(&path, "%s/%s", name, ent->d_name) < 0) { + ret = -1; + break; + } + if (chown(path, uid, gid) < 0) { If uid/gid change from non root user X to user Y, then wouldn't this just fail anyway?
Why would it fail if root does that? libvirtd runs as root.
Guess I was thinking of session mode or unprivileged mode.
Still using the -1, -1 logic like other chown/chmod consumers may actually be a benefit here at least w/r/t not failing.
Not sure what you mean. uid = -1 passed to chown() means, don't change it. Same for gid = -1.
I think I redescribed that a bit above. It wasn't fresh in my mind, but essentially looking at the various virFile API's that would check uid/gid vs. -1 and thinking how qemu.conf and storage pool <permissions> used -1, -1... Again the details escape me at the moment.
[...]
+static int +virTPMCreateEmulatorStorage(const char *storagepath, + bool *created, + uid_t swtpm_user) +{ + int ret = -1; + char *swtpmStorageDir = virTPMGetTPMStorageDir(storagepath); + + if (!swtpmStorageDir) + return -1; + + if (virTPMEmulatorInitStorage(swtpmStorageDir) < 0) + return -1; + + *created = false; + + if (!virFileExists(storagepath)) + *created = true;> + + if (virDirCreate(storagepath, 0700, swtpm_user, swtpm_user, 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?
You are right. I need a swtpm_group, eh ? On my system tss happens to be mapped to 59 for uid and gid.
Still may want to consider using the -1, -1 because it is handled via the File API's.
-1 uid and -1 gid passed to chown() would mean don't change it. One problem is that a user may edit /etc/libvirt/qemu.conf and change this line here from root to tss:
swtpm_user = "tss"
In that case all file ownerships have to be adjusted so that the swtpm can access the files. That's why I am always adjusting the ownerships before swtpm or the other tools are started.
I suspect by this time you've dug in a bit more and perhaps have more details than I can recall at the moment.
[...]
I broke this patch up into several patches. Thanks for looking at it.
Stefan
Thanks... I know it's painful from experience ;-) John