[PATCH v2 2/4] virpidfile: Refactor virPidFileReadPathIfAlive

[Michal Prívozník]

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 at suse.de>
> ---
>  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