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@xxxxxxx>
Software Engineer, SUSE Labs Core
