Re: [PATCH] bhyve: process: Log error message when kill fails

[Roman Bogorodskiy <bogorodskiy@xxxxxxxxx>]

  Cole Robinson wrote:

> virProcessKillPainfully will raise a libvirt error, so log it. We
> can drop the manual pid logging since ProcessKill always reports
> the pid in its error message.
> ---
>  src/bhyve/bhyve_process.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c
> index 14588a9..f2ec712 100644
> --- a/src/bhyve/bhyve_process.c
> +++ b/src/bhyve/bhyve_process.c
> @@ -283,9 +283,8 @@ virBhyveProcessStop(bhyveConnPtr driver,
>  
>      /* First, try to kill 'bhyve' process */
>      if (virProcessKillPainfully(vm->pid, true) != 0)
> -        VIR_WARN("Failed to gracefully stop bhyve VM '%s' (pid: %d)",
> -                 vm->def->name,
> -                 (int)vm->pid);
> +        VIR_WARN("Failed to gracefully stop bhyve VM '%s': %s",
> +                 vm->def->name, virGetLastErrorMessage());
>  
>      /* Cleanup network interfaces */
>      bhyveNetCleanup(vm);

If we want to call virGetLastErrorMessage() here, the check should be a
little more complex because virProcessKillPainfully() could return 1 if
it failed to kill with SIGTERM and killed with SIGKILL and in this case
it doesn't set error.

What do you think about this bit?

diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c
index f2ec712..133589a 100644
--- a/src/bhyve/bhyve_process.c
+++ b/src/bhyve/bhyve_process.c
@@ -263,6 +263,7 @@ virBhyveProcessStop(bhyveConnPtr driver,
                     virDomainShutoffReason reason)
 {
     int ret = -1;
+    int kill_ret = -1;
     virCommandPtr cmd = NULL;
     bhyveDomainObjPrivatePtr priv = vm->privateData;
 
@@ -282,9 +283,17 @@ virBhyveProcessStop(bhyveConnPtr driver,
          bhyveMonitorClose(priv->mon);
 
     /* First, try to kill 'bhyve' process */
-    if (virProcessKillPainfully(vm->pid, true) != 0)
-        VIR_WARN("Failed to gracefully stop bhyve VM '%s': %s",
-                 vm->def->name, virGetLastErrorMessage());
+    kill_ret = virProcessKillPainfully(vm->pid, true); 
+    if (kill_ret != 0) {
+        if (kill_ret == 1) {
+            VIR_WARN("Failed to gracefully stop bhyve VM '%s' (pid: %d)",
+                     vm->def->name,
+                     (int)vm->pid);
+        } else {
+            VIR_WARN("Failed to kill bhyve process for VM '%s': %s",
+                     vm->def->name, virGetLastErrorMessage());
+        }
+    }

PS Actually, bhyve at some point (or even from the start) started to
support triggering ACPI shutdown by sending SIGTERM. So I think I'll
actually need to implement domainShutdown that only sends SIGTERM and
drop a warning when virProcessKillPainfully() returns 1 in domainDestroy
as it's sort of implied by design.

Roman Bogorodskiy

Attachment: signature.asc
Description: PGP signature

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