Hi Michal,
I think it probably makes sense then to just do the lock checking specifically
for tpm and gpu cases and not to change the behavior of
virPidFileReadPathIfAlive (since it is used in several places in the project).
Thank you for the review. I will also follow your other suggestions and submit v3.
Thanks.
On 02/02/2022 11:33, Michal Prívozník wrote:
> On 1/13/22 13:42, Vasiliy Ulyanov wrote:
>> If the binary path is not provided check that the pid file is locked by
>> the owner process.
>>
>> Signed-off-by: Vasiliy Ulyanov <vulyanov@xxxxxxx>
>> ---
>> src/util/virpidfile.c | 20 ++++++++++++++++++++
>> 1 file changed, 20 insertions(+)
>>
>> diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c
>> index 7069f8343d..8ddc336d6c 100644
>> --- a/src/util/virpidfile.c
>> +++ b/src/util/virpidfile.c
>> @@ -216,6 +216,26 @@ int virPidFileReadPathIfAlive(const char *path,
>> #endif
>>
>> if (!binPath) {
>> + int fd;
>
> This can be:
>
> VIR_AUTOCLOSE fd = -1;
>
> which uses __attribute__((cleanup)) magic to automatically close the FD
> once the control goes out of this block.
>
>> + pid_t ownerPid;
>> +
>> + if ((fd = open(path, O_RDONLY)) < 0)
>> + return -1;
>> +
>> + if (virFileGetLockOwner(fd, 0, 1, &ownerPid) < 0) {
>> + VIR_FORCE_CLOSE(fd);
>> + return -1;
>> + }
>> +
>> + if (VIR_CLOSE(fd) < 0)
>> + return -1;
>> +
>> + /* A pid file should be locked by the process owning it. */
>> + if (ownerPid != retPid) {
>> + *pid = -1;
>> + return 0;
>> + }
>> +
>> /* we only knew the pid, and that pid is alive, so we can
>> * return it.
>> */
>
> This deserves to be documented in description of
> virPidFileReadPathIfAlive(). I mean, we want to document that the pid
> file had to be previously locked via virFileLock() at byte 0. And bonus
> points for mentioning that virCommandSetPidFile() should be used instead
> of --pidfile argument if virPidFileReadPathIfAlive() is to be used on
> the pidfile.
>
> Then, instead of calling virFileGetLockOwner() we can just try to lock
> given range, e.g. like this:
>
> if (virFileLock(fd, false, 0, 1, false) >= 0) {
> /* The file isn't locked. PID is stale. */
> return -1;
> }
>
> If this virFileLock() succeeded then we know the file wasn't locked and
> thus the PID we read from the file is stale one. If the lock failed then
> we know the file is still locked and thus the PID we've read from the
> file is valid one. IOW, this can be written as:
>
> VIR_AUTOCLOSE fd = -1;
>
> if ((fd = open(path, O_RDWR)) < 0)
> return -1;
>
> if (virFileLock(fd, false, 0, 1, false) >= 0) {
> /* The file isn't locked. PID is stale. */
> return -1;
> }
>
> /* we only knew the pid, and that pid is alive, so we can
> * return it.
> */
> *pid = retPid;
> return 0;
>
>
> That leaves us with virChrdevLockFileCreate() which passes NULL as
> @binPath but doesn't use lock the "pidfile". In fact, it uses files as
> locks, not pid files.
>
> Michal
>
--
Vasily Ulyanov <vulyanov@xxxxxxx>
Software Engineer, SUSE Labs Core
