Re: [libvirt] [PATCH] Split all QEMU process mangement code into separate file

29 Jan 2011


      On 01/28/2011 06:21 AM, Daniel P. Berrange wrote:
...
Move the qemudStartVMDaemon and qemudShutdownVMDaemon
methods into a separate file, renaming them to
qemuProcessStart, qemuProcessStop. All helper methods
called by these are also moved & renamed to match
* src/Makefile.am: Add qemu_process.c/.h
* src/qemu/qemu_command.c: Add emuDomainAssignPCIAddresses
s/ emu/ qemu/
...
* src/qemu/qemu_command.h: Add VNC port min/max
* src/qemu/qemu_domain.c, src/qemu/qemu_domain.h: Add
  domain event queue helpers
* src/qemu/qemu_driver.c, src/qemu/qemu_driver.h: Remove
  all QEMU process startup/shutdown functions
* src/qemu/qemu_process.c, src/qemu/qemu_process.h: Add
  all QEMU process startup/shutdown functions
...
+++ b/src/qemu/qemu_domain.c
@@ -29,6 +29,7 @@
 #include "logging.h"
 #include "virterror_internal.h"
 #include "c-ctype.h"
+#include "event.h"
#include <sys/time.h>
@@ -41,6 +42,61 @@
 #define timeval_to_ms(tv)       (((tv).tv_sec * 1000ull) + ((tv).tv_usec / 1000))
+static void qemuDomainEventDispatchFunc(virConnectPtr conn,
+                                        virDomainEventPtr event,
+                                        virConnectDomainEventGenericCallback cb,
+                                        void *cbopaque,
+                                        void *opaque)
+{
+    struct qemud_driver *driver = opaque;
+
+    /* Drop the lock whle dispatching, for sake of re-entrancy */
s/whle/while/
...
+++ b/src/qemu/qemu_driver.c
+#  define KVM_CAP_NR_VCPUS 9       /* returns max vcpus per vm */
+# endif
+
+
+#define timeval_to_ms(tv)       (((tv).tv_sec * 1000ull) + ((tv).tv_usec / 1000))
This will fail 'make syntax-check' if you have cppi installed.
...
-static int doStartCPUs(struct qemud_driver *driver, virDomainObjPtr vm, virConnectPtr conn)
Hmm, you also did some reorganization as part of moving functions
between files (this appears early in the old qemu_driver.c, but late in
the new qemu_process.c).  That's okay, it just took me a bit longer to
review.
...
+
 static void
-qemudAutostartConfigs(struct qemud_driver *driver) {
+qemuAutostartDomains(struct qemud_driver *driver)
Interesting mix of renames and code movement in the same patch.  I'm not
going to make you split it now, but it might have been nicer as two commits.
...
-
-/**
- * qemudRemoveDomainStatus
- *
- * remove all state files of a domain from statedir
- *
- * Returns 0 on success
- */
 static int
-qemudRemoveDomainStatus(struct qemud_driver *driver,
-                        virDomainObjPtr vm)
+qemuSecurityInit(struct qemud_driver *driver)
 {
-    char ebuf[1024];
-    char *file = NULL;
-
-    if (virAsprintf(&file, "%s/%s.xml", driver->stateDir, vm->def->name) < 0) {
-        virReportOOMError();
-        return(-1);
-    }
-
-    if (unlink(file) < 0 && errno != ENOENT && errno != ENOTDIR)
-        VIR_WARN("Failed to remove domain XML for %s: %s",
-                 vm->def->name, virStrerror(errno, ebuf, sizeof(ebuf)));
-    VIR_FREE(file);
+    virSecurityManagerPtr mgr = virSecurityManagerNew(driver->securityDriverName,
+                                                      driver->allowDiskFormatProbing);
+    if (!mgr)
+        goto error;
Especially in places like this, where the rename interfered with the
diff algorithm to produce a difficult-to-read diff.
...
-
-struct virReconnectDomainData {
It took me a while to see the rename virReconnectDomainData ->
qemuProcessReconnectData.
...
-static int qemudStartVMDaemon(virConnectPtr conn,
-                              struct qemud_driver *driver,
...
...
-    if (!(cmd = qemuBuildCommandLine(conn, driver, vm->def, priv->monConfig,
-                                     priv->monJSON != 0, qemuCmdFlags,
-                                     migrateFrom, stdin_fd,
-                                     vm->current_snapshot, vmop)))
-        goto cleanup;
-
-    if (qemuDomainSnapshotSetCurrentInactive(vm, driver->snapshotDir) < 0)
-        goto cleanup;
Why was the SetCurrentInactive line commented out in the move?
...
+static void
+qemuProcessReconnect(void *payload, const char *name ATTRIBUTE_UNUSED, void *opaque)
+{
...
+    if (virDomainObjUnref(obj) > 0) {
+        /* We can't get the monitor back, so must kill the VM
+         * to remove danger of it ending up running twice if
+         * user tries to start it again later */
+        qemudShutdownVMDaemon(driver, obj, 0);
Wouldn't this cause a compile error, since you renamed the function to
qemuProcessStop?  Indeed:

cc1: warnings being treated as errors
qemu/qemu_process.c: In function 'qemuProcessReconnect':
qemu/qemu_process.c:1854:9: error: implicit declaration of function
'qemudShutdownVMDaemon' [-Wimplicit-function-declaration]
qemu/qemu_process.c:1854:9: error: nested extern declaration of
'qemudShutdownVMDaemon' [-Wnested-externs]
qemu/qemu_process.c: In function 'qemuProcessStart':
qemu/qemu_process.c:1949:9: error: implicit declaration of function
'fstat' [-Wimplicit-function-declaration]
qemu/qemu_process.c:1949:9: error: nested extern declaration of 'fstat'
[-Wnested-externs]
qemu/qemu_process.c:1954:9: error: implicit declaration of function
'S_ISFIFO' [-Wimplicit-function-declaration]
qemu/qemu_process.c:1954:9: error: nested extern declaration of
'S_ISFIFO' [-Wnested-externs]

Also, you're still missing a clean 'make syntax-check' run; at least
po/POTFILES.in needs work.

I like the idea of this patch, but it needs a bit more work.

-- 
Eric Blake   eblake@redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

    