On 03/19/2016 03:04 PM, Roman Bogorodskiy wrote:
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()); + } + }
Ah, sorry for not looking close enough at KillPainfully return codes. That sounds good to me, I was mostly just looking for candidates to convert to virGetLastErrorMessage() :) Thanks, Cole