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, µ) != 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;