On 9/14/21 7:34 PM, Nick Chevsky wrote:
Hi Michal,
> The patch is correct, but what we already have is qemuExtDevicesStop()
> being called from qemuProcessStop(). The former will eventually call
> qemuTPMEmulatorStop() which should kill the swtmp process, shouldn't it?
I'd missed this and that's great to hear, because it means the
upstream bug (i.e. swtpm behaving as though --terminate was given even
when it wasn't) can be fixed now, without breaking libvirt.
> Or this patch is there to kill swtmp earlier, i.e. as soon as it sees
> HUP on the socket?
That's the thing; this is already (incorrectly) happening right now:
swtpm is self-terminating as soon as it sees HUP on the socket, even
though --terminate wasn't given. In other words, we're currently
seeing --terminate behavior *without* --terminate. Due to this
upstream bug, I suspect swtpm is already shutting itself down by the
time libvirt executes qemuExtDevicesStop().
Once the upstream bug is fixed, swtpm will no longer self-terminate on
HUP *unless* --terminate was given. If libvirt doesn't add
--terminate, swtpm will then stay up (as it should) and be killed by
qemuExtDevicesStop() instead. Does that make sense?
While the end result will ultimately be the same, I'd still argue that
it's a good idea for libvirt to start invoking swtpm with --terminate
since (a) it's free, (b) it's more graceful, (c) it takes some
responsibility off libvirt, and (d) it adds an extra layer of
redundancy by allowing swtpm to try to shut itself down first and, if
that fails, have libvirt's qemuExtDevicesStop() kill it.
Just to be clear, I am not against this patch, in fact the opposite.
Just wanted to make clear I understand what's going on. It's good to
have redundancy.
Looking at git blame of swtpm.c I can see that --terminate was there
since the initial commit so we don't need any additional machinery to
determine whether the argument is supported or not. That's good.
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
and pushed. Congratulations on your first libvirt contribution!
Michal