Hi Michal, On 11/01/2022 11:00, Michal Prívozník wrote:
Firstly, this fixes the problem only for swtpm case, but there is one other place where the same problem can happen (qemuVhostUserGPUGetPid()).
Sure, I can also try to fix the vhost gpu case. Was mostly focused on tpm hence did not look at other places.
From conceptual POV, what may now happen is that when PID wrapped we may wrongly detect another process as swtpm.
Right, valid point...
I'm wondering whether we can ditch the @binPath argument of virPidFileReadPathIfAlive() completely. A pid file should be locked by the process owning it [1], therefore what we could do is read given file and try to lock it. If we succeeded it means that the process is dead and we read a stale PID. If locking fails then the process is still running and the PID we read is valid.
1: This is how libvirt treats pidfiles (see virPidFileAcquirePath()). Question is whether other tools which use their own pidfiles do the same. For instance, swtpm is given '--pid file=/some/path' argument but I haven't checked whether it behaves as expected. Also, to make things worse there are at least two types of file locks on Linux and both are independent of each other, so question then is what type of lock does given tool use.
Unfortunately swtpm does not lock the pid file at all :(
But this could all be mitigated by using virCommandSetPidFile() when spawning the command instead of passing arbitrary --pid arguments.
So I can drop --pid and --daemon args from swtpm cmd and add virCommandSetPidFile(cmd, pidfile); virCommandDaemonize(cmd); Then in virPidFileReadPathIfAlive() I can add a lock check. Hm... Nice, that seems like a good way to go. -- Vasily Ulyanov <vulyanov@suse.de> Software Engineer, SUSE Labs Core