[Libvir] PATCH: Autodetect QEMU version
When I tested out the libvirt 0.2.0 in Fedora rawhide I discovered that the syntax for specifying VNC display numbers changes in QEMU 0.9.0. Previously you would use -vnc 7 Now you need to use -vnc :7 For current Fedora RPM I just hacked in a workaround to always add ':', but we need to be able to detect the version properly. So I'm attaching a patch which spawns '/usr/bin/qemu' and groks its command line help to extract the version number. It then uses appropriate VNC argument syntax based on the version. At the same time I also check for whether -no-kqemu is available so we can turn off KQEMU support if it is not available. 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 -=|
Hi Dan, On Wed, 2007-02-21 at 15:45 +0000, Daniel P. Berrange wrote:
At the same time I also check for whether -no-kqemu is available so we can turn off KQEMU support if it is not available.
Score! :)
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 ...)
+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
+ 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?)
+ qemudDebug("Version %d %d %d Cooked version: %d, with KQEMU ? %d",
Need a "\n"
+ major, minor, micro, *version, *hasKQEMU); + + 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.
Index: qemud/driver.h =================================================================== RCS file: /data/cvs/libvirt/qemud/driver.h,v retrieving revision 1.3 diff -u -p -r1.3 driver.h --- 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.
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 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. Cheers, Mark.
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 -=|
Hey, Looks cool, just spotted ... On Thu, 2007-02-22 at 18:32 +0000, Daniel P. Berrange wrote:
+ 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);
You want: qemudLog(QEMUD_ERR, "Failed to wait on PID %lu : %s", (unsigned long)child, strerror(errno)); (Also, should use pid_t for the child PID) Cheers, Mark.
participants (2)
-
Daniel P. Berrange -
Mark McLoughlin