bhyve: fix virBhyveProcessStop()

Currently, there are two (at least) issues in virBhyveProcessStop().

Before going into details, a quick overview of the bhyve shutdown
process. It is a two stage process: first, the main bhyve
process gets destroyed (either via an external command or within the
guest), then the resources need to be cleaned up using the bhyvectl(8)
tool.

The first issue is that if virCommandRun() for bhyvectl(8) fails,
virBhyveProcessStop() jumps to the 'cleanup' label and misses cleaning
of some resources.

The second issue is more serious. Currently, monitor is closed only
after running of the bhyvectl(8) command. That means that the monitor
could catch the domain destroy event and try to run
virBhyveProcessStop() on the same domain again, resulting in trying
to release already released resources, such as the monitor itself.

Address by:

 * Making virCommandRun() on bhyvectl(8) non-critical. Even if it
   fails, we try to clean up all resources. We consider the function
   failed (return value 1) though.

 * Close monitor before running bhyvectl(8)

Additionally, do not verify that virBhyveProcessBuildDestroyCmd()
returns non-NULL, there could be only allocation errors.
And with 'glib' they result in an abort() so no need
to worry about those.

Reported-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>

diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c
index 26320200c5dd503f95c21e5d2a467caf38f2d8fc..842ff0d6fc4c0672dd98824c84f80d9849de7f9c 100644 (file)
--- a/src/bhyve/bhyve_process.c
+++ b/src/bhyve/bhyve_process.c
@@ -515,7 +515,7 @@ virBhyveProcessStop(struct _bhyveConn *driver,
                     virDomainObj *vm,
                     virDomainShutoffReason reason)
 {
-    int ret = -1;
+    int ret = 0;
     g_autoptr(virCommand) cmd = NULL;
     bhyveDomainObjPrivate *priv = vm->privateData;
 
@@ -531,15 +531,19 @@ virBhyveProcessStop(struct _bhyveConn *driver,
         return -1;
     }
 
-    if (!(cmd = virBhyveProcessBuildDestroyCmd(driver, vm->def)))
-        return -1;
-
-    if (virCommandRun(cmd, NULL) < 0)
-        goto cleanup;
-
+    /* Destroy monitor before running the actual destroy command to prevent
+     * it from detecting VM shutdown and entering this cleanup routine again */
     if ((priv != NULL) && (priv->mon != NULL))
          bhyveMonitorClose(priv->mon);
 
+    cmd = virBhyveProcessBuildDestroyCmd(driver, vm->def);
+    if (virCommandRun(cmd, NULL) < 0) {
+        /* Only failure of the actual destroy command warrants unsuccessful return code,
+         * other failures are not considered critical */
+        ret = -1;
+        VIR_WARN("Failed to run the domain destroy command");
+    }
+
     bhyveProcessStopHook(driver, vm, VIR_HOOK_BHYVE_OP_STOPPED);
 
     /* Cleanup network interfaces */
@@ -554,8 +558,6 @@ virBhyveProcessStop(struct _bhyveConn *driver,
         }
     }
 
-    ret = 0;
-
     virCloseCallbacksDomainRemove(vm, NULL, bhyveProcessAutoDestroy);
 
     virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, reason);
@@ -563,8 +565,6 @@ virBhyveProcessStop(struct _bhyveConn *driver,
     vm->def->id = -1;
 
     bhyveProcessStopHook(driver, vm, VIR_HOOK_BHYVE_OP_RELEASE);
-
- cleanup:
     virPidFileDelete(BHYVE_STATE_DIR, vm->def->name);
     bhyveProcessRemoveDomainStatus(BHYVE_STATE_DIR, vm->def->name);