[libvirt] [PATCH v5 18/36] qemu_process: Catch process free before process stop

Chris Venteicher posted 36 patches 7 years, 2 months ago
There is a newer version of this series
[libvirt] [PATCH v5 18/36] qemu_process: Catch process free before process stop
Posted by Chris Venteicher 7 years, 2 months ago
Catch execution paths where qemuProcessQmpFree is called before
qemuProcessQmpStop then report error and force stop before proceeding.

Also added public function header and debug message.

Signed-off-by: Chris Venteicher <cventeic@redhat.com>
---
 src/qemu/qemu_process.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index e9b50745d3..1b6adf1b64 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -8107,13 +8107,28 @@ static qemuMonitorCallbacks callbacks = {
 };
 
 
+/**
+ * qemuProcessQmpFree:
+ * @proc: Stores Process and Connection State
+ *
+ * Free process data structure.
+ */
 void
 qemuProcessQmpFree(qemuProcessQmpPtr proc)
 {
+    VIR_DEBUG("proc=%p, proc->mon=%p", proc, (proc ? proc->mon : NULL));
+
     if (!proc)
         return;
 
-    qemuProcessQmpStop(proc);
+    /* This should never be non-NULL if we get here, but just in case... */
+    if (proc->mon || proc->pid) {
+        VIR_ERROR(_("Unexpected QEMU still active during process free"
+                    " emulator: %s, pid: %lld, mon: %p"),
+                  NULLSTR(proc->binary), (long long)proc->pid, proc->mon);
+        qemuProcessQmpStop(proc);
+    }
+
     VIR_FREE(proc->binary);
     VIR_FREE(proc->libDir);
     VIR_FREE(proc->monpath);
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 18/36] qemu_process: Catch process free before process stop
Posted by Jiri Denemark 7 years, 1 month ago
On Sun, Dec 02, 2018 at 23:10:12 -0600, Chris Venteicher wrote:
> Catch execution paths where qemuProcessQmpFree is called before
> qemuProcessQmpStop then report error and force stop before proceeding.
> 
> Also added public function header and debug message.
> 
> Signed-off-by: Chris Venteicher <cventeic@redhat.com>
> ---
>  src/qemu/qemu_process.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index e9b50745d3..1b6adf1b64 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -8107,13 +8107,28 @@ static qemuMonitorCallbacks callbacks = {
>  };
>  
>  
> +/**
> + * qemuProcessQmpFree:
> + * @proc: Stores Process and Connection State

Strange capitalization again.

> + *
> + * Free process data structure.
> + */
>  void
>  qemuProcessQmpFree(qemuProcessQmpPtr proc)
>  {
> +    VIR_DEBUG("proc=%p, proc->mon=%p", proc, (proc ? proc->mon : NULL));
> +

I don't think we need to log qemuProcessQmpFree(NULL) calls. I'd just
push this debug message after the check for proc == NULL.

>      if (!proc)
>          return;
>  
> -    qemuProcessQmpStop(proc);
> +    /* This should never be non-NULL if we get here, but just in case... */
> +    if (proc->mon || proc->pid) {
> +        VIR_ERROR(_("Unexpected QEMU still active during process free"
> +                    " emulator: %s, pid: %lld, mon: %p"),
> +                  NULLSTR(proc->binary), (long long)proc->pid, proc->mon);
> +        qemuProcessQmpStop(proc);
> +    }
> +

Instead of doing this, I think it would make more sense to merge Stop
and Free functions into a single one.

>      VIR_FREE(proc->binary);
>      VIR_FREE(proc->libDir);
>      VIR_FREE(proc->monpath);

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list