Re: [Libvir] PATCH: Autodetect QEMU version

["Daniel P. Berrange" <berrange redhat com>]

On Wed, Feb 21, 2007 at 05:26:52PM +0000, Mark McLoughlin wrote:
> On Wed, 2007-02-21 at 15:45 +0000, Daniel P. Berrange wrote:
> >      if (vm->def->graphicsType == QEMUD_GRAPHICS_VNC) {
> >          char port[10];
> > -        snprintf(port, 10, "%d", vm->def->vncActivePort - 5900);
> > +        int ret;
> > +        ret = snprintf(port, sizeof(port),
> > +                       (server->qemuVersion >= 9000 ?
> > +                        ":%d" : "%d"),
> > +                       vm->def->vncActivePort - 5900);
> 
> 	How about something like:
> 
>   server->qemuCmdFlags & QEMUD_CMD_VNC_PORT_NEEDS_COLON ? ":%d" : "%d"
> 
> 	That way we could keep the actual logic around what version does what
> in qemudExtractVersion()
> 
> 	(Ditto for QEMUD_CMD_HAS_KQEMU ...)

Seems reasonable.


> > +int qemudExtractVersion(const char *qemu, int *version, int *hasKQEMU) {
> > +    int child;
> > +    int newstdout[2];
> > +
> > +    if (pipe(newstdout) < 0) {
> > +        return -1;
> > +    }
> > +
> > +    if ((child = fork()) < 0) {
> 
> 	Close the pipes here

Fixed.

> > +        if ((got = read(newstdout[0], help, sizeof(help)-1)) < 0)
> > +            goto cleanup2;
> 
> 	Handle EINTR here ... it's quite a likely place to hit it, I guess
> (e.g. SIGCHLD?)

Yes, I also check & try in the waitpid() call too now.

> > +    cleanup2:
> > +        if (close(newstdout[0]) < 0)
> > +            ret = -1;
> > +
> > +        if (waitpid(child, &got, 0) != child ||
> > +            WEXITSTATUS(got) != 1) {
> > +            qemudLog(QEMUD_ERR, "Unexpected exit status from qemu %d pid %d", got, child);
> > +            ret = -1;
> > +        }
> 
> 	Don't know that we actually want to fail under both these
> circumstances.

Switched it to only fail on the waitpid return value case. For unexpected
exit status we simply log a warning message.

> > --- qemud/driver.h      14 Feb 2007 16:20:38 -0000      1.3
> > +++ qemud/driver.h      21 Feb 2007 15:40:41 -0000
> > @@ -30,6 +30,7 @@
> >  void qemudReportError(struct qemud_server *server,
> >                        int code, const char *fmt, ...);
> >  
> > +int qemudExtractVersion(const char *qemu, int *version, int *hasKQEMU);
> 
> 	The driver seems to be a strange place to put this function.

Not quite sure what I was thinking. Have moved it to conf.c instead
so its a static function now.

> > diff -u -p -r1.23 qemud.c
> > --- qemud/qemud.c       20 Feb 2007 17:51:41 -0000      1.23
> > +++ qemud/qemud.c       21 Feb 2007 15:40:42 -0000
> > @@ -411,20 +411,26 @@ static struct qemud_server *qemudInitial
>  
> > +    if (!(binary = qemudLocateBinaryForArch(server, QEMUD_VIRT_QEMU, "i686")))
> > +        goto cleanup;
> > +    if (qemudExtractVersion(binary, &server->qemuVersion, &server->qemuHasKQEMU) < 0)
> > +        goto cleanup;
> > +    free(binary);
> > +    binary = NULL;
> 
> 	Note, this means we don't have support for networks if something's
> wrong with qemu ... think we should just try and locate the binary when
> starting domains and return an error if it fails.

The info is also needed in the getVersionCall for virConnectGetVersion, so
I've made a thin wrapper to lazy-init the version number when needed.

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

Index: qemud/conf.c
===================================================================
RCS file: /data/cvs/libvirt/qemud/conf.c,v
retrieving revision 1.28
diff -u -p -r1.28 conf.c
--- qemud/conf.c	22 Feb 2007 10:39:38 -0000	1.28
+++ qemud/conf.c	22 Feb 2007 18:20:21 -0000
@@ -31,6 +31,7 @@
 #include <unistd.h>
 #include <errno.h>
 #include <fcntl.h>
+#include <sys/wait.h>
 
 #include <libxml/parser.h>
 #include <libxml/tree.h>
@@ -257,6 +258,112 @@ static char *qemudLocateBinaryForArch(st
     return path;
 }
 
+
+static int qemudExtractVersionInfo(const char *qemu, int *version, int *flags) {
+    int child;
+    int newstdout[2];
+
+    *flags = 0;
+    *version = 0;
+
+    if (pipe(newstdout) < 0) {
+        return -1;
+    }
+
+    if ((child = fork()) < 0) {
+        close(newstdout[0]);
+        close(newstdout[1]);
+        return -1;
+    }
+
+    if (child == 0) { /* Kid */
+        if (close(STDIN_FILENO) < 0)
+            goto cleanup1;
+        if (close(STDERR_FILENO) < 0)
+            goto cleanup1;
+        if (close(newstdout[0]) < 0)
+            goto cleanup1;
+        if (dup2(newstdout[1], STDOUT_FILENO) < 0)
+            goto cleanup1;
+
+        /* Just in case QEMU is translated someday.. */
+        setenv("LANG", "C", 1);
+        execl(qemu, qemu, (char*)NULL);
+
+    cleanup1:
+        _exit(-1); /* Just in case */
+    } else { /* Parent */
+        char help[4096]; /* Ought to be enough to hold QEMU help screen */
+        int got, ret = -1;
+        int major, minor, micro;
+
+        if (close(newstdout[1]) < 0)
+            goto cleanup2;
+
+    reread:
+        if ((got = read(newstdout[0], help, sizeof(help)-1)) < 0) {
+            if (errno == EINTR)
+                goto reread;
+            goto cleanup2;
+        }
+        help[got] = '\0';
+
+        if (sscanf(help, "QEMU PC emulator version %d.%d.%d", &major,&minor, &micro) != 3) {
+            goto cleanup2;
+        }
+
+        *version = (major * 1000 * 1000) + (minor * 1000) + micro;
+        if (strstr(help, "-no-kqemu"))
+            *flags |= QEMUD_CMD_FLAG_KQEMU;
+        if (*version >= 9000)
+            *flags |= QEMUD_CMD_FLAG_VNC_COLON;
+        ret = 0;
+
+        qemudDebug("Version %d %d %d  Cooked version: %d, with KQEMU ? %d",
+                   major, minor, micro, *version, *hasKQEMU);
+
+    cleanup2:
+        if (close(newstdout[0]) < 0)
+            ret = -1;
+
+    rewait:
+        if (waitpid(child, &got, 0) != child) {
+            if (errno == EINTR) {
+                goto rewait;
+            }
+            qemudLog(QEMUD_ERR, "Unexpected exit status from qemu %d pid %d", got, child);
+            ret = -1;
+        }
+        /* Check & log unexpected exit status, but don't fail,
+         * as there's really no need to throw an error if we did
+         * actually read a valid version number above */
+        if (WEXITSTATUS(got) != 1) {
+            qemudLog(QEMUD_WARN, "Unexpected exit status '%d', qemu probably failed", got);
+        }
+
+        return ret;
+    }
+}
+
+int qemudExtractVersion(struct qemud_server *server) {
+    char *binary = NULL;
+
+    if (server->qemuVersion > 0)
+        return 0;
+
+    if (!(binary = qemudLocateBinaryForArch(server, QEMUD_VIRT_QEMU, "i686")))
+        return -1;
+
+    if (qemudExtractVersionInfo(binary, &server->qemuVersion, &server->qemuCmdFlags) < 0) {
+        free(binary);
+        return -1;
+    }
+
+    free(binary);
+    return 0;
+}
+
+
 /* Parse the XML definition for a disk */
 static struct qemud_vm_disk_def *qemudParseDiskXML(struct qemud_server *server,
                                                    xmlNodePtr node) {
@@ -950,9 +1057,13 @@ int qemudBuildCommandLine(struct qemud_s
     struct qemud_vm_disk_def *disk = vm->def->disks;
     struct qemud_vm_net_def *net = vm->def->nets;
 
+    if (qemudExtractVersion(server) < 0)
+        return -1;
+
     len = 1 + /* qemu */
         2 + /* machine type */
-        (vm->def->virtType == QEMUD_VIRT_QEMU ? 1 : 0) + /* Disable kqemu */
+        (((server->qemuCmdFlags & QEMUD_CMD_FLAG_KQEMU) &&
+          (vm->def->virtType == QEMUD_VIRT_QEMU)) ? 1 : 0) + /* Disable kqemu */
         2 * vm->def->ndisks + /* disks*/
         (vm->def->nnets > 0 ? (4 * vm->def->nnets) : 2) + /* networks */
         2 + /* memory*/
@@ -977,9 +1088,10 @@ int qemudBuildCommandLine(struct qemud_s
         goto no_memory;
     if (!((*argv)[++n] = strdup(vm->def->os.machine)))
         goto no_memory;
-    if (vm->def->virtType == QEMUD_VIRT_QEMU) {
+    if ((server->qemuCmdFlags & QEMUD_CMD_FLAG_KQEMU) && 
+        (vm->def->virtType == QEMUD_VIRT_QEMU)) {
         if (!((*argv)[++n] = strdup("-no-kqemu")))
-        goto no_memory;
+            goto no_memory;
     }
     if (!((*argv)[++n] = strdup("-m")))
         goto no_memory;
@@ -996,8 +1108,8 @@ int qemudBuildCommandLine(struct qemud_s
         goto no_memory;
 
     if (!(vm->def->features & QEMUD_FEATURE_ACPI)) {
-    if (!((*argv)[++n] = strdup("-no-acpi")))
-        goto no_memory;
+        if (!((*argv)[++n] = strdup("-no-acpi")))
+            goto no_memory;
     }
 
     for (i = 0 ; i < vm->def->os.nBootDevs ; i++) {
@@ -1103,7 +1215,14 @@ int qemudBuildCommandLine(struct qemud_s
 
     if (vm->def->graphicsType == QEMUD_GRAPHICS_VNC) {
         char port[10];
-        snprintf(port, 10, "%d", vm->def->vncActivePort - 5900);
+        int ret;
+        ret = snprintf(port, sizeof(port),
+                       ((server->qemuCmdFlags & QEMUD_CMD_FLAG_VNC_COLON) ?
+                        ":%d" : "%d"),
+                       vm->def->vncActivePort - 5900);
+        if (ret < 0 || ret >= (int)sizeof(port))
+            goto error;
+
         if (!((*argv)[++n] = strdup("-vnc")))
             goto no_memory;
         if (!((*argv)[++n] = strdup(port)))
Index: qemud/conf.h
===================================================================
RCS file: /data/cvs/libvirt/qemud/conf.h,v
retrieving revision 1.7
diff -u -p -r1.7 conf.h
--- qemud/conf.h	15 Feb 2007 19:04:45 -0000	1.7
+++ qemud/conf.h	22 Feb 2007 18:21:15 -0000
@@ -26,6 +26,8 @@
 
 #include "internal.h"
 
+int qemudExtractVersion(struct qemud_server *server);
+
 int qemudBuildCommandLine(struct qemud_server *server,
                           struct qemud_vm *vm,
                           char ***argv);
Index: qemud/driver.c
===================================================================
RCS file: /data/cvs/libvirt/qemud/driver.c,v
retrieving revision 1.10
diff -u -p -r1.10 driver.c
--- qemud/driver.c	16 Feb 2007 18:30:55 -0000	1.10
+++ qemud/driver.c	22 Feb 2007 18:21:15 -0000
@@ -274,6 +274,9 @@ struct qemud_vm *qemudFindVMByName(const
 }
 
 int qemudGetVersion(struct qemud_server *server) {
+    if (qemudExtractVersion(server) < 0)
+        return -1;
+
     return server->qemuVersion;
 }
 
Index: qemud/internal.h
===================================================================
RCS file: /data/cvs/libvirt/qemud/internal.h,v
retrieving revision 1.12
diff -u -p -r1.12 internal.h
--- qemud/internal.h	16 Feb 2007 18:30:55 -0000	1.12
+++ qemud/internal.h	22 Feb 2007 18:21:24 -0000
@@ -152,6 +152,13 @@ enum qemud_vm_grapics_type {
     QEMUD_GRAPHICS_VNC,
 };
 
+/* Internal flags to keep track of qemu command line capabilities */
+enum qemud_cmd_flags {
+    QEMUD_CMD_FLAG_KQEMU = 1,
+    QEMUD_CMD_FLAG_VNC_COLON = 2,
+};
+
+
 enum qemud_vm_features {
     QEMUD_FEATURE_ACPI = 1,
 };
@@ -275,6 +282,7 @@ struct qemud_server {
     int nsockets;
     struct qemud_socket *sockets;
     int qemuVersion;
+    int qemuCmdFlags; /* values from enum qemud_cmd_flags */
     int nclients;
     struct qemud_client *clients;
     int sigread;
Index: qemud/qemud.c
===================================================================
RCS file: /data/cvs/libvirt/qemud/qemud.c,v
retrieving revision 1.23
diff -u -p -r1.23 qemud.c
--- qemud/qemud.c	20 Feb 2007 17:51:41 -0000	1.23
+++ qemud/qemud.c	22 Feb 2007 18:21:26 -0000
@@ -417,8 +417,6 @@ static struct qemud_server *qemudInitial
         return NULL;
     }
 
-    /* XXX extract actual version */
-    server->qemuVersion = (0*1000000)+(8*1000)+(0);
     /* We don't have a dom-0, so start from 1 */
     server->nextvmid = 1;
     server->sigread = sigread;