[PATCH] qemu: use a fixed directory for saving qmp capabilities process

Bihong Yu posted 1 patch 3 years, 8 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/cd9d7009-1236-f2d7-9979-cf85c1fe8533@huawei.com
src/qemu/qemu_process.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
[PATCH] qemu: use a fixed directory for saving qmp capabilities process
Posted by Bihong Yu 3 years, 8 months ago
>From c7ee36417b88df7dcfe5e18d1eb72b6d7c175268 Mon Sep 17 00:00:00 2001
From: Bihong Yu <yubihong@huawei.com>
Date: Tue, 14 Jul 2020 11:17:46 +0800
Subject: [PATCH] qemu: use a fixed directory for saving qmp capabilities
 process

In some case, the qmp capabilities process possible residues. So we need to
cleanup the residual process during start libvirt. For this reason, we
should use a fixed directory for saving qmp capabilities process.

Change-Id: I6a74a046e192eee169a0e2677ce3aed9ded5e0ed
Signed-off-by:Bihong Yu <yubihong@huawei.com>
---
 src/qemu/qemu_process.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 70fc24b..eba14ed 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -8450,21 +8450,24 @@ static int
 qemuProcessQMPInit(qemuProcessQMPPtr proc)
 {
     g_autofree char *template = NULL;
+    virErrorPtr orig_err;

     VIR_DEBUG("proc=%p, emulator=%s", proc, proc->binary);

-    template = g_strdup_printf("%s/qmp-XXXXXX", proc->libDir);
+    template = g_strdup_printf("%s/qmp-capabilities", proc->libDir);

-    if (!(proc->uniqDir = g_mkdtemp(template))) {
+    if (g_mkdir_with_parents(template, 0700) < 0) {
         virReportSystemError(errno,
                              _("Failed to create unique directory with "
                                "template '%s' for probing QEMU"),
                              template);
         return -1;
     }
-    /* if g_mkdtemp succeeds, proc->uniqDir is now the owner of
-     * the string. Set template to NULL to avoid freeing
-     * the memory in this case */
+    /*
+     * if g_mkdir_with_parents succeeds, assign the template to proc->uniqDir.
+     * Then set template to NULL to avoid freeing the memory in this case.
+     */
+    proc->uniqDir = template;
     template = NULL;

     if (qemuProcessQEMULabelUniqPath(proc) < 0)
@@ -8481,6 +8484,10 @@ qemuProcessQMPInit(qemuProcessQMPPtr proc)
      */
     proc->pidfile = g_strdup_printf("%s/%s", proc->uniqDir, "qmp.pid");

+    virErrorPreserveLast(&orig_err);
+    if (virPidFileForceCleanupPath(proc->pidfile) < 0)
+        VIR_WARN("Unable to kill qemu QMP process");
+    virErrorRestore(&orig_err);
     return 0;
 }

-- 
1.8.3.1

Re: [PATCH] qemu: use a fixed directory for saving qmp capabilities process
Posted by Jiri Denemark 3 years, 8 months ago
On Tue, Jul 14, 2020 at 11:20:00 +0800, Bihong Yu wrote:
> >From c7ee36417b88df7dcfe5e18d1eb72b6d7c175268 Mon Sep 17 00:00:00 2001
> From: Bihong Yu <yubihong@huawei.com>
> Date: Tue, 14 Jul 2020 11:17:46 +0800
> Subject: [PATCH] qemu: use a fixed directory for saving qmp capabilities
>  process
> 
> In some case, the qmp capabilities process possible residues. So we need to
> cleanup the residual process during start libvirt. For this reason, we
> should use a fixed directory for saving qmp capabilities process.
> 
> Change-Id: I6a74a046e192eee169a0e2677ce3aed9ded5e0ed
> Signed-off-by:Bihong Yu <yubihong@huawei.com>
> ---
>  src/qemu/qemu_process.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)

We can't do this because this code is used in more places than just QMP
capabilities probing. Specifically, APIs for CPU comparison and baseline
on s390x are passed to QEMU using this code. Which means we can have
multiple QEMU processes running at the same time.

We should rather make sure such temporary directories are not left
behind and possible cleanup them when QEMU driver starts to remove those
which were left behind when libvirtd gets restarted.

NACK