1. Patchew
  2. Libvirt
  3. View series

[PATCH] qemu: Do not keep swtmp pidfile around after stopping

Martin Kletzander posted 1 patch 1 year, 8 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/e342c657e6f80c456f0c0a4db703135318d56704.1660832456.git.mkletzan@redhat.com
src/qemu/qemu_tpm.c | 31 ++++++++++++++-----------------
1 file changed, 14 insertions(+), 17 deletions(-)
[PATCH] qemu: Do not keep swtmp pidfile around after stopping
Posted by Martin Kletzander 1 year, 8 months ago 
Just like the socket, remove the pidfile when TPM emulator is being stopped.  In
order to make this a bit cleaner, try to remove it even if swtpm_ioctl does not
exist.

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
 src/qemu/qemu_tpm.c | 31 ++++++++++++++-----------------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index 584c787b700b..50e8c19f3a0b 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -795,28 +795,25 @@ qemuTPMEmulatorStop(const char *swtpmStateDir,
     g_autofree char *pathname = NULL;
     g_autofree char *errbuf = NULL;
     g_autofree char *swtpm_ioctl = virTPMGetSwtpmIoctl();
+    g_autofree char *pidfile = qemuTPMEmulatorPidFileBuildPath(swtpmStateDir,
+                                                               shortName);
+    if (swtpm_ioctl &&
+        (pathname = qemuTPMEmulatorSocketBuildPath(swtpmStateDir, shortName)) &&
+        virFileExists(pathname) &&
+        (cmd = virCommandNew(swtpm_ioctl))) {
 
-    if (!swtpm_ioctl)
-        return;
-
-    if (!(pathname = qemuTPMEmulatorSocketBuildPath(swtpmStateDir, shortName)))
-        return;
-
-    if (!virFileExists(pathname))
-        return;
-
-    cmd = virCommandNew(swtpm_ioctl);
-    if (!cmd)
-        return;
+        virCommandAddArgList(cmd, "--unix", pathname, "-s", NULL);
 
-    virCommandAddArgList(cmd, "--unix", pathname, "-s", NULL);
+        virCommandSetErrorBuffer(cmd, &errbuf);
 
-    virCommandSetErrorBuffer(cmd, &errbuf);
+        ignore_value(virCommandRun(cmd, NULL));
 
-    ignore_value(virCommandRun(cmd, NULL));
+        /* clean up the socket */
+        unlink(pathname);
+    }
 
-    /* clean up the socket */
-    unlink(pathname);
+    if (pidfile)
+        virPidFileDeletePath(pidfile);
 }
 
 
