[PATCH v2 0/4] qemu_tpm: Get swtpm pid without binary validation
v2 of https://listman.redhat.com/archives/libvir-list/2022-January/msg00008.html The v2 series introduces the new approach following the previous review comments. It adds a function for pidfile lock validation (i.e. if its locked by the expected process) and adds the new check to virPidFileReadPathIfAlive. The daemonization and pidfile handling for swtpm command are now handled by libvirt. The fix to qemu_vhost_user_gpu is currently done by visual code checking. Not really sure how to test e2e as I dont have a quick reproducer atm :( Note: I wasn't sure about the refactoring of virPidFileReadPathIfAlive (i.e. whether to remove the binary path from there). For now decided to introduce the new lock check only if the path is not provided. But I am open for suggestions on how to better fit the new check. Vasiliy Ulyanov (4): virfile: Add virFileGetLockOwner function virpidfile: Refactor virPidFileReadPathIfAlive qemu_tpm: Get swtpm pid without binary validation qemu: gpu: Get pid without binary validation src/libvirt_private.syms | 1 + src/qemu/qemu_tpm.c | 26 +++++++++----------- src/qemu/qemu_vhost_user_gpu.c | 9 +++---- src/util/virfile.c | 45 ++++++++++++++++++++++++++++++++++ src/util/virfile.h | 2 ++ src/util/virpidfile.c | 20 +++++++++++++++ 6 files changed, 82 insertions(+), 21 deletions(-) -- 2.34.1
The function is used to retrieve the PID of the process holding an exclusive lock on the file. Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de> --- src/libvirt_private.syms | 1 + src/util/virfile.c | 45 ++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 2 ++ 3 files changed, 48 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5b76e66e61..214f375a91 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2224,6 +2224,7 @@ virFileFreeACLs; virFileGetACLs; virFileGetDefaultHugepage; virFileGetHugepageSize; +virFileGetLockOwner; virFileGetMountReverseSubtree; virFileGetMountSubtree; virFileGetXAttr; diff --git a/src/util/virfile.c b/src/util/virfile.c index d6faf7e3d2..b9149fb0d7 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -460,6 +460,43 @@ int virFileUnlock(int fd, off_t start, off_t len) } +/** + * virFileGetLockOwner: + * @fd: file descriptor to get the lock from + * @start: byte offset for the lock + * @len: length of the lock (0 to specify entire remaining file from @start) + * @pid: variable to return the PID of the process owning the lock + * + * Attempt to retrieve the PID of the process holding an exclusive lock + * on the file @fd. + * + * Returns 0 on success, or -errno on error. If the file is not locked @pid + * will be set ot -1. + */ +int virFileGetLockOwner(int fd, + off_t start, + off_t len, + pid_t *pid) +{ + struct flock fl = { + .l_type = F_WRLCK, + .l_whence = SEEK_SET, + .l_start = start, + .l_len = len, + }; + + *pid = -1; + + if (fcntl(fd, F_GETLK, &fl) < 0) + return -errno; + + if (fl.l_type != F_UNLCK) + *pid = fl.l_pid; + + return 0; +} + + #else /* WIN32 */ @@ -480,6 +517,14 @@ int virFileUnlock(int fd G_GNUC_UNUSED, return -ENOSYS; } +int virFileGetLockOwner(int fd G_GNUC_UNUSED, + off_t start G_GNUC_UNUSED, + off_t len G_GNUC_UNUSED, + pid_t *pid G_GNUC_UNUSED) +{ + return -ENOSYS; +} + #endif /* WIN32 */ diff --git a/src/util/virfile.h b/src/util/virfile.h index 967c9a9b4f..0f4aa6e441 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -122,6 +122,8 @@ int virFileLock(int fd, bool shared, off_t start, off_t len, bool waitForLock) G_GNUC_NO_INLINE; int virFileUnlock(int fd, off_t start, off_t len) G_GNUC_NO_INLINE; +int virFileGetLockOwner(int fd, off_t start, off_t len, pid_t *pid) + G_GNUC_NO_INLINE; typedef int (*virFileRewriteFunc)(int fd, const void *opaque); int virFileRewrite(const char *path, -- 2.34.1
On 1/13/22 13:42, Vasiliy Ulyanov wrote:
The function is used to retrieve the PID of the process holding an exclusive lock on the file.
Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de> --- src/libvirt_private.syms | 1 + src/util/virfile.c | 45 ++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 2 ++ 3 files changed, 48 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5b76e66e61..214f375a91 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2224,6 +2224,7 @@ virFileFreeACLs; virFileGetACLs; virFileGetDefaultHugepage; virFileGetHugepageSize; +virFileGetLockOwner; virFileGetMountReverseSubtree; virFileGetMountSubtree; virFileGetXAttr; diff --git a/src/util/virfile.c b/src/util/virfile.c index d6faf7e3d2..b9149fb0d7 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -460,6 +460,43 @@ int virFileUnlock(int fd, off_t start, off_t len) }
+/** + * virFileGetLockOwner: + * @fd: file descriptor to get the lock from + * @start: byte offset for the lock + * @len: length of the lock (0 to specify entire remaining file from @start) + * @pid: variable to return the PID of the process owning the lock + * + * Attempt to retrieve the PID of the process holding an exclusive lock + * on the file @fd.
Here I'd mention that the lock has to be acquired via virFileLock(), because there are plenty of file locks (at least on Linux) that are not compatible with each other. But I don't think we will need this function after all. Michal
If the binary path is not provided check that the pid file is locked by the owner process. Signed-off-by: Vasiliy Ulyanov <vulyanov@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; + 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. */ -- 2.34.1
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@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
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@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
-- Vasily Ulyanov <vulyanov@suse.de> Software Engineer, SUSE Labs Core
Access to /proc/[pid]/exe may be restricted in certain environments (e.g. in containers) and any attempt to stat(2) or readlink(2) the file will result in 'permission denied' error if the calling process does not have CAP_SYS_PTRACE capability. According to proc(5) manpage: Permission to dereference or read (readlink(2)) this symbolic link is governed by a ptrace access mode PTRACE_MODE_READ_FSCREDS check; see ptrace(2). The binary validation in virPidFileReadPathIfAlive may fail with EACCES. Therefore instead do only the check that the pidfile is locked by the correct process. To ensure this is always the case the daemonization and pidfile handling of the swtpm command is now controlled by libvirt. Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de> --- src/qemu/qemu_tpm.c | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 7e7b01768e..792ee19bbd 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -258,13 +258,12 @@ qemuTPMEmulatorGetPid(const char *swtpmStateDir, const char *shortName, pid_t *pid) { - g_autofree char *swtpm = virTPMGetSwtpm(); g_autofree char *pidfile = qemuTPMEmulatorCreatePidFilename(swtpmStateDir, shortName); if (!pidfile) return -1; - if (virPidFileReadPathIfAlive(pidfile, pid, swtpm) < 0) + if (virPidFileReadPathIfAlive(pidfile, pid, NULL) < 0) return -1; return 0; @@ -721,7 +720,7 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm, virCommandClearCaps(cmd); - virCommandAddArgList(cmd, "socket", "--daemon", "--ctrl", NULL); + virCommandAddArgList(cmd, "socket", "--ctrl", NULL); virCommandAddArgFormat(cmd, "type=unixio,path=%s,mode=0600", tpm->data.emulator.source->data.nix.path); @@ -751,8 +750,8 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm, if (!(pidfile = qemuTPMEmulatorCreatePidFilename(swtpmStateDir, shortName))) goto error; - virCommandAddArg(cmd, "--pid"); - virCommandAddArgFormat(cmd, "file=%s", pidfile); + virCommandSetPidFile(cmd, pidfile); + virCommandDaemonize(cmd); if (tpm->data.emulator.hassecretuuid) { if (!virTPMSwtpmCapsGet(VIR_TPM_SWTPM_FEATURE_CMDARG_PWD_FD)) { @@ -905,7 +904,6 @@ qemuExtTPMStartEmulator(virQEMUDriver *driver, { g_autoptr(virCommand) cmd = NULL; int exitstatus = 0; - g_autofree char *errbuf = NULL; g_autoptr(virQEMUDriverConfig) cfg = NULL; g_autofree char *shortName = virDomainDefGetShortName(vm->def); int cmdret = 0, timeout, rc; @@ -930,8 +928,6 @@ qemuExtTPMStartEmulator(virQEMUDriver *driver, if (qemuExtDeviceLogCommand(driver, vm, cmd, "TPM Emulator") < 0) return -1; - virCommandSetErrorBuffer(cmd, &errbuf); - if (qemuSecurityStartTPMEmulator(driver, vm, cmd, cfg->swtpm_user, cfg->swtpm_group, &exitstatus, &cmdret) < 0) @@ -939,22 +935,22 @@ qemuExtTPMStartEmulator(virQEMUDriver *driver, if (cmdret < 0 || exitstatus != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Could not start 'swtpm'. exitstatus: %d, " - "error: %s"), exitstatus, errbuf); + _("Could not start 'swtpm'. exitstatus: %d"), exitstatus); return -1; } - /* check that the swtpm has written its pid into the file */ + /* check that the swtpm has written its pid into the file and the control + * socket has been created. */ + rc = qemuTPMEmulatorGetPid(cfg->swtpmStateDir, shortName, &pid); + if ((rc == 0 && pid == (pid_t)-1) || rc < 0) + goto error; timeout = 1000; /* ms */ while (timeout > 0) { - rc = qemuTPMEmulatorGetPid(cfg->swtpmStateDir, shortName, &pid); - if (rc < 0) { + if (!virFileExists(tpm->data.emulator.source->data.nix.path)) { timeout -= 50; g_usleep(50 * 1000); continue; } - if (rc == 0 && pid == (pid_t)-1) - goto error; break; } if (timeout <= 0) -- 2.34.1
On 1/13/22 13:42, Vasiliy Ulyanov wrote:
Access to /proc/[pid]/exe may be restricted in certain environments (e.g. in containers) and any attempt to stat(2) or readlink(2) the file will result in 'permission denied' error if the calling process does not have CAP_SYS_PTRACE capability. According to proc(5) manpage:
Permission to dereference or read (readlink(2)) this symbolic link is governed by a ptrace access mode PTRACE_MODE_READ_FSCREDS check; see ptrace(2).
The binary validation in virPidFileReadPathIfAlive may fail with EACCES. Therefore instead do only the check that the pidfile is locked by the correct process. To ensure this is always the case the daemonization and pidfile handling of the swtpm command is now controlled by libvirt.
Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de> --- src/qemu/qemu_tpm.c | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 7e7b01768e..792ee19bbd 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -258,13 +258,12 @@ qemuTPMEmulatorGetPid(const char *swtpmStateDir, const char *shortName, pid_t *pid) { - g_autofree char *swtpm = virTPMGetSwtpm(); g_autofree char *pidfile = qemuTPMEmulatorCreatePidFilename(swtpmStateDir, shortName); if (!pidfile) return -1;
- if (virPidFileReadPathIfAlive(pidfile, pid, swtpm) < 0) + if (virPidFileReadPathIfAlive(pidfile, pid, NULL) < 0) return -1;
return 0; @@ -721,7 +720,7 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm,
virCommandClearCaps(cmd);
- virCommandAddArgList(cmd, "socket", "--daemon", "--ctrl", NULL); + virCommandAddArgList(cmd, "socket", "--ctrl", NULL); virCommandAddArgFormat(cmd, "type=unixio,path=%s,mode=0600", tpm->data.emulator.source->data.nix.path);
@@ -751,8 +750,8 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm, if (!(pidfile = qemuTPMEmulatorCreatePidFilename(swtpmStateDir, shortName))) goto error;
- virCommandAddArg(cmd, "--pid"); - virCommandAddArgFormat(cmd, "file=%s", pidfile); + virCommandSetPidFile(cmd, pidfile); + virCommandDaemonize(cmd);
if (tpm->data.emulator.hassecretuuid) { if (!virTPMSwtpmCapsGet(VIR_TPM_SWTPM_FEATURE_CMDARG_PWD_FD)) { @@ -905,7 +904,6 @@ qemuExtTPMStartEmulator(virQEMUDriver *driver, { g_autoptr(virCommand) cmd = NULL; int exitstatus = 0; - g_autofree char *errbuf = NULL; g_autoptr(virQEMUDriverConfig) cfg = NULL; g_autofree char *shortName = virDomainDefGetShortName(vm->def); int cmdret = 0, timeout, rc; @@ -930,8 +928,6 @@ qemuExtTPMStartEmulator(virQEMUDriver *driver, if (qemuExtDeviceLogCommand(driver, vm, cmd, "TPM Emulator") < 0) return -1;
- virCommandSetErrorBuffer(cmd, &errbuf); -
We can use virCommandSetErrorFD(), which ...
if (qemuSecurityStartTPMEmulator(driver, vm, cmd, cfg->swtpm_user, cfg->swtpm_group, &exitstatus, &cmdret) < 0) @@ -939,22 +935,22 @@ qemuExtTPMStartEmulator(virQEMUDriver *driver,
if (cmdret < 0 || exitstatus != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Could not start 'swtpm'. exitstatus: %d, " - "error: %s"), exitstatus, errbuf); + _("Could not start 'swtpm'. exitstatus: %d"), exitstatus); return -1; }
- /* check that the swtpm has written its pid into the file */ + /* check that the swtpm has written its pid into the file and the control + * socket has been created. */ + rc = qemuTPMEmulatorGetPid(cfg->swtpmStateDir, shortName, &pid); + if ((rc == 0 && pid == (pid_t)-1) || rc < 0) + goto error; timeout = 1000; /* ms */ while (timeout > 0) { - rc = qemuTPMEmulatorGetPid(cfg->swtpmStateDir, shortName, &pid); - if (rc < 0) { + if (!virFileExists(tpm->data.emulator.source->data.nix.path)) { timeout -= 50; g_usleep(50 * 1000); continue; } - if (rc == 0 && pid == (pid_t)-1) - goto error;
Can then be used here to report error message that swtpm printed onto its stderr. Look at qemuProcessStartManagedPRDaemon() which does that.
break; } if (timeout <= 0)
Michal
The binary validation in virPidFileReadPathIfAlive may fail with EACCES if the calling process does not have CAP_SYS_PTRACE capability. Therefore instead do only the check that the pidfile is locked by the correct process. Fixes the same issue as faee4e3dbf550597cd9700eff8289ea089df3c7a. Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de> --- src/qemu/qemu_vhost_user_gpu.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_vhost_user_gpu.c b/src/qemu/qemu_vhost_user_gpu.c index ef198a4820..66d2f93b66 100644 --- a/src/qemu/qemu_vhost_user_gpu.c +++ b/src/qemu/qemu_vhost_user_gpu.c @@ -54,7 +54,6 @@ qemuVhostUserGPUCreatePidFilename(const char *stateDir, /* * qemuVhostUserGPUGetPid: - * @binpath: path of executable associated with the pidfile * @stateDir: the directory where vhost-user-gpu writes the pidfile into * @shortName: short name of the domain * @alias: video device alias @@ -65,8 +64,7 @@ qemuVhostUserGPUCreatePidFilename(const char *stateDir, * set to -1; */ static int -qemuVhostUserGPUGetPid(const char *binPath, - const char *stateDir, +qemuVhostUserGPUGetPid(const char *stateDir, const char *shortName, const char *alias, pid_t *pid) @@ -76,7 +74,7 @@ qemuVhostUserGPUGetPid(const char *binPath, if (!(pidfile = qemuVhostUserGPUCreatePidFilename(stateDir, shortName, alias))) return -1; - if (virPidFileReadPathIfAlive(pidfile, pid, binPath) < 0) + if (virPidFileReadPathIfAlive(pidfile, pid, NULL) < 0) return -1; return 0; @@ -253,8 +251,7 @@ qemuExtVhostUserGPUSetupCgroup(virQEMUDriver *driver, if (!shortname) return -1; - rc = qemuVhostUserGPUGetPid(video->driver->vhost_user_binary, - cfg->stateDir, shortname, video->info.alias, &pid); + rc = qemuVhostUserGPUGetPid(cfg->stateDir, shortname, video->info.alias, &pid); if (rc < 0 || (rc == 0 && pid == (pid_t)-1)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not get process id of vhost-user-gpu")); -- 2.34.1
Hey, ping here :) Any thoughts or suggestions about the proposed patch series? On 13/01/2022 13:42, Vasiliy Ulyanov wrote:
v2 of https://listman.redhat.com/archives/libvir-list/2022-January/msg00008.html
The v2 series introduces the new approach following the previous review comments. It adds a function for pidfile lock validation (i.e. if its locked by the expected process) and adds the new check to virPidFileReadPathIfAlive. The daemonization and pidfile handling for swtpm command are now handled by libvirt.
The fix to qemu_vhost_user_gpu is currently done by visual code checking. Not really sure how to test e2e as I dont have a quick reproducer atm :(
Note: I wasn't sure about the refactoring of virPidFileReadPathIfAlive (i.e. whether to remove the binary path from there). For now decided to introduce the new lock check only if the path is not provided. But I am open for suggestions on how to better fit the new check.
Vasiliy Ulyanov (4): virfile: Add virFileGetLockOwner function virpidfile: Refactor virPidFileReadPathIfAlive qemu_tpm: Get swtpm pid without binary validation qemu: gpu: Get pid without binary validation
src/libvirt_private.syms | 1 + src/qemu/qemu_tpm.c | 26 +++++++++----------- src/qemu/qemu_vhost_user_gpu.c | 9 +++---- src/util/virfile.c | 45 ++++++++++++++++++++++++++++++++++ src/util/virfile.h | 2 ++ src/util/virpidfile.c | 20 +++++++++++++++ 6 files changed, 82 insertions(+), 21 deletions(-)
-- Vasily Ulyanov <vulyanov@suse.de> Software Engineer, SUSE Labs Core
participants (3)
-
Michal Prívozník -
Vasiliy Ulyanov -
Vasily Ulyanov