6:42 a.m.
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
6:42 a.m.
New subject: [PATCH v2 1/4] virfile: Add virFileGetLockOwner function
The function is used to retrieve the PID of the process holding an
exclusive lock on the file.
Signed-off-by: Vasiliy Ulyanov <vulyanov(a)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
4:33 a.m.
New subject: [PATCH v2 1/4] virfile: Add virFileGetLockOwner function
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(a)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
6:42 a.m.
New subject: [PATCH v2 2/4] virpidfile: Refactor virPidFileReadPathIfAlive
If the binary path is not provided check that the pid file is locked by
the owner process.
Signed-off-by: Vasiliy Ulyanov <vulyanov(a)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
4:33 a.m.
New subject: [PATCH v2 2/4] virpidfile: Refactor virPidFileReadPathIfAlive
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(a)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
6:59 a.m.
New subject: [PATCH v2 2/4] virpidfile: Refactor virPidFileReadPathIfAlive
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(a)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(a)suse.de>
Software Engineer, SUSE Labs Core
6:42 a.m.
New subject: [PATCH v2 3/4] qemu_tpm: Get swtpm pid without binary validation
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(a)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
4:33 a.m.
New subject: [PATCH v2 3/4] qemu_tpm: Get swtpm pid without binary validation
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(a)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
6:42 a.m.
New subject: [PATCH v2 4/4] qemu: gpu: Get pid without binary validation
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(a)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
3:29 a.m.
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(a)suse.de>
Software Engineer, SUSE Labs Core
1058
days inactive
1078
days old
9 comments
3 participants
participants (3)
-
Michal Prívozník -
Vasiliy Ulyanov -
Vasily Ulyanov