7:07 p.m.
[...]
>>
> 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