-- 
2.37.2
Re: [PATCH] qemu: Do not keep swtmp pidfile around after stopping
Posted by Michal Prívozník 1 year, 8 months ago 
On 8/18/22 16:20, Martin Kletzander wrote:
> Just like the socket, remove the pidfile when TPM emulator is being stopped.  In
> order to make this a bit cleaner, try to remove it even if swtpm_ioctl does not
> exist.
> 
> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> ---
>  src/qemu/qemu_tpm.c | 31 ++++++++++++++-----------------
>  1 file changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
> index 584c787b700b..50e8c19f3a0b 100644
> --- a/src/qemu/qemu_tpm.c
> +++ b/src/qemu/qemu_tpm.c
> @@ -795,28 +795,25 @@ qemuTPMEmulatorStop(const char *swtpmStateDir,
>      g_autofree char *pathname = NULL;
>      g_autofree char *errbuf = NULL;
>      g_autofree char *swtpm_ioctl = virTPMGetSwtpmIoctl();
> +    g_autofree char *pidfile = qemuTPMEmulatorPidFileBuildPath(swtpmStateDir,
> +                                                               shortName);
> +    if (swtpm_ioctl &&
> +        (pathname = qemuTPMEmulatorSocketBuildPath(swtpmStateDir, shortName)) &&
> +        virFileExists(pathname) &&
> +        (cmd = virCommandNew(swtpm_ioctl))) {

virCommandNew() succeeds, always.

>  
> -    if (!swtpm_ioctl)
> -        return;
> -
> -    if (!(pathname = qemuTPMEmulatorSocketBuildPath(swtpmStateDir, shortName)))
> -        return;
> -
> -    if (!virFileExists(pathname))
> -        return;
> -
> -    cmd = virCommandNew(swtpm_ioctl);
> -    if (!cmd)
> -        return;
> +        virCommandAddArgList(cmd, "--unix", pathname, "-s", NULL);
>  
> -    virCommandAddArgList(cmd, "--unix", pathname, "-s", NULL);
> +        virCommandSetErrorBuffer(cmd, &errbuf);
>  
> -    virCommandSetErrorBuffer(cmd, &errbuf);
> +        ignore_value(virCommandRun(cmd, NULL));
>  
> -    ignore_value(virCommandRun(cmd, NULL));
> +        /* clean up the socket */
> +        unlink(pathname);
> +    }
>  
> -    /* clean up the socket */
> -    unlink(pathname);
> +    if (pidfile)
> +        virPidFileDeletePath(pidfile);

Here, we can be more aggressive: virPidFileForceCleanupPath().

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal
Re: [PATCH] qemu: Do not keep swtmp pidfile around after stopping
Posted by Michal Prívozník 1 year, 8 months ago 
On 8/23/22 16:19, Michal Prívozník wrote:
> On 8/18/22 16:20, Martin Kletzander wrote:
>> Just like the socket, remove the pidfile when TPM emulator is being stopped.  In
>> order to make this a bit cleaner, try to remove it even if swtpm_ioctl does not
>> exist.
>>
>> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>> ---
>>  src/qemu/qemu_tpm.c | 31 ++++++++++++++-----------------
>>  1 file changed, 14 insertions(+), 17 deletions(-)
>>

What I forgot to mention is that qemuTPMEmulatorStop() which is called
from qemuTPMEmulatorStart() can be removed, because if you accept my
suggestion and go with virPidFileForceCleanupPath() there's no chance
for the swtpm process to run at either the end of qemuTPMEmulatorStop()
or at the time of qemuTPMEmulatorStart(). But that can be done in a
follow up patch.

Michal
Re: [PATCH] qemu: Do not keep swtmp pidfile around after stopping
Posted by Michal Prívozník 1 year, 8 months ago 
On 8/24/22 13:19, Michal Prívozník wrote:
> On 8/23/22 16:19, Michal Prívozník wrote:
>> On 8/18/22 16:20, Martin Kletzander wrote:
>>> Just like the socket, remove the pidfile when TPM emulator is being stopped.  In
>>> order to make this a bit cleaner, try to remove it even if swtpm_ioctl does not
>>> exist.
>>>
>>> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>>> ---
>>>  src/qemu/qemu_tpm.c | 31 ++++++++++++++-----------------
>>>  1 file changed, 14 insertions(+), 17 deletions(-)
>>>
> 
> What I forgot to mention is that qemuTPMEmulatorStop() which is called
> from qemuTPMEmulatorStart() can be removed, because if you accept my
> suggestion and go with virPidFileForceCleanupPath() there's no chance
> for the swtpm process to run at either the end of qemuTPMEmulatorStop()
> or at the time of qemuTPMEmulatorStart(). But that can be done in a
> follow up patch.

In fact, it is completely independent. Because the pid file is in form of:

  ${swtpmStateDir}/${shortName}-swtpm.pid

where ${shortName} is the result of virDomainDefGetShortName() thus it
contains domain ID and therefore, it's never ever the same across two
'virsh start's of a domain. I'm sorry I haven't realized this during
review earlier.

Michal
Re: [PATCH] qemu: Do not keep swtmp pidfile around after stopping
Posted by Martin Kletzander 1 year, 8 months ago 
On Thu, Aug 18, 2022 at 04:20:56PM +0200, Martin Kletzander wrote:
>Just like the socket, remove the pidfile when TPM emulator is being stopped.  In
>order to make this a bit cleaner, try to remove it even if swtpm_ioctl does not
>exist.
>

Forgot to mention the BZ:

https://bugzilla.redhat.com/show_bug.cgi?id=2111301

>Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>---
> src/qemu/qemu_tpm.c | 31 ++++++++++++++-----------------
> 1 file changed, 14 insertions(+), 17 deletions(-)
>
>diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
>index 584c787b700b..50e8c19f3a0b 100644
>--- a/src/qemu/qemu_tpm.c
>+++ b/src/qemu/qemu_tpm.c
>@@ -795,28 +795,25 @@ qemuTPMEmulatorStop(const char *swtpmStateDir,
>     g_autofree char *pathname = NULL;
>     g_autofree char *errbuf = NULL;
>     g_autofree char *swtpm_ioctl = virTPMGetSwtpmIoctl();
>+    g_autofree char *pidfile = qemuTPMEmulatorPidFileBuildPath(swtpmStateDir,
>+                                                               shortName);
>+    if (swtpm_ioctl &&
>+        (pathname = qemuTPMEmulatorSocketBuildPath(swtpmStateDir, shortName)) &&
>+        virFileExists(pathname) &&
>+        (cmd = virCommandNew(swtpm_ioctl))) {
>
>-    if (!swtpm_ioctl)
>-        return;
>-
>-    if (!(pathname = qemuTPMEmulatorSocketBuildPath(swtpmStateDir, shortName)))
>-        return;
>-
>-    if (!virFileExists(pathname))
>-        return;
>-
>-    cmd = virCommandNew(swtpm_ioctl);
>-    if (!cmd)
>-        return;
>+        virCommandAddArgList(cmd, "--unix", pathname, "-s", NULL);
>
>-    virCommandAddArgList(cmd, "--unix", pathname, "-s", NULL);
>+        virCommandSetErrorBuffer(cmd, &errbuf);
>
>-    virCommandSetErrorBuffer(cmd, &errbuf);
>+        ignore_value(virCommandRun(cmd, NULL));
>
>-    ignore_value(virCommandRun(cmd, NULL));
>+        /* clean up the socket */
>+        unlink(pathname);
>+    }
>
>-    /* clean up the socket */
>-    unlink(pathname);
>+    if (pidfile)
>+        virPidFileDeletePath(pidfile);
> }
>
>
>-- 
>2.37.2
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